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.

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

Enable shared PID namespace by default for docker pods #45236

Merged
merged 1 commit into from
May 10, 2017

Conversation

verb
Copy link
Contributor

@verb verb commented May 2, 2017

What this PR does / why we need it: This PR enables PID namespace sharing for docker pods by default, bringing the behavior of docker in line with the other CRI runtimes when used with docker >= 1.13.1.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): ref #1615

Special notes for your reviewer: cc @dchen1107 @yujuhong

Release note:

Kubernetes now shares a single PID namespace among all containers in a pod when running with docker >= 1.13.1. This means processes can now signal processes in other containers in a pod, but it also means that the `kubectl exec {pod} kill 1` pattern will cause the pod to be restarted rather than a single container.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 2, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-ci-robot
Copy link
Contributor

Hi @verb. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 2, 2017
@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels May 2, 2017
@yujuhong yujuhong assigned dchen1107 and yujuhong and unassigned jessfraz and mwielgus May 2, 2017
@spiffxp
Copy link
Member

spiffxp commented May 2, 2017

@k8s-bot ok to test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 2, 2017
@yujuhong
Copy link
Contributor

yujuhong commented May 3, 2017

@verb, the experimental flag was just introduced 4 days ago in #41583. Why changing them now?

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 3, 2017
@verb
Copy link
Contributor Author

verb commented May 3, 2017

@yujuhong the plan in the original proposal was to change the default behavior but provide an "escape hatch". When it became clear that Docker 1.13.1 would not be qualified for 1.6, @dchen1107 asked to make it experimental opt-in.

That CL (#41583) didn't merge in time for 1.6. Since it already had an LGTM I decided to just let it merge (I didn't realize at the time I was going to have to get additional LGTMs because of rebasing) and follow up with a CL to change the default behavior for 1.7.

Anyway, I'm just implementing the initial proposal, but I'm happy to revisit the rollout plan.

@verb verb force-pushed the sharedpid-2-default branch from 9614032 to b668371 Compare May 3, 2017 17:24
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 3, 2017
@verb
Copy link
Contributor Author

verb commented May 5, 2017

Update ftr: @yujuhong, @dchen1107 and I met offline and agreed to make this a default for 1.7, so this PR is just waiting for review.

@yujuhong
Copy link
Contributor

yujuhong commented May 9, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 9, 2017
@verb
Copy link
Contributor Author

verb commented May 9, 2017

@k8s-bot kops aws e2e test this
@k8s-bot gce etcd3 e2e test this

@dchen1107
Copy link
Member

/lgtm

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dchen1107, verb, yujuhong

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 10, 2017
@verb
Copy link
Contributor Author

verb commented May 10, 2017

@k8s-bot kops aws e2e test this

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@krmayankk
Copy link

@verb why was it experimental earlier and what makes it non experimental now. is there more documentation on how to use it ? Do we need to enable in kubelet earlier ?

@verb
Copy link
Contributor Author

verb commented Jul 20, 2017

@krmayankk lack of defined process. It is automatically enabled for supported docker versions, >= 1.13.1. Note that 1.13.1 has not been qualified for use with Kubernetes yet. See kubernetes/community#207 for additional details of the rollout.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants