-
Notifications
You must be signed in to change notification settings - Fork 104
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
Simplify instance controller reconciliation #1551
Conversation
Summary: current instance controller reconciliation logic with regard to detecting currently active plan can be much simpler now that we have IAW (instance admission webhook) controlling the scheduled plan. Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
@@ -145,35 +128,6 @@ func (i *Instance) PlanStatus(plan string) *PlanStatus { | |||
return nil | |||
} | |||
|
|||
// annotateSnapshot stores the current spec of Instance into the snapshot annotation | |||
// this information is used when executing update/upgrade plans, this overrides any snapshot that existed before | |||
func (i *Instance) AnnotateSnapshot() 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.
Fun fact: we don't need to save instance spec in the annotation anymore
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.
Fun fact 2: It's probably coming back for correct patching from the client side ;) But that doesn't concern the instance controller anymore, and it's good that it's gone here, having two versions in the annotations would be quite confusing
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.
Fun fact 3: I was hoping that we can avoid that and use patchStrategy: replace
on the corresponding instance spec fields 😉 But yeah, having two of these would be very confusing.
Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
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.
So much deleted code, I like it :)
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.
Lot's of code getting removed, awesome! I'm not fully in the loop on what particular steps are provided by the admission webhook. As a result, I mostly looked at what the old functions in the instance controller did and what they have been replaced with. My comments are mostly minor, but there also seems to be some changes to existing behavior, especially the call to ensurePlanStatusInitialized
that we should double-check to not have unexpected consequences.
Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
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.
LGTM!
Summary:
current instance controller reconciliation logic with regard to detecting currently active plan can be much simpler now that we have IAW (instance admission webhook) controlling the scheduled plan.
Signed-off-by: Aleksey Dukhovniy alex.dukhovniy@googlemail.com