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

Promote PidLimits to GA #94140

Merged
merged 1 commit into from Sep 9, 2020
Merged

Conversation

derekwaynecarr
Copy link
Member

What type of PR is this?
/kind feature

What this PR does / why we need it:
Promotes pid limiting features to GA after being enabled and on-by-default for a year.

Special notes for your reviewer:
There have been no known issues with the feature, and its been enabled for over a year by default.

Does this PR introduce a user-facing change?:

Promote SupportNodePidsLimit to GA to provide node to pod pid isolation
Promote SupportPodPidsLimit to GA to provide ability to limit pids per pod

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/kubernetes/enhancements/pull/1952

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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 Aug 20, 2020
@k8s-ci-robot k8s-ci-robot added area/test sig/node Categorizes an issue or PR as relevant to SIG Node. approved Indicates a PR has been approved by an approver from all required OWNERS files. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Aug 20, 2020
@derekwaynecarr
Copy link
Member Author

/assign @dashpole @dchen1107 @sjenning @liggitt

@dashpole
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 20, 2020
@dashpole
Copy link
Contributor

/kind feature
/priority important-longterm

@k8s-ci-robot k8s-ci-robot added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Aug 20, 2020
@liggitt
Copy link
Member

liggitt commented Aug 21, 2020

do the feature tags need to be removed from node tests like SIGDescribe("PodPidsLimit [Serial] [Feature:SupportPodPidsLimit][NodeFeature:SupportPodPidsLimit]" now that this is unconditionally enabled?

should the pids cgroup be moved out of the CgroupsOptional / CgroupsV2Optional now that these features are always enabled?

	Cgroups: []string{"cpu", "cpuacct", "cpuset", "devices", "freezer", "memory"},
	CgroupsOptional: []string{
		// The hugetlb cgroup is optional since some kernels are compiled without support for huge pages
		// and therefore lacks corresponding hugetlb cgroup
		"hugetlb",
		// The pids cgroup is optional since it is only used when at least one of the feature flags "SupportPodPidsLimit" and
		// "SupportNodePidsLimit" is enabled
		"pids",
	},
	CgroupsV2:         []string{"cpu", "cpuset", "devices", "freezer", "memory"},
	CgroupsV2Optional: []string{"hugetlb", "pids"},

@derekwaynecarr
Copy link
Member Author

thanks @liggitt , i was not sure if we removed the internal references to the gate in the subsequent release. will update the pr.

@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 24, 2020
@SergeyKanzhelev
Copy link
Member

follow up: kubernetes/system-validators#18

system-validators are in vendor folder. I don't think there is a rush to update it though

@liggitt liggitt added this to the v1.20-phase-feature milestone Aug 28, 2020
@SergeyKanzhelev
Copy link
Member

/kind feature

@SergeyKanzhelev
Copy link
Member

/remove needs-kind

@dims
Copy link
Member

dims commented Sep 9, 2020

/kind feature
/approve
/lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

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

@nikhita
Copy link
Member

nikhita commented Sep 9, 2020

removing and re-adding kind/feature to make the bot remove needs-kind

/remove-kind feature

@k8s-ci-robot k8s-ci-robot removed the kind/feature Categorizes issue or PR as related to a new feature. label Sep 9, 2020
@nikhita
Copy link
Member

nikhita commented Sep 9, 2020

/kind feature

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Sep 9, 2020
@neolit123
Copy link
Member

/retest

GCR pull flake in pull-kubernetes-conformance-kind-ga-only-parallel.

@k8s-ci-robot k8s-ci-robot merged commit 293a53f into kubernetes:master Sep 9, 2020
@tengqm
Copy link
Contributor

tengqm commented Sep 10, 2020

This needs docs.

@SergeyKanzhelev
Copy link
Member

This needs docs.

There is a little bit of docs:

https://kubernetes.io/docs/reference/command-line-tools-reference/kubelet/#options:~:text=%2D%2Dpod%2Dmax%2Dpids%20int

And config doc page points to here:

// PodPidsLimit is the maximum number of pids in any pod.
// Requires the SupportPodPidsLimit feature gate to be enabled.
// Dynamic Kubelet Config (beta): If dynamically updating this field, consider that
// lowering it may prevent container processes from forking after the change.
// Default: -1
// +optional
PodPidsLimit *int64 `json:"podPidsLimit,omitempty"`

What kind of docs do you have in mind?

@sftim
Copy link
Contributor

sftim commented Sep 15, 2020

@SergeyKanzhelev I suggest:
add a new page inside https://kubernetes.io/docs/concepts/policy/ about limiting PID use for pods and nodes, and have https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/ link to that new page.

@SergeyKanzhelev
Copy link
Member

@SergeyKanzhelev I suggest:
add a new page inside https://kubernetes.io/docs/concepts/policy/ about limiting PID use for pods and nodes, and have https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/ link to that new page.

@sftim PTAL kubernetes/website#23929

@zoujili
Copy link

zoujili commented Mar 11, 2021

@derekwaynecarr

My os version is Linux h07c09304.sqa.eu95 3.10.0-327.ali2008.alios7.x86_64
kublet met error as below:

Failed to start ContainerManager failed to initialize top level QOS containers: failed to update top level Burstable QOS cgroup : failed to set supported cgroup subsystems for cgroup kubepods burstable]: failed to find subsystem mount for required subsystem: pids
what can i do for this without upgrade the kernel?

thanks for reply

@SergeyKanzhelev
Copy link
Member

@zoujili I suggest to file a separate issue instead of commenting in the merged PR.

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 area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. 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. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet