fix bugs of container cpu shares when cpu request set to zero #108832

waynepeking348 · 2022-03-20T13:59:41Z

What type of PR is this?

/kind bug

What this PR does / why we need it:
For container resources, if limits are specified, but requests are not, default requests are set to limits in API server, and kubelet repeat this logic for containers running on existing Kubernetes cluster, but it works with some mistake for cpu.
Actually, if cpu request is set to zero (instead of not specified), kubelet will also consider it as equal to limits, and set cpu shares as to limit instead of 2 (minShares).
This is unexpected, think that if we have 2 containers, one with requests set as:

limits:
    cpu: "4"
  requests:
    cpu: "2"

and the other with requests set as:

limits:
    cpu: "4"
  requests:
    cpu: "0"

the cpu shares (in container level cgroup) for first one would be set as 2048 while the latter as 4096
but the expected cpu shares would be 2048 and 2 instead

Special notes for your reviewer:

I have one PR before #100986, but it's too out-of-date, so I open a new one rebased on current master

Does this PR introduce a user-facing change?:

fix relative cpu priority for pods where containers explicitly request zero cpu by giving the lowest priority instead of falling back to the cpu limit to avoid possible cpu starvation of other pods

k8s-ci-robot · 2022-03-20T13:59:50Z

Hi @waynepeking348. 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.

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.

waynepeking348 · 2022-03-20T14:02:16Z

@andrewsykim @odinuge would you please have a look at this fix? thx

yangjunmyfm192085

/ok-to-test

yangjunmyfm192085 · 2022-03-21T06:29:14Z

/triage accepted

waynepeking348 · 2022-03-21T15:18:54Z

@yangjunmyfm192085 hi~ those checks have all been passed, could you please help merge this pr?

odinuge

Thanks for creating this PR @waynepeking348!

This fixes a problem where a user could insert a "sky high" cpu limit and then explicitly set 0 as the request, and starve all other workloads!

I think we should back port this one as well, since it fixes a real problem that can cause system instability!

/priority important-longterm

Except for the minor nit;
/lgtm

I think the changelog value should be rephrased to something like (tho. my English is not my native language, so happy for others to phrase it better);

Fix relative cpu priority for pods where containers explicitly request zero cpu by giving the lowest priority instead of falling back to the cpu limit to avoid possible cpu starvation of other pods

odinuge · 2022-03-21T17:13:42Z

pkg/kubelet/kuberuntime/kuberuntime_container_linux.go

@@ -138,7 +142,7 @@ func (m *kubeGenericRuntimeManager) calculateLinuxResources(cpuRequest, cpuLimit
 	// If request is not specified, but limit is, we want request to default to limit.
 	// API server does this for new containers, but we repeat this logic in Kubelet
 	// for containers running on existing Kubernetes clusters.
-	if cpuRequest.IsZero() && !cpuLimit.IsZero() {
+	if cpuRequest == nil {


Nit; I think we should either keep the && !cpuLimit.IsZero() to ensure we are in line with the comment above, or rewrite that comment to make the "behavior" explicitly clear if someone comes and tries to rewrite it.

This change is a noop, but I think I prefer that one.

Suggested change

if cpuRequest == nil {

if cpuRequest == nil && !cpuLimit.IsZero() {

odinuge · 2022-03-21T17:27:19Z

Since you engaged on the old PR;
/cc @ehashman

waynepeking348 · 2022-03-22T03:24:56Z

/retest

waynepeking348 · 2022-03-22T04:07:09Z

@yangjunmyfm192085 @odinuge @ehashman hi～ I changed code to in line with the comment

if cpuRequest == nil && cpuLimit != nil {
	cpuShares = int64(cm.MilliCPUToShares(cpuLimit.MilliValue()))
} else {

and change log is also changed according to @odinuge suggestion

I think I need both /lgtm and /appoved label to make it merged, thx~

odinuge

/lgtm

waynepeking348 · 2022-03-23T01:40:47Z

approved

@odinuge I think I still need a /approved label to make it merged

yangjunmyfm192085

/cc @mrunalp

yangjunmyfm192085

/assign @mrunalp

waynepeking348 · 2022-04-07T05:54:48Z

/assign @mrunalp

hi, I think @mrunalp has missed this info, and I have tried to contact with him in slack, but also without reply, can anyone help to merge this pr please?
/assign @derekwaynecarr @klueska @yujuhong

waynepeking348 · 2022-04-08T02:30:08Z

/assign @derekwaynecarr @klueska @yujuhong

k8s-triage-robot · 2022-07-27T06:59:17Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

waynepeking348 · 2022-08-21T00:52:23Z

/remove-lifecycle stale

k8s-ci-robot · 2022-08-23T03:53:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mrunalp, odinuge, waynepeking348

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:

~~pkg/kubelet/OWNERS~~ [mrunalp]

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

waynepeking348 · 2022-08-23T05:39:25Z

/milestone v1.25

k8s-ci-robot · 2022-08-23T05:39:27Z

@waynepeking348: You must be a member of the kubernetes/milestone-maintainers GitHub team to set the milestone. If you believe you should be able to issue the /milestone command, please contact your Milestone Maintainers Team and have them propose you as an additional delegate for this responsibility.

In response to this:

/milestone v1.25

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.

fix bugs of container cpu shares when cpu request set to zero

4c87589

k8s-ci-robot requested review from andrewsykim and odinuge March 20, 2022 14:00

k8s-ci-robot added area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 20, 2022

yangjunmyfm192085 reviewed Mar 21, 2022

View reviewed changes

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 Mar 21, 2022

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 21, 2022

fix test cases for cpu shares

0b6d270

odinuge approved these changes Mar 21, 2022

View reviewed changes

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 Mar 21, 2022

k8s-ci-robot assigned odinuge Mar 21, 2022

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

waynepeking348 requested review from yangjunmyfm192085 and odinuge March 22, 2022 03:58

odinuge approved these changes Mar 22, 2022

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 22, 2022

waynepeking348 requested a review from odinuge March 23, 2022 02:05

yangjunmyfm192085 reviewed Mar 27, 2022

View reviewed changes

k8s-ci-robot requested a review from mrunalp March 27, 2022 06:42

yangjunmyfm192085 reviewed Mar 27, 2022

View reviewed changes

k8s-ci-robot assigned mrunalp Mar 27, 2022

k8s-ci-robot assigned derekwaynecarr, klueska and yujuhong Apr 8, 2022

waynepeking348 requested a review from yangjunmyfm192085 April 28, 2022 06:39

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 27, 2022

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 21, 2022

mrunalp approved these changes Aug 23, 2022

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 23, 2022

k8s-ci-robot merged commit 17dd76f into kubernetes:master Aug 23, 2022

SIG Node PR Triage automation moved this from Needs Approver to Done Aug 23, 2022

k8s-ci-robot added this to the v1.26 milestone Aug 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix bugs of container cpu shares when cpu request set to zero #108832

fix bugs of container cpu shares when cpu request set to zero #108832

waynepeking348 commented Mar 20, 2022 •

edited

k8s-ci-robot commented Mar 20, 2022

waynepeking348 commented Mar 20, 2022

yangjunmyfm192085 left a comment

yangjunmyfm192085 commented Mar 21, 2022

waynepeking348 commented Mar 21, 2022

odinuge left a comment

odinuge Mar 21, 2022

odinuge commented Mar 21, 2022

waynepeking348 commented Mar 22, 2022 •

edited

waynepeking348 commented Mar 22, 2022

odinuge left a comment

waynepeking348 commented Mar 23, 2022

yangjunmyfm192085 left a comment

yangjunmyfm192085 left a comment

waynepeking348 commented Apr 7, 2022 •

edited

waynepeking348 commented Apr 8, 2022

k8s-triage-robot commented Jul 27, 2022

waynepeking348 commented Aug 21, 2022

k8s-ci-robot commented Aug 23, 2022

waynepeking348 commented Aug 23, 2022

k8s-ci-robot commented Aug 23, 2022

	if cpuRequest == nil {
	if cpuRequest == nil && !cpuLimit.IsZero() {

fix bugs of container cpu shares when cpu request set to zero #108832

fix bugs of container cpu shares when cpu request set to zero #108832

Conversation

waynepeking348 commented Mar 20, 2022 • edited

k8s-ci-robot commented Mar 20, 2022

waynepeking348 commented Mar 20, 2022

yangjunmyfm192085 left a comment

Choose a reason for hiding this comment

yangjunmyfm192085 commented Mar 21, 2022

waynepeking348 commented Mar 21, 2022

odinuge left a comment

Choose a reason for hiding this comment

odinuge Mar 21, 2022

Choose a reason for hiding this comment

odinuge commented Mar 21, 2022

waynepeking348 commented Mar 22, 2022 • edited

waynepeking348 commented Mar 22, 2022

odinuge left a comment

Choose a reason for hiding this comment

waynepeking348 commented Mar 23, 2022

yangjunmyfm192085 left a comment

Choose a reason for hiding this comment

yangjunmyfm192085 left a comment

Choose a reason for hiding this comment

waynepeking348 commented Apr 7, 2022 • edited

waynepeking348 commented Apr 8, 2022

k8s-triage-robot commented Jul 27, 2022

waynepeking348 commented Aug 21, 2022

k8s-ci-robot commented Aug 23, 2022

waynepeking348 commented Aug 23, 2022

k8s-ci-robot commented Aug 23, 2022

waynepeking348 commented Mar 20, 2022 •

edited

waynepeking348 commented Mar 22, 2022 •

edited

waynepeking348 commented Apr 7, 2022 •

edited