[controller] Ensure k8s statefulset statuses are fresh #271
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
In debugging #268, I discovered that in all cases where we updated more than 1 statefulset, it was because the observed generation on the set was 1 less than the set's actual generation. This means that the Kubernetes statefulset controller had not yet processed the update, and that the information on `Status` that we check (ready replicas, current replicas, etc.) was not up-to-date. Also out of paranoia, I changed the update behavior to ensure that we keep the old object meta fields around when we send the `Update` to Kubernetes. I think if this isn't the case, we potentially overwrite a set even if it had changed since we last process it. This PR ensures that we stop processing the cluster if any of the set statuses are not fresh, and that we use familiar conflict methods on set updated. Closes #268.
schallert
commented
Feb 10, 2021
pkg/controller/controller.go
Outdated
| @@ -73,6 +73,7 @@ const ( | |||
|
|
|||
| var ( | |||
| errOrphanedPod = errors.New("pod does not belong to an m3db cluster") | |||
| errNoSetLabel = errors.New("pod does not have a parent statefulset error") | |||
There was a problem hiding this comment.
Unused. Will remove.
pkg/controller/controller.go
Outdated
| // If any of the statefulsets aren't ready, wait until they are as we'll get | ||
| // another event (ready == bootstrapped) | ||
| for _, sts := range childrenSets { | ||
| c.logger.Info("processing set", |
There was a problem hiding this comment.
Still need to decide whether these logs are too noisy. If we had them, however, debugging this issue would have been much easier.
There was a problem hiding this comment.
Can you simply make these debug logs and then make it easy to turn on debug logging? (might already be easy by just changing the config file?)
Codecov Report
@@ Coverage Diff @@
## master #271 +/- ##
==========================================
+ Coverage 75.62% 76.10% +0.47%
==========================================
Files 32 32
Lines 2343 2381 +38
==========================================
+ Hits 1772 1812 +40
+ Misses 427 426 -1
+ Partials 144 143 -1 Continue to review full report at Codecov.
|
wesleyk
approved these changes
Feb 10, 2021
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.
In debugging #268, I discovered that in all cases where we updated more
than 1 statefulset, it was because the observed generation on the set
was 1 less than the set's actual generation. This means that the
Kubernetes statefulset controller had not yet processed the update, and
that the information on
Statusthat we check (ready replicas, currentreplicas, etc.) was not up-to-date.
Also out of paranoia, I changed the update behavior to ensure that we
keep the old object meta fields around when we send the
UpdatetoKubernetes. I think if this isn't the case, we potentially overwrite a
set even if it had changed since we last process it.
This PR ensures that we stop processing the cluster if any of the set
statuses are not fresh, and that we use familiar conflict methods on set
updated.
Closes #268.