fix(operator): hash resolved runtime versions - #12633
Conversation
This comment has been minimized.
This comment has been minimized.
|
/ok to test ae7779b |
|
/ok to test 0d9087a |
|
/ok to test 2f61332 |
WalkthroughChangesRuntime version hashing
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
deploy/operator/internal/runtimeversion/runtimeversion_test.go (1)
108-145: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
t.Logheadings for each test step.Each new test has setup, execution, and assertion blocks without the required story headings.
deploy/operator/internal/runtimeversion/runtimeversion_test.go#L108-L145: Add headings for case setup and resolver verification.deploy/operator/internal/dynamo/hash_test.go#L299-L364: Add headings for DGD construction and hash comparison.As per coding guidelines, “In Go tests, use
t.Logto explain the test's story, with one heading before each block implementing a test step.”🤖 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/runtimeversion/runtimeversion_test.go` around lines 108 - 145, The table-driven TestResolve cases in deploy/operator/internal/runtimeversion/runtimeversion_test.go lines 108-145 need t.Log headings for the setup and resolver verification steps; add one heading before each corresponding block. Apply the same requirement in deploy/operator/internal/dynamo/hash_test.go lines 299-364 by adding headings before DGD construction and hash comparison, without changing test behavior.Source: Coding guidelines
deploy/operator/internal/dynamo/hash_test.go (1)
300-314: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the local fixture factory closure.
newDGDcontains bespoke fixture construction. Move this logic to a named test helper that creates a fresh DGD for each call. Keep the table inputs local to this test.As per coding guidelines, “In Go tests, avoid hiding bespoke test logic in closures; reserve closures for standard helpers such as
Eventually.”🤖 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/dynamo/hash_test.go` around lines 300 - 314, The local newDGD closure should be replaced with a named test helper that constructs and returns a fresh DynamoGraphDeployment on every invocation, preserving the current image and runtime-version override setup. Keep the table-specific inputs in the test and use the named helper from the test cases instead of hiding bespoke fixture logic in a closure.Source: Coding guidelines
🤖 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_dynamocomponentdeployments.yaml`:
- Line 11458: Update the v1alpha1 description at
deploy/operator/config/crd/bases/nvidia.com_dynamocomponentdeployments.yaml:11458
and the v1beta1 description at
deploy/operator/config/crd/bases/nvidia.com_dynamocomponentdeployments.yaml:20422-20423
to state that rollout-triggering changes apply only when the resolved runtime
version is 1.5.0 or newer; explicitly exclude unresolved and pre-1.5.0 versions
while preserving the existing image consistency guidance.
---
Nitpick comments:
In `@deploy/operator/internal/dynamo/hash_test.go`:
- Around line 300-314: The local newDGD closure should be replaced with a named
test helper that constructs and returns a fresh DynamoGraphDeployment on every
invocation, preserving the current image and runtime-version override setup.
Keep the table-specific inputs in the test and use the named helper from the
test cases instead of hiding bespoke fixture logic in a closure.
In `@deploy/operator/internal/runtimeversion/runtimeversion_test.go`:
- Around line 108-145: The table-driven TestResolve cases in
deploy/operator/internal/runtimeversion/runtimeversion_test.go lines 108-145
need t.Log headings for the setup and resolver verification steps; add one
heading before each corresponding block. Apply the same requirement in
deploy/operator/internal/dynamo/hash_test.go lines 299-364 by adding headings
before DGD construction and hash comparison, without changing test behavior.
🪄 Autofix
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: cbee4843-3148-4380-95c6-61a4fd5cf70f
📒 Files selected for processing (8)
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/hash.godeploy/operator/internal/dynamo/hash_test.godeploy/operator/internal/runtimeversion/runtimeversion.godeploy/operator/internal/runtimeversion/runtimeversion_test.go
|
/ok to test 0d6f4b6 |
There was a problem hiding this comment.
I like this approach better than the RuntimeProfile one — hashing the resolved runtime version is simpler, and it handles image-derived and explicit versions consistently.
One small thought: 1.5.0 is defined both as the hash floor and as the first runtime gate version. Maybe we can keep that in one place, or add a test to make sure they don't drift later.
It might also be worth adding a quick test or note for v1.5.0 prerelease/nightly images, since they may roll once when the operator starts using version hashing.
Otherwise, looks good to me.
|
/ok to test 57f67b3 |
It doesn't make sense to define the canary health check like the following: CanaryHealthChecks = Gate{
Name: "CanaryHealthChecks",
MinRuntimeVersion: minimumHashedRuntimeVersion
}@sttts is helping add golden test manifests for previous runtime versions so we can prevent gates that would change rendering of older runtime version PodSpecs. I've also added a comment in gates.go to be explicit that a gate can only be defined for the current version or newer. I think this is sufficient.
The release artifacts documentation is explicit that the prerelease/nightlies are purely experimental and not for production use. We are only concerned about preventing rollouts on production releases. I don't think this requires further documentation. |
|
/ok to test 03cf0b5 |
3e904c7 to
a37ff69
Compare
|
/ok to test a37ff69 |
a37ff69 to
d4ab904
Compare
|
/ok to test d4ab904 |
d4ab904 to
4b4cefc
Compare
|
/ok to test 4b4cefc |
4b4cefc to
7c8780f
Compare
|
/ok to test 7c8780f |
There was a problem hiding this comment.
The latest shape looks good overall, and I do not see a new functional blocker beyond the accepted v1-only migration limitation. I have two follow-ups before approval:
-
The latest force-push reintroduced the test-style issues that were fixed earlier. TestComputeBetaDGDWorkersSpecHash_UsesResolvedRuntimeVersion uses a local newDGD closure, and the new runtime-version, hash, and rollout tests are missing the required t.Log story headings. Please restore the named helper and test narration required by deploy/operator/AGENTS.md.
-
Please make the public field documentation explicit that only resolved runtime versions 1.5.0 or newer are included in the worker hash. The current wording, changing it may trigger a rollout, does not explain that unresolved and pre-1.5.0 versions remain excluded.
Signed-off-by: Thomas Montfort <tjmontfort12@gmail.com>
7c8780f to
6af032d
Compare
|
/ok to test 6af032d |
|
Addressed in
I intentionally kept the public field documentation at that user-visible behavior; it does not expose the worker-hash implementation or enumerate the unresolved/pre- |
Summary
Include the canonical resolved runtime version in the v2 worker hash.
runtimeVersionOverridewhen present, otherwise from the main image tag.>= 1.5.0.runtimeVersionOverride, so an image-derived version and an equivalent explicit override produce the same hash.Behavior
The resolved runtime version becomes part of the v2 worker-generation fingerprint.
1.5.0to1.5.1changes the v2 hash and triggers the normal managed rollout.1.5.0override when the image already resolves to1.5.0leaves the v2 hash unchanged and does not roll workers.This PR does not add migration state, target-hash annotations, or worker-rollout restructuring.
Validation
go test -count=1 -vet=off ./internal/dynamo ./internal/runtimeversiongo test -count=1 -vet=off ./internal/controller -run '^TestShouldTriggerRollingUpdate_UsesResolvedRuntimeVersion$'git diff --check