fix(podgrouper): skip WorkloadRunner wrapper when selecting the grouping plugin - #2067
Merged
Merged
Conversation
…ing plugin Wrapping a workload in a run.ai WorkloadRunner adds a level to the owner chain (Pod -> ... -> DynamoGraphDeployment -> WorkloadRunner). Plugin selection keys on the top owner's GVK, and WorkloadRunner had no entry, so every wrapped workload fell through to the default grouper: a single flat PodGroup with minMember 1 and group-wide topology constraints, instead of the per-clique gangs the wrapped kind's plugin would build. Register WorkloadRunner as a skip-top-owner kind, matching the legacy run.ai wrappers. That alone is not enough. skipTopOwnerGrouper resolved the next owner through a raw exact-GVK map lookup, bypassing the hub's wildcard-version fallback and the Karta fallback hub. The DGD is registered with a wildcard version, so a WorkloadRunner -> DGD chain missed and dropped back to the default grouper anyway. Resolve through the hub instead, so a skipped owner picks the same plugin a top owner would. Chained skips are now reachable, so guard the empty owner list that the existing slice arithmetic would panic on. Also add the missing RBAC for dynamographdeployments. Without it the chain walk 403s at the DGD level and silently truncates, which happens to still group correctly via PodCliqueSet but never propagates the DGD's labels and annotations onto the PodGroup. Signed-off-by: gshaibi <gshaibi@nvidia.com>
|
Total coverage: 54.6% -> 54.6% (delta 0.00%) Merging this branch will increase overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
…osure Both new branches were untested: the len(otherOwners) == 0 fallback in skipTopOwnerGrouper and the closure that resolves a skipped owner through the hub. The hub case exercises the real wiring end to end rather than a stubbed resolver. Signed-off-by: gshaibi <gshaibi@nvidia.com>
The recursion always bottoms out at the pod, which resolves to the PodJob grouper rather than back to this one, so the owner list is never exhausted here. Carrying a branch for a state no caller can produce is noise. Signed-off-by: gshaibi <gshaibi@nvidia.com>
Signed-off-by: gshaibi <gshaibi@nvidia.com>
… filled Signed-off-by: gshaibi <gshaibi@nvidia.com>
davidLif
approved these changes
Aug 13, 2026
Collaborator
📝 Changelog fragment recordedThanks! This PR added the changelog fragment(s) below. Pending fragments are folded into |
Collaborator
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin v0.16
git worktree add -d .worktree/backport-2067-to-v0.16 origin/v0.16
cd .worktree/backport-2067-to-v0.16
git switch --create backport-2067-to-v0.16
git cherry-pick -x ef66b0969db564af9d9bff1580917938558d83db 74091065123ec6905f4e021c05bfa1458b18b927 3ea43fbd7bad6032b55075d2999e8ca8bb654976 9f16afde714470db82565066d3961f769ac86c44 35b8090575db968d2c229d2583a319d851fba7fa |
Collaborator
|
Successfully created backport PR for |
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Wrapping a workload in a
run.aiWorkloadRunneradds a level to the owner chain:GetPGMetadataselects the plugin by the top owner's GVK, and there was no entry forWorkloadRunner, so every wrapped workload fell through todefaultGrouper. The result is one flat PodGroup withminMember: 1carrying group-wide topology constraints, instead of the per-clique gangs the wrapped kind's plugin would build. Withkai.scheduler/topology-required-placement: kubernetes.io/hostnamethat forces unrelated components onto a single node, leaving GPU workers unschedulable even when capacity exists elsewhere. The same workload applied directly, without the wrapper, groups correctly.Two changes are needed, and either one alone is a no-op:
Register
WorkloadRunneras a skip-top-owner kind, matching the existing legacyrun.aiwrappers. UsesVersion: "*", as those do.Make
skipTopOwnerGrouperresolve through the hub. It previously looked up the next owner via a raw exact-GVK map lookup, bypassing both the hub's wildcard-version fallback and the Karta fallback hub. Since the DGD is registered withVersion: "*", aWorkloadRunner -> DGDchain missed that lookup and dropped straight back todefaultGrouper— the same flat PodGroup, via a second route.NewSkipTopOwnerGroupernow takes aGrouperResolver; the hub passes a closure overGetPodGrouperPlugin(bound lazily, since the Karta fallback is assigned after construction, and to avoid thehub->skiptopownerimport cycle).After both, the wrapped path resolves identically to the direct one:
Chained skips are newly reachable. The recursion is bounded: each hop drops one owner, and it bottoms out at the pod, which resolves to the PodJob grouper rather than back to the skip grouper.
Separately, this adds missing RBAC for
nvidia.com/dynamographdeployments. A plugin was registered for the DGD but no permission was ever granted. In a stock install the chain walk 403s at the DGD level andhandleGetOwnerErrorsilently truncates toPodCliqueSet— grouping still looks right, so it is invisible, but the DGD-level plugin never runs and its labels/annotations are never propagated onto the PodGroup.Related Issues
Related to #856 (plugin matching falling back through the owner chain) — that issue is closed, so this does not auto-close it.
Checklist
make changelog(or applied theskip-changeloglabel). Do not editCHANGELOG.mddirectly — pending fragments are folded into it at release time.Breaking Changes
None.
NewSkipTopOwnerGrouper's signature changed from a plugin map to aGrouperResolver, but the constructor returns an unexported type and has a single in-tree caller.Behavior change worth noting: an owner that previously fell through to
defaultGrouperinside the skip path can now resolve to a wildcard-version or Karta-backed plugin. That is the intent of the fix, and it only affects chains that pass through a skip-top-owner kind.Additional Notes
Reviewer guidance — the second change is the load-bearing one. I verified this rather than assuming it: reverting the resolver to exact-only lookup makes the new chained test fail (
Expected <empty> to equal "Grove Grouper") while change 1 is fully in place. A backport that takes only theWorkloadRunnertable entry would ship a fix that changes nothing.Tests added:
skiptopowner— the chained case[PodClique, PodCliqueSet, DGD, WorkloadRunner]must land on the Grove grouper, plus label propagation across both skipped wrapper levels. The DGD fixture isv1beta1against aVersion: "*"registration, so it also covers wildcard resolution through the skip path.hub—WorkloadRunnerresolves to the skip-top-owner grouper, at both a known version and an unknown one via the wildcard.make validateandgo test ./pkg/podgrouper/...pass; RBAC was regenerated withmake manifests.A backport to
v0.16should follow, and the two changes above are sufficient for the reported case: the non-wrapped path already produces correct per-clique PodGroups on the affected cluster, so the DGD resolves there and the wrapper level is the entire delta.Separately,
v0.16still registers the DGD with an exactv1alpha1wheremainusesVersion: "*"(widened in #1870, which shipped in v0.17.0 only). Carrying that over is worthwhile hardening, but it fixes a different symptom: av1beta1DGD onv0.16would break the non-wrapped path too, which is not what was reported. Best kept as its own change.v0.16also has no Karta fallback hub, so the resolver change there should stay wildcard-only.