-
Notifications
You must be signed in to change notification settings - Fork 103
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 Ready condition for Instance type #1706
Conversation
07dca3f
to
952bf5e
Compare
3840d31
to
25329ed
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.
This looks great already! Found some nits, but also have some questions around setting the "Unknown" state.
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.
Good start, left a few comments and nits 👍
@@ -536,5 +560,6 @@ func scheduledPlan(i *kudoapi.Instance, ov *kudoapi.OperatorVersion) (string, ty | |||
i.Spec.PlanExecution.Status = kudoapi.ExecutionNeverRun | |||
} | |||
|
|||
i.SetReadinessUnknownForDeploy() |
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 func scheduledPlan
was supposed to simply return the i.Spec.PlanExecution.PlanName
. Now, scheduling a cleanup
is already an ugly exception we need to make, but this method shouldn't update instance status.
// SetReadinessUnknownForDeploy sets an unknown state for Status.Conditions.Ready | ||
// this will modify instance only when deploy, upgrade, update plan is running | ||
func (i *Instance) SetReadinessUnknownForDeploy() { | ||
if i.Spec.PlanExecution.PlanName == DeployPlanName || i.Spec.PlanExecution.PlanName == UpgradePlanName || i.Spec.PlanExecution.PlanName == UpdatePlanName { |
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 long if
probably deserves a helper method of its own e.g. func IsDeployUpdateOrUpgradeScheduled
or smth :D
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 I am not sure :D I think long if vs long ugly name of method 🤷
c5abe48
to
960385b
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.
Almost there! Because I was just working on IsHealthy
for #1707, I noticed something around readinessMessage
that may need some changes.
@@ -536,5 +563,8 @@ func scheduledPlan(i *kudoapi.Instance, ov *kudoapi.OperatorVersion) (string, ty | |||
i.Spec.PlanExecution.Status = kudoapi.ExecutionNeverRun | |||
} | |||
|
|||
if i.Spec.PlanExecution.PlanName == kudoapi.DeployPlanName || i.Spec.PlanExecution.PlanName == kudoapi.UpgradePlanName || i.Spec.PlanExecution.PlanName == kudoapi.UpdatePlanName { |
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 not sure if we use these things somewhere else, but I wouldn't mind something like kudoapi.isDefaultPlan(planName)
or similar...
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 Aleksey wanted that as well I just don't know how to name it ... I am not sure default plan means something to me really. I don't want to do "IsDeployOrUpgradeOrUpdate" 🤷 so I ended up keeping this explicitly :/
e45f8ac
to
2a3e605
Compare
@@ -536,5 +576,8 @@ func scheduledPlan(i *kudoapi.Instance, ov *kudoapi.OperatorVersion) (string, ty | |||
i.Spec.PlanExecution.Status = kudoapi.ExecutionNeverRun | |||
} | |||
|
|||
if i.Spec.PlanExecution.PlanName == kudoapi.DeployPlanName || i.Spec.PlanExecution.PlanName == kudoapi.UpgradePlanName || i.Spec.PlanExecution.PlanName == kudoapi.UpdatePlanName { | |||
i.SetReadinessUnknown() |
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.
Again, this "simple getter" (with an exception) shouldn't update Instance status as a side effect. Let's move this someplace else.
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.
how is this a getter if this method modifies Spec? :D
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 meant in this comment that this is an ugly but necessary side-effect that is at least about the plan name. Instance status modification is an even bigger one.
21e255d
to
c5a46b7
Compare
Signed-off-by: Alena Varkockova <[email protected]>
Signed-off-by: Jan Schlicht <[email protected]>
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!
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.
One nit but otherwise lgtm 🚢
Signed-off-by: Jan Schlicht <[email protected]> Co-authored-by: Andreas Neumann <[email protected]>
Signed-off-by: Jan Schlicht <[email protected]>
Signed-off-by: Jan Schlicht <[email protected]>
Signed-off-by: Jan Schlicht <[email protected]>
Signed-off-by: Alena Varkockova [email protected]
What this PR does / why we need it:
This is an implementation of KEP-34. It adds
Status.Conditions.Ready
to an Instance while this property is being computed from the owned resources of certain types.