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

Set pids limit at pod level #57973

Merged
merged 2 commits into from Jan 26, 2018

Conversation

@dims
Member

dims commented Jan 8, 2018

What this PR does / why we need it:

Add a new Alpha Feature to set a maximum number of pids per Pod.
This is to allow the use case where cluster administrators wish
to limit the pids consumed per pod (example when running a CI system).

By default, we do not set any maximum limit, If an administrator wants
to enable this, they should enable SupportPodPidsLimit=true in the
--feature-gates= parameter to kubelet and specify the limit using the
--pod-max-pids parameter.

The limit set is the total count of all processes running in all
containers in the pod.

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

Special notes for your reviewer:

Release note:

New alpha feature to limit the number of processes running in a pod. Cluster administrators will be able to place limits by using the new kubelet command line parameter --pod-max-pids. Note that since this is a alpha feature they will need to enable the "SupportPodPidsLimit" feature.
@dims

This comment has been minimized.

Member

dims commented Jan 8, 2018

/assign @tallclair
/assign @thockin
/assign @dchen1107

cc @BenTheElder

@dims

This comment has been minimized.

Member

dims commented Jan 8, 2018

/test pull-kubernetes-unit

@dims

This comment has been minimized.

Member

dims commented Jan 8, 2018

/test pull-kubernetes-cross

@tallclair

Thanks, looks much cleaner.

@@ -214,6 +214,12 @@ const (
//
// Implement IPVS-based in-cluster service load balancing
SupportIPVSProxyMode utilfeature.Feature = "SupportIPVSProxyMode"
// owner: @dims
// beta: v1.10

This comment has been minimized.

@tallclair

This comment has been minimized.

@dims

dims Jan 8, 2018

Member

Duh, cut-n-paste error :) fixing

@@ -197,6 +197,8 @@ type KubeletConfiguration struct {
// The CIDR to use for pod IP addresses, only used in standalone mode.
// In cluster mode, this is obtained from the master.
PodCIDR string `json:"podCIDR"`
// PodPidsLimit is the maximum number of pids in any pod.
PodPidsLimit int64

This comment has been minimized.

@tallclair

tallclair Jan 8, 2018

Member

I think this should be a pointer, and also it needs a json tag.

This comment has been minimized.

@dims

dims Jan 8, 2018

Member

done

@@ -463,6 +470,10 @@ func (m *cgroupManagerImpl) Create(cgroupConfig *CgroupConfig) error {
Resources: resources,
}
if cgroupConfig.ResourceParameters.PodPidsLimit != nil {

This comment has been minimized.

@tallclair

tallclair Jan 8, 2018

Member

I think you should check the featuregate here?

This comment has been minimized.

@dims

dims Jan 8, 2018

Member

done

@@ -430,6 +433,10 @@ func (m *cgroupManagerImpl) Update(cgroupConfig *CgroupConfig) error {
Paths: cgroupPaths,
}
if cgroupConfig.ResourceParameters.PodPidsLimit != nil {

This comment has been minimized.

@tallclair

tallclair Jan 8, 2018

Member

Check the feature gate here?

This comment has been minimized.

@dims

dims Jan 8, 2018

Member

done

@dims

This comment has been minimized.

Member

dims commented Jan 9, 2018

/assign @brendandburns

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Jan 9, 2018

@dims: GitHub didn't allow me to assign the following users: brendanburns.

Note that only kubernetes members can be assigned.

In response to this:

/assign @brendanburns

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.

@derekwaynecarr

This comment has been minimized.

Member

derekwaynecarr commented Jan 9, 2018

/cc @sjenning

I did a quick pass and this looks generally good. Will take a deeper look shortly.

@k8s-ci-robot k8s-ci-robot requested a review from sjenning Jan 9, 2018

@dchen1107

This comment has been minimized.

Member

dchen1107 commented Jan 9, 2018

Looks good to me, thanks.

cc/ @mtaufen

@yujuhong yujuhong self-assigned this Jan 9, 2018

@yujuhong

This comment has been minimized.

Contributor

yujuhong commented Jan 9, 2018

Would like to take a look of this later.

@dims

This comment has been minimized.

Member

dims commented Jan 11, 2018

@yujuhong @mtaufen @sjenning can you please take a look when you get a chance?

cc @kubernetes/sig-node-pr-reviews

@@ -453,6 +453,8 @@ func AddKubeletConfigFlags(fs *pflag.FlagSet, c *kubeletconfig.KubeletConfigurat
fs.Int32Var(&c.MaxPods, "max-pods", c.MaxPods, "Number of Pods that can run on this Kubelet.")
fs.StringVar(&c.PodCIDR, "pod-cidr", c.PodCIDR, "The CIDR to use for pod IP addresses, only used in standalone mode. In cluster mode, this is obtained from the master.")
fs.Int64Var(c.PodPidsLimit, "pod-max-pids", *c.PodPidsLimit, "<Warning: Alpha feature> Tune a pod's pids limit. Set -1 for unlimited.")

This comment has been minimized.

@sjenning

sjenning Jan 11, 2018

Contributor

nit reword: "Set the maximum number of processes per pod"

This comment has been minimized.

@dims

dims Jan 11, 2018

Member

Done!

@sjenning

This comment has been minimized.

Contributor

sjenning commented Jan 11, 2018

/lgtm

@dims

This comment has been minimized.

Member

dims commented Jan 12, 2018

@sjenning i've fixed the nit. can you please peek and reapply lgtm?

@yujuhong

Code looks good overall.

I have some concerns about setting the limit. Without the node-level limit, the per-pod limit might need to be chosen carefully to prevent the node from be brought down, which I believe is the main issue this PR is trying to address.

@@ -478,6 +478,8 @@ func AddKubeletConfigFlags(fs *pflag.FlagSet, c *kubeletconfig.KubeletConfigurat
fs.Int32Var(&c.MaxPods, "max-pods", c.MaxPods, "Number of Pods that can run on this Kubelet.")
fs.StringVar(&c.PodCIDR, "pod-cidr", c.PodCIDR, "The CIDR to use for pod IP addresses, only used in standalone mode. In cluster mode, this is obtained from the master.")
fs.Int64Var(c.PodPidsLimit, "pod-max-pids", *c.PodPidsLimit, "<Warning: Alpha feature> Set the maximum number of processes per pod. Set -1 for unlimited.")

This comment has been minimized.

@yujuhong

yujuhong Jan 12, 2018

Contributor

What about 0?

This comment has been minimized.

@dims

dims Jan 12, 2018

Member

i should probably drop Set -1 for unlimited. as we only use this if podPidsLimit > 0

This comment has been minimized.

@dims
Set pids limit at pod level
Add a new Alpha Feature to set a maximum number of pids per Pod.
This is to allow the use case where cluster administrators wish
to limit the pids consumed per pod (example when running a CI system).

By default, we do not set any maximum limit, If an administrator wants
to enable this, they should enable `SupportPodPidsLimit=true` in the
`--feature-gates=` parameter to kubelet and specify the limit using the
`--pod-max-pids` parameter.

The limit set is the total count of all processes running in all
containers in the pod.
@derekwaynecarr

This comment has been minimized.

Member

derekwaynecarr commented Jan 22, 2018

@dims -- can you open a feature in the features repo so we can track this work over its life-time?

I am fine with the code as-is for its first iteration, but per the other feedback, we should get a design started for long-term evolution of this feature in 1.11+.

/approve
/lgtm

@dims

This comment has been minimized.

Member

dims commented Jan 22, 2018

@derekwaynecarr - yep will file one today (at least a WIP). Thanks!

@dims

This comment has been minimized.

Member

dims commented Jan 23, 2018

@derekwaynecarr please see kubernetes/enhancements#546

@brendandburns @dchen1107 @lavalamp @smarterclayton @thockin - Need your approval per list in pkg/OWNERS. PTAL

@dims

This comment has been minimized.

Member

dims commented Jan 23, 2018

/test all

@BenTheElder

This comment has been minimized.

Member

BenTheElder commented Jan 23, 2018

@dims

This comment has been minimized.

Member

dims commented Jan 24, 2018

/test pull-kubernetes-e2e-kubeadm-gce-canary

@dims

This comment has been minimized.

Member

dims commented Jan 24, 2018

Please ignore "pull-kubernetes-e2e-kubeadm-gce-canary" failure. It's not a required job and there's a race between the build artifacts and when the job starts actually. So totally unrelated.

/test all

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Jan 24, 2018

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

Test name Commit Details Rerun command
pull-kubernetes-e2e-kubeadm-gce-canary 3df1ce5 link /test pull-kubernetes-e2e-kubeadm-gce-canary

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.

@dims

This comment has been minimized.

Member

dims commented Jan 25, 2018

@brendandburns @dchen1107 @lavalamp @smarterclayton @thockin - can one of you please approve for "pkg/" thanks!

Please ignore the gce-canary failure, it's not a required test and failing for an unrelated known reason (job depends on output of bazel-build and there's a problem with the stored artifact url)

@smarterclayton

This comment has been minimized.

Contributor

smarterclayton commented Jan 26, 2018

/approve

Given other feedback and sig consensus, no voiced concerns, benefit for real users with the possibility of a more comprehensive user facing solution later on.

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Jan 26, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, dims, sjenning, smarterclayton

Associated issue: #43783

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

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

@dims

This comment has been minimized.

Member

dims commented Jan 26, 2018

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Jan 26, 2018

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

@k8s-merge-robot k8s-merge-robot merged commit bf11116 into kubernetes:master Jan 26, 2018

13 of 14 checks passed

pull-kubernetes-e2e-kubeadm-gce-canary Job failed.
Details
Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation dims authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke-gci Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@BenTheElder BenTheElder referenced this pull request Jan 26, 2018

Open

Use PID limits #6462

@tallclair tallclair referenced this pull request Feb 13, 2018

Merged

Graduate kubeletconfig API group to beta #53833

33 of 33 tasks complete

@tengqm tengqm referenced this pull request Feb 13, 2018

Open

Document the SupportPodPidsLimit feature gate #7386

1 of 2 tasks complete

Youkoulayley pushed a commit to Youkoulayley/acs-engine that referenced this pull request Apr 20, 2018

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