[DevaanshPathak]

Summary

• Default nil NetworkTopology.HighestTierAllowed to 1 in Volcano.Build so runtimes with networkTopology set but highestTierAllowed omitted no longer panic.
• Deep-copy resource quantities before Mul so Build does not mutate the caller's Info.TemplateSpec resources.
• Add a unit test covering nil HighestTierAllowed.

Fixes #4100

Test plan

• go test ./pkg/runtime/framework/plugins/volcano/ -count=1
• 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 johnugeorge 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 @johnugeorge, gentle ping for a review and /ok-to-test when you have time. Thanks!

[abhishekKokadwar]

Thanks @DevaanshPathak. Both changes look correct to me. I looked further into the DeepCopy one since it is the less obvious of the two, and it turns out to be a real bug the current tests cannot see.

The mutation is real with ordinary values. A fractional binary quantity like 0.5Gi parses into the inf.Dec form, and for those Quantity.Mul writes through the shared *inf.Dec, so the value copy in the range loop does not protect the caller. Integer-backed values such as 300m or 1Gi are unaffected. The existing test data already has the triggering shape: the worker PodSet with 0.5Gi memory and Count: 4.

Why the suite does not catch it. With the DeepCopy reverted and your HighestTierAllowed change kept, TestVolcano still passes, for two reasons:

• info is compared before Build runs, never after.
• The cases that use that PodSet pass createBaseInfo.TemplateSpec as both info and expectInfo. Copying the struct still shares the PodSets slice and its SinglePodRequests maps, so a mutation appears on both sides of any comparison.

Two small test changes pin it: build a fresh *runtime.Info per use (turn createBaseInfo into a function), and after Build add

if c.expectInfo != nil {
	if diff := gocmp.Diff(c.expectInfo.TemplateSpec.PodSets, c.info.TemplateSpec.PodSets); len(diff) != 0 {
		t.Errorf("Build mutated info.TemplateSpec.PodSets (-want, +got):\n%s", diff)
	}
}

With both in place, the existing PodGroup exists but trainjob suspended case fails on master:

- s"memory": {d: s"536870912.000000000", Format: "BinarySI"},
+ s"memory": {d: s"2147483648.000000000", Format: "BinarySI"},

That is 0.5Gi becoming 2Gi, the 4x multiplier written back into the caller's PodSet. On this branch it passes. The same loop exists in the Coscheduling plugin, which I think your #4105 covers.

On HighestTierAllowed: ptr.Deref(..., 1) matches the default Volcano itself declares (+kubebuilder:default=1) and the trainer CRD carries. CRD defaults are applied on write and again when reading from etcd, so as you note in #4100 the nil only shows up on paths that skip API defaulting, like tests or fake clients. That reads as hardening, which seems fine.

I also checked this against #4127, which touches the priority class block in the same function; the two merge cleanly.

How I checked this

Which parsed values alias. For each value: parse it, take a value copy as the range loop does, call Mul(4) on the copy, and check whether the original changed.

Value	Original mutated
500m, 250m, 1, 1.5, 0.1, 1e3, 100n, 1.000000001	no
2Gi, 512Mi	no
0.5Gi	yes
0.0000000001	yes
9223372036854775807, 1Ei, 8Ei	yes

So the trigger is a fractional binary-suffix value, sub-nano precision, or a value too large for the int64 form.

Build itself. A PodSet with Count: 4 and a single CPU request of 500m, calling Build and reading SinglePodRequests back afterwards:

	int64-backed 500m	500m forced to Dec with ToDec()
master	500m	2000m
this branch	500m	500m

Existing suite. On this branch with only the DeepCopy reverted (HighestTierAllowed change kept), go test ./pkg/runtime/framework/plugins/volcano/ passes. With the two test changes above applied, master (which has no DeepCopy) fails PodGroup exists but trainjob suspended with the diff shown earlier, and this branch passes.

Merge. git merge-tree --write-tree between this branch and #4127 reports no conflicts.

[DevaanshPathak]

Thanks @abhishekKokadwar for digging into the DeepCopy path — that fractional 0.5Gi aliasing explanation was really helpful.

I updated the tests along the lines you suggested:

• createBaseInfo is now a newBaseInfo() factory so each case gets its own PodSet ResourceLists
• after Build, we assert info.TemplateSpec.PodSets still matches expectInfo

go test ./pkg/runtime/framework/plugins/volcano/ passes locally. Still looking for /ok-to-test and a maintainer review when someone has time.

[DevaanshPathak]

Hi @johnugeorge, gentle ping for a review and /ok-to-test when you have time. Abhishek's review comments are addressed (DeepCopy plus the factory/mutation tests), and the Volcano package tests pass locally. Thanks!

[abhishekKokadwar]

Thanks @DevaanshPathak, I went through 2eb7e4e. Both test changes are in as described, and they do their job: on this branch go test ./pkg/runtime/framework/plugins/volcano/ passes, and

• reverting only the DeepCopy fails TestVolcano with Build mutated info.TemplateSpec.PodSets, the same 0.5Gi to 2Gi diff as before (536870912 to 2147483648),
• reverting only the ptr.Deref(..., 1) panics with a nil pointer dereference.

One thing to fix before this can merge: the branch is based on 44819cf, and since then #4059 added two cases to TestVolcano (PodGroup does not exist and trainjob suspended and PodGroup does not exist and trainjob resumed from suspend) whose info and expectInfo still use createBaseInfo.TemplateSpec. Git merges the two cleanly, but the result does not compile:

volcano_test.go:401:19: undefined: createBaseInfo

After rebasing onto master, replacing those four with newBaseInfo().TemplateSpec is enough. I tried that on a merge of this branch with current master (b12f968): the package tests and go vet pass, and the DeepCopy revert still fails the new PodSets check, now on #4059's cases as well.

Since the PR has not run CI yet:

/ok-to-test