-
Notifications
You must be signed in to change notification settings - Fork 116
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 incremental update for StatefulSet #307
Conversation
07905e0
to
19743da
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.
I think this is looking pretty good. Just wanted to put a few questions in to make sure I understand. If we've smoke-tested this I think we're ok to ship if we're on the same page re: all the comments.
obj, err := decodeUnstructured(fmt.Sprintf(`{ | ||
"kind": "StatefulSet", | ||
"apiVersion": "apps/v1beta1", | ||
"metadata": { |
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 think we're intermingling spaces and tabs here. I don't think it particularly matters which we pick, but we're using spaces in the deployment examples.
return obj | ||
} | ||
|
||
func statefulsetUpdate(namespace, name, targetService string) *unstructured.Unstructured { |
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.
Is this supposed to be the user-provided inputs that update StatefulSet
, or the StatefulSet
after the new user inputs are provided? I believe it's the first, in which case is it intentional that .status
is missing?
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.
Yeah, was supposed to be the user-provided StatefulSet
return obj | ||
} | ||
|
||
func statefulsetAdded(namespace, name, targetService string) *unstructured.Unstructured { |
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.
The difference between each of these is kind of subtle -- what do you think about putting a little comment above each to point out the major differences?
pkg/await/apps_statefulset.go
Outdated
replicasReady bool | ||
currentGeneration int64 | ||
|
||
statefulsetErrors map[string]string |
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.
Hmm, doesn't seem like this is actually populated?
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.
Ha, looks like you're right. Will fix.
// | ||
// The success conditions are somewhat complex: | ||
// | ||
// 1. `.status.replicas`, `.status.currentReplicas` and `.status.readyReplicas` match the |
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.
Ah, looks like we also have to check .status.updatedReplicas
if it's an update rather than a create?
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'm using it for the status messages, but it's not actually part of the success condition.
Here's a table that's illustrative (I'll add to the doc as well):
Current replicas Ready replicas Updated replicas Notes
3 3 --
<Update image>
2 3 -- observedGeneration/updateRevision changes
2 2 --
2 2 1
1 3 1
1 2 1
1 2 2
-- 3 2
-- 2 2
-- 2 3
3 3 3 currentRevision updated
3 3 --
75b71be
to
1aac165
Compare
@hausdorff I think I've addressed all your feedback. |
@lblackstone @hausdorff Ready to merge? |
@joeduffy This was delayed by kubecon, I told @lblackstone that I'd pick up the remaining work and make the tweaks necessary. |
Per my earlier comment, I'm merging this because (a) looks reasonable, and (b) I understand this to be smoke-tested fairly extensively. |
Fixes #300