fix: Clean up pods of fulfilled nodes when workflow manual retry. Fix…

[jswxstw]

Fixes #12028

Motivation

PodGC strategy: OnWorkflowSuccess.
Pods of the previously successful steps were not cleaned up after workflow manual retry.

Modifications

Pods of fulfilled nodes will be relabeled completed=false when workflow manual retry.

Verification

Workflow Demo

apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  generateName: workflow-fail-with-argument-
spec:
  entrypoint: main
  podGC:
    strategy: OnWorkflowSuccess
  arguments:
    parameters:
      - name: code
        value: 1
  templates:
  - name: main
    steps:
    - - name: print
        template: print-with-argument
        arguments:
          parameters:
            - name: code
              value: '{{workflow.parameters.code}}'
    - - name: check-status
        template: fail-with-argument
        arguments:
          parameters:
            - name: code
              value: '{{workflow.parameters.code}}'
  - name: print-with-argument
    inputs:
      parameters:
        - name: code
    container:
      image: docker/whalesay:latest
      command: [sh, -c]
      args: ["echo {{inputs.parameters.code}}"]
  - name: fail-with-argument
    inputs:
      parameters:
        - name: code
    container:
      image: python:alpine3.6
      command: [python, -c]
      args: ["import sys; sys.exit({{inputs.parameters.code}})"]

1. Workflow executed failed first time.

2. Retry the failed workflow manually, then workflow executes successfully and changes to succeeded phase.

3. All pods were successfully cleaned up.

[agilgur5]

this is a pretty minor permission given the Server already has delete, but I was thinking this logic may make more sense on the Controller actually.

When the Controller detects a retry, it can check the Workflow's child Pods.

Need to think a bit more about how that would work, but that would preserve the existing separation of duties between Server and Controller, where the Server is just a simple intermediary for users that can be bypassed with correct RBAC.
The Server primarily reads and listens to changes, and its modifications are limited to signaling the Controller to perform an action (via a label for instance). But the Controller logic is actually responsible to perform the actions themselves (such as retrying)

[agilgur5]

Hmmm I see that podsToDelete already broke the separation a bit... we honestly may want to refactor that too...

@terrytangyuan do you have any thoughts on this? Specifically, the current behavior with the Server having Pod modification logic actually invalidates your answer in #12027 (comment). I think it ideally should behave the way you described there (and how I described above), if possible.

[terrytangyuan]

You are right. RBAC modifications is required for my answer in #12027 (comment)

[agilgur5]

@terrytangyuan I was actually looking for your thoughts on the approach. I think we should refactor this logic to not have the Server perform anything but a label modification, and then the Controller should actually delete, reset, etc child Pods as needed (as that is the Controller's responsibility, not the Server's)

[sarabala1979]

@terrytangyuan I agree with @agilgur5. Server should not update or delete other than Argo workflow CRDs. If some user directly updates the WF spec then this will not work. Controller should handle the scenario to clean the workflow pods based on GCStrategy.
I remember @ishitasequeira was proposed to delete all workflow pods "label based" like workflowname deletion if the workflow complete.

[agilgur5]

Julie agreed in #12419 (comment) as well, so I filed #12538 as a tracking issue for this refactor

[terrytangyuan]

SGTM

[agilgur5]

I was gonna say that this may make sense to place into a helper function, but I see that #7988 (comment) explicitly moved the kubeClient logic out of the helper function (when retry was introduced for archived workflows in #7988).

So this matches existing behavior with podsToDelete above. If we refactored these into the Controller (per above comment), this would go away though 🤔

[agilgur5]

Thanks for taking the time to investigate and fix this!

Per the in-line comments, we may want to refactor some existing logic that this PR highlights as potentially having broken the Controller/Server separation of responsibilities.

I think the current code more or less makes sense within that existing context (the re-processing checks in the Controller are a bit confusing, though that's partially due to the existing logic. retries are also one of the most complex parts of the codebase), but we may want to change that existing code.

[jswxstw]

Retries(both auto or manual) are indeed the most complex parts of the codebase, which bring a lot of problems like #12009, #12010.
Moving the implementation into Controller really make sense, I'm glad to see this improvement.