fix(operator): remove lws worker gpu pre-validation - #11115
Conversation
Signed-off-by: Peter Pan <Peter.Pan@daocloud.io>
|
👋 Hi panpan0000! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
WalkthroughThe controller's ChangesGPU Requirement Removal
Estimated code review effort: 1 (Trivial) | ~5 minutes Related PRs: None identified. Suggested labels: None Suggested reviewers: None Poem: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
🧹 Nitpick comments (1)
deploy/operator/internal/controller/dynamocomponentdeployment_controller_test.go (1)
3459-3468: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider asserting the resulting container has no GPU resource set.
The test confirms
generateWorkerPodTemplateSpecsucceeds without a GPU limit on input, but doesn't assert that the generated container'sResources.Limits/Requestsactually omitnvidia.com/gpu. Since the test's purpose is specifically to lock in the "GPU not required" behavior, checking this closes the loop against future regressions (e.g., some other path force-defaulting a GPU limit).♻️ Suggested additional assertion
require.Equal(t, commonconsts.MainContainerName, got.Spec.Containers[0].Name) + _, hasGPULimit := got.Spec.Containers[0].Resources.Limits[corev1.ResourceName(commonconsts.KubeResourceGPUNvidia)] + require.False(t, hasGPULimit, "worker pod template should not require nvidia.com/gpu") }🤖 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/dynamocomponentdeployment_controller_test.go` around lines 3459 - 3468, The test for generateWorkerPodTemplateSpec currently verifies success and labels, but it does not lock in the “no GPU required” behavior. Update the test to assert the returned container from got.Spec.Containers[0] has no nvidia.com/gpu entry in either Resources.Limits or Resources.Requests, so future changes cannot silently add a GPU resource.
🤖 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.
Nitpick comments:
In
`@deploy/operator/internal/controller/dynamocomponentdeployment_controller_test.go`:
- Around line 3459-3468: The test for generateWorkerPodTemplateSpec currently
verifies success and labels, but it does not lock in the “no GPU required”
behavior. Update the test to assert the returned container from
got.Spec.Containers[0] has no nvidia.com/gpu entry in either Resources.Limits or
Resources.Requests, so future changes cannot silently add a GPU resource.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d031715b-eb01-4d46-89de-ddd77d36384a
📒 Files selected for processing (2)
deploy/operator/internal/controller/dynamocomponentdeployment_controller.godeploy/operator/internal/controller/dynamocomponentdeployment_controller_test.go
💤 Files with no reviewable changes (1)
- deploy/operator/internal/controller/dynamocomponentdeployment_controller.go
|
Thanks for the contribution @panpan0000. @sttts @tmonty12 @julienmancuso to help review |
|
/ok to test 6ad355e |
|
thx |
Overview:
Summary
nvidia.com/gpumaincontainer validation and leave NVIDIA-specific GMS/DRA paths unchangedDetails:
generateWorkerPodTemplateSpec()maincontainer command/args and no GPU resourceValidation
go test ./internal/controller -run TestGenerateWorkerPodTemplateSpecDoesNotRequireGPUResource -count=1go test ./internal/controller -run Test_generateDeployment_Strategy -count=1Where should the reviewer start?
deploy/operator/internal/controller/dynamocomponentdeployment_controller.godeploy/operator/internal/controller/dynamocomponentdeployment_controller_test.goRelated Issues
🔗 This PR is linked to an issue:
Summary by CodeRabbit