Repository navigation
refactor(operator): separate managed Grove components - #15311
Conversation
Signed-off-by: Dr. Stefan Schimanski <sschimanski@nvidia.com>
Signed-off-by: Dr. Stefan Schimanski <sschimanski@nvidia.com>
Signed-off-by: Dr. Stefan Schimanski <sschimanski@nvidia.com>
Signed-off-by: Dr. Stefan Schimanski <sschimanski@nvidia.com>
Signed-off-by: Dr. Stefan Schimanski <sschimanski@nvidia.com>
andrewpaprotsky
left a comment
There was a problem hiding this comment.
One P2 correctness finding inline, verified by running the same real-API-server test through the complete Grove program on base 4ad02ea and head 25303c8: the base preserves the unrelated adapter, while the head deletes it.
CI note: the operator job also fails on goconst diagnostics for the repeated lpx and prefill test literals, preventing subsequent operator tests from running. The base passed with the same lint configuration. This is a separate CI blocker, not another behavioral finding: https://github.com/ai-dynamo/dynamo/actions/runs/36437691522/job/108979940999
Signed-off-by: Dr. Stefan Schimanski <sschimanski@nvidia.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: ai-dynamo/dynamo/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (30)
💤 Files with no reviewable changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughChangesThe Grove reconciliation path now passes the complete Grove delegation-aware reconciliation
Priority: ⬇️ Low Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to Delegated components can cause another graph’s scaling adapter to be deleted when names collide. Protect adapter ownership before merging. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 18.02% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 111 functions across 28 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Warning Some tools did not complete. Review the errors below. 🔧 golangci-lint (2.13.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
Check controller ownership and observed-object preconditions before deleting disabled or orphaned scaling adapters. Derive elastic-EP Service identity from the rendered PCS so legacy workers remain reachable. Update the cluster-test scaler caller, add regression coverage, and remove redundant comments and assertions requested in review. Signed-off-by: Andrew Paprotsky <apaprotskyi@nvidia.com>
Merge upstream-lpx-integration into sttts-lpx-managed-components to restore parent ancestry while preserving the reviewed commits and fixes. Signed-off-by: Andrew Paprotsky <apaprotskyi@nvidia.com>
Signed-off-by: Andrew Paprotsky <apaprotskyi@nvidia.com>
Signed-off-by: Andrew Paprotsky <apaprotskyi@nvidia.com>
Preload the host libcuda alias for the TRTLLM checkpoint test without pinning a driver version. Signed-off-by: Andrew Paprotsky <apaprotskyi@nvidia.com>
Signed-off-by: Andrew Paprotsky <apaprotskyi@nvidia.com>
Signed-off-by: Andrew Paprotsky <apaprotskyi@nvidia.com>
Signed-off-by: Andrew Paprotsky <apaprotskyi@nvidia.com>
Signed-off-by: Andrew Paprotsky <apaprotskyi@nvidia.com>
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Approved. The head writes the same objects as the base, except one new Service label (P3, inline) and the adapter ownership fix from the earlier thread. I read c88e0b6fea.
- [P3] Services for
prefillanddecodecomponents gainnvidia.com/dynamo-sub-component-type.dgd_grove_stable_resources_reconciler.go:151. - Open: this PR is stacked on #15062 and must merge after it.
What I ran at 3546a4316e, 0a0f361285 and c88e0b6fea.
- Render: 1,952 reconcile scenarios, as the inline comment describes. In both trees, 66 of them stop at the same first error. Three causes are DRA off (30), no registered InferencePool kind (18) and checkpoints off (6). The other two are a runtime version override that is not a semantic version (6) and a container ResourceClaim without a matching pod claim (6). These scenarios compare only that error.
- Admission: the deleted
dynamographdeployment_lpx.goheld only the component filter. Its one consumer is the combined Grove name-length rule atdynamographdeployment.go:370. That rule gives the same admission results and PCS names at base and head for 976 fixture variants and a 28,980-row name-length grid. A limit raised by one changes 610 rows of that grid. - Tests: the four changed packages pass in all three trees with envtest, and Ginkgo ran 75 of 75 specs. At
c88e0b6fea, 2,754 tests pass, and none fail or skip. The 3 removed tests have replacements with the same cases. - Mutations: each of these fails named tests:
IsDelegatedinverted, a nil predicate,ManagedComponentsthat returns every component,DelegatedComponentsthat returns none, and thedynamofilter turned off. Removal of eitherIsControlledByguard in the adapter reconciler failsTestDGDScalingAdaptersReconciler_DeletesOnlyObservedOwnedAdapters. - Merges: four of the five parent merges match a redo with
git merge-treeexactly. The fifth,0a0f361285, differs only in its one conflict,tests/deploy/test_dynamocheckpoint.py, which keeps the version of the parent. deploy/operator/internal/CLAUDE.mdholds@AGENTS.md.
Signed-off-by: Andrew Paprotsky <apaprotskyi@nvidia.com>
Signed-off-by: Andrew Paprotsky <apaprotskyi@nvidia.com>
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Approved. I read 9244da86c6. The fix does what the reply on line 151 says, and the new test fails without it. One new P3 nit is inline.
- [P3] A component without
typewhose pod template setsnvidia.com/dynamo-component-typegets an emptynvidia.com/dynamo-sub-component-typelabel on its Service.dgd_grove_stable_resources_reconciler.go:149. - Fixed: the P3 at line 151. Native Services no longer carry the label. My reply on that thread has the numbers.
- Open: this PR is stacked on #15062 and must merge after it.
What I checked on the delta from 75eb210 to 9244da8.
- Shape:
9244da86c6is one commit on top of75eb210415. It changes onlydgd_grove_stable_resources_reconciler.goand its test file. A redo of the merge75eb210415withgit merge-treegives the same tree, and the PR diff is the same before and after that merge. - Render probe: 1,952 scenarios, the same as last round. The head gives the same objects, events and status as the base
5143224563. Only the write log differs. The head does not send the adapter deletes that got NotFound at the base, because of the ownership fix ind3de18eb52. - Service label probe: 450 scenarios that create Services in native and legacy mode, with and without user-set labels. Every Service at the head matches the base, except in the P3 case.
- Test:
TestGroveStableResourcesReconcilerElasticEPServicesSelectRenderedPodsfails 10 of 12 subtests on75eb210415and passes 12 of 12 at the head. Three mutants of the fix each fail their own subtests. - Whole package: Pre Merge job
operator(109789020394) ran the fullinternal/controllersuite with envtest assets onb8d3f54071, the head merged intoa41283bf39. It passed. I did not run the full suite myself. - Limits: the label probe runs each fixture as written, in Kubernetes discovery only, with one reconcile after the legacy rewrite.
Signed-off-by: Andrew Paprotsky <apaprotskyi@nvidia.com>
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Approved. I read a247e0c77c. No finding of mine is open.
- Fixed: the P3 at line 149. The fix is the guard that I suggested, so I am partly reviewing my own suggestion. My reply on that thread has the numbers.
- Open: this PR is stacked on #15062 and must merge after it.
What I checked on the delta from 9244da8 to a247e0c.
- Shape:
a247e0c77cis one commit on top of9244da86c6. It changes onlydgd_grove_stable_resources_reconciler.go(+1/-1) and its test file (+7/-1). The base5143224563did not move. - Probes: the render probe gives the same output at the head and at
9244da86c6in all 1,952 scenarios. The labels probe creates the Services of 450 scenarios. Its output differs only by the 141 empty labels that the fix removes. - Test:
TestGroveStableResourcesReconcilerElasticEPServicesSelectRenderedPodspasses 14 of 14 subtests at the head. With the guard reverted, only the 2 newtype-from-pod-labelsubtests fail. - Package: I ran all of
./internal/controller/with the envtest assets for Kubernetes 1.30.0, at the head and at9244da86c6. Both runs pass 335 tests and 75 Ginkgo specs. The only difference is the 2 new subtests. With an empty assets folder, the 6 envtest tests fail, so the passing runs used the assets. - CI: the Pre Merge
operatorjob ran the module tests with envtest onef5af56078. That commit merges the head with #15062 andmainatdf8ad66921. The job passed.
Summary
DynamoGraphDeploymentvalue complete throughout the Grove workload program.groveProgram.Reconcilethrough the optionalgroveReconcileRequest.IsDelegatedpredicate.ManagedComponents()andDelegatedComponents().deploy/operator/internal/AGENTS.md.Context
PR #15062 adds LPX as a delegated component path inside the Grove workload program. Its initial factoring projected non-LPX components into a second, filtered DGD and passed both the source and projected DGDs through the Grove reconcilers.
This stacked PR keeps the API object and its ownership boundary explicit:
Nested Grove workload reconcilers receive the request and do not reclassify LPX ownership. The PCS renderer deep-copies one selected component immediately before applying legacy compatibility and worker-hash labels. The temporary copy stays inside that render iteration. Stable-resource selectors use the synchronized PCS labels, while GPU-shape resolution and readiness use the same DGD and predicate as rendering.
Semantic behavior
groveReconcileRequestor its DGD plus predicate.IsDelegatedtreats every component as managed.There is no API, CRD, migration, or rollout-policy change. This PR is stacked on #15062 and must be rebased or retargeted when that PR changes or merges.
Relates to #14473.
Validation
CGO_ENABLED=0 go test ./internal/dynamo -count=1CGO_ENABLED=0 go test ./internal/controller/lpx -count=1./internal/controller./internal/webhook/validation./internal/controllerand./internal/webhook/validationgit diff --checkThe complete
./internal/controllerand webhook suites also contain envtests; those cannot start locally because the worktree does not containbin/k8s/1.30.0-darwin-arm64/etcd.Summary by CodeRabbit