feat(operator): add runtimeVersionOverride to DGD components - #10494
Conversation
110cef6 to
d522934
Compare
WalkthroughThis PR adds a new optional ChangesRuntime Version Tracking for Dynamo Components
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
deploy/helm/charts/platform/components/operator/crds/nvidia.com_dynamographdeployments.yaml (1)
20659-20659: 💤 Low valueMinor capitalization inconsistency in field description.
This description starts with lowercase "runtimeVersion" while the first occurrence (line 11674) starts with uppercase "RuntimeVersion". For consistency, consider using the same capitalization in both locations. The lowercase form aligns better with standard Kubernetes field description conventions.
📝 Suggested consistency fix
- description: |- - runtimeVersion is the Dynamo runtime version for this component to determine the + description: |- + RuntimeVersion is the Dynamo runtime version for this component to determine theOr alternatively, update line 11674 to use lowercase for both.
🤖 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/helm/charts/platform/components/operator/crds/nvidia.com_dynamographdeployments.yaml` at line 20659, Update the field description text so the capitalization of the field name is consistent: change the lowercase "runtimeVersion" in the description at the second occurrence to match the preferred style (use "runtimeVersion" in lower camel case) or alternatively change the first occurrence "RuntimeVersion" to "runtimeVersion"; locate the two descriptions referencing the Dynamo runtime version (search for "runtimeVersion" and "RuntimeVersion" in the CRD) and make both descriptions use the same lowercase "runtimeVersion".
🤖 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/helm/charts/platform/components/operator/crds/nvidia.com_dynamocomponentdeployments.yaml`:
- Around line 11379-11386: The description for the CRD field currently uses
"RuntimeVersion" (PascalCase) but the actual field name is runtimeVersion
(camelCase); update the description text for the runtimeVersion property (the
description block under the runtimeVersion schema) to use "runtimeVersion"
everywhere instead of "RuntimeVersion", and make the same replacement in the
other occurrence noted (the hunk around lines 20231-20238) so both descriptions
consistently reference the camelCase field name.
In `@deploy/operator/config/crd/bases/nvidia.com_dynamographdeployments.yaml`:
- Line 11673: The description for the CRD field incorrectly capitalizes the
field name as "RuntimeVersion"; update the description text to use the exact
field name "runtimeVersion" (lowercase r) so it matches the actual field
`runtimeVersion` in the CRD and maintain consistency across both occurrences
(the hunk at 11673 and the matching hunk at 20658); find the description lines
associated with the `runtimeVersion` field and replace "RuntimeVersion" with
"runtimeVersion" in both places.
In `@deploy/operator/internal/webhook/defaulting/runtimeversion.go`:
- Around line 40-57: Extract the duplicated image-accessor helpers into a shared
package (e.g., internal/webhook/helpers): create exported functions
AlphaMainContainerImage(spec
*nvidiacomv1alpha1.DynamoComponentDeploymentSharedSpec) string and
BetaMainContainerImage(spec
*nvidiacomv1beta1.DynamoComponentDeploymentSharedSpec) string that contain the
current logic from alphaMainContainerImage and betaMainContainerImage, update
the defaulting code in runtimeversion.go to call helpers.AlphaMainContainerImage
/ helpers.BetaMainContainerImage, remove the duplicate implementations from
validation/runtimeversion.go and any other files, and update imports accordingly
so both defaulting and validation use the single shared helper implementation.
In `@deploy/operator/internal/webhook/validation/dynamocomponentdeployment.go`:
- Around line 49-57: The code currently discards warnings from
sharedValidator.Validate when validateAlphaRuntimeVersion fails; change the
error return to preserve those warnings by returning (warnings, err) instead of
(nil, err). Specifically, in the function that calls
sharedValidator.Validate(ctx) and then
validateAlphaRuntimeVersion(&v.deployment.Spec.DynamoComponentDeploymentSharedSpec,
"spec"), update the error path so any error from validateAlphaRuntimeVersion is
returned together with the previously collected warnings.
---
Nitpick comments:
In
`@deploy/helm/charts/platform/components/operator/crds/nvidia.com_dynamographdeployments.yaml`:
- Line 20659: Update the field description text so the capitalization of the
field name is consistent: change the lowercase "runtimeVersion" in the
description at the second occurrence to match the preferred style (use
"runtimeVersion" in lower camel case) or alternatively change the first
occurrence "RuntimeVersion" to "runtimeVersion"; locate the two descriptions
referencing the Dynamo runtime version (search for "runtimeVersion" and
"RuntimeVersion" in the CRD) and make both descriptions use the same lowercase
"runtimeVersion".
🪄 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: e1acf20f-0e21-474b-8d33-99230b68c364
📒 Files selected for processing (23)
deploy/helm/charts/platform/components/operator/crds/nvidia.com_dynamocomponentdeployments.yamldeploy/helm/charts/platform/components/operator/crds/nvidia.com_dynamographdeployments.yamldeploy/operator/api/v1alpha1/conversion_field_coverage_test.godeploy/operator/api/v1alpha1/dynamocomponentdeployment_conversion_test.godeploy/operator/api/v1alpha1/dynamocomponentdeployment_types.godeploy/operator/api/v1alpha1/dynamographdeployment_conversion_test.godeploy/operator/api/v1alpha1/shared_spec_conversion.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/runtimeversion/runtimeversion.godeploy/operator/internal/runtimeversion/runtimeversion_test.godeploy/operator/internal/webhook/defaulting/dynamocomponentdeployment_handler.godeploy/operator/internal/webhook/defaulting/dynamocomponentdeployment_handler_test.godeploy/operator/internal/webhook/defaulting/dynamographdeployment_handler.godeploy/operator/internal/webhook/defaulting/dynamographdeployment_handler_test.godeploy/operator/internal/webhook/defaulting/runtimeversion.godeploy/operator/internal/webhook/validation/dynamocomponentdeployment.godeploy/operator/internal/webhook/validation/dynamocomponentdeployment_test.godeploy/operator/internal/webhook/validation/dynamographdeployment.godeploy/operator/internal/webhook/validation/dynamographdeployment_test.godeploy/operator/internal/webhook/validation/runtimeversion.godocs/kubernetes/api-reference.md
b7b16a9 to
1888281
Compare
1888281 to
63f1d2a
Compare
nv-tusharma
left a comment
There was a problem hiding this comment.
Approving .github and deploy pytest changes
Signed-off-by: Dr. Stefan Schimanski <sschimanski@nvidia.com>
Co-authored-by: Thomas Montfort <tjmontfort12@gmail.com> Signed-off-by: Dr. Stefan Schimanski <sschimanski@nvidia.com>
Signed-off-by: Dr. Stefan Schimanski <sschimanski@nvidia.com>
|
/ok to test 8bc8a19 |
@sttts, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
|
/ok to test 8bc8a19 |
@sttts, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
|
/ok to test f7a03f6 |
|
/ok to test 84e22b6 |
Signed-off-by: Dr. Stefan Schimanski <sschimanski@nvidia.com>
|
/ok to test a764b96 |
|
/ok to test b3d1a9c |
Summary
Adds
runtimeVersionOverrideas an explicit declaration of Dynamo runtime compatibility when an image tag does not provide it.The override controls version-dependent operator behavior only. It never rewrites or pins the image reference.
Changes by layer
API and CRDs
runtimeVersionOverrideto:v1alpha1andv1beta1, including conversion.1.4.0.Admission
mainimage; a non-semver image requiresruntimeVersionOverride.mainimage but does not require the override because DCDs are controller-generated.spec.imagehas no parseable semantic-version tag.autoApply: falseflow:DGDR controller and profiler
Documentation and test support
Validation
go test ./internal/runtimeversion ./api/v1alpha1 -count=1KUBEBUILDER_ASSETS=<envtest assets> go test ./internal/webhook/validation ./internal/controller -count=1make SHELL=/bin/sh manifestsmake SHELL=/bin/sh generate-api-docsgit diff --checkTracking