feat(operator): scaffold DGD placement score status wiring#11057
Open
ashnamehrotra wants to merge 6 commits into
Open
feat(operator): scaffold DGD placement score status wiring#11057ashnamehrotra wants to merge 6 commits into
ashnamehrotra wants to merge 6 commits into
Conversation
Signed-off-by: ashnamehrotra <ashnamehrotra@gmail.com>
ashnamehrotra
temporarily deployed
to
external_collaborator
GitHub Actions
Inactive
Contributor
github-actions
Bot
added
feat
external-contribution
Contributor
|
/ok to test b29bb9a |
github-actions
Bot
added
the
deployment::k8s
|
Contributor
WalkthroughAdds ChangesPlacement Score for DynamoGraphDeployment
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deploy/operator/internal/controller/dynamographdeployment_controller.go`:
- Around line 292-300: Reset the placement status fields in
dynamographdeployment_controller.go so stale values do not survive non-Grove or
lookup-failure reconciles. In the reconcile logic around r.isGrovePathway and
getExistingGrovePodCliqueSet, explicitly clear or set
dynamoDeployment.Status.PlacementScore and
dynamoDeployment.Status.PlacementScoreState on the non-Grove branch and when
pcsErr is returned, instead of only updating them after AggregatePlacementScore
succeeds. Use the existing symbols r.isGrovePathway,
getExistingGrovePodCliqueSet, and dynamo.AggregatePlacementScore to place the
fix in the same status-update path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 03ddbb02-5731-442a-8efc-7bfe4bb28257
📒 Files selected for processing (11)
deploy/operator/api/v1alpha1/conversion_field_coverage_test.godeploy/operator/api/v1alpha1/dynamographdeployment_conversion.godeploy/operator/api/v1alpha1/dynamographdeployment_conversion_test.godeploy/operator/api/v1alpha1/dynamographdeployment_types.godeploy/operator/api/v1alpha1/zz_generated.deepcopy.godeploy/operator/api/v1beta1/common.godeploy/operator/api/v1beta1/dynamographdeployment_types.godeploy/operator/api/v1beta1/zz_generated.deepcopy.godeploy/operator/internal/controller/dynamographdeployment_controller.godeploy/operator/internal/dynamo/grove.godeploy/operator/internal/dynamo/grove_test.go
Comment on lines
+292
to
+300
| if r.isGrovePathway(dynamoDeployment) { | ||
| pcs, pcsErr := r.getExistingGrovePodCliqueSet(ctx, dynamoDeployment) | ||
| if pcsErr != nil { | ||
| logger.V(1).Info("Failed to get PodCliqueSet for placement score", "error", pcsErr) | ||
| } else { | ||
| score, state := dynamo.AggregatePlacementScore(pcs) | ||
| dynamoDeployment.Status.PlacementScore = score | ||
| dynamoDeployment.Status.PlacementScoreState = state | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reset placement status on non-Grove and lookup-error paths.
Lines 292-300 only overwrite these fields on the happy Grove path, so a DGD can keep stale status.placementScore* values after Grove is disabled or after a transient getExistingGrovePodCliqueSet failure. Please clear or explicitly set them on every reconcile path instead of leaving the previous status intact.
Suggested fix
- if r.isGrovePathway(dynamoDeployment) {
+ dynamoDeployment.Status.PlacementScore = nil
+ dynamoDeployment.Status.PlacementScoreState = nvidiacomv1beta1.PlacementScoreStateUnsupported
+ if r.isGrovePathway(dynamoDeployment) {
pcs, pcsErr := r.getExistingGrovePodCliqueSet(ctx, dynamoDeployment)
if pcsErr != nil {
logger.V(1).Info("Failed to get PodCliqueSet for placement score", "error", pcsErr)
+ dynamoDeployment.Status.PlacementScoreState = nvidiacomv1beta1.PlacementScoreStateUnknown
} else {
score, state := dynamo.AggregatePlacementScore(pcs)
dynamoDeployment.Status.PlacementScore = score
dynamoDeployment.Status.PlacementScoreState = state
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if r.isGrovePathway(dynamoDeployment) { | |
| pcs, pcsErr := r.getExistingGrovePodCliqueSet(ctx, dynamoDeployment) | |
| if pcsErr != nil { | |
| logger.V(1).Info("Failed to get PodCliqueSet for placement score", "error", pcsErr) | |
| } else { | |
| score, state := dynamo.AggregatePlacementScore(pcs) | |
| dynamoDeployment.Status.PlacementScore = score | |
| dynamoDeployment.Status.PlacementScoreState = state | |
| } | |
| dynamoDeployment.Status.PlacementScore = nil | |
| dynamoDeployment.Status.PlacementScoreState = nvidiacomv1beta1.PlacementScoreStateUnsupported | |
| if r.isGrovePathway(dynamoDeployment) { | |
| pcs, pcsErr := r.getExistingGrovePodCliqueSet(ctx, dynamoDeployment) | |
| if pcsErr != nil { | |
| logger.V(1).Info("Failed to get PodCliqueSet for placement score", "error", pcsErr) | |
| dynamoDeployment.Status.PlacementScoreState = nvidiacomv1beta1.PlacementScoreStateUnknown | |
| } else { | |
| score, state := dynamo.AggregatePlacementScore(pcs) | |
| dynamoDeployment.Status.PlacementScore = score | |
| dynamoDeployment.Status.PlacementScoreState = state | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/operator/internal/controller/dynamographdeployment_controller.go`
around lines 292 - 300, Reset the placement status fields in
dynamographdeployment_controller.go so stale values do not survive non-Grove or
lookup-failure reconciles. In the reconcile logic around r.isGrovePathway and
getExistingGrovePodCliqueSet, explicitly clear or set
dynamoDeployment.Status.PlacementScore and
dynamoDeployment.Status.PlacementScoreState on the non-Grove branch and when
pcsErr is returned, instead of only updating them after AggregatePlacementScore
succeeds. Use the existing symbols r.isGrovePathway,
getExistingGrovePodCliqueSet, and dynamo.AggregatePlacementScore to place the
fix in the same status-update path.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Signed-off-by: ashnamehrotra <ashnamehrotra@gmail.com>
ashnamehrotra
temporarily deployed
to
external_collaborator
GitHub Actions
Inactive
Contributor
|
/ok to test 0c56c9e |
Signed-off-by: ashnamehrotra <ashnamehrotra@gmail.com>
ashnamehrotra
temporarily deployed
to
external_collaborator
GitHub Actions
Inactive
Contributor
|
/ok to test de57d0f |
github-actions
Bot
added
the
documentation
sttts
reviewed
Jul 2, 2026
sttts
reviewed
Jul 2, 2026
sttts
reviewed
Jul 2, 2026
- Drop the NotReported PlacementScoreState (removed by DEP; not
operationally distinct from Unknown). Empty PodGang statuses now
report Unknown.
- Enforce the [0.0, 1.0] score contract at the CRD boundary via
Minimum=0 / Maximum=1 kubebuilder markers on the placementScore
field in both v1alpha1 and v1beta1.
- Document the score contract on the API field: normalized [0,1],
higher-is-better, minimum aggregation as a worst-placement signal,
comparability only within the same scheduler scoring contract.
- Fix the reporting lifecycle so every reconcile leaves status
consistent:
* non-Grove pathway sets Unsupported and clears any stale score,
* PCS read failure sets Unknown and clears any stale score,
* successful aggregation writes through unchanged.
- Regenerate CRDs, api-reference, and the operator Helm chart CRD copy.
Signed-off-by: ashnamehrotra <ashnamehrotra@gmail.com>
ashnamehrotra
temporarily deployed
to
external_collaborator
GitHub Actions
Inactive
Contributor
|
/ok to test d86e8fd |
Address review feedback from @sttts on PR ai-dynamo#11057: - Introduce PlacementStatus struct (score + state) on both v1alpha1 and v1beta1 DGDStatus, replacing the flat placementScore / placementScoreState fields. This future-proofs the schema against new placement signals (scheduler contract version, last-report timestamp, per-unit reports) that would otherwise be a schema break to add later. - Update the operator helper to read logr.Logger from ctx via log.FromContext(ctx) instead of taking it as a parameter, and drop the now-unused go-logr/logr import. - Rename helper to updatePlacementStatus and write through a fresh PlacementStatus value so stale fields are never carried across reconciles. - Regenerate CRDs, api-reference, helm CRD copy, and deepcopy. Signed-off-by: ashnamehrotra <ashnamehrotra@gmail.com>
ashnamehrotra
temporarily deployed
to
external_collaborator
GitHub Actions
Inactive
Contributor
|
/ok to test a93fe91 |
Resolve conflicts in the DGD controller (keep placement-status write, drop the duplicate reconcile-error check that main removed) and in grove.go (keep AggregatePlacementScore alongside main's shortened comment on specToGroveTopologyConstraint). Regenerate deepcopy, CRDs, api-reference, helm docs, and helm CRD copy to pick up both trees. Signed-off-by: ashnamehrotra <ashnamehrotra@gmail.com>
ashnamehrotra
temporarily deployed
to
external_collaborator
GitHub Actions
Inactive
Contributor
|
/ok to test e3b76e7 |
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.
Summary
status.placementScorestatus.placementScoreStatev1beta1andv1alpha1, including conversion wiring and conversion field-set acknowledgment.placementScoreunset until Grove exposes a score field in a consumable API shape.Unsupported/NotReported).Validation
GOCACHE=/tmp/dynamo-go-cache go test ./api/v1alpha1 ./api/v1beta1 -count=1GOCACHE=/tmp/dynamo-go-cache go test ./api/v1alpha1 ./internal/dynamo -count=1GOCACHE=/tmp/dynamo-go-cache go build ./...(fromdeploy/operator)Scope Notes
Reported/Partialyet, because current Grove API does not exposeplacementScorewhere Dynamo can read it.Summary by CodeRabbit
New Features
Bug Fixes