fix: propagate PodCliqueSet annotations to PodGang #500
Conversation
buildResource in podgang.go was constructing PodGang metadata without copying pcs.Annotations. As a result, scheduler annotations such as nvidia.com/kai-scheduler-queue set on a PodCliqueSet were not propagated to the PodGang. The kai-scheduler reads queue assignment from the PodGang, so the missing annotation caused QueueDoesNotExist even when the queue itself was valid. Fix this by merging pcs.Annotations into pg.Annotations with lo.Assign, while keeping computed annotations such as topology authoritative when keys overlap. This matches the existing annotation propagation pattern used for Pods.
Ronkahn21
left a comment
There was a problem hiding this comment.
Thanks for raising the issue and the fix! The root cause analysis is spot on. One concern: the current lo.Assign call replaces all existing PodGang annotations (and labels) instead of merging — see inline comments.
Ronkahn21
left a comment
There was a problem hiding this comment.
Thanks for raising the issue and the fix! The root cause analysis is spot on. One concern: the current lo.Assign call replaces all existing PodGang annotations instead of merging — see inline comments.
Preserve existing PodGang annotations when buildResource merges PodCliqueSet annotations, and expand buildResource test coverage to cover both create and update paths, including TAS-disabled preservation and TAS-enabled topology override behavior. Co-authored-by: Ron Kahn <122778260+Ronkahn21@users.noreply.github.com> Signed-off-by: w1t <1955451+w1t@users.noreply.github.com>
Good catch, thanks. I’ve updated the fix and addressed the inline comments. |
|
@w1t Please rebase your branch (fixing the conflicts), |
gflarity
left a comment
There was a problem hiding this comment.
Looks good. Thanks for the contribution. Please see the minor comments, and you'll need to rebase.
Afterwards we can run the full test suite, re-approve and merge :)
| } | ||
| pg.Annotations[apicommonconstants.AnnotationTopologyName] = grovecorev1alpha1.DefaultClusterTopologyName | ||
| topologyAnnotations[apicommonconstants.AnnotationTopologyName] = grovecorev1alpha1.DefaultClusterTopologyName | ||
| } |
There was a problem hiding this comment.
There's missing space after the first comma. goimports (part of CI) will catch this.
| } | |
| pg.Annotations = lo.Assign(pg.Annotations, pcs.Annotations, topologyAnnotations) |
| require.NoError(t, err) | ||
|
|
||
| for expectedKey, expectedValue := range test.expectedLabels { | ||
| actualValue, exists := pg.Labels[expectedKey] |
There was a problem hiding this comment.
These assertions verify that every expected key is present with the right value, but they don't verify there are no unexpected extra entries. A stray annotation slipping through would go undetected.
Consider using a direct map comparison instead:
assert.Equal(t, test.expectedLabels, pg.Labels)
assert.Equal(t, test.expectedAnnotations, pg.Annotations)Note: there will be some rebase issues you'll also need to account for.
|
stale PR, will be resolved in #573 |
What type of PR is this?
/kind bug
What this PR does / why we need it:
Problem
When creating a disaggregated instance via Dynamo with a custom
nvidia.com/kai-scheduler-queueannotation, the resultingPodGangis created without that annotation.
The kai-scheduler reads queue assignment from the
PodGang, so itraises
QueueDoesNotExisteven though the queue exists and wasvalidated successfully by the Dynamo operator.
Root cause
buildResourceinpodgang.goconstructsPodGangmetadata but doesnot copy
pcs.Annotationsintopg.Annotations.This differs from the existing behavior in
pod.go, where annotationsfrom the parent resource are propagated to each
Pod.Fix
Merge
pcs.Annotationsintopg.Annotationsusinglo.Assign, withcomputed annotations taking precedence over user-provided values when
the same key is present.
Test
Added TestPodGangAnnotationsFromPodCliqueSet in podgang_test.go.
The test calls buildResource with a PodCliqueSet carrying scheduler
annotations and asserts that the resulting PodGang contains the same
annotations. Covers three cases:
single annotation (nvidia.com/kai-scheduler-queue)
multiple scheduler annotations
mixed custom and scheduler annotations
Which issue(s) this PR fixes:
Fixes #490
Special notes for your reviewer:
The merge order in lo.Assign(pcs.Annotations, topologyAnnotations)
is intentional — topology annotations derived during reconciliation
overwrite any user-provided annotation with the same key.
Does this PR introduce an API change?
NONE