Add ShareProcessNamespace to PodSpec #58716

verb · 2018-01-23T21:29:05Z

What this PR does / why we need it: This adds an option to the Kubernetes API to configure sharing a process (PID) namespace between the containers in a pod, as described in the shared pid proposal.

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

Special notes for your reviewer:
Questions for API reviewer:

Is the documentation sufficient to describe the new option?
Does the new field better belong in the security context?
is prepending "alpha" to json/proto fields the right thing to do?

Release note:

v1.Pod now has a field to configure whether a single process namespace should be shared between all containers in a pod. This feature is in alpha preview.

verb · 2018-01-24T12:31:55Z

/retest

verb · 2018-01-26T17:37:33Z

@smarterclayton I think you have the most context/preference on this change, but if you don't have time to review let me know and I'll try to find another API reviewer. Thanks!

smarterclayton · 2018-01-27T21:01:58Z

pkg/apis/core/types.go

+	// HostPID and ShareProcessNamespace cannot both be set.
+	// Optional: Default to false.
+	// +optional
+	ShareProcessNamespace *bool


What's the argument for or against putting this into PodSecurityContext (alongside HostPID)?

If there's an argument against, should we be creating a new nested struct that contains process or namespace settings?

How much stuff should be at the top level of PodSpec, vs more properly isolated?

If we put it in PodSecurityContext, is there a reason to call this SharedPID instead of SharedProcessNamespace? Or should it be ProcessNamespace string with constant values Host, Shared, ... for future extension?

I think putting it in PodSecurityContext is reasonable, but I'm lacking context on its intended use so I wanted to discuss it with you prior to writing custom conversion to the flattened v1.PodSpec. In my mind, this isn't really a security setting, but based on community response to this feature I think many disagree.

I started with SharedPID as the name, but now I agree with your initial assessment that it isn't worth keeping a nondescriptive name just for the sake of consistency.

An enum is the approach we took in the CRI, but that API is still in alpha. It actually didn't occur to me to do something similar in the core API but keep the bools in v1, is that what you mean? That seems reasonable to me, but I haven't fully grokked how the versioned/nonversioned interfaces are used.

@smarterclayton I went ahead and moved ShareProcessNamespace to PodSecurityContext, but I'll hold off on changing field types until you get a chance to respond.

verb · 2018-01-30T10:05:01Z

Oh, I should point out that I put some questions for reviewers in the PR description.

verb · 2018-02-05T21:55:25Z

/retest

liggitt · 2018-02-08T15:03:11Z

pkg/apis/core/validation/validation_test.go

-	priorityEnabled := utilfeature.DefaultFeatureGate.Enabled("PodPriority")
-	defer func() {
-		var err error
+	defer func(podPriority, shareProcessNamespace bool) {


defer utilfeaturetesting.SetFeatureGateDuringTest(...)() will do this for you

egad, I've been wanting that for like a year. I wish I had sent you a PR sooner. Fixed.

liggitt · 2018-02-08T16:27:24Z

pkg/apis/core/validation/validation_test.go

-	err = utilfeature.DefaultFeatureGate.Set("PodPriority=false")
-	if err != nil {
-		t.Errorf("Failed to disable feature gate for PodPriority: %v", err)
+	if err := utilfeature.DefaultFeatureGate.Set("PodPriority=false,PodShareProcessNamespace=false"); err != nil {


Done. I discarded the return function rather than having a redundant defer.

liggitt · 2018-02-08T17:39:21Z

handling of alpha field and test bits LGTM. will defer to other reviewers more involved with the original proposal

yujuhong · 2018-02-08T19:17:16Z

pkg/apis/core/validation/validation_test.go

-		t.Errorf("Failed to enable feature gate for PodPriority: %v", err)
-		return
-	}
+	defer utilfeaturetesting.SetFeatureGateDuringTesting(t, utilfeature.DefaultFeatureGate, features.PodPriority, true)()


Should be SetFeatureGateDuringTest(

eeep, thanks. not sure how I managed that.

smarterclayton · 2018-02-19T21:38:12Z

pkg/apis/core/types.go

+	// HostPID and ShareProcessNamespace cannot both be set.
+	// Optional: Default to false.
+	// This field is alpha-level and is honored only by servers that enable the PodShareProcessNamespace feature.
+	// +k8s:conversion-gen=false


Why do you have conversion-gen=false on here? Just because the predecessors have it?

I thought it would be necessary because the Spec.ShareProcessNamespace vs Spec.SecurityContext.ShareProcessNamespace path difference needed manual conversion.

smarterclayton · 2018-02-19T21:40:34Z

Other than the existing concern about how this tears down walls between containers prior to us resolving the shared pod sidecar use case, this is fine. It has legitimate value and is still alpha.

/approve

smarterclayton · 2018-02-19T21:40:44Z

One question.

dchen1107 · 2018-02-21T19:44:09Z

/test pull-kubernetes-e2e-kops-aws

dchen1107 · 2018-02-21T21:08:51Z

/lgtm

k8s-ci-robot · 2018-02-21T21:09:45Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~api/OWNERS~~ [dchen1107,smarterclayton]
~~docs/api-reference/OWNERS~~ [dchen1107,smarterclayton]
~~pkg/api/OWNERS~~ [dchen1107,smarterclayton]
~~pkg/apis/core/OWNERS~~ [dchen1107,smarterclayton]
~~staging/src/k8s.io/api/OWNERS~~ [dchen1107,smarterclayton]

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

k8s-github-robot · 2018-02-21T21:52:17Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-ci-robot · 2018-02-21T21:53:02Z

@verb: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gce	`a41de1b`	link	`/test pull-kubernetes-e2e-gce`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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-github-robot · 2018-02-21T23:40:34Z

Automatic merge from submit-queue (batch tested with PRs 58716, 59977, 59316, 59884, 60117). If you want to cherry-pick this change to another branch, please follow the instructions here.

Automatic merge from submit-queue (batch tested with PRs 60148, 60022, 59125, 60068, 60154). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Adding support for per-pod process namespace sharing in kubelet **What this PR does / why we need it**: This enables process namespace sharing between containers in a pod as described in the [Shared PID Namespace](https://github.com/kubernetes/community/blob/master/contributors/design-proposals/node/pod-pid-namespace.md#container-runtime-interface-changes) proposal but leaves it disconnected pending merge of #58716. **Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*: WIP #1615 **Special notes for your reviewer**: **Release note**: ```release-note NONE ```

k8s-ci-robot requested review from derekwaynecarr, nikhiljindal, pmorie and yujuhong January 23, 2018 21:29

k8s-github-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Jan 23, 2018

yujuhong assigned yujuhong and smarterclayton Jan 23, 2018

verb force-pushed the pid branch from 26942c9 to 2b70784 Compare January 24, 2018 10:33

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 Jan 24, 2018

mrunalp mentioned this pull request Jan 24, 2018

*: Add --pid-namespace=[container|pod|pod-container] cri-o/cri-o#1280

Closed

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 26, 2018

verb force-pushed the pid branch from 2b70784 to 2ce1a42 Compare January 27, 2018 06:53

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 27, 2018

smarterclayton reviewed Jan 27, 2018

View reviewed changes

This was referenced Jan 31, 2018

Adding support for per-pod process namespace sharing in kubelet #59125

Merged

Configurable Pod Process Namespace Sharing kubernetes/enhancements#495

Closed

verb force-pushed the pid branch from 2ce1a42 to d8d030c Compare February 1, 2018 14:40

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 3, 2018

verb force-pushed the pid branch from d8d030c to 3b58498 Compare February 4, 2018 20:18

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 4, 2018

verb force-pushed the pid branch from 3b58498 to e738ef8 Compare February 5, 2018 21:12

liggitt reviewed Feb 8, 2018

View reviewed changes

verb force-pushed the pid branch from 3b331b0 to 4c5de95 Compare February 8, 2018 15:37

liggitt reviewed Feb 8, 2018

View reviewed changes

verb force-pushed the pid branch from 4c5de95 to 6e3420b Compare February 8, 2018 17:34

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 8, 2018

verb force-pushed the pid branch from 6e3420b to e7aa8ac Compare February 8, 2018 17:59

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 8, 2018

yujuhong reviewed Feb 8, 2018

View reviewed changes

verb force-pushed the pid branch from e7aa8ac to 5930b9f Compare February 8, 2018 19:34

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 13, 2018

verb force-pushed the pid branch from 5930b9f to dd5284d Compare February 15, 2018 14:11

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 15, 2018

smarterclayton reviewed Feb 19, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 19, 2018

verb added 2 commits February 21, 2018 19:02

Kubernetes API for Shared Process Namespace

2343600

Generated code for Shared Process Namespace

a41de1b

verb force-pushed the pid branch from dd5284d to a41de1b Compare February 21, 2018 18:32

k8s-ci-robot assigned dchen1107 Feb 21, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 21, 2018

k8s-github-robot merged commit 97297dc into kubernetes:master Feb 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ShareProcessNamespace to PodSpec #58716

Add ShareProcessNamespace to PodSpec #58716

verb commented Jan 23, 2018 •

edited

verb commented Jan 24, 2018

verb commented Jan 26, 2018

smarterclayton Jan 27, 2018

smarterclayton Jan 27, 2018

smarterclayton Jan 27, 2018

verb Jan 29, 2018

verb Feb 1, 2018

verb commented Jan 30, 2018

verb commented Feb 5, 2018

liggitt Feb 8, 2018 •

edited

verb Feb 8, 2018

liggitt Feb 8, 2018

verb Feb 8, 2018

liggitt commented Feb 8, 2018

yujuhong Feb 8, 2018

verb Feb 8, 2018

smarterclayton Feb 19, 2018

verb Feb 20, 2018

smarterclayton commented Feb 19, 2018

smarterclayton commented Feb 19, 2018

dchen1107 commented Feb 21, 2018

dchen1107 commented Feb 21, 2018

k8s-ci-robot commented Feb 21, 2018

k8s-github-robot commented Feb 21, 2018

k8s-ci-robot commented Feb 21, 2018 •

edited

k8s-github-robot commented Feb 21, 2018

Add ShareProcessNamespace to PodSpec #58716

Add ShareProcessNamespace to PodSpec #58716

Conversation

verb commented Jan 23, 2018 • edited

verb commented Jan 24, 2018

verb commented Jan 26, 2018

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

verb commented Jan 30, 2018

verb commented Feb 5, 2018

liggitt Feb 8, 2018 • edited

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

liggitt commented Feb 8, 2018

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

smarterclayton commented Feb 19, 2018

smarterclayton commented Feb 19, 2018

dchen1107 commented Feb 21, 2018

dchen1107 commented Feb 21, 2018

k8s-ci-robot commented Feb 21, 2018

k8s-github-robot commented Feb 21, 2018

k8s-ci-robot commented Feb 21, 2018 • edited

k8s-github-robot commented Feb 21, 2018

verb commented Jan 23, 2018 •

edited

liggitt Feb 8, 2018 •

edited

k8s-ci-robot commented Feb 21, 2018 •

edited