[DevaanshPathak]

Summary

• Copy PodSet resource quantities before Mul in CoScheduling.Build, so the caller's requests stay unchanged.
• Repeated reconciliation no longer inflates PodGroup minResources.
• Add a unit test that builds twice and checks both the PodGroup totals and the original requests.

Fixes #4104

Test plan

• go test ./pkg/runtime/framework/plugins/coscheduling/ -count=1 -run TestBuildDoesNotMutatePodSetRequests|TestCoScheduling
• CI on this PR

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[google-oss-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Details

Needs approval from an approver in each of these files:

• OWNERS

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

[DevaanshPathak]

Hi @astefanutti, gentle ping for a review and /ok-to-test when you have a moment so the full CI suite can run. Thanks!

[abhishekKokadwar]

Thanks @DevaanshPathak. The change itself is safe and I think it is worth having, but I could not get the test to demonstrate it, so I looked into why.

The new test passes without the fix

At cb4d7c34, with coscheduling.go reverted to the merge base and the test kept:

	Result
PR head	ok
coscheduling.go from master, PR test	ok

The reason is in Quantity.Mul. Ranging over the ResourceList copies the Quantity struct. For values held as int64Amount, which covers 500m, 1Gi and the other values in the test, Mul only changes the copy. The original is only changed when the quantity is held as inf.Dec, because the copy shares that pointer. So the 1Gi to 2Gi to 4Gi example in #4104 does not happen with those values.

A variant that does pin it

Converting the requests with ToDec() before the loop makes the test fail on master and pass on this branch:

for _, ps := range info.TemplateSpec.PodSets {
	for name, q := range ps.SinglePodRequests {
		q.ToDec()
		ps.SinglePodRequests[name] = q
	}
}

On master that gives Build mutated PodSet "node" requests, with memory at 2147483648 after the first call and 4294967296 after the second.

Impact

I could not find a path where this shows up in a running controller. RuntimeInfo builds a new Info through runtime.NewInfo on each NewObjects call, and PodRequests deep-copies the quantities, so Build does not see the same Info twice. I read this from the code and did not run a cluster, so please correct me if there is a caller I missed. If that holds, it may be worth rewording the description from "repeated reconciliation inflates minResources" to a hardening change.

Volcano has the same loop

pkg/runtime/framework/plugins/volcano/volcano.go:200 has the identical quantity.Mul(int64(count)) on the range copy. It would make sense to fix both here.

[DevaanshPathak]

Thanks @abhishekKokadwar — you were right that the original test values stay on the int64 path, so Mul never aliases.

I converted the PodSet requests with ToDec() before Build, matching the variant you sketched. The test still expects the original requests and the same minResources across two Build calls, so it now fails on master and passes here.

On the controller path: I also could not find a caller that reuses the same Info across reconciles (NewObjects builds a fresh one). I still think the DeepCopy is worth keeping as hardening, since plugins can hold onto Info and Volcano had the same loop (#4101). Happy to reword the PR description if you would rather frame it that way.

Still looking for /ok-to-test and a maintainer review when someone has time.

[abhishekKokadwar]

Thanks @DevaanshPathak, the ToDec() change does what it needs to. I checked 487965a:

Check	Result
go test ./pkg/runtime/framework/plugins/coscheduling/ on the PR head	ok
coscheduling.go from the merge base, with the PR's tests	TestBuildDoesNotMutatePodSetRequests fails
go vet on the package, gofmt on both files	clean

With the fix reverted, the failure shows exactly the bug from #4104: the node request goes from 1Gi to 2Gi after the first Build and to 4Gi after the second, and the PodGroup minResources changes with it. So the test now pins the change, which it did not before.

On the controller path we agree: NewObjects builds a fresh Info, so this is hardening rather than a live bug. Keeping the DeepCopy makes sense to me for the reason you gave. One small thing: the description still says "Repeated reconciliation no longer inflates PodGroup minResources". Rewording that to say Build no longer mutates the caller's requests, and that no current caller reuses an Info, would match what the test shows and save a reviewer from looking for that path.

I also see the same copy for Volcano is in #4101. Since that PR's title is about HighestTierAllowed, a line in each description pointing at the other might help reviewers find both halves.

The branch is 67 commits behind master but still merges cleanly. The Go unit and integration suite has not run here yet because the PR has no ok-to-test.