feat(operator): hash effective runtime profiles - #12617
Conversation
Co-authored-by: Thomas Montfort <tjmontfort12@gmail.com> Signed-off-by: xianlubird <xianlubird@gmail.com>
|
👋 Hi xianlubird! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
WalkthroughThe operator now resolves runtime versions into feature profiles, propagates profiles through component contexts, and includes effective profiles in worker hashes. API and CRD documentation now describes runtime-gated Pod rendering and rollout behavior. ChangesRuntime profile and worker hashing
Estimated code review effort: 4 (Complex) | ~35 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/config/crd/bases/nvidia.com_dynamographdeployments.yaml`:
- Around line 11758-11760: Update the runtimeVersionOverride description at
deploy/operator/config/crd/bases/nvidia.com_dynamographdeployments.yaml:11758-11760
and the corresponding description at
deploy/operator/config/crd/bases/nvidia.com_dynamographdeployments.yaml:20882-20884
to permit intentional runtime override changes independently of image changes,
while retaining guidance that such changes may alter runtime-gated Pod rendering
and trigger a rollout.
In `@deploy/operator/internal/dynamo/graph_test.go`:
- Around line 1430-1448: Add t.Log headings in the relevant t.Run test flow
before the generateComponentContext call and before the assertion block,
including the runtime-profile case and the additionally referenced case. Use
concise messages describing each test step while leaving the test logic
unchanged.
In `@deploy/operator/internal/dynamo/hash_test.go`:
- Around line 95-116: The test
TestComputeLegacyAlphaDGDWorkersSpecHash_IgnoresRuntimeProfile should cover a
runtime-profile gate transition: initialize RuntimeVersionOverride with the
pre-gate version 1.3.0, then change it to the gate version 1.4.0 before
recomputing the hash. Preserve the existing equal-hash assertion and test
structure.
🪄 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: 7982c13e-6b1e-4cf7-8f2b-da48102493b9
📒 Files selected for processing (16)
deploy/operator/api/v1alpha1/dynamocomponentdeployment_types.godeploy/operator/api/v1beta1/dynamocomponentdeployment_types.godeploy/operator/config/crd/bases/nvidia.com_dynamocomponentdeployments.yamldeploy/operator/config/crd/bases/nvidia.com_dynamographdeployments.yamldeploy/operator/internal/dynamo/component_common.godeploy/operator/internal/dynamo/graph.godeploy/operator/internal/dynamo/graph_test.godeploy/operator/internal/dynamo/hash.godeploy/operator/internal/dynamo/hash_test.godeploy/operator/internal/dynamo/runtime_profile.godeploy/operator/internal/dynamo/runtime_profile_test.godeploy/operator/internal/features/runtime/gates.godeploy/operator/internal/features/runtime/profile.godeploy/operator/internal/features/runtime/profile_test.godeploy/operator/internal/runtimeversion/runtimeversion.godeploy/operator/internal/runtimeversion/runtimeversion_test.go
This comment has been minimized.
This comment has been minimized.
Signed-off-by: xianlubird <xianlubird@gmail.com>
|
/ok to test 373eb71 |
| Spec v1beta1.DynamoComponentDeploymentSpec `json:"spec"` | ||
| Labels map[string]string `json:"labels,omitempty"` | ||
| Annotations map[string]string `json:"annotations,omitempty"` | ||
| RuntimeProfile *runtimefeatures.RuntimeProfile `json:"runtimeProfile,omitempty"` |
There was a problem hiding this comment.
This needs thought why it is correct to add the gates to the hash. And further, what it means when gates changes, or gates are introduced in a version, but are false (then the hash should not change?).
And we need a plan for when gates become true, whether we tolerate automatic rollouts, in general or only by opt-in.
|
Closing in favor of #12633 |
Summary
runtimeVersionOverrideor the main image tagRuntimeProfileComponentContextso rendering and hashing can consume the same gate decisionsruntimeVersionOverridecan alter runtime-gated Pod rendering and trigger a rolloutWhy
#10494 intentionally excluded
runtimeVersionOverridefrom worker hashing because it did not affect rendered resources at that time. After #12421 introduced runtime-version feature gates, changing only the override can change the renderedPodSpec.Hashing the raw override would be too sensitive:
1.4.0and2.0.0currently enable the same gates and should not create different worker generations. This change hashes the effective rendering profile instead:1.3.01.3.0→1.4.01.4.0→2.0.01.5.0→ override1.5.0This ensures changes that affect rendered worker behavior use the managed rolling-update path without introducing rollouts for compatibility-version changes that produce identical rendering.
This is an upstream-focused version of Thomas Montfort's follow-up proposal in xianlubird#2, with the unrelated
graph_test.goconsolidation left out to keep the review surface focused.Validation
make testmake manifestsmake vetgo test ./internal/runtimeversion ./internal/features/runtime ./internal/dynamomake lintwas also attempted locally, but the repository-pinned golangci-lint v1.64.8 failed while loading the Go 1.26 module withno go files to analyze; it did not report a source finding.Related Issues
Summary by CodeRabbit
New Features
Documentation