fix(executor): Handle sidecar killing in a process-namespace-shared pod #4575

dtaniwaki · 2020-11-20T14:33:16Z

Checklist:

My organization is added to USERS.md.

I found sending SIGTERM fails to stop a container if shareProcessNamespace: true is set with the k8sapi executor because the PID of a container isn't 1. I understand it doesn't make sense using the option without the pns executor, but my company's cluster forcibly set shareProcessNamespace: true but we haven't evaluated the pns executor yet. We should at least report the invalid option to users.

I also improved error messages during sidecar killing.

Signed-off-by: Daisuke Taniwaki <daisuketaniwaki@gmail.com>

dtaniwaki · 2020-11-20T15:42:59Z

workflow/executor/common/common.go

@@ -80,6 +80,9 @@ func TerminatePodWithContainerID(c KubernetesClientInterface, containerID string
 		log.Infof("Container %s is already terminated: %v", container.ContainerID, container.State.Terminated.String())
 		return nil
 	}
+	if pod.Spec.ShareProcessNamespace != nil && *pod.Spec.ShareProcessNamespace {
+		return fmt.Errorf("cannot terminate a process-namespace-shared Pod %s", pod.Name)


If it immediately returns an error here, the wait container exit with 0 and the pod succeeds. It's also the same in other error returns in this method too. Was it expected behavior?

Added a unit test of this method. I think we should have e2e tests for all the executors. If we had, this functional test had found the problem.

We have e2e smoke test for 3 of the executors?

Ah, you're right. The problem occurs on the combination of k8sapi and shareProcessNamespace.

Signed-off-by: Daisuke Taniwaki <daisuketaniwaki@gmail.com>

alexec · 2020-11-23T23:16:48Z

LGTM

Signed-off-by: github@finnesand.no <github@finnesand.no> feat(ui): Add Template/Cron workflow filter to workflow page. Closes argoproj#4532 (argoproj#4543) Signed-off-by: Tianchu Zhao <evantczhao@gmail.com> feat(executor): Auto create s3 bucket if not present. Signed-off-by: Alex Capras <alexcapras@gmail.com> Apply codegen Signed-off-by: Alex Capras <alexcapras@gmail.com> Add argo-e2e label to test wf Signed-off-by: Alex Capras <alexcapras@gmail.com> chore: Updated stress test YAML (argoproj#4569) Signed-off-by: Alex Collins <alex_collins@intuit.com> docs: Updated kubectl apply command in manifests README (argoproj#4577) Signed-off-by: Stefan Gloutnikov <stefan@gloutnikov.com> feat(controller): Make MAX_OPERATION_TIME configurable. Close argoproj#4239 (argoproj#4562) Signed-off-by: Alex Collins <alex_collins@intuit.com> docs: Fix a typo in example (argoproj#4590) Signed-off-by: Takayoshi Nishida <takayoshi.nishida@gmail.com> feat(controller): Retry transient offload errors. Resolves argoproj#4464 (argoproj#4482) Signed-off-by: Alex Collins <alex_collins@intuit.com> fix(server): use the correct name when downloading artifacts (argoproj#4579) Signed-off-by: Daniel Herman <dherman@factset.com> fix(server): serve artifacts directly from disk to support large artifacts (argoproj#4589) Signed-off-by: Daniel Herman <dherman@factset.com> fix(executor): Handle sidecar killing in a process-namespace-shared pod (argoproj#4575) Signed-off-by: Daisuke Taniwaki <daisuketaniwaki@gmail.com> docs: Add JSON schema for IDE validation (argoproj#4581) Signed-off-by: Paul Brabban <paul.brabban@gmail.com> refactor: Use polling model for workflow phase metric (argoproj#4557) Signed-off-by: Simon Behar <simbeh7@gmail.com> Addressing reviewers comments Signed-off-by: Alex Capras <alexcapras@gmail.com> Addressing reviewers comments docs: Minor typo fix (argoproj#4610) Signed-off-by: Paavo Pokkinen <paavo.pokkinen@vaimo.com> fix(controller): Prevent tasks with names starting with digit to use either 'depends' or 'dependencies' (argoproj#4598) Signed-off-by: terrytangyuan <terrytangyuan@gmail.com> fix(docs): Bring minio chart instructions up to date (argoproj#4586) Signed-off-by: Ranga Krishnan <ranga@bei.re> fix(executor): Fixed waitMainContainerStart returning prematurely. Closes argoproj#4599 (argoproj#4601) Signed-off-by: fsiegmund <siegmund@slb.com> feat(controller): Enhanced artifact repository ref. See argoproj#3184 (argoproj#4458) Signed-off-by: Alex Collins <alex_collins@intuit.com> fix: Null check pagination variable (argoproj#4617) Signed-off-by: Simon Behar <simbeh7@gmail.com> fix: Perform fields filtering server side (argoproj#4595) Signed-off-by: Simon Behar <simbeh7@gmail.com> fix(server): Correct webhook event payload marshalling. Fixes argoproj#4572 (argoproj#4594) Signed-off-by: Alex Collins <alex_collins@intuit.com> feat(ui): Add columns--narrower-height to AttributeRow (argoproj#4371) fix: Fix TestCleanFieldsExclude (argoproj#4625) Signed-off-by: Simon Behar <simbeh7@gmail.com> fix(argo-server): fix global variable validation error with reversed dag.tasks (argoproj#4369) Signed-off-by: chenyu.zheng <chenyu.zheng@hulu.com> fix: derive jsonschema and fix up issues, validate examples dir… (argoproj#4611) Signed-off-by: Paul Brabban <paul.brabban@gmail.com> fix(ui): Reference secrets in EnvVars. Fixes argoproj#3973 (argoproj#4419) Signed-off-by: Alejandro Tejera <aletepe@gmail.com> fix(ui): Fix Snyk issues (argoproj#4631) Signed-off-by: Alex Collins <alex_collins@intuit.com> feat(executor): More informative log when executors do not support output param from base image layer (argoproj#4620) Signed-off-by: terrytangyuan <terrytangyuan@gmail.com> Codegen patch. Signed off by alexcapras@gmail.com Codegen patch. Signed off by alexcapras@gmail.com Delete test.patch

Signed-off-by: github@finnesand.no <github@finnesand.no> feat(ui): Add Template/Cron workflow filter to workflow page. Closes argoproj#4532 (argoproj#4543) Signed-off-by: Tianchu Zhao <evantczhao@gmail.com> feat(executor): Auto create s3 bucket if not present. Signed-off-by: Alex Capras <alexcapras@gmail.com> Apply codegen Signed-off-by: Alex Capras <alexcapras@gmail.com> Add argo-e2e label to test wf Signed-off-by: Alex Capras <alexcapras@gmail.com> chore: Updated stress test YAML (argoproj#4569) Signed-off-by: Alex Collins <alex_collins@intuit.com> docs: Updated kubectl apply command in manifests README (argoproj#4577) Signed-off-by: Stefan Gloutnikov <stefan@gloutnikov.com> feat(controller): Make MAX_OPERATION_TIME configurable. Close argoproj#4239 (argoproj#4562) Signed-off-by: Alex Collins <alex_collins@intuit.com> docs: Fix a typo in example (argoproj#4590) Signed-off-by: Takayoshi Nishida <takayoshi.nishida@gmail.com> feat(controller): Retry transient offload errors. Resolves argoproj#4464 (argoproj#4482) Signed-off-by: Alex Collins <alex_collins@intuit.com> fix(server): use the correct name when downloading artifacts (argoproj#4579) Signed-off-by: Daniel Herman <dherman@factset.com> fix(server): serve artifacts directly from disk to support large artifacts (argoproj#4589) Signed-off-by: Daniel Herman <dherman@factset.com> fix(executor): Handle sidecar killing in a process-namespace-shared pod (argoproj#4575) Signed-off-by: Daisuke Taniwaki <daisuketaniwaki@gmail.com> docs: Add JSON schema for IDE validation (argoproj#4581) Signed-off-by: Paul Brabban <paul.brabban@gmail.com> refactor: Use polling model for workflow phase metric (argoproj#4557) Signed-off-by: Simon Behar <simbeh7@gmail.com> Addressing reviewers comments Signed-off-by: Alex Capras <alexcapras@gmail.com> Addressing reviewers comments docs: Minor typo fix (argoproj#4610) Signed-off-by: Paavo Pokkinen <paavo.pokkinen@vaimo.com> fix(controller): Prevent tasks with names starting with digit to use either 'depends' or 'dependencies' (argoproj#4598) Signed-off-by: terrytangyuan <terrytangyuan@gmail.com> fix(docs): Bring minio chart instructions up to date (argoproj#4586) Signed-off-by: Ranga Krishnan <ranga@bei.re> fix(executor): Fixed waitMainContainerStart returning prematurely. Closes argoproj#4599 (argoproj#4601) Signed-off-by: fsiegmund <siegmund@slb.com> feat(controller): Enhanced artifact repository ref. See argoproj#3184 (argoproj#4458) Signed-off-by: Alex Collins <alex_collins@intuit.com> fix: Null check pagination variable (argoproj#4617) Signed-off-by: Simon Behar <simbeh7@gmail.com> fix: Perform fields filtering server side (argoproj#4595) Signed-off-by: Simon Behar <simbeh7@gmail.com> fix(server): Correct webhook event payload marshalling. Fixes argoproj#4572 (argoproj#4594) Signed-off-by: Alex Collins <alex_collins@intuit.com> feat(ui): Add columns--narrower-height to AttributeRow (argoproj#4371) fix: Fix TestCleanFieldsExclude (argoproj#4625) Signed-off-by: Simon Behar <simbeh7@gmail.com> fix(argo-server): fix global variable validation error with reversed dag.tasks (argoproj#4369) Signed-off-by: chenyu.zheng <chenyu.zheng@hulu.com> fix: derive jsonschema and fix up issues, validate examples dir… (argoproj#4611) Signed-off-by: Paul Brabban <paul.brabban@gmail.com> fix(ui): Reference secrets in EnvVars. Fixes argoproj#3973 (argoproj#4419) Signed-off-by: Alejandro Tejera <aletepe@gmail.com> fix(ui): Fix Snyk issues (argoproj#4631) Signed-off-by: Alex Collins <alex_collins@intuit.com> feat(executor): More informative log when executors do not support output param from base image layer (argoproj#4620) Signed-off-by: terrytangyuan <terrytangyuan@gmail.com> Codegen patch. Signed off by alexcapras@gmail.com Codegen patch. Signed off by alexcapras@gmail.com Delete test.patch Signed-off-by: Alex Capras <alexcapras@gmail.com>

…od (#4575) Signed-off-by: Daisuke Taniwaki <daisuketaniwaki@gmail.com>

alexec · 2020-12-03T00:55:56Z

Merged into https://github.com/argoproj/argo/releases/tag/v2.12.0-rc4

dtaniwaki added 2 commits November 20, 2020 23:34

fix(executor): Handle sidecar killing in a process-namespace-shared pod

2ac86c3

Signed-off-by: Daisuke Taniwaki <daisuketaniwaki@gmail.com>

Merge branch 'master' into handle-sigterm-failure

471ff8c

dtaniwaki commented Nov 20, 2020

View reviewed changes

whynowy assigned alexec Nov 20, 2020

dtaniwaki added 2 commits November 21, 2020 12:22

Add executor commont test

7ef21b1

Signed-off-by: Daisuke Taniwaki <daisuketaniwaki@gmail.com>

Merge branch 'master' into handle-sigterm-failure

14fe1e5

alexec approved these changes Nov 23, 2020

View reviewed changes

Merge branch 'master' into handle-sigterm-failure

3b82e03

alexec added this to the v3.0 milestone Nov 23, 2020

Merge branch 'master' into handle-sigterm-failure

a5165af

alexec merged commit ef779bb into argoproj:master Nov 24, 2020

dtaniwaki deleted the handle-sigterm-failure branch November 25, 2020 04:46

alexec pushed a commit that referenced this pull request Dec 3, 2020

fix(executor): Handle sidecar killing in a process-namespace-shared p…

8e887e7

…od (#4575) Signed-off-by: Daisuke Taniwaki <daisuketaniwaki@gmail.com>

alexec modified the milestones: v3.0, v2.12 Dec 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(executor): Handle sidecar killing in a process-namespace-shared pod #4575

fix(executor): Handle sidecar killing in a process-namespace-shared pod #4575

dtaniwaki commented Nov 20, 2020 •

edited

dtaniwaki Nov 20, 2020 •

edited

alexec Nov 20, 2020

dtaniwaki Nov 21, 2020 •

edited

alexec Nov 24, 2020

dtaniwaki Nov 24, 2020

alexec commented Nov 23, 2020

alexec commented Dec 3, 2020

fix(executor): Handle sidecar killing in a process-namespace-shared pod #4575

fix(executor): Handle sidecar killing in a process-namespace-shared pod #4575

Conversation

dtaniwaki commented Nov 20, 2020 • edited

dtaniwaki Nov 20, 2020 • edited

Choose a reason for hiding this comment

alexec Nov 20, 2020

Choose a reason for hiding this comment

dtaniwaki Nov 21, 2020 • edited

Choose a reason for hiding this comment

alexec Nov 24, 2020

Choose a reason for hiding this comment

dtaniwaki Nov 24, 2020

Choose a reason for hiding this comment

alexec commented Nov 23, 2020

alexec commented Dec 3, 2020

dtaniwaki commented Nov 20, 2020 •

edited

dtaniwaki Nov 20, 2020 •

edited

dtaniwaki Nov 21, 2020 •

edited