feat(podGC): add Workflow-level DeleteDelayDuration

[agilgur5]

Motivation

• previously, this could only be set globally for the entire controller

• now, users can configure this per Workflow as needed / wanted

  • and can override the global setting as desired
  • in particular, this matches how strategy etc can already be set per Workflow
    • so users have a single place to configure all options of podGC

• in the future (I assume after some deprecation window), the global setting could be removed

  • workflowDefaults can achieve the same thing now

Resolves #9894

• I took a bit of a different approach from there; with more explicit and consistent naming and typing
  • Using metav1.Duration was more consistent with the global and simplified the logic as well
  • Added more comments and more consistent comments as well

Modifications

• add DeleteDelayDuration to Workflow-level podGC
  • add to workflow_types.go, then add logic to use this in pod_cleanup.go
  • add to PodGC examples as well
  • +codegen

Verification

So pod_cleanup_test.go actually has no unit test for the whole queuePodsForCleanup() function, so I couldn't just add a unit there. That function could require some mocking to properly test, so I didn't introduce a whole new test suite for that at this point.

Notes to Reviewers

Main issue I have is with the codegen for fields.md, where it explains metav1.Duration a bit misleadingly 😕

• Everywhere else correctly shows that it boils down to just a string as well, but not this generated section 😕
• See in-line comment about this

Related, durations seem a bit inconsistently typed in the codebase and spec. Some are ints, some are strings, and others are Durations.

• Not sure if there's a reason for that? I would guess there is some history?

[terrytangyuan]

@juliev0 Could you help review this?

[agilgur5]

Not sure how codegen is failing, I actually haven't been able to reproduce that locally, even after making sure my deps were updated and re-running several times. Can manually fix that, but odd that my local install is not producing the same output

[juliev0]

Not sure how codegen is failing, I actually haven't been able to reproduce that locally, even after making sure my deps were updated and re-running several times. Can manually fix that, but odd that my local install is not producing the same output

I wonder if it could be running a different version of Go from what you are? looks like this is using 1.20: https://github.com/argoproj/argo-workflows/blob/master/.github/workflows/ci-build.yaml#L205

[juliev0]

@agilgur5 Looks good. Once you get the CI all passing, let me know and I can approve.

[agilgur5]

I wonder if it could be running a different version of Go from what you are? looks like this is using 1.20: https://github.com/argoproj/argo-workflows/blob/master/.github/workflows/ci-build.yaml#L205

So I'm using the dev container on my current machine, which is also using Go 1.20 😅
Confirmed that too:

vscode ➜ ~/go/src/github.com/argoproj/argo-workflows (feat-workflow-podgc-delay) $ go version
go version go1.20.5 linux/amd64

Strange. Might try rebuilding the dev container from scratch 🤷

[agilgur5]

Might try rebuilding the dev container from scratch 🤷

fresh build solved it 🤷

@agilgur5 Looks good. Once you get the CI all passing, let me know and I can approve.

@juliev0 should be good to go now!