Skip to content

Update pod resize test to accept new cpu.weight conversion. #132791

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Update pod resize test to accept new cpu.weight conversion. #132791

wants to merge 1 commit into from
+259 −112

Conversation

bitoku
Copy link
Contributor

@bitoku bitoku commented Jul 7, 2025 •
edited
Loading

What type of PR is this?

/kind failing-test

What this PR does / why we need it:

Because of the changes in conversion of cpu.weight in cgroupv2, the InPlacePodVerticalScaling tests no longer pass.

This change enables graceful migration from the old conversion to the new conversion, not to break the CI.

Which issue(s) this PR is related to:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

None

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

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 7, 2025
@k8s-ci-robot
Copy link
Contributor

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

@k8s-ci-robot k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/dependency Issues or PRs related to dependency changes area/test sig/node Categorizes an issue or PR as relevant to SIG Node. labels Jul 7, 2025
@k8s-ci-robot k8s-ci-robot added the sig/testing Categorizes an issue or PR as relevant to SIG Testing. label Jul 7, 2025
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Jul 7, 2025
@k8s-ci-robot k8s-ci-robot requested review from dims, logicalhan and a team July 7, 2025 15:50
@bitoku bitoku mentioned this pull request Jul 7, 2025
Merged
haircommander
haircommander reviewed Jul 7, 2025
View reviewed changes
test/e2e/common/node/framework/cgroups/cgroups.go Outdated
@@ -218,9 +219,12 @@ func getExpectedCPUShares(rr *v1.ResourceRequirements, podOnCgroupv2 bool) int64
}
if podOnCgroupv2 {
// TODO: This fomula should be a shared function.
return 1 + ((shares-2)*9999)/262142
// TODO: Remove the old conversion once container runtimes are updated.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a link to the oci runtime changes/issues to track why we check both?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the description. let me know if anything unclear.

@bitoku bitoku force-pushed the cpu-weight branch 2 times, most recently from 9e7062f to b6d4eda Compare July 7, 2025 16:25
giuseppe
giuseppe approved these changes Jul 8, 2025
View reviewed changes
Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@giuseppe
Copy link
Member

giuseppe commented Jul 8, 2025

There are other instances of this conversion in the code. Will they be addressed separately?

@bitoku
Copy link
Contributor Author

bitoku commented Jul 8, 2025

@giuseppe I just didn't realize other instances. I'd like to include them in this PR. Can you point out for me?

@giuseppe
Copy link
Member

giuseppe commented Jul 8, 2025

@giuseppe I just didn't realize other instances. I'd like to include them in this PR. Can you point out for me?

these are the ones I see:

➜ git grep 262142
pkg/kubelet/cm/cgroup_manager_linux.go: return 1 + ((*cpuShares-2)*9999)/262142
pkg/kubelet/cm/cgroup_v2_manager_linux.go:      return uint64((((cpuShares - 2) * 9999) / 262142) + 1)
pkg/kubelet/cm/cgroup_v2_manager_linux.go:      return uint64((((cpuWeight - 1) * 262142) / 9999) + 2)
test/e2e/common/node/framework/cgroups/cgroups.go:              return 1 + ((shares-2)*9999)/262142
test/e2e_node/node_container_manager_test.go:   return 1 + ((shares-2)*9999)/262142

most of them are mechanical, except return uint64((((cpuWeight - 1) * 262142) / 9999) + 2) that needs the inverse function

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jul 15, 2025
@bitoku bitoku force-pushed the cpu-weight branch 2 times, most recently from 88b9c9b to 36b9084 Compare July 16, 2025 12:30
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 16, 2025
@bitoku
Copy link
Contributor Author

bitoku commented Jul 16, 2025

Do we want to reimplement the conversion in kubernetes, not importing from opencontainers/runc?
I'm not sure how to handle pull-kubernetes-typecheck.

@haircommander
Copy link
Contributor

@bitoku I think it could be fixed by setting the test as linux only similar to https://github.com/kubernetes/kubernetes/blob/master/test/e2e_node/cgroup_driver_from_cri_test.go#L1-L2

@k8s-ci-robot 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 Aug 8, 2025
@bitoku
Copy link
Contributor Author

bitoku commented Aug 8, 2025

/retest

@bitoku bitoku mentioned this pull request Aug 8, 2025
Closed
haircommander
haircommander reviewed Aug 8, 2025
View reviewed changes
test/e2e/node/pod_resize.go Outdated
@@ -1,3 +1,5 @@
//go:build linux
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm @tallclair do we expect to have pod resize on windows? if so, we may need to factor out the relevant bits to be _linux.go, but I'm gonna guess it's out of scope for now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as I checked them, those tests are skipped, but it may be better to use _linux.go instead just in case.

@k8s-ci-robot 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 Aug 8, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bitoku, giuseppe
Once this PR has been reviewed and has the lgtm label, please assign liggitt 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:

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

@bitoku bitoku mentioned this pull request Aug 11, 2025
Merged
@bitoku
Copy link
Contributor Author

bitoku commented Aug 11, 2025

The CI is green. PTAL

@haircommander
Copy link
Contributor

/lgtm

not urgent, we can get this in 1.35

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

LGTM label has been added.

Git tree hash: ec9e87dd03b566a2b1938e880613f0cabc88f8c8

@bart0sh bart0sh moved this from Triage to Needs Approver in SIG Node: code and documentation PRs Aug 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dims dims Awaiting requested review from dims

@logicalhan logicalhan Awaiting requested review from logicalhan

2 more reviewers

@haircommander haircommander haircommander left review comments

@giuseppe giuseppe giuseppe approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@haircommander haircommander

Labels
area/dependency Issues or PRs related to dependency changes area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. 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/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
SIG Node CI/Test Board
Status: PRs - Needs Reviewer
SIG Node: code and documentation PRs
Status: Needs Approver
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bitoku @k8s-ci-robot @giuseppe @haircommander