-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨ Implement Reconcile mode for ClusterResourceSet #7497
Conversation
Hi @g-gaston. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all sounds reasonable to me. Looking forward to using this new reconciliation strategy.
My understanding of this change is that we are adding a Reconcile resolution strategy to the Cluster Resource Set CRD. The new Reconcile strategy will update existing Secret and ConfigMap objects contents when they change in the CRS.
The bulk of this PRs changes business logic changes appear to come from the ApplyClusterResourceSet
function. Some of its business logic is refactored into clusterresourceset_scope.go
, where hashing and other utility functions are abstracted into the ResourceReconcileScope object.
Outside of the unit tests, how did you test this implementation?
exp/addons/internal/controllers/clusterresourceset_controller.go
Outdated
Show resolved
Hide resolved
The tests in I also tested this manually running some capd clusters with tilt. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a few nits, didn't get to the controllers folder yet
exp/addons/internal/controllers/clusterresourceset_controller.go
Outdated
Show resolved
Hide resolved
/retest |
7232186
to
0c5ee40
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this!!
Overall looks good to me, although I have to say I don't have a lot of experience with the CRS code base.
Would be nice to get some reviews from the other folks interested in this feature.
exp/addons/internal/controllers/clusterresourceset_controller.go
Outdated
Show resolved
Hide resolved
exp/addons/internal/controllers/clusterresourceset_scope_test.go
Outdated
Show resolved
Hide resolved
Co-authored-by: Stefan Büringer <4662360+sbueringer@users.noreply.github.com>
74edeaf
to
6c656f8
Compare
exp/addons/internal/controllers/clusterresourceset_controller.go
Outdated
Show resolved
Hide resolved
exp/addons/internal/controllers/clusterresourceset_controller.go
Outdated
Show resolved
Hide resolved
exp/addons/internal/controllers/clusterresourceset_controller.go
Outdated
Show resolved
Hide resolved
// The create call is idempotent, so if the object already exists | ||
// then do not consider it to be an error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup Fabrizio is right.
There are two usages of createUnstructured
. l.127 and l.168. In l.168 we check for already exists in l.127 we don't.
Or is that intentional? (that we ignore already exists in ApplyOnce and we surface it in Reconcile?)
Co-authored-by: Stefan Büringer <4662360+sbueringer@users.noreply.github.com>
57c87cc
to
2891a9c
Compare
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few nits, otherwise lgtm from my side (+ linter)
exp/addons/internal/controllers/clusterresourceset_controller_test.go
Outdated
Show resolved
Hide resolved
exp/addons/internal/controllers/clusterresourceset_controller_test.go
Outdated
Show resolved
Hide resolved
exp/addons/internal/controllers/clusterresourceset_controller_test.go
Outdated
Show resolved
Hide resolved
g.Expect(env.Update(ctx, testCluster)).To(Succeed()) | ||
|
||
t.Log("Updating the test config map with the missing namespace resource") | ||
missingNamespace := randomNamespaceForTest(t) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the linter is complaining about the way you're generating the name anyway. WDYT about just using something like this?
missingNamespace := randomNamespaceForTest(t) | |
missingNamespace := fmt.Sprintf("clusterresourceset-apply-once-%s", util.RandomString(6)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can change it if you prefer but I thought it was a useful function to have.
btw fixed the linter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine for me to keep just seems more complicated to have our own random func here vs. using the util that we use in a lot of other places
(usually we also use a prefix for the namespace which roughly identifies the test, just makes it easier to map resources to tests when debugging envtest based tests)
Up to you
…t.go typo Co-authored-by: Stefan Büringer <4662360+sbueringer@users.noreply.github.com>
Thank you very much! /lgtm /assign @fabriziopandini |
LGTM label has been added. Git tree hash: cd1d00bb36ba52dc2f56564cb2ba15c199e378d7
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great job
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
Implements
Reconcile
mode forClusterResourceSet
's, introduced by proposal #6555Fixes: #4807