Workflow marked as finished before template level onExit hooks finished when stop workflow

[toyamagu-2021]

Pre-requisites

• I have double-checked my configuration
• I can confirm the issues exists when I tested with :latest
• I'd like to contribute the fix myself (see contributing guide)

What happened/what you expected to happen?

• Workflow does not wait for template-level onExit hooks to be finished when stopping
  • onExit hooks for template-level onExit hook is marked as Running Permanently
  • 
• This might be the root cause of flakiness of TestStopBehavior

  • argo-workflows/test/e2e/signals_test.go

    Line 27 in d0588ea

    func (s *SignalsSuite) TestStopBehavior() {

  • https://github.com/argoproj/argo-workflows/actions/runs/6288766349/job/17074596262?pr=11839

Condition "to be done" met after 9s
Checking expectation stop-terminate-wpvxk
stop-terminate-wpvxk : Failed Stopped with strategy 'Stop'
    signals_test.go:44: 
        	Error Trace:	/home/runner/work/argo-workflows/argo-workflows/test/e2e/signals_test.go:44
        	            				/home/runner/work/argo-workflows/argo-workflows/test/e2e/fixtures/then.go:68
        	            				/home/runner/work/argo-workflows/argo-workflows/test/e2e/fixtures/then.go:43
        	            				/home/runner/work/argo-workflows/argo-workflows/test/e2e/signals_test.go:36
        	Error:      	Not equal: 
        	            	expected: "Succeeded"
        	            	actual  : "Running"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1,2 +1,2 @@
        	            	-(v1alpha1.NodePhase) (len=9) "Succeeded"
        	            	+(v1alpha1.NodePhase) (len=7) "Running"
        	            	 
        	Test:       	TestSignalsSuite/TestStopBehavior
=== FAIL: SignalsSuite/TestStopBehavior

Version

latest

Paste a small workflow that reproduces the issue. We must be able to run the workflow; don't enter a workflows that uses private images.

apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  generateName: stop-terminate-
spec:
  entrypoint: main
  onExit: exit
  templates:
    - name: main
      dag:
        tasks:
          - name: A
            template: echo
            onExit: exit-template

    - name: echo
      container:
        image: argoproj/argosay:v1
        command: [ sleep ]
        args: [ "999" ]

    - name: exit
      container:
        image: argoproj/argosay:v1
        command: [ sleep ]
        args: [ "10" ]

    - name: exit-template
      container:
        image: argoproj/argosay:v1
        command: [ sleep ]
        args: [ "20" ]

Logs from the workflow controller

kubectl logs -n argo deploy/workflow-controller | grep ${workflow}

Logs from in your workflow's wait container

kubectl logs -n argo -c wait -l workflows.argoproj.io/workflow=${workflow},workflow.argoproj.io/phase!=Succeeded

[agilgur5]

Wow nice find and good hypothesis! I didn't realize there were two hooks on that Workflow, but that could make a lot of sense if that is root cause. Fairly edge case edge case.

If the controller has marked it as stopped and moved on, then the NodeStatus never gets updated 🤔

[toyamagu-2021]

Yeah. Truely edge case which might not problem for our users but for us suffered from flakiness :)

For workflow-level hooks, controller will wait for onExit NodeStatus:

• argo-workflows/workflow/controller/operator.go

  Line 430 in 6399ac7

  if woc.execWf.Spec.HasExitHook() && woc.GetShutdownStrategy().ShouldExecute(true) {

But will not wait for template-level hooks of dag?:

• argo-workflows/workflow/controller/dag.go

  Line 290 in 6399ac7

  _, _, err := woc.runOnExitNode(ctx, dagCtx.GetTask(taskName).GetExitHook(woc.execWf.Spec.Arguments), taskNode, dagCtx.boundaryID, dagCtx.tmplCtx, "tasks."+taskName, scope)

[skytt]

same questions. stopped workflow was archived with dag onExit hook running. And the archived workflow cannot retry again because of the running exit hook. Is there any fix processing?

[toyamagu-2021]

I'm afraid there is no fix going on currently.

[toyamagu-2021]

Thanks for sharing your use-case.
Can we get over this by checking NodeStatuses recursively? I might know the related logic, so I'll look into it.