share pid namespace for Pod container #1149

weiwei04 · 2017-11-13T07:07:03Z

Signed-off-by: Wei Wei weiwei.inf@gmail.com

- What I did

enable share pid namespace for containers in a pod, and add a --disable-share-pid-namespace flag to disable this feature

- How I did it

vim

- How to verify it

start crio with/without --disable-shared-pid-namespace

- Description for the changelog

enable share pid namespace for containers in a pod and add a --disable-share-pid-namespace flag to disable this feature

closes #1135
closes #488

openshift-ci-robot · 2017-11-13T07:07:10Z

Hi @weiwei04. Thanks for your PR.

I'm waiting for a openshift 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.

I understand the commands that are listed here.

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.

weiwei04 · 2017-11-13T08:10:05Z

@rhatdan @runcom please take a look, thanks :)

rhatdan · 2017-11-13T11:36:09Z

/ok-to-test

rhatdan · 2017-11-13T11:36:54Z

cmd/crio/config.go

@@ -115,6 +115,9 @@ default_mounts = [
 # pids_limit is the number of processes allowed in a container
 pids_limit = {{ .PidsLimit }}

+# using a shared PID namespace for containers in a pod


default to using shared PID Namespace for containers in a pod

rhatdan · 2017-11-13T11:37:50Z

cmd/crio/main.go

@@ -296,6 +299,10 @@ func main() {
 			Value: libkpod.DefaultPidsLimit,
 			Usage: "maximum number of processes allowed in a container",
 		},
+		cli.BoolFlag{
+			Name:  "share-pid-namespace",
+			Usage: "using a shared PID namespace for containers in a pod",


default to use a spared PID Namespace for containes in a pod (Default is false)

Do we want the default to be true?

rhatdan · 2017-11-13T11:38:43Z

docs/crio.8.md

@@ -118,6 +118,9 @@ set the CPU profile file path
 **--pids-limit**=""
  Maximum number of processes allowed in a container (default: 1024)

+**--share-pid-namespace**=""
+  Using a shared PID namespace for containers in a pod (default: false)


Default to use ashared PID namespace for containers in a pod (default: false)

rhatdan · 2017-11-13T11:39:17Z

docs/crio.conf.5.md

@@ -87,6 +87,9 @@ Example:
 **pids_limit**=""
  Maximum number of processes allowed in a container (default: 1024)

+**share_pid_namespace**=""
+  Using a shared PID namespace for containers in a pod (default: false)


Default to use a shared PID namespace for containers in a pod (default: false)

rhatdan · 2017-11-13T11:39:48Z

libkpod/config.go

@@ -121,6 +121,9 @@ type RuntimeConfig struct {
 	// NoPivot instructs the runtime to not use `pivot_root`, but instead use `MS_MOVE`
 	NoPivot bool `toml:"no_pivot"`

+	// SharePidNamespace instructs the runtime to share pid namespace


// SharePidNamespace instructs the runtime to share pid namespace by default

rhatdan · 2017-11-13T11:41:20Z

Thanks this looks pretty good, except for some nits on docs.
Could you add a test.

rhatdan · 2017-11-13T11:44:07Z

Biggest question for @mrunalp and @runcom is should we default this to True.

runcom · 2017-11-13T12:03:06Z

Biggest question for @mrunalp and @runcom is should we default this to True.

I think so, docker in kube already share the pid ns kubernetes/kubernetes#45236
but the first thing it comes to mind is openshift builds, they use the pid of the cri-o container to get the network namespace afaict and this could break that.

This is related to #488 and that issue talks about an init container as well

mrunalp · 2017-11-13T15:49:40Z

There was some discussion around this being passed over the CRI. That hasn't happened yet and so I think that we should probably default to False instead till that CRI field is added.

weiwei04 · 2017-11-14T09:20:27Z

changed from --share-pid-namespace to --disable-pid-namespace(like kubelet --docker-disable-shared-pid flag) add a e2e test, please take a look @rhatdan @runcom @mrunalp thanks :)

Some TODOs:

For unit test, I'd like to open another pr to split createSandboxContainer into smaller functions and add more unit test. (Also glad to add more unit tests in other file/function, If you have some important files/functions want to add more unit test in mind)
When I add the e2e, make validate are enforcing deprecate crioctl in favor of crictl, so I add a separate namespaces.bats file for share pid namespace e2e. Are we planning to replace crioctl in *.bats with crictl, if so I and my colleague would like to help.

rhatdan · 2017-11-14T15:05:24Z

Yes we are planning on moving from crioctl to crictl.
This PR Needs a Rebase.
I am fine with merging this once we pass tests.
@weiwei04 Tests sound great, and help on moving from crioctl to crictl, would be great.

runcom · 2017-11-14T15:06:55Z

Are we planning to replace crioctl in *.bats with crictl, if so I and my colleague would like to help.

❤️

weiwei04 · 2017-11-15T01:12:20Z

Rebased @rhatdan please take a look :)

TomSweeneyRedHat

(moved comment to correct line).

TomSweeneyRedHat

Otherwise the code LGTM.

TomSweeneyRedHat · 2017-11-15T12:14:53Z

cmd/crio/config.go

@@ -115,6 +115,9 @@ default_mounts = [
 # pids_limit is the number of processes allowed in a container
 pids_limit = {{ .PidsLimit }}

+# disable using a shared PID namespace for containers in a pod
+disable_share_pid_namespace = {{ .DisableSharePIDNamespace }}


Shoulda spotted this yesterday, but I think this variable and especially the customer facing parameter should be
disable_shared_pid_namespace (added 'd' to share). I'd make all of the variables 'shared' instead of "share".

I agree change it to shared.

weiwei04 · 2017-11-15T16:41:19Z

@TomSweeneyRedHat changed to disable-shared-pid-namespace, please take a look, thanks! :)

TomSweeneyRedHat · 2017-11-15T17:23:00Z

@weiwei04 LGTM, thanks for the touch up. Going to kick off the tests as you've initial green happiness.

TomSweeneyRedHat · 2017-11-15T17:23:09Z

/test all

mrunalp · 2017-11-16T01:04:04Z

/test e2e_fedora

weiwei04 · 2017-11-16T15:40:18Z

Ping @rhatdan for merge :)

mrunalp · 2017-11-16T17:30:23Z

test/namespaces.bats

+@test "pod disable shared pid namespace" {
+	export DISABLE_SHARED_PID_NAMESPACE="true"
+
+	start_crio


You can just do

DISABLE_SHARED_PID_NAMESPACE="true" start_crio

There is no need to export or add in cleanup then.

Signed-off-by: Wei Wei <weiwei.inf@gmail.com>

mrunalp · 2017-11-17T15:55:26Z

/test all

mrunalp · 2017-11-17T18:12:46Z

LGTM

rhatdan · 2017-11-17T19:52:17Z

Should this be backported into the v1.8 branch?

weiwei04 requested review from mrunalp and runcom as code owners November 13, 2017 07:07

openshift-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 13, 2017

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes labels Nov 13, 2017

weiwei04 force-pushed the share_pid_namespace branch from 384ca40 to 10c1796 Compare November 13, 2017 07:17

openshift-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 13, 2017

rhatdan reviewed Nov 13, 2017

View reviewed changes

weiwei04 force-pushed the share_pid_namespace branch from 10c1796 to 3e9f4dc Compare November 14, 2017 06:21

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 14, 2017

weiwei04 force-pushed the share_pid_namespace branch 2 times, most recently from 4a253bd to 80285f6 Compare November 14, 2017 07:01

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 14, 2017

weiwei04 force-pushed the share_pid_namespace branch from 80285f6 to 7bf8bb8 Compare November 14, 2017 07:49

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 14, 2017

weiwei04 force-pushed the share_pid_namespace branch 2 times, most recently from 5367b83 to e91916b Compare November 14, 2017 08:27

weiwei04 force-pushed the share_pid_namespace branch from e91916b to 29a5bae Compare November 14, 2017 15:44

TomSweeneyRedHat reviewed Nov 15, 2017

View reviewed changes

weiwei04 force-pushed the share_pid_namespace branch 2 times, most recently from fab79f3 to 9468bdf Compare November 15, 2017 16:00

mrunalp reviewed Nov 16, 2017

View reviewed changes

share pid namespace for Pod container

702ab3e

Signed-off-by: Wei Wei <weiwei.inf@gmail.com>

weiwei04 force-pushed the share_pid_namespace branch from 9468bdf to 702ab3e Compare November 17, 2017 01:56

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 17, 2017

mrunalp merged commit d68da89 into cri-o:master Nov 17, 2017

bergwolf mentioned this pull request Nov 28, 2017

grpc: specify if shared pidns is needed in CreateSandbox kata-containers/agent#12

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

share pid namespace for Pod container #1149

share pid namespace for Pod container #1149

weiwei04 commented Nov 13, 2017 •

edited

openshift-ci-robot commented Nov 13, 2017

weiwei04 commented Nov 13, 2017

rhatdan commented Nov 13, 2017

rhatdan Nov 13, 2017

rhatdan Nov 13, 2017

rhatdan Nov 13, 2017

rhatdan Nov 13, 2017

rhatdan Nov 13, 2017

rhatdan Nov 13, 2017

rhatdan commented Nov 13, 2017

rhatdan commented Nov 13, 2017

runcom commented Nov 13, 2017

mrunalp commented Nov 13, 2017

weiwei04 commented Nov 14, 2017

rhatdan commented Nov 14, 2017

runcom commented Nov 14, 2017

weiwei04 commented Nov 15, 2017

TomSweeneyRedHat left a comment •

edited

TomSweeneyRedHat left a comment

TomSweeneyRedHat Nov 15, 2017

rhatdan Nov 15, 2017

weiwei04 commented Nov 15, 2017

TomSweeneyRedHat commented Nov 15, 2017

TomSweeneyRedHat commented Nov 15, 2017

mrunalp commented Nov 16, 2017

weiwei04 commented Nov 16, 2017

mrunalp Nov 16, 2017

mrunalp commented Nov 17, 2017

mrunalp commented Nov 17, 2017

rhatdan commented Nov 17, 2017

share pid namespace for Pod container #1149

share pid namespace for Pod container #1149

Conversation

weiwei04 commented Nov 13, 2017 • edited

openshift-ci-robot commented Nov 13, 2017

weiwei04 commented Nov 13, 2017

rhatdan commented Nov 13, 2017

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

Choose a reason for hiding this comment

rhatdan commented Nov 13, 2017

rhatdan commented Nov 13, 2017

runcom commented Nov 13, 2017

mrunalp commented Nov 13, 2017

weiwei04 commented Nov 14, 2017

rhatdan commented Nov 14, 2017

runcom commented Nov 14, 2017

weiwei04 commented Nov 15, 2017

TomSweeneyRedHat left a comment • edited

Choose a reason for hiding this comment

TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weiwei04 commented Nov 15, 2017

TomSweeneyRedHat commented Nov 15, 2017

TomSweeneyRedHat commented Nov 15, 2017

mrunalp commented Nov 16, 2017

weiwei04 commented Nov 16, 2017

Choose a reason for hiding this comment

mrunalp commented Nov 17, 2017

mrunalp commented Nov 17, 2017

rhatdan commented Nov 17, 2017

weiwei04 commented Nov 13, 2017 •

edited

TomSweeneyRedHat left a comment •

edited