Add support for Rook 1.10 #84
Conversation
emosbaugh
commented
Nov 22, 2022
emosbaugh
commented
- Updates go mod dependency to github.com/rook/rook v1.10.6
- Prefer JSON patch to update operation for Rook CRDs due to API stability concerns
- Prefer RookCephNS constant
|
[sc-60219] |
| "testing" | ||
|
|
||
| "github.com/blang/semver" | ||
| "github.com/golang/mock/gomock" |
There was a problem hiding this comment.
Why do we need to use gomock?
There was a problem hiding this comment.
I need to mock an interface for testing. Do you have something against this package or mocking in general? What would you like to see instead?
There was a problem hiding this comment.
In the projects I have been working with, they usually have a policy to avoid us much as possible deps to try make things easier to keep maintained. I did not use gomock so far and I can not be opinionated in this way. I am ok with 👍
In the future if we found a better option we can revisit this one.
| "github.com/replicatedhq/ekco/pkg/logger" | ||
| "github.com/replicatedhq/ekco/pkg/util" | ||
| cephv1 "github.com/rook/rook/pkg/apis/ceph.rook.io/v1" | ||
| rookfake "github.com/rook/rook/pkg/client/clientset/versioned/fake" |
There was a problem hiding this comment.
Not a blocker for we move forward here since we can discuss and improve it afterwords.
However, we are using controller-runtime (https://github.com/rook/rook/blob/master/go.mod#L40) so ideally we should use ENV TEST instead of fake clients. It brings a poor experience and has many limitations, see:
- https://github.com/kubernetes-sigs/controller-runtime/blob/master/FAQ.md#q-wheres-the-fake-client--how-do-i-use-it and add a known limitations section to fake Client kubernetes-sigs/controller-runtime#1031
- Also, controller-runtime has a old desired to deprecate/remove and it was not only removed because a lot of people has been using it: Reconsider deprecation of pkg/client/fake kubernetes-sigs/controller-runtime#768
Therefore, I'd like to suggest we use to do the tests ENV TEST ginkgo and gomega as it is done in other k8s projects such as controller-runtime as well. Following some examples:
- Kubebuilder: https://github.com/kubernetes-sigs/kubebuilder/blob/master/testdata/project-v3-with-deploy-image/controllers/memcached_controller_test.go
- Controller-Runtime: https://github.com/kubernetes-sigs/kubebuilder/blob/master/testdata/project-v3-with-deploy-image/controllers/memcached_controller_test.go
There was a problem hiding this comment.
Agreed but out of scope. Can you create a story in Replicated's internal Shortcut?
There was a problem hiding this comment.
I will do my best 👍
| github.com/blang/semver v3.5.1+incompatible | ||
| github.com/coreos/go-systemd/v22 v22.4.0 | ||
| github.com/gin-gonic/gin v1.8.1 | ||
| github.com/golang/mock v1.6.0 |
There was a problem hiding this comment.
Do we really need to add this dep?
|
|
||
| func NewController(config ControllerConfig, log *zap.SugaredLogger) *Controller { | ||
| return &Controller{Config: config, Log: log} | ||
| syncExecutor := k8s.NewSyncExecutor(config.Client.CoreV1(), config.ClientConfig) |
There was a problem hiding this comment.
What the NewSyncExecutor does?
Why do we need it?
There was a problem hiding this comment.
I could not figure out how to use k8s test client with remotecommand so I instead mocked the container exec. If you can point me to an example I can remove this mock.
There was a problem hiding this comment.
I do not think fake client work within. We can check afterwords if ENV TEST allow us to to that.
I would need to spend so effort researching and doing some POCs either. So, we can let it for future follow ups.
pkg/cluster/rook_ceph.go
Outdated
| monCount := 3 | ||
| if nodeCount < 3 { | ||
| monCount = 1 |
There was a problem hiding this comment.
Do we really need it (it seems make harder read the code) could we not to do the check directly since the values are fixed like:
// We only try to reduce the quantity of X when .... X because Y
if (cluster.Spec.Mon.Count == 3) || (cluster.Spec.Mon.Count < 3) {
c.Log.Debugf("Will not reduce mon count from %s to %s", cluster.Spec.Mon.Count, monCount)
return nil
}There was a problem hiding this comment.
This is not equivalent because we never set mon count to 2
|
|
||
| if !(changed || doFullReconcile) { | ||
| if !(len(patches) > 0 || doFullReconcile) { | ||
| return false, nil |
There was a problem hiding this comment.
Why the return is false?
Since we are using controller-runtime I'd expected we have the reconcile and be using the return options to restart the Reconcile implemented for it:
- With the error (to do the reconciliation again):
return ctrl.Result{}, err - Without an error (to do the reconciliation again to re-check the operations without return an error):
return ctrl.Result{Requeue: true}, nil - Therefore, to stop the Reconcile, use:
return ctrl.Result{}, nil - Reconcile again after X time:
return ctrl.Result{RequeueAfter: nextRun.Sub(r.Now())}, nil
For more details, check the Reconcile and its Result godoc.
There was a problem hiding this comment.
Can you create a story in Replicated's internal Shortcut?
There was a problem hiding this comment.
Addressed the reasoning for return value in docstring comment
| } | ||
|
|
||
| func (c *Controller) ReconcileMgrCount(ctx context.Context, rookVersion semver.Version, count int) error { | ||
| func (c *Controller) ReconcileMgrCount(ctx context.Context, rookVersion semver.Version, nodeCount int) error { |
There was a problem hiding this comment.
Not a blocker since all implementations seems to follow the same approach, however, why are we not using req ctrl.Request (ctrl.Result, error) ?
Also, why the name is Controller instead of RockController for example?
Also, why we have controllers under the pkg directory? We should not allow export the controllers and inside of the pkg ideally we should have only APIs and code implementations that we want make available for other projects if/when them import this one. Are we importing ecko in any other place as a dependency?
There was a problem hiding this comment.
Can you create a story in Replicated's internal Shortcut?
| RookCephSharedFSMetadataPool = "rook-shared-fs-metadata" | ||
| RookCephSharedFSDataPool = "rook-shared-fs-data0" | ||
| CephDeviceHealthMetricsPool = "device_health_metrics" | ||
| RookCephNS = "rook-ceph" |
There was a problem hiding this comment.
| RookCephNS = "rook-ceph" | |
| rookCephNS = "rook-ceph" |
Do we use this const outside of this package?
If not, do we really need to export them?
If we need to export them since they are used in another place could we properly doc/comment each one? Note that it would not pass in a lint.
There was a problem hiding this comment.
i'd rather not change this as there are many exported constants in this file that likely do not need to be exported for the same reason
There was a problem hiding this comment.
I think we could change all right? We do not need to export anything from here.
Also, all that is exported need to have a comment/doc otherwise when we be able to add the lint here it will fail.
There was a problem hiding this comment.
Possibly but its out of scope of this change
camilamacedo86
left a comment
camilamacedo86
left a comment
There was a problem hiding this comment.
I think the main things are addressed and a lot of things that we could to do is out of scope of this change. So, it seems OK for we move forward. Thank you for your attention.
