Custodian controller improvements #180
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gardener-robot
added
area/robustness
gardener-robot-ci-2
added
the
reviewed/ok-to-test
gardener-robot-ci-1
added
needs/ok-to-test
amshuman-kr
approved these changes
Jun 1, 2021
Comment on lines
-210
to
+202
| filteredStatefulSets, err := m.ClaimStatefulsets(statefulSets) | ||
| err = m.cl.List(ctx, statefulSets, client.InNamespace(etcd.Namespace), client.MatchingLabelsSelector{Selector: selector}) |
There was a problem hiding this comment.
Thanks for this correction ❤️
I have created the #186 to separately address simplification of the claim logic.
Comment on lines
+80
to
86
| // TODO: (timuthy) remove this as it could block important health checks | ||
| if etcd.Status.LastError != nil && *etcd.Status.LastError != "" { | ||
| logger.Info(fmt.Sprintf("Requeue item because of last error: %v", *etcd.Status.LastError)) | ||
| return ctrl.Result{ | ||
| RequeueAfter: 30 * time.Second, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
I agree 👍 This should be removed.
gardener-robot
added
reviewed/lgtm
|
@abdasgupta Can you please give your comments? Based on that I will merge. |
|
I have went through the PR. I am fine with the changes. I also tested it in local cluster. It works as it should. Thanks @timuthy for your PR. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
How to categorize this PR?
/area robustness
/kind enhancement
What this PR does / why we need it:
This PR improves several aspects of the custodian and etcd controllers as a preparation for #151:
logruscreate/update/deleteevents foretcdandcreate/deleteevents forstatefulset, as well asstatus updateforstatefulsets. Additionally, a mapper has been introduced which maps thestatefulsetfor which an event was triggered to the correspondingetcdresource. Earlier, this has only worked because in most of the casesetcdandstatefulsethave the samenamespace/name.statefulsetanymore, since this is the job of theetcd controller.statefulsetcondition sync has been removed as I didn't find anystatefulsetswhichconditionsin our system and further more, we should not try to sync and information fromstatefulset.statustoetcdby default. If astatusfulset.status.conditionis really of interest, then it can be read directly from thestatefulset.I created dedicated commits to simply the review.
Which issue(s) this PR fixes:
Fixes parts of #151
Special notes for your reviewer:
Previously, the custodian controller was setup by the following instruction:
builder.For(&appsv1.StatefulSet{}).Owns(&druidv1alpha1.Etcd{})This caused event triggers for all
statefulsets (ref) (-> we don't need to enqueue for events of allstatefulsets in the cluster) andetcds with anownerReference(ref) (->etcds don't have an owner reference).Release note: