fix: allocate reusableCPUs first before allocate other available CPUs to avoid CPU leaking when cpu-manager-policy=static #131966

Chunxia202410 · 2025-05-26T12:12:56Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

In the case of cpu-manager-policy=static, when allocate CPUs to a container of a pod, the CPU manager supports reusing the CPUs which have been allocated to the non-restartable Init containers of the pod.

But In some cases, the app container can not reuse the CPU of Init containers, which lead to CPU leaking.

For example:
defaultCpuSet: {{0,10},{1,11},{2,12},{3,13},{4,14},{5,15},{6,16},{7,17},{8,18},{9,19}}
system reserved CPU: {0}
For a pod, Init container request 1 CPU, and app container request 2 CPUs.
the result in the cpu_manager_state is

{
  "policyName":"static",
  "defaultCpuSet":"0,2-9,12-19",
  "entries":{
    "bc2c2aa2-ec4e-43eb-b656-a0928d92f19e":{
      "test-c-1":"1,11",
      "test-init-c-1":"10"   leaking CPU
    }
  },
  "checksum":3859770841
}

CPU leakage means that this CPU not be used by the app container, and also can not be used by other pod, because it not release to the defaultCpuSet.
CPU leakage may cause the error UnexpectedAdmissionError to occur when create a new pod, because the number of available CPUs in the scheduler does not match the number of available CPUs in kubelet. The detail please refer to #129556 (comment).

There are two possible solutions (#112228 (comment))

Solution1: Allocate resuable CPUs first before allocate other available CPUs.
Solution2: Release reusable CPUs which not be used by the AppContainer after the initContainer exits.

If my implement correctly for Solution2 in #131764, it has been proved that it might not a good solution, because it can not cover the pod restart case.

This PR implement the solution1, allocate reusableCPUs first before allocate other available CPUs.

Which issue(s) this PR fixes:

Fixes #112228

k8s-ci-robot · 2025-05-26T12:12:59Z

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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-sigs/prow repository.

k8s-ci-robot · 2025-05-26T12:13:05Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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-sigs/prow repository.

k8s-ci-robot · 2025-05-26T12:13:06Z

Hi @Chunxia202410. 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-sigs/prow repository.

Chunxia202410 · 2025-05-26T12:15:05Z

/sig node

ffromani · 2025-05-26T21:24:14Z

/ok-to-test

Chunxia202410 · 2025-05-27T05:52:10Z

/test pull-kubernetes-e2e-kind-ipv6

ffromani · 2025-05-27T08:43:38Z

/assign

this is a quite delicate fix in a key area of cpu management. We will need:

a very solid rationale in the commit message/PR presentation. It's fine to defer to issue/past discussion the full details, but this PR alone should be self-sufficient to grasp the change, the rationale, the implication
extensive test coverage (unit tests are not sufficient I think, but I can possibly be proven wrong)
it is not excluded we will need a flag or a feature gate to change the behavior we had for years and ppl (implicitly?) depend on, much like we had to do for the quota issue (github.com/fix: pods meeting qualifications for static placement when cpu-manager-policy=static should not have cfs quota enforcement #127525)

Chunxia202410 · 2025-06-04T05:38:29Z

/test pull-kubernetes-unit-windows-master

k8s-ci-robot · 2025-06-04T07:34:59Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Chunxia202410
Once this PR has been reviewed and has the lgtm label, please assign derekwaynecarr for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

pkg/features/OWNERS
pkg/kubelet/cm/cpumanager/OWNERS
test/compatibility_lifecycle/reference/OWNERS
test/e2e_node/OWNERS

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

Chunxia202410 · 2025-06-04T10:22:28Z

/test pull-kubernetes-unit
/test pull-kubernetes-unit-windows-master

Chunxia202410 · 2025-06-04T11:11:21Z

a very solid rationale in the commit message/PR presentation. It's fine to defer to issue/past discussion the full details, but this PR alone should be self-sufficient to grasp the change, the rationale, the implication

Update PR description.

extensive test coverage (unit tests are not sufficient I think, but I can possibly be proven wrong)

Add 5 e2e test cases in cpumanager_test.go.

it is not excluded we will need a flag or a feature gate to change the behavior we had for years and ppl (implicitly?) depend on, much like we had to do for the quota issue (github.com/fix: pods meeting qualifications for static placement when cpu-manager-policy=static should not have cfs quota enforcement #127525)

Add feature gate InheritReusableCPUsFirst

@ffromani , I have modified the PR description and code based on your suggestion, please take a look, thank you very much.

Chunxia202410 · 2025-06-06T05:43:53Z

/test pull-kubernetes-unit-windows-master

Chunxia202410 · 2025-06-11T02:45:42Z

/test pull-kubernetes-e2e-gce

esotsal · 2025-06-11T07:09:28Z

pkg/features/kube_features.go

@@ -976,6 +976,18 @@ const (
 	// operation when scheduling a Pod by setting the `metadata.labels` field on the submitted Binding,
 	// similar to how `metadata.annotations` behaves.
 	PodTopologyLabelsAdmission featuregate.Feature = "PodTopologyLabelsAdmission"
+


I think with the introduction of a new feature gate, versioned_kube_features.go and versioned_feature_list.yaml need to be updated as well.

Thanks you for your comments, I have already updated the versioned_feature_list.yaml file. Could you clarify if I missed any required modifications in this file?
And the code in versioned_kube_features.go have been moved to kube_features.go in this commit, I have also updated the feature version mappings in kube_features.go accordingly.

Thanks i missed that.

…void CPU leaking when cpu-manager-policy=static

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 26, 2025

k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. labels May 26, 2025

github-project-automation bot added this to SIG Node: code and documentation PRs May 26, 2025

github-project-automation bot moved this to Triage in SIG Node: code and documentation PRs May 26, 2025

k8s-ci-robot removed the do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label May 26, 2025

k8s-ci-robot requested review from derekwaynecarr and klueska May 26, 2025 12:13

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 May 26, 2025

Chunxia202410 force-pushed the cpu_manager_reusableCPUs branch 2 times, most recently from 4815c9a to d687e4e Compare May 27, 2025 02:18

Chunxia202410 force-pushed the cpu_manager_reusableCPUs branch from d687e4e to 2cc77e6 Compare May 27, 2025 08:17

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 27, 2025

k8s-ci-robot assigned ffromani May 27, 2025

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Jun 3, 2025

github-project-automation bot added this to SIG Node CI/Test Board Jun 3, 2025

github-project-automation bot moved this to Triage in SIG Node CI/Test Board Jun 3, 2025

Chunxia202410 force-pushed the cpu_manager_reusableCPUs branch 4 times, most recently from cdb95aa to b300a34 Compare June 4, 2025 04:36

Chunxia202410 force-pushed the cpu_manager_reusableCPUs branch 2 times, most recently from c753094 to ca0cb48 Compare June 4, 2025 07:34

Chunxia202410 force-pushed the cpu_manager_reusableCPUs branch from ca0cb48 to 2ad6a19 Compare June 4, 2025 09:33

Chunxia202410 force-pushed the cpu_manager_reusableCPUs branch from 2ad6a19 to 0906e4a Compare June 4, 2025 10:39

SergeyKanzhelev moved this from Triage to Archive-it in SIG Node CI/Test Board Jun 4, 2025

Chunxia202410 force-pushed the cpu_manager_reusableCPUs branch 2 times, most recently from 361a188 to 926bf3c Compare June 6, 2025 03:02

Chunxia202410 force-pushed the cpu_manager_reusableCPUs branch 2 times, most recently from b8f1961 to 157e38a Compare June 10, 2025 06:00

bart0sh moved this from Triage to Needs Reviewer in SIG Node: code and documentation PRs Jun 10, 2025

Chunxia202410 force-pushed the cpu_manager_reusableCPUs branch from 157e38a to e238ae6 Compare June 11, 2025 01:54

esotsal reviewed Jun 11, 2025

View reviewed changes

Chunxia202410 force-pushed the cpu_manager_reusableCPUs branch from e238ae6 to 9d49b9b Compare June 12, 2025 05:59

allocate reusableCPUs first before allocate other available CPUs to a…

85b45af

…void CPU leaking when cpu-manager-policy=static

Chunxia202410 force-pushed the cpu_manager_reusableCPUs branch from 9d49b9b to 85b45af Compare June 13, 2025 02:25

fix: allocate reusableCPUs first before allocate other available CPUs to avoid CPU leaking when cpu-manager-policy=static #131966

Are you sure you want to change the base?

fix: allocate reusableCPUs first before allocate other available CPUs to avoid CPU leaking when cpu-manager-policy=static #131966

Conversation

Chunxia202410 commented May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Uh oh!

k8s-ci-robot commented May 26, 2025

Uh oh!

k8s-ci-robot commented May 26, 2025

Uh oh!

k8s-ci-robot commented May 26, 2025

Uh oh!

Chunxia202410 commented May 26, 2025

Uh oh!

ffromani commented May 26, 2025

Uh oh!

Chunxia202410 commented May 27, 2025

Uh oh!

ffromani commented May 27, 2025

Uh oh!

Chunxia202410 commented Jun 4, 2025

Uh oh!

k8s-ci-robot commented Jun 4, 2025

Uh oh!

Chunxia202410 commented Jun 4, 2025

Uh oh!

Chunxia202410 commented Jun 4, 2025

Uh oh!

Chunxia202410 commented Jun 6, 2025

Uh oh!

Chunxia202410 commented Jun 11, 2025

Uh oh!

esotsal Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

Chunxia202410 Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

esotsal Jun 12, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Chunxia202410 commented May 26, 2025 •

edited

Loading