Skip to content

Use shareable IPC for sandbox container#70826

Merged
k8s-ci-robot merged 1 commit intokubernetes:masterfrom
kolyshkin:shareable-ipc-sandbox
Jan 1, 2019
Merged

Use shareable IPC for sandbox container#70826
k8s-ci-robot merged 1 commit intokubernetes:masterfrom
kolyshkin:shareable-ipc-sandbox

Conversation

@kolyshkin
Copy link
Copy Markdown
Contributor

@kolyshkin kolyshkin commented Nov 8, 2018

Currently, Docker make IPC of every container shareable by default,
which means other containers can join it's IPC namespace. This is
implemented by creating a tmpfs mount on the host, and then
bind-mounting it to a container's /dev/shm. Other containers
that want to share the same IPC (and the same /dev/shm) can also
bind-mount the very same host's mount.

Now, since moby/moby@7120976d7
(moby/moby#34087) there is a possiblity
to have per-daemon default of having "private" IPC mode,
meaning all the containers created will have non-shareable
/dev/shm.

For shared IPC to work in the above scenario, we need to
explicitly make the "pause" container's IPC mode as "shareable",
which is what this commit does.

To test: add "default-ipc-mode: private" to /etc/docker/daemon.json,
try using kube as usual, there should be no errors.

Fix inability to use k8s with dockerd having default IPC mode set to private.

/kind bug

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 8, 2018
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

Hi @kolyshkin. 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 /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.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

@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 Nov 8, 2018
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


Details

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 cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Nov 8, 2018
@k8s-ci-robot k8s-ci-robot requested review from mtaufen and vishh November 8, 2018 22:32
@k8s-ci-robot k8s-ci-robot added area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Nov 8, 2018
@kolyshkin
Copy link
Copy Markdown
Contributor Author

/check-cla

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Nov 8, 2018
@ddebroy
Copy link
Copy Markdown
Contributor

ddebroy commented Nov 9, 2018

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 9, 2018
@ddebroy
Copy link
Copy Markdown
Contributor

ddebroy commented Nov 9, 2018

/assign @vishh

@kolyshkin
Copy link
Copy Markdown
Contributor Author

CI failure:

W1109 02:55:20.724] 2018/11/09 02:55:20 process.go:153: Running: kubectl get nodes -ojson
W1109 02:55:21.148] Unable to connect to the server: dial tcp: lookup api.e2e-113032-dba53.test-cncf-aws.k8s.io on 10.63.240.10:53: no such host
W1109 02:55:21.151] 2018/11/09 02:55:21 process.go:155: Step 'kubectl get nodes -ojson' finished in 524.399862ms
etc

looks more like a glitch in the test env (although I'm not an expert here)

@kolyshkin
Copy link
Copy Markdown
Contributor Author

@vishh PTAL

@kolyshkin
Copy link
Copy Markdown
Contributor Author

/retest

Currently, Docker make IPC of every container shareable by default,
which means other containers can join it's IPC namespace. This is
implemented by creating a tmpfs mount on the host, and then
bind-mounting it to a container's /dev/shm. Other containers
that want to share the same IPC (and the same /dev/shm) can also
bind-mount the very same host's mount.

Now, since moby/moby@7120976d7
(moby/moby#34087) there is a possiblity
to have per-daemon default of having "private" IPC mode,
meaning all the containers created will have non-shareable
/dev/shm.

For shared IPC to work in the above scenario, we need to
explicitly make the "pause" container's IPC mode as "shareable",
which is what this commit does.

To test: add "default-ipc-mode: private" to /etc/docker/daemon.json,
try using kube as usual, there should be no errors.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin kolyshkin force-pushed the shareable-ipc-sandbox branch from 17ae115 to 1dca64f Compare November 28, 2018 19:11
@kolyshkin
Copy link
Copy Markdown
Contributor Author

rebased to master HEAD (clean rebase)

@kolyshkin
Copy link
Copy Markdown
Contributor Author

apparently CI failure was caused by the broken master -- rebase has fixed this.

@kolyshkin
Copy link
Copy Markdown
Contributor Author

How do we move this forward? Anything I can clarify/fix/address? @vishh @ddebroy

@kad
Copy link
Copy Markdown
Member

kad commented Dec 13, 2018

What would help with older versions of the docker? Maybe there is need for some docker version check?

@mtaufen
Copy link
Copy Markdown
Contributor

mtaufen commented Dec 14, 2018

/assign @yujuhong

@yujuhong
Copy link
Copy Markdown
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 15, 2018
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kolyshkin, yujuhong

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 15, 2018
@ash2k
Copy link
Copy Markdown
Member

ash2k commented Jan 1, 2019

/test pull-kubernetes-godeps

@k8s-ci-robot k8s-ci-robot merged commit e76322e into kubernetes:master Jan 1, 2019
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. area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/node Categorizes an issue or PR as relevant to SIG Node. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants