Avoid unintentional patches to Quantity fields

[brejman]

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:

1. User requests 1G
2. 1G is converted to an internal representation (int64) as 1000000000
3. 1000000000 is converted to Quantity with BinarySI format
4. Quantity is sent to the API as "1000000000" because it isn't representable with a binary suffix
5. Quantity is read from the API as "1G" because lack of suffix is interpreted as DecimalSI format
6. 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:

1. Always use DecimalSI. This would fix the issue, but the Workload wouldn't use binary suffixes, e.g. "1Mi" would end up as "1048576"
2. 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]

The committers listed above are authorized under a signed CLA.

• ✅ login: brejman / name: Bartosz (a82f46f)

[k8s-ci-robot]

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]

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]

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	a82f46f
🔍 Latest deploy log	https://app.netlify.com/projects/kubernetes-sigs-kueue/deploys/6904c388d630850008d940c7

[mimowo]

/ok-to-test

[brejman]

/retest
Known issue #7390

[brejman]

Added integration test from #7217 @olekzabl

[mimowo]

log the error so that we can find out debugging that it went wrong

[mimowo]

Ah ok, this would never happen as the resource.Quantity binary was serialized by the library function, so it would need to be an internal serialization / deserialization bug in the library. I think this it is ok to assume this does not fail. In the worst case scenario we fallback to "binary" as before.

Otherwise we would need to propagate the logger via 20 or so functions. So, I'm ok to avoid the complication for this case.

[mimowo]

@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]

/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

[k8s-infra-cherrypick-robot]

@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
/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

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]

[APPROVALNOTIFIER] This PR is APPROVED

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

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

[mimowo]

Question, what it means "round-trips" in this context, since the serialization looks different: "1.5M" vs "1500k"?

[brejman]

The test case name doesn't participate in the test logic. The initial value is int64 and not a string. I just found the 1.5 string easier to read, in particular for MiB.

I think if we parsed a string "1.5M", it wouldn't roundtrip and would serialize back to "1500k". But it's not a problem, because "1.5M" could only be provided as an input which is then converted to int64 before being sent to the API, and after that conversion it consistently roundtrips.

I updated the test case name

[mimowo]

Actually, also have this question: https://github.com/kubernetes-sigs/kueue/pull/7430/files#r2480708605

[mimowo]

/lgtm cancel
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.

[mimowo]

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

[brejman]

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]

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

[mimowo]

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