Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 6 commits into from
Nov 24, 2020
Merged

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

merged 6 commits into from
Nov 24, 2020

Conversation

dtaniwaki
Copy link
Member

@dtaniwaki dtaniwaki commented Nov 20, 2020 •
edited
Loading

Checklist:

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.

dtaniwaki
dtaniwaki commented Nov 20, 2020
View reviewed changes
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)
Copy link
Member Author

@dtaniwaki dtaniwaki Nov 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test?

Copy link
Member Author

@dtaniwaki dtaniwaki Nov 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have e2e smoke test for 3 of the executors?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@alexec
Copy link
Contributor

alexec commented Nov 23, 2020

LGTM

@alexec alexec added this to the v3.0 milestone Nov 23, 2020
@alexec alexec merged commit ef779bb into argoproj:master Nov 24, 2020
@dtaniwaki dtaniwaki deleted the handle-sigterm-failure branch November 25, 2020 04:46
alexcapras pushed a commit to alexcapras/argo that referenced this pull request Dec 2, 2020
@secretlifeof @alexcapras
docs: Clean-up examples. Fixes argoproj#4124 (argoproj#4128)
d133355 
Signed-off-by: [email protected] <[email protected]>

feat(ui): Add Template/Cron workflow filter to workflow page. Closes argoproj#4532 (argoproj#4543)

Signed-off-by: Tianchu Zhao <[email protected]>

feat(executor): Auto create s3 bucket if not present.

Signed-off-by: Alex Capras <[email protected]>

Apply codegen

Signed-off-by: Alex Capras <[email protected]>

Add argo-e2e label to test wf

Signed-off-by: Alex Capras <[email protected]>

chore: Updated stress test YAML (argoproj#4569)

Signed-off-by: Alex Collins <[email protected]>

docs: Updated kubectl apply command in manifests README (argoproj#4577)

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

feat(controller): Make MAX_OPERATION_TIME configurable. Close argoproj#4239 (argoproj#4562)

Signed-off-by: Alex Collins <[email protected]>

docs: Fix a typo in example (argoproj#4590)

Signed-off-by: Takayoshi Nishida <[email protected]>

feat(controller): Retry transient offload errors. Resolves argoproj#4464 (argoproj#4482)

Signed-off-by: Alex Collins <[email protected]>

fix(server): use the correct name when downloading artifacts (argoproj#4579)

Signed-off-by: Daniel Herman <[email protected]>

fix(server): serve artifacts directly from disk to support large artifacts (argoproj#4589)

Signed-off-by: Daniel Herman <[email protected]>

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

Signed-off-by: Daisuke Taniwaki <[email protected]>

docs: Add JSON schema for IDE validation (argoproj#4581)

Signed-off-by: Paul Brabban <[email protected]>

refactor: Use polling model for workflow phase metric (argoproj#4557)

Signed-off-by: Simon Behar <[email protected]>

Addressing reviewers comments

Signed-off-by: Alex Capras <[email protected]>

Addressing reviewers comments

docs: Minor typo fix (argoproj#4610)

Signed-off-by: Paavo Pokkinen <[email protected]>

fix(controller): Prevent tasks with names starting with digit to use either 'depends' or 'dependencies' (argoproj#4598)

Signed-off-by: terrytangyuan <[email protected]>

fix(docs): Bring minio chart instructions up to date (argoproj#4586)

Signed-off-by: Ranga Krishnan <[email protected]>

fix(executor): Fixed waitMainContainerStart returning prematurely. Closes argoproj#4599 (argoproj#4601)

Signed-off-by: fsiegmund <[email protected]>

feat(controller): Enhanced artifact repository ref. See argoproj#3184 (argoproj#4458)

Signed-off-by: Alex Collins <[email protected]>

fix: Null check pagination variable (argoproj#4617)

Signed-off-by: Simon Behar <[email protected]>

fix: Perform fields filtering server side (argoproj#4595)

Signed-off-by: Simon Behar <[email protected]>

fix(server): Correct webhook event payload marshalling. Fixes argoproj#4572 (argoproj#4594)

Signed-off-by: Alex Collins <[email protected]>

feat(ui): Add columns--narrower-height to AttributeRow (argoproj#4371)

fix: Fix TestCleanFieldsExclude (argoproj#4625)

Signed-off-by: Simon Behar <[email protected]>

fix(argo-server): fix global variable validation error with reversed dag.tasks (argoproj#4369)

Signed-off-by: chenyu.zheng <[email protected]>

fix: derive jsonschema and fix up issues, validate examples dir… (argoproj#4611)

Signed-off-by: Paul Brabban <[email protected]>

fix(ui): Reference secrets in EnvVars. Fixes argoproj#3973  (argoproj#4419)

Signed-off-by: Alejandro Tejera <[email protected]>

fix(ui): Fix Snyk issues (argoproj#4631)

Signed-off-by: Alex Collins <[email protected]>

feat(executor): More informative log when executors do not support output param from base image layer (argoproj#4620)

Signed-off-by: terrytangyuan <[email protected]>

Codegen patch. Signed off by [email protected]

Codegen patch. Signed off by [email protected]

Delete test.patch
alexcapras pushed a commit to alexcapras/argo that referenced this pull request Dec 2, 2020
@secretlifeof @alexcapras
docs: Clean-up examples. Fixes argoproj#4124 (argoproj#4128)
1a0cebb 
Signed-off-by: [email protected] <[email protected]>

feat(ui): Add Template/Cron workflow filter to workflow page. Closes argoproj#4532 (argoproj#4543)

Signed-off-by: Tianchu Zhao <[email protected]>

feat(executor): Auto create s3 bucket if not present.

Signed-off-by: Alex Capras <[email protected]>

Apply codegen

Signed-off-by: Alex Capras <[email protected]>

Add argo-e2e label to test wf

Signed-off-by: Alex Capras <[email protected]>

chore: Updated stress test YAML (argoproj#4569)

Signed-off-by: Alex Collins <[email protected]>

docs: Updated kubectl apply command in manifests README (argoproj#4577)

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

feat(controller): Make MAX_OPERATION_TIME configurable. Close argoproj#4239 (argoproj#4562)

Signed-off-by: Alex Collins <[email protected]>

docs: Fix a typo in example (argoproj#4590)

Signed-off-by: Takayoshi Nishida <[email protected]>

feat(controller): Retry transient offload errors. Resolves argoproj#4464 (argoproj#4482)

Signed-off-by: Alex Collins <[email protected]>

fix(server): use the correct name when downloading artifacts (argoproj#4579)

Signed-off-by: Daniel Herman <[email protected]>

fix(server): serve artifacts directly from disk to support large artifacts (argoproj#4589)

Signed-off-by: Daniel Herman <[email protected]>

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

Signed-off-by: Daisuke Taniwaki <[email protected]>

docs: Add JSON schema for IDE validation (argoproj#4581)

Signed-off-by: Paul Brabban <[email protected]>

refactor: Use polling model for workflow phase metric (argoproj#4557)

Signed-off-by: Simon Behar <[email protected]>

Addressing reviewers comments

Signed-off-by: Alex Capras <[email protected]>

Addressing reviewers comments

docs: Minor typo fix (argoproj#4610)

Signed-off-by: Paavo Pokkinen <[email protected]>

fix(controller): Prevent tasks with names starting with digit to use either 'depends' or 'dependencies' (argoproj#4598)

Signed-off-by: terrytangyuan <[email protected]>

fix(docs): Bring minio chart instructions up to date (argoproj#4586)

Signed-off-by: Ranga Krishnan <[email protected]>

fix(executor): Fixed waitMainContainerStart returning prematurely. Closes argoproj#4599 (argoproj#4601)

Signed-off-by: fsiegmund <[email protected]>

feat(controller): Enhanced artifact repository ref. See argoproj#3184 (argoproj#4458)

Signed-off-by: Alex Collins <[email protected]>

fix: Null check pagination variable (argoproj#4617)

Signed-off-by: Simon Behar <[email protected]>

fix: Perform fields filtering server side (argoproj#4595)

Signed-off-by: Simon Behar <[email protected]>

fix(server): Correct webhook event payload marshalling. Fixes argoproj#4572 (argoproj#4594)

Signed-off-by: Alex Collins <[email protected]>

feat(ui): Add columns--narrower-height to AttributeRow (argoproj#4371)

fix: Fix TestCleanFieldsExclude (argoproj#4625)

Signed-off-by: Simon Behar <[email protected]>

fix(argo-server): fix global variable validation error with reversed dag.tasks (argoproj#4369)

Signed-off-by: chenyu.zheng <[email protected]>

fix: derive jsonschema and fix up issues, validate examples dir… (argoproj#4611)

Signed-off-by: Paul Brabban <[email protected]>

fix(ui): Reference secrets in EnvVars. Fixes argoproj#3973  (argoproj#4419)

Signed-off-by: Alejandro Tejera <[email protected]>

fix(ui): Fix Snyk issues (argoproj#4631)

Signed-off-by: Alex Collins <[email protected]>

feat(executor): More informative log when executors do not support output param from base image layer (argoproj#4620)

Signed-off-by: terrytangyuan <[email protected]>

Codegen patch. Signed off by [email protected]

Codegen patch. Signed off by [email protected]

Delete test.patch

Signed-off-by: Alex Capras <[email protected]>
alexec pushed a commit that referenced this pull request Dec 3, 2020
@dtaniwaki @alexec
fix(executor): Handle sidecar killing in a process-namespace-shared p…
8e887e7 
…od (#4575)

Signed-off-by: Daisuke Taniwaki <[email protected]>
@alexec alexec modified the milestones: v3.0, v2.12 Dec 3, 2020
@alexec
Copy link
Contributor

alexec commented Dec 3, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexec alexec alexec approved these changes

Assignees

@alexec alexec

Labels
None yet
Projects
None yet
Milestone
v2.12
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@dtaniwaki @alexec