-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
fix(controller): shutdownstrategy on running workflow #5289
Conversation
Signed-off-by: Saravanan Balasubramanian <[email protected]>
workflow/controller/exec_control.go
Outdated
@@ -27,7 +27,7 @@ func (woc *wfOperationCtx) applyExecutionControl(ctx context.Context, pod *apiv1 | |||
return nil | |||
case apiv1.PodPending: | |||
// Check if we are currently shutting down | |||
if woc.execWf.Spec.Shutdown != "" { | |||
if woc.wf.Spec.Shutdown != "" { |
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 is really easy to make mistakes. I know that there are notes on the use of wf
v.s. execWf
but I wonder if we could refactor a bit to prevent the misuse of woc.execWf
on Suspend
and Shutdown
.
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.
Yes, Mainly Shutdown
and Suspend
can be changed during the runtime. The above fix is only for the WorkflowTemplateRef
scenario rest of the scenario will work as expected.
Possible refactoring for prevent:
- Code can update
Shutdown
andSuspend
on workflow.specand
workflow.status.storedWorkflowSpec`. - Code needs to construct the
execWf
in each execution. This is a little bit costly operation on each execution.
I am thinking option 1, We just use execWf
for all places. Let me talk to the team about what they think.
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.
After given thought. if the user did patch through kubectl
then option 1 won't work.
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 come woc.wf
and woc.execWf
are not the same? this is very confusing
should we actually be using woc.wf.ExecSpec()
Another way to fix this issue is checking |
I think if we create one func on woc, we can make sure that func is correct |
Signed-off-by: Saravanan Balasubramanian <[email protected]>
Signed-off-by: Saravanan Balasubramanian <[email protected]>
// Only delete pods that are not part of an onExit handler if we are "Stopping" or all pods if we are "Terminating" | ||
_, onExitPod := pod.Labels[common.LabelKeyOnExit] | ||
|
||
if !woc.execWf.Spec.Shutdown.ShouldExecute(onExitPod) { | ||
woc.log.Infof("Deleting Pending pod %s/%s as part of workflow shutdown with strategy: %s", pod.Namespace, pod.Name, woc.execWf.Spec.Shutdown) | ||
if !woc.GetShutdownStrategy().ShouldExecute(onExitPod) { |
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.
minor - could even be woc.ShouldExecute(...)
Signed-off-by: Saravanan Balasubramanian [email protected]
fixes #5288
Checklist: