Avoid unintentional patches to Quantity fields #7430

brejman · 2025-10-29T15:40:23Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

In case the job requests memory in decimal format (e.g. "1G"), the Workload's status.admission.podSetAssignments[].resourceUsage.memory is unintentionally patched due to Quantity round trip issues.

For example:

User requests 1G
1G is converted to an internal representation (int64) as 1000000000
1000000000 is converted to Quantity with BinarySI format
Quantity is sent to the API as "1000000000" because it isn't representable with a binary suffix
Quantity is read from the API as "1G" because lack of suffix is interpreted as DecimalSI format
The "1G" value is then sent to the API in a patch request intended for another field as a diff.

This PR changes no.3 to auto-detect the format and avoid unintentional diff.

While normally such diff shouldn't really matter other than adding some noise in managed fields, it hits kubernetes/kubernetes#134902, which makes the workload impossible to evict.

Which issue(s) this PR fixes:

Fixes #6966

Special notes for your reviewer:

This change makes it much more expensive to compute the Quantity.

I've considered a couple of alternatives:

Always use DecimalSI. This would fix the issue, but the Workload wouldn't use binary suffixes, e.g. "1Mi" would end up as "1048576"
Leave it be and wait for a fix to Non-SSA update of a map’s entry prevents deleting the map via SSA kubernetes/kubernetes#134902. I think it's worth addressing this unintentional patch anyway, to avoid unintended consequences in the future.

Does this PR introduce a user-facing change?

Fix eviction of jobs with memory requests in decimal format

linux-foundation-easycla · 2025-10-29T15:40:30Z

The committers listed above are authorized under a signed CLA.

✅ login: brejman / name: Bartosz (fa46493)

k8s-ci-robot · 2025-10-29T15:40:32Z

Welcome @brejman!

It looks like this is your first PR to kubernetes-sigs/kueue 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kueue has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2025-10-29T15:40:32Z

Hi @brejman. Thanks for your PR.

I'm waiting for a github.com 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.

netlify · 2025-10-29T15:40:56Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`fa46493`
🔍 Latest deploy log	https://app.netlify.com/projects/kubernetes-sigs-kueue/deploys/690b6be2d1251f0008571c5f

mimowo · 2025-10-29T15:40:57Z

/ok-to-test

brejman · 2025-10-29T16:37:33Z

/retest
Known issue #7390

brejman · 2025-10-30T11:21:07Z

Added integration test from #7217 @olekzabl

pkg/resources/requests.go

mimowo · 2025-10-31T09:21:34Z

@brejman please check locally (without putting all the cases here) if this change addresses all the known problematic variants listed under the issue: #6966, in the PR: https://github.com/olekzabl/kueue/blob/38eef366ed6a3e6b64b08ab8c9800abc20d5cca3/test/integration/singlecluster/controller/admissionchecks/provisioning/provisioning_test.go#L1848-L1977

mimowo · 2025-10-31T09:26:29Z

/lgtm
/approve
/cherrypick release-0.14
/cherrypick release-0.13

Feel free to unhold if you confirm all cases are addressed by this change, as requested here: #7430 (comment)
/hold

mimowo · 2025-10-31T09:42:41Z

Overall it would be good to have another pair on eyes on this, maybe @olekzabl or @PBundyra could review?

brejman · 2025-10-31T11:34:22Z

Actually, I just tested locally - reverted the Implementation changes and the test is still passing, so seems like the test isn't really verifying the issue. Please double check that.

The change from no-suffix to decimal suffix doesn't seem to be detected as a diff anymore and I can't seem to reproduce the issue, even on older repo revisions, where I'm confident I was able to in the past. I'm taking a deeper look.

brejman · 2025-10-31T12:37:04Z

I checked again and it does reproduce on an old revision. #7369 makes it not reproducible. Still looking into why.

mimowo · 2025-10-31T12:42:58Z

interesting maybe the additional layer of convertion webhooks somehow fixes the issue by additional serialization/deserialization step. If this is confirmed your PR is still worth it, bevause it will allow to fix 0.13 and 0.14.

and also the issue would return when we move storage to v1beta2 and thus drop convertion webhooks again

pkg/resources/requests_test.go

pkg/resources/requests.go

k8s-ci-robot · 2025-11-05T11:57:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brejman, mimowo, olekzabl

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:

~~OWNERS~~ [mimowo]

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

brejman · 2025-11-05T14:06:51Z

/hold cancel

@brejman please check locally (without putting all the cases here) if this change addresses all the known problematic variants listed under the issue: #6966, in the PR: https://github.com/olekzabl/kueue/blob/38eef366ed6a3e6b64b08ab8c9800abc20d5cca3/test/integration/singlecluster/controller/admissionchecks/provisioning/provisioning_test.go#L1848-L1977

I patched the tests locally and changed the "stuck" expectation to "works". All test cases pass with the fix, and fail without it (checked before #7369). At HEAD the issue is not reproducible probably due to the conversion webhooks as noted previously.

PTAL

mimowo · 2025-11-06T07:40:40Z

/lgtm
/cherrypick release-0.14
/cherrypick release-0.13

k8s-infra-cherrypick-robot · 2025-11-06T07:40:43Z

@mimowo: once the present PR merges, I will cherry-pick it on top of release-0.13, release-0.14 in new PRs and assign them to you.

In response to this:

/lgtm
/cherrypick release-0.14
/cherrypick release-0.13

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-11-06T07:40:46Z

LGTM label has been added.

Git tree hash: 7d64e2cffab3b3e0c1538e71be9b0f270fb0b07a

k8s-infra-cherrypick-robot · 2025-11-06T08:09:40Z

@mimowo: new pull request created: #7549

In response to this:

/lgtm
/cherrypick release-0.14
/cherrypick release-0.13

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-infra-cherrypick-robot · 2025-11-06T08:10:16Z

@mimowo: new pull request created: #7550

In response to this:

/lgtm
/cherrypick release-0.14
/cherrypick release-0.13

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.

mimowo · 2025-11-06T08:33:49Z

The cherrypicks need to be prepared "manually". The ./hack/cherry_pick_pull.sh script is useful for that

…ty fields (#7556) * Ensure roundtrip success for Quantities * Fixed conflicts --------- Co-authored-by: Michał Woźniak <[email protected]>

…ty fields (#7557) * Ensure roundtrip success for Quantities * Fix imports * [release-0.14] Integration test for preemption from AC flavors. (#7168) * Integration test for preemption from AC flavors. * Naming fix to make a tool happy * Minor comment fix * Reduce the number of test cases * Replace testCase(...) with ginkgo.Entry(nil, ...) * Fix import ordering * Remove cases for #6966; evolve DRY -> DAMP * Forgotten: remove `admissionCheckUsage` * Adjusting to updated origin * Fixes to address comments * Forgotten: FDescribe -> Describe * Forgotten follow-up --------- Co-authored-by: Olek Zabłocki <[email protected]> --------- Co-authored-by: Michał Woźniak <[email protected]> Co-authored-by: Olek Zabłocki <[email protected]>

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/bug Categorizes issue or PR as related to a bug. labels Oct 29, 2025

k8s-ci-robot requested a review from gabesaba October 29, 2025 15:40

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 Oct 29, 2025

k8s-ci-robot requested a review from PBundyra October 29, 2025 15:40

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 29, 2025

brejman force-pushed the issue-6966 branch 2 times, most recently from 1c9b01f to 4a217ac Compare October 29, 2025 16:36

brejman force-pushed the issue-6966 branch from 4a217ac to 86152ef Compare October 30, 2025 11:19

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 Oct 30, 2025

brejman marked this pull request as ready for review October 30, 2025 11:21

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 30, 2025

k8s-ci-robot requested a review from kannon92 October 30, 2025 11:21

mimowo reviewed Oct 30, 2025

View reviewed changes

pkg/resources/requests.go Show resolved Hide resolved

brejman force-pushed the issue-6966 branch from 86152ef to a82f46f Compare October 31, 2025 14:11

olekzabl approved these changes Nov 5, 2025

View reviewed changes

pkg/resources/requests_test.go Show resolved Hide resolved

pkg/resources/requests.go Outdated Show resolved Hide resolved

brejman force-pushed the issue-6966 branch from a82f46f to 7928b26 Compare November 5, 2025 12:25

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 5, 2025

brejman force-pushed the issue-6966 branch from 7928b26 to 9342edb Compare November 5, 2025 15:12

Ensure roundtrip success for Quantities

fa46493

brejman force-pushed the issue-6966 branch from 9342edb to fa46493 Compare November 5, 2025 15:23

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 6, 2025

k8s-ci-robot merged commit f23e774 into kubernetes-sigs:main Nov 6, 2025
23 checks passed

k8s-ci-robot added this to the v0.15 milestone Nov 6, 2025

k8s-infra-cherrypick-robot mentioned this pull request Nov 6, 2025

[release-0.13] Avoid unintentional patches to Quantity fields #7549

Closed

k8s-infra-cherrypick-robot mentioned this pull request Nov 6, 2025

[release-0.14] Avoid unintentional patches to Quantity fields #7550

Closed

This was referenced Nov 6, 2025

Automated cherry pick of #7430: Avoid unintentional patches to Quantity fields #7556

Merged

Automated cherry pick of #7430: Avoid unintentional patches to Quantity fields #7557

Merged

Singularity23x0 pushed a commit to Singularity23x0/kueue that referenced this pull request Nov 7, 2025

Ensure roundtrip success for Quantities (kubernetes-sigs#7430)

2c0904f

olekzabl mentioned this pull request Nov 24, 2025

WIP: Integration test for preemption from AC flavors - a test case demonstrating issue [EXPERIMENT] #7217

Closed

Avoid unintentional patches to Quantity fields #7430

Avoid unintentional patches to Quantity fields #7430

Uh oh!

Conversation

brejman commented Oct 29, 2025

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Uh oh!

linux-foundation-easycla bot commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Oct 29, 2025

Uh oh!

k8s-ci-robot commented Oct 29, 2025

Uh oh!

netlify bot commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Uh oh!

mimowo commented Oct 29, 2025

Uh oh!

brejman commented Oct 29, 2025

Uh oh!

brejman commented Oct 30, 2025

Uh oh!

Uh oh!

mimowo commented Oct 31, 2025

Uh oh!

mimowo commented Oct 31, 2025

Uh oh!

mimowo commented Oct 31, 2025

Uh oh!

brejman commented Oct 31, 2025

Uh oh!

brejman commented Oct 31, 2025

Uh oh!

mimowo commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

k8s-ci-robot commented Nov 5, 2025

Uh oh!

brejman commented Nov 5, 2025

Uh oh!

mimowo commented Nov 6, 2025

Uh oh!

k8s-infra-cherrypick-robot commented Nov 6, 2025

Uh oh!

k8s-ci-robot commented Nov 6, 2025

Uh oh!

Uh oh!

k8s-infra-cherrypick-robot commented Nov 6, 2025

Uh oh!

k8s-infra-cherrypick-robot commented Nov 6, 2025

Uh oh!

mimowo commented Nov 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

linux-foundation-easycla bot commented Oct 29, 2025 •

edited

Loading

netlify bot commented Oct 29, 2025 •

edited

Loading

mimowo commented Oct 31, 2025 •

edited

Loading