feat(controller): configurable terminationGracePeriodSeconds

[tczhao]

allow configure terminationGracePeriodSeconds from template

It sets the max wait time between SIGTERM and SIGKILL for main and sidecars

Resolves: #1012

Checklist:

• My organization is added to USERS.md.

[simster7]

This LGTM, but before merging I'll bring this up with the team to make sure we still want this spec. It has been a while since that issue was opened

[alexec]

What’s the use case for this? You’ve sidecars that take >30s to terminate?

[tczhao]

I personally don't have any use case for this,
but it seems there's a demand according to thread #1012 and a PR is asked

[tczhao]

Hi @ryan4yin @fabio-rigato @kerneltime @matveybrant,

Could you give us some use cases on when a configurable terminationGracePeriod would be needed?

[simster7]

Hi @tczhao. We currently won't want to support this as its own field in our spec (i.e. as Workflow.spec.tempalte.terminationGracePeriodSeconds). Our current idea is to use the Pod.spec.terminationGracePeriodSeconds field for this. The user can set that field using podSpectPatch and the executor can read from that field instead to apply this logic. We decided on this because we don't want to commit our own types to this yet. Let us know if you have comments on this, otherwise please refactor the PR to this new spec

[tczhao]

That make sense, will do the change

[simster7]

@tczhao Bump! Still working on this?

[tczhao]

@simster7 updated, please have a look

[simster7]

Hi @tczhao, this looks great thanks. Could you please include an E2E test that makes use of the podSpecPatch field? (example: https://github.com/argoproj/argo/blob/master/examples/pod-spec-patch.yaml).

This is crucial to ensure that a user can set this field right from their workflow

[tczhao]

@simster7 e2e test added