feat(controller): Delay pod GC deletion by a configurable amount (default 5s) #6168

stefansedich · 2021-06-17T04:27:15Z

The PR #5033 was meant to fix the issue of delaying pod deletion for some period so things like logging scrapers can do their thing before the pod goes away.

Based on my comments #5033 (comment) this won't actually work as intended and another solution is needed.

My fix in this PR aims to delay the actual deletion by some configurable value and the deletion logic will then either delete immediately or queue the deletion for after this period.

@terrytangyuan tagging you as you did the original PR and tagging @alexec as you were the reviewer, as you might have more insight here. I have written an initial test here but am happy to expand coverage if desired.

One area I am not sure about is what other docs need to be updated for an API change and if that is a manual process or if there is some automation around that, it was not clear to me as I could not find any documentation explaining how to make contributions that add to the spec and this is my first time so just let me know what else I need to do.

Checklist:

My organization is added to USERS.md.

Tips:

Your PR needs to pass the required checks before it can be approved. If the check is not required (e.g. E2E tests) it does not need to pass
Sign-off your commits to pass the DCO check: git commit --signoff.
Run make pre-commit -B to fix codegen or lint problems.
Say how how you tested your changes. If you changed the UI, attach screenshots.

sarabala1979 · 2021-06-17T05:58:33Z

pkg/apis/workflow/v1alpha1/workflow_types.go

@@ -862,6 +864,11 @@ func (podGC *PodGC) GetLabelSelector() *metav1.LabelSelector {
 	return nil
 }

+// GetDelay gets the podGC delay as a duration.
+func (podGC *PodGC) GetDelay() time.Duration {
+	return time.Duration(podGC.DelaySeconds) * time.Second


Will it handle the default? what will the delay be if the user didn't specify in Workflow?

DelaySeconds should default to 0 and GetDelay in this case should return 0 seconds, meaning the deletion will be queued right away.

sarabala1979 · 2021-06-21T20:56:44Z

pkg/apis/workflow/v1alpha1/workflow_types.go

@@ -840,6 +840,8 @@ type PodGC struct {
 	Strategy PodGCStrategy `json:"strategy,omitempty" protobuf:"bytes,1,opt,name=strategy,casttype=PodGCStrategy"`
 	// LabelSelector is the label selector to check if the pods match the labels before being added to the pod GC queue.
 	LabelSelector *metav1.LabelSelector `json:"labelSelector,omitempty" protobuf:"bytes,2,opt,name=labelSelector"`
+	// DelaySeconds is how long to delay pod GC for.
+	DelaySeconds uint32 `json:"delaySeconds,omitempty" protobuf:"bytes,3,opt,name=delaySeconds"`


Can you change it to intstr.IntOrString some it can be parameterized by parameters?

@sarabala1979 done, hopefully I got that right.

alexec · 2021-06-22T21:59:08Z

I spoke with @sarabala1979 today and we think it is likely that few users will be impacted by a short delay on Pod GC, and many who'll benefit.

I suggest we have a single global value this suitable default. No per-workflow configuration.

stefansedich · 2021-06-22T22:03:33Z

I spoke with @sarabala1979 today and we think it is likely that few users will be impacted by a short delay on Pod GC, and many who'll benefit.

I suggest we have a single global value this suitable default. No per-workflow configuration.

Thanks @alexec this makes sense, will make things simpler and only require some more sleeps in the tests, did you talk about a reasonable default?

And by global default value do you mean configurable with config still?

alexec · 2021-06-22T22:13:03Z

I don't really know what a sensible default would be - maybe 10s? How long were you thinking you'd need? It should be configurable using the workflow-controller-configmap.

stefansedich · 2021-06-22T22:16:07Z

I don't really know what a sensible default would be - maybe 10s? How long were you thinking you'd need? It should be configurable using the workflow-controller-configmap.

In our case I planned to configure a default of 1m or so, 10s sounds like a reasonable default however for most and this does simplify things quite a bit, let me get that done.

stefansedich · 2021-06-22T22:56:27Z

config/config.go

-	// Defaults to 30 seconds.
+	// PodGCGracePeriodSeconds specifies the duration in seconds before a terminating pod is forcefully killed.
+	// Value must be non-negative integer. A zero value indicates that the pod will be forcefully terminated immediately.
+	// Defaults to the Kubernetes default of 30 seconds.


I changed this comment to signify what this option does, as the new option below handles the actual deletion delay.

stefansedich · 2021-06-22T22:56:57Z

config/config.go

@@ -144,6 +149,14 @@ func (c Config) GetResourceRateLimit() ResourceRateLimit {
 	}
 }

+func (c Config) GetPodGCDeleteDelay() time.Duration {
+	if c.PodGCDeleteDelay == nil {
+		return 30 * time.Second


Defaulting to 30s, happy to lower but this was somewhat in the middle of the suggested 10s and the 1m we would run in our environment.

I think this should be lower - 5s

Lowered to 5s by default.

stefansedich · 2021-06-22T22:57:15Z

test/e2e/pod_cleanup_test.go

@@ -21,7 +21,7 @@ type PodCleanupSuite struct {
 	fixtures.E2ESuite
 }

-const enoughTimeForPodCleanup = 3 * time.Second
+const enoughTimeForPodCleanup = 33 * time.Second


Padding out tests as they will need a little more time to "wait" now as there is a default deletion delay.

stefansedich · 2021-06-22T22:58:01Z

@alexec @sarabala1979 have made an initial cut at the changes moving to the config, let me know what you think.

stefansedich · 2021-06-22T22:59:05Z

config/config.go

+	// PodGCDelaySeconds specifies the duration in seconds before the pods in the GC queue get deleted.
+	// Value must be non-negative integer. A zero value indicates that the pods will be deleted immediately.
+	// Defaults to 30 seconds.
+	PodGCDeleteDelay *metav1.Duration `json:"podGCDeleteDelay,omitempty"`


I used a duration here, I was torn between using seconds like the other option above or moving to the more user-friendly duration so went with the more friendly option.

Please rename with type PodGCDeleteDelayDuration

Ok done @sarabala1979

terrytangyuan

I believe you'll have to fix the DCO check.

stefansedich · 2021-06-23T19:28:15Z

I believe you'll have to fix the DCO check.

Fixed!

Signed-off-by: Stefan Sedich <[email protected]>

alexec · 2021-06-25T17:20:19Z

I've marked this for v3.2, but we may consider back-porting to v3.1.

done

Signed-off-by: Stefan Sedich <[email protected]> Signed-off-by: uturunku1 <[email protected]>

* Update events.md (#6119) Trying to use the argo workflows events and I noticed that some crucial explanations are missing here. I would like to add: - A simple WorkflowTemplate bound to the WorkflowEventBinding, to show what is triggered by the curl that send the event - Some infos about the process that bind the event to the workflow template: - template creation - event binding apply - api call to trigger the workflow template creation Plus: there is a little mistake in the selector: metadata["x-argo"] instead of metadata["X-Argo-E2E"] I would like to correct it in order to avoid mistakes during the curl. Hope this is appreciated! ;) Denis Signed-off-by: uturunku1 <[email protected]> * docs: Add note on the requirements of resource templates. Fixes #5566 (#6125) Signed-off-by: Yuan Tang <[email protected]> Signed-off-by: uturunku1 <[email protected]> * docs: updated CHANGELOG.md (#6127) Signed-off-by: GitHub <[email protected]> Co-authored-by: alexec <[email protected]> Signed-off-by: uturunku1 <[email protected]> * add troubleshooting notes section for running-locally docs (#6132) Co-authored-by: uturunku1 <“[email protected]”> Signed-off-by: uturunku1 <[email protected]> * fix(executor): Check whether any errors within checkResourceState() are transient. Fixes #6118. (#6134) Signed-off-by: Yuan Tang <[email protected]> Signed-off-by: uturunku1 <[email protected]> * build: Remove PNS_PRIVILEGED=true (#6138) Signed-off-by: Alex Collins <[email protected]> Signed-off-by: uturunku1 <[email protected]> * docs: Document the extraction of data from a k8s resource (#6102) * Document the extraction of data from a k8s resource * remove reference to lines of a file that can be outdated Co-authored-by: Yuan Tang <[email protected]> * Remove yaml snippet and only keep the link to the example Co-authored-by: Yuan Tang <[email protected]> Signed-off-by: uturunku1 <[email protected]> * build image output to docker (#6128) Co-authored-by: Alex Collins <[email protected]> Signed-off-by: uturunku1 <[email protected]> * chore: Update stress rig and docs. Fixes #6136 (#6141) Signed-off-by: Alex Collins <[email protected]> Signed-off-by: uturunku1 <[email protected]> * chore: Upgrade Alibaba OSS to use more secure ListObjectsV2() (#6142) Signed-off-by: Yuan Tang <[email protected]> Signed-off-by: uturunku1 <[email protected]> * fix: Allow setting workflow input parameters in UI. Fixes #4234 (#5319) * fix: Allow setting workflow input parameters in UI. Fixes #4234 Signed-off-by: Kenny Trytek <[email protected]> * fix: Allow setting workflow input parameters in UI. Fixes #4234 - Allow workflow input parameters as well as entrypoint parameters. Signed-off-by: Kenny Trytek <[email protected]> Signed-off-by: uturunku1 <[email protected]> * fix(controller): Performance improvement for Sprig. Fixes #6135 (#6140) Signed-off-by: uturunku1 <[email protected]> * update from v0.19.6 to v0.20.4 and indirect dependencies Signed-off-by: uturunku1 <“[email protected]”> Signed-off-by: uturunku1 <[email protected]> * exec.GetAuthenticator takes two arguments in the k8s-client-go v0.20.4 Signed-off-by: uturunku1 <“[email protected]”> Signed-off-by: uturunku1 <[email protected]> * update makefile to use [email protected] Signed-off-by: uturunku1 <“[email protected]”> Signed-off-by: uturunku1 <[email protected]> * docs: Fix release-notes.md Signed-off-by: uturunku1 <[email protected]> * docs: Update Graviti's website link (#6148) Signed-off-by: uturunku1 <[email protected]> * fix(ui): Fix-up local storage namespaces. Fixes #6109 (#6144) Signed-off-by: uturunku1 <[email protected]> * fix(executor): Capture emissary main-logs. Fixes #6145 (#6146) Signed-off-by: uturunku1 <[email protected]> * fix(ui): Fix event-flow scrolling. Fixes #6133 (#6147) Signed-off-by: uturunku1 <[email protected]> * test: Fix logging test (#6159) Signed-off-by: Alex Collins <[email protected]> Signed-off-by: uturunku1 <[email protected]> * feat(ui): Add checkbox to check all workflows in list. Fixes #6069 (#6158) Signed-off-by: uturunku1 <[email protected]> * docs: Use 'depends' instead of 'dependencies' in examples (#6166) Signed-off-by: Simon Behar <[email protected]> Signed-off-by: uturunku1 <[email protected]> * feat(server): Allow redirect_uri to be automatically resolved when using sso (#6167) Signed-off-by: uturunku1 <[email protected]> * fix(controller): Allow retry on transient errors when validating workflow spec. Fixes #6163 (#6178) Signed-off-by: Yuan Tang <[email protected]> Signed-off-by: uturunku1 <[email protected]> * fix(controller): dehydrate workflow before deleting offloaded node status (#6112) Signed-off-by: uturunku1 <[email protected]> * docs: updated CHANGELOG.md (#6160) Signed-off-by: GitHub <[email protected]> Co-authored-by: alexec <[email protected]> Signed-off-by: uturunku1 <[email protected]> * docs: Remove RBAC for SSO from Roadmap (Already implemented) (#6174) It looks like RBAC for SSO is already implemented by #4198 so hopefully it can be removed from the roadmap as it is also documented? https://argoproj.github.io/argo-workflows/argo-server-sso/#sso-rbac Signed-off-by: uturunku1 <[email protected]> * docs: updated CHANGELOG.md (#6187) Signed-off-by: GitHub <[email protected]> Co-authored-by: alexec <[email protected]> Signed-off-by: uturunku1 <[email protected]> * docs: Fix changelog order for .0 tags (#6188) Signed-off-by: Alex Collins <[email protected]> Signed-off-by: uturunku1 <[email protected]> * fix(controller): Wrong validate order when validate DAG task's argument (#6190) Signed-off-by: BOOK <[email protected]> Signed-off-by: uturunku1 <[email protected]> * fix rebase conflict Signed-off-by: uturunku1 <[email protected]> * run go mod tidy Signed-off-by: uturunku1 <[email protected]> * refactor: Remove the need for pod annotations to be mounted as a volume (#6022) Signed-off-by: Antony Chazapis <[email protected]> Signed-off-by: uturunku1 <[email protected]> * docs: ContainerSets do not have 'depends' (#6199) Signed-off-by: Simon Behar <[email protected]> Signed-off-by: uturunku1 <[email protected]> * fix: Fix security issues related to file closing and paths (G307 & G304) (#6200) Signed-off-by: Yuan Tang <[email protected]> Signed-off-by: uturunku1 <[email protected]> * docs: Add links to Python examples to description annotations (#6202) Signed-off-by: Yuan Tang <[email protected]> Signed-off-by: uturunku1 <[email protected]> * docs(executor): document k8s executor behaviour with program warnings (#6212) * docs(executor): document k8s executor behaviour with program warnings Signed-off-by: Tianchu Zhao <[email protected]> * docs(executor): fix typo Signed-off-by: Tianchu Zhao <[email protected]> Signed-off-by: uturunku1 <[email protected]> * fix: Fix certain sibling tasks not connected to parent (#6193) Signed-off-by: Simon Behar <[email protected]> Signed-off-by: uturunku1 <[email protected]> * feat(ui): Add copy to clipboard shortcut (#6217) Signed-off-by: Simon Behar <[email protected]> Signed-off-by: uturunku1 <[email protected]> * docs: updated CHANGELOG.md (#6220) Signed-off-by: GitHub <[email protected]> Co-authored-by: alexec <[email protected]> Signed-off-by: uturunku1 <[email protected]> * docs: Add KarrotPay in USERS.md (#6221) Signed-off-by: Byungjin Park <[email protected]> Signed-off-by: uturunku1 <[email protected]> * run go mod tidy Signed-off-by: uturunku1 <[email protected]> * docs: Add workflow-count-resourcequota.yaml example (#6225) Signed-off-by: Alex Collins <[email protected]> Signed-off-by: uturunku1 <[email protected]> * fix: Reduce argoexec image size (#6197) Signed-off-by: Alex Collins <[email protected]> Signed-off-by: uturunku1 <[email protected]> * fix(conttroller): Always set finishedAt dote. Fixes #6135 (#6139) Signed-off-by: Alex Collins <[email protected]> Signed-off-by: uturunku1 <[email protected]> * feat: Add support for deletion delay when using PodGC (#6168) Signed-off-by: Stefan Sedich <[email protected]> Signed-off-by: uturunku1 <[email protected]> * docs: update bug report template (#6236) Signed-off-by: uturunku1 <[email protected]> * docs: updated CHANGELOG.md (#6242) Signed-off-by: GitHub <[email protected]> Co-authored-by: alexec <[email protected]> Signed-off-by: uturunku1 <[email protected]> * fix(executor): emissary - make argoexec executable from non-root containers. Fixes #6238 (#6247) Signed-off-by: Yuan Gong <[email protected]> Co-authored-by: Alex Collins <[email protected]> Signed-off-by: uturunku1 <[email protected]> * feat: Introduce when condition to retryStrategy (#6114) Signed-off-by: Simon Behar <[email protected]> Signed-off-by: uturunku1 <[email protected]> * ci: Add Go code security scanner via gosec. Fixes #6203 (#6232) Signed-off-by: Yuan Tang <[email protected]> Signed-off-by: uturunku1 <[email protected]> * docs: fix end of files, new lines and remove multiple lines (#6240) Signed-off-by: NikeNano <[email protected]> Signed-off-by: uturunku1 <[email protected]> * docs: add json destructuring example (#6250) Signed-off-by: Michael Crenshaw <[email protected]> Co-authored-by: Alex Collins <[email protected]> Signed-off-by: uturunku1 <[email protected]> * fix(executor): Tolerate docker re-creating containers. Fixes #6244 (#6252) Signed-off-by: Alex Collins <[email protected]> Signed-off-by: uturunku1 <[email protected]> * fix(executor): emissary - make /var/run/argo files readable from non-root users. Fixes #6238 (#6304) Signed-off-by: Yuan Gong <[email protected]> Signed-off-by: uturunku1 <[email protected]> * docs(controller): add missing emissary executor (#6291) Signed-off-by: Tianchu Zhao <[email protected]> Signed-off-by: uturunku1 <[email protected]> * docs: docs and hacks improvements (#6310) Signed-off-by: Michael Crenshaw <[email protected]> Signed-off-by: uturunku1 <[email protected]> * fix(cli): Only list needed fields. Fixes #6000 (#6298) * fix(cli): Only list needed fields Signed-off-by: Alex Collins <[email protected]> * ok Signed-off-by: Alex Collins <[email protected]> Signed-off-by: uturunku1 <[email protected]> * docs: Fix typo (#6311) Signed-off-by: Byungjin Park <[email protected]> Co-authored-by: Saravanan Balasubramanian <[email protected]> Signed-off-by: uturunku1 <[email protected]> * require sso redirect url to be an argo url (#6211) Signed-off-by: Brandon Goode <[email protected]> Co-authored-by: Alex Collins <[email protected]> Signed-off-by: uturunku1 <[email protected]> * docs: code format (#6269) - Add yaml rendering - Add bash rendering Co-authored-by: Simon Behar <[email protected]> Signed-off-by: uturunku1 <[email protected]> * feat(controller): Store artifact repository in workflow status. Fixes #6255 (#6299) Signed-off-by: Alex Collins <[email protected]> Signed-off-by: uturunku1 <[email protected]> * docs: document using ingress with TLS enabled (#6324) Signed-off-by: valorl <[email protected]> Signed-off-by: uturunku1 <[email protected]> * docs: document how to access hyphenated steps in expression templates (#6318) Signed-off-by: Michael Crenshaw <[email protected]> Signed-off-by: uturunku1 <[email protected]> * feat(controller): Differentiate CronWorkflow submission vs invalid spec error metrics (#6309) * feat(controller): Differentiate CronWorkflow submission vs invalid spec error metrics Signed-off-by: Yuan Tang <[email protected]> * Address feedback Signed-off-by: Yuan Tang <[email protected]> Co-authored-by: Alex Collins <[email protected]> Signed-off-by: uturunku1 <[email protected]> * chore: deleted wft.yaml Signed-off-by: Alex Collins <[email protected]> Signed-off-by: uturunku1 <[email protected]> * ci: only run Snyk once a day on master Signed-off-by: Alex Collins <[email protected]> Signed-off-by: uturunku1 <[email protected]> * fix(controller): Not updating StoredWorkflowSpec when WFT changed during workflow running (#6342) Signed-off-by: Saravanan Balasubramanian <[email protected]> Signed-off-by: uturunku1 <[email protected]> * fix(cli): v3.1 Argo Auth Token (#6344) * fix(cli): v3.1 Argo Auth Token Signed-off-by: Saravanan Balasubramanian <[email protected]> * update Signed-off-by: Saravanan Balasubramanian <[email protected]> Signed-off-by: uturunku1 <[email protected]> * docs: Add Alibaba Group to USERS.md (#6353) Signed-off-by: Yuan Tang <[email protected]> Signed-off-by: uturunku1 <[email protected]> * fix(crd): temp fix 34s timeout bug for k8s 1.20+ (#6350) Signed-off-by: Tianchu Zhao <[email protected]> Signed-off-by: uturunku1 <[email protected]> * docs: updated CHANGELOG.md (#6348) Signed-off-by: GitHub <[email protected]> Co-authored-by: sarabala1979 <[email protected]> Signed-off-by: uturunku1 <[email protected]> * docs(users): Add WooliesX (#6358) Signed-off-by: Tianchu Zhao <[email protected]> Signed-off-by: uturunku1 <[email protected]> * fix(cli): Overridding name/generateName when creating CronWorkflows if specified (#6308) Signed-off-by: Yuan Tang <[email protected]> Signed-off-by: uturunku1 <[email protected]> * feat(controller): sortDAGTasks supports sort by field Depends (#6307) Signed-off-by: BOOK <[email protected]> Signed-off-by: uturunku1 <[email protected]> * fix(fields): handle nexted fields when excluding (#6359) Signed-off-by: AntoineDao <[email protected]> Signed-off-by: uturunku1 <[email protected]> * feat(controller): Allow configurable host name label key when retrying different hosts (#6341) Signed-off-by: Yuan Tang <[email protected]> Signed-off-by: uturunku1 <[email protected]> * pull argo-events changes update versions in go.mod and go.sum Signed-off-by: uturunku1 <[email protected]> * run go mod tidy Signed-off-by: uturunku1 <[email protected]> * fix(controller): allow workflow.duration to pass validator (#6376) Signed-off-by: Tianchu Zhao <[email protected]> Signed-off-by: uturunku1 <[email protected]> * fix(controller): fix retry on transient errors when validating workflow spec (#6370) Signed-off-by: Tianchu Zhao <[email protected]> Co-authored-by: Saravanan Balasubramanian <[email protected]> Signed-off-by: uturunku1 <[email protected]> * fix: examples/ci.yaml indent (#6328) Signed-off-by: kungho.back <[email protected]> Signed-off-by: uturunku1 <[email protected]> * chore: import grafana dashboard (#6365) Signed-off-by: GitHub <[email protected]> Signed-off-by: uturunku1 <[email protected]> * fix(gcs): throw argo not found error if key not exist (#6393) Signed-off-by: AntoineDao <[email protected]> Signed-off-by: uturunku1 <[email protected]> * Revert "fix: examples/ci.yaml indent (#6328)" This reverts commit 3f72fe5. Signed-off-by: Alex Collins <[email protected]> Signed-off-by: uturunku1 <[email protected]> * fix: Server crash when opening timeline tab for big workflows (#6369) Signed-off-by: Alexander Matyushentsev <[email protected]> Co-authored-by: Saravanan Balasubramanian <[email protected]> Co-authored-by: Alex Collins <[email protected]> Signed-off-by: uturunku1 <[email protected]> * docs: Add 4intelligence (#6400) Signed-off-by: Thiago Gil <[email protected]> Signed-off-by: uturunku1 <[email protected]> * docs: Add note on additional required permission for createBucketIfNotPresent for OSS driver (#6378) Signed-off-by: Yuan Tang <[email protected]> Signed-off-by: uturunku1 <[email protected]> * fix(controller): allow initial duration to be 0 instead of current_time-0 (#6389) Signed-off-by: Tianchu Zhao <[email protected]> Signed-off-by: uturunku1 <[email protected]> * fix(controller): Mark workflows wait for semaphore as pending. Fixes #6351 (#6356) Signed-off-by: Alex Collins <[email protected]> Signed-off-by: uturunku1 <[email protected]> * docs: Updating upgrading.md. Closes #6314 Signed-off-by: Alex Collins <[email protected]> Signed-off-by: uturunku1 <[email protected]> * not need to convert to unstructured.unstructured I was getting this error controller_test.go: pkg/mod/k8s.io/[email protected]/tools/cache/reflector.go:167: Failed to watch *unstructured.Unstructured: failed to list *unstructured.Unstructured: item[0]: can't assign or convert unstructured.Unstructured into v1alpha1.Workflow Based on this comment, it seems like the conversion is not needed: kubernetes-sigs/controller-runtime#524 (comment) Signed-off-by: uturunku1 <[email protected]> * run make pre-commit -B Signed-off-by: uturunku1 <[email protected]> * fix potential file inclusion via variable lint error there is a risk that an unintended file path will be specified. So uuse filepath.Clean() to clean up possible bad paths Signed-off-by: uturunku1 <[email protected]> * fix format issue Signed-off-by: uturunku1 <[email protected]> Co-authored-by: Denis Bellotti <[email protected]> Co-authored-by: Yuan Tang <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: alexec <[email protected]> Co-authored-by: uturunku1 <“[email protected]”> Co-authored-by: Christophe Blin <[email protected]> Co-authored-by: meijin <[email protected]> Co-authored-by: kennytrytek <[email protected]> Co-authored-by: Caden <[email protected]> Co-authored-by: Simon Behar <[email protected]> Co-authored-by: Stefan Sedich <[email protected]> Co-authored-by: Reijer Copier <[email protected]> Co-authored-by: Brandon High <[email protected]> Co-authored-by: BOOK <[email protected]> Co-authored-by: Antony Chazapis <[email protected]> Co-authored-by: Tianchu Zhao <[email protected]> Co-authored-by: Byungjin Park (Claud) <[email protected]> Co-authored-by: Yuan (Bob) Gong <[email protected]> Co-authored-by: Niklas Hansson <[email protected]> Co-authored-by: Michael Crenshaw <[email protected]> Co-authored-by: Saravanan Balasubramanian <[email protected]> Co-authored-by: brgoode <[email protected]> Co-authored-by: Valér Orlovský <[email protected]> Co-authored-by: Alex Collins <[email protected]> Co-authored-by: sarabala1979 <[email protected]> Co-authored-by: Antoine Dao <[email protected]> Co-authored-by: KUNG HO BACK <[email protected]> Co-authored-by: Zadkiel <[email protected]> Co-authored-by: Alexander Matyushentsev <[email protected]> Co-authored-by: Thiago Bittencourt Gil <[email protected]>

prein · 2021-12-16T13:09:30Z

Is this meant to have no per-workflow configuration or no configuration at all? I'm not sure, reading the conversation above.
What I would find best for my scenarios would be if I could set different delay for failed and for succeeded workflows. I would then have much longer delay for failed ones to allow for manual inspection.

alexec · 2021-12-16T17:40:22Z

The goal of this PR is to delay all deletion by a minimum amount of time to allow for other services to clean them up. I think you just need normal PodGC.

stefansedich requested a review from jessesuen as a code owner June 17, 2021 04:27

sarabala1979 reviewed Jun 17, 2021

View reviewed changes

alexec assigned sarabala1979 Jun 17, 2021

stefansedich closed this Jun 17, 2021

stefansedich deleted the pod-gc-delay branch June 17, 2021 23:43

stefansedich restored the pod-gc-delay branch June 17, 2021 23:44

stefansedich reopened this Jun 17, 2021

This comment was marked as resolved.

Sign in to view

sarabala1979 previously requested changes Jun 21, 2021

View reviewed changes

stefansedich force-pushed the pod-gc-delay branch 2 times, most recently from f35e65d to 27d5b14 Compare June 22, 2021 19:29

stefansedich force-pushed the pod-gc-delay branch from 27d5b14 to f42fc3e Compare June 22, 2021 22:55

stefansedich commented Jun 22, 2021

View reviewed changes

stefansedich requested a review from sarabala1979 June 23, 2021 16:43

terrytangyuan reviewed Jun 23, 2021

View reviewed changes

stefansedich force-pushed the pod-gc-delay branch from 8f2e152 to 611cec5 Compare June 23, 2021 19:28

feat: Add support for deletion delay when using PodGC

69728e2

Signed-off-by: Stefan Sedich <[email protected]>

stefansedich force-pushed the pod-gc-delay branch from 611cec5 to 69728e2 Compare June 23, 2021 19:28

Merge branch 'master' into pod-gc-delay

fb15065

alexec changed the title ~~feat: Add support for deletion delay when using PodGC~~ feat(controller): Add support for deletion delay when using PodGC Jun 25, 2021

alexec changed the title ~~feat(controller): Add support for deletion delay when using PodGC~~ feat(controller): Delay pod GC deletion by a configurable amount (default 5s) Jun 25, 2021

alexec enabled auto-merge (squash) June 25, 2021 17:19

alexec added this to the v3.2 milestone Jun 25, 2021

alexec added 2 commits June 25, 2021 14:01

Merge branch 'master' into pod-gc-delay

47e8880

Merge branch 'master' into pod-gc-delay

52e7c73

alexec approved these changes Jun 26, 2021

View reviewed changes

alexec merged commit 73a36d8 into argoproj:master Jun 26, 2021

sarabala1979 mentioned this pull request Jul 13, 2021

v3.1.2 cherry-pick #6330

Closed

17 tasks

uturunku1 pushed a commit to newrelic-forks/argo-workflows that referenced this pull request Jul 22, 2021

feat: Add support for deletion delay when using PodGC (argoproj#6168)

1be4e13

Signed-off-by: Stefan Sedich <[email protected]> Signed-off-by: uturunku1 <[email protected]>

alexec mentioned this pull request Dec 9, 2021

workflows stuck because controller is not terminating istio sidecar #7214

Closed

tooptoop4 mentioned this pull request Oct 1, 2022

PodGC Timeout #8539

Closed

agilgur5 added area/gc Garbage collection, such as TTLs, retentionPolicy, delays, and more area/controller Controller issues, panics labels Feb 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(controller): Delay pod GC deletion by a configurable amount (default 5s) #6168

feat(controller): Delay pod GC deletion by a configurable amount (default 5s) #6168

stefansedich commented Jun 17, 2021

sarabala1979 Jun 17, 2021

stefansedich Jun 17, 2021

This comment was marked as resolved.

sarabala1979 Jun 21, 2021

stefansedich Jun 22, 2021

alexec commented Jun 22, 2021

stefansedich commented Jun 22, 2021

alexec commented Jun 22, 2021

stefansedich commented Jun 22, 2021

stefansedich Jun 22, 2021

stefansedich Jun 22, 2021

alexec Jun 23, 2021

stefansedich Jun 23, 2021

stefansedich Jun 22, 2021

stefansedich commented Jun 22, 2021

stefansedich Jun 22, 2021

sarabala1979 Jun 23, 2021

stefansedich Jun 23, 2021

terrytangyuan left a comment

stefansedich commented Jun 23, 2021

alexec commented Jun 25, 2021

prein commented Dec 16, 2021

alexec commented Dec 16, 2021

feat(controller): Delay pod GC deletion by a configurable amount (default 5s) #6168

feat(controller): Delay pod GC deletion by a configurable amount (default 5s) #6168

Conversation

stefansedich commented Jun 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexec commented Jun 22, 2021

stefansedich commented Jun 22, 2021

alexec commented Jun 22, 2021

stefansedich commented Jun 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefansedich commented Jun 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terrytangyuan left a comment

Choose a reason for hiding this comment

stefansedich commented Jun 23, 2021

alexec commented Jun 25, 2021

prein commented Dec 16, 2021

alexec commented Dec 16, 2021