fix(controller): shutdownstrategy on running workflow #5289
Conversation
Signed-off-by: Saravanan Balasubramanian <sarabala1979@gmail.com>
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.
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.
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
ShutdownandSuspendon workflow.specandworkflow.status.storedWorkflowSpec`. - Code needs to construct the
execWfin 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.
After given thought. if the user did patch through kubectl then option 1 won't work.
alexec
changed the title
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 |
| // 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.
minor - could even be woc.ShouldExecute(...)
Signed-off-by: Saravanan Balasubramanian sarabala1979@gmail.com
fixes #5288
Checklist: