WIP: ensure cgroups conversion computation in e2e tests builds for all platforms #135739
Conversation
|
Please note that we're already in Code Freeze for the upcoming v1.35.0 release. Adding the milestone to this PR is strictly prohibited without proper approval. If this PR needs to be included in the v1.35.0 release:
We're also in Test Freeze for the Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Fri Dec 12 15:42:09 UTC 2025. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: BenTheElder 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 |
|
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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. |
|
/test pull-kubernetes-cross |
| // container runtimes, we check if either the old or the new conversion matches the actual value for now. | ||
| // TODO: Remove the old conversion once container runtimes are updated. | ||
| oldConverted := 1 + ((shares-2)*9999)/262142 | ||
| converted := libcontainercgroups.ConvertCPUSharesToCgroupV2Value(uint64(shares)) |
There was a problem hiding this comment.
Ugh, this utility is portable, but the package it is contained in is not.
We'll need to split out this code.
/hold
There was a problem hiding this comment.
944f49c to
7612eca
Compare
|
/hold I think inlining the simple conversion computation makes more sense, but we might need to do the whole third_party thing, I'm not sure where the line is ... it's fairly straightforward math but ... cc @kubernetes/dep-approvers |
| } | ||
| } | ||
|
|
||
| // this matches opencontainers/cgroups.ConvertCPUSharesToCgroupV2Value |
There was a problem hiding this comment.
I'm not sure at what point we need to jump through more hoops to do this, if it were just clamping to some well known values I think we'd just reimplement it ... not sure for this one @kubernetes/dep-approvers
There was a problem hiding this comment.
explicit
/hold
again
in fact let me mark this WIP
c8a8f69 to
5070bed
Compare
|
@BenTheElder: The following test failed, say
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-sigs/prow repository. I understand the commands that are listed here. |
allows us to trivially support all platforms for the test binary this conversion ideally needs to be stable anyhow to avoid test incompatibility ... opencontainers/cgroups#18 kubernetes#135214
5070bed to
1716029
Compare
What type of PR is this?
/kind bug
What this PR does / why we need it:
Ensures podresize tests work when the test binary is compiled for non-linux platforms (the pods created by the test will still be for linux in the cluster under test, but the test itself can be on mac/windows)
Follow up https://github.com/kubernetes/kubernetes/pull/132791/changes#r2615407057
We can see the one utility used from opencontainers/cgroups is portable: https://github.com/opencontainers/cgroups/blob/e0c56cb31dcb3cb2b3d1554b20dd01ced32e2a2b/utils.go#L425
Which issue(s) this PR is related to:
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: