feat(nvca): add control-plane cluster validator role, gateway and storage checks - #781
feat(nvca): add control-plane cluster validator role, gateway and storage checks#781rohithb-hub wants to merge 6 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe cluster validator now supports compute-plane and control-plane roles. Control-plane validation adds storage, Gateway API, Envoy Gateway, load-balancer, and node-overlay checks. The CLI creates a dynamic client and parses ChangesRole-aware cluster validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ClusterValidatorCLI
participant clustervalidator.Run
participant KubernetesAPI
participant ValidationSummary
ClusterValidatorCLI->>clustervalidator.Run: pass dynamic client and role
clustervalidator.Run->>KubernetesAPI: execute role-specific checks
KubernetesAPI-->>clustervalidator.Run: return check results
clustervalidator.Run->>ValidationSummary: build role-specific rows and readiness
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (10)
src/compute-plane-services/nvca/internal/clustervalidator/checks.go (3)
876-877: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the Gateway API install URL to a version.
The recommendation points at
releases/latest/download/standard-install.yaml.latestmoves. An operator who follows this text months from now can install a Gateway API version that differs from the one the validator expects, which reproduces the failure the recommendation was meant to resolve. Reference the minimum supported release tag instead.🤖 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 `@src/compute-plane-services/nvca/internal/clustervalidator/checks.go` around lines 876 - 877, Update the Gateway API installation recommendation in the validator checks to replace the moving releases/latest URL with the minimum supported Gateway API release tag, preserving the standard-install.yaml asset path.
936-949: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider the Ready condition instead of the Running phase.
Status.Phase == corev1.PodRunningis true for a pod whose container is restarting or failing its readiness probe. The check reports "Installed and Running" for a gateway controller that serves no traffic. Counting pods whosePodReadycondition isTruegives an accurate signal. The row is non-critical, so this affects the operator's diagnosis rather than the verdict.🤖 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 `@src/compute-plane-services/nvca/internal/clustervalidator/checks.go` around lines 936 - 949, The pod health count in the gateway validation check should use each pod’s Ready condition being True instead of Status.Phase == corev1.PodRunning. Update the running-count loop near EnvoyGatewayOK to count ready pods while preserving the existing logging, no-pods message, and non-critical verdict behavior.
1116-1117: 🎯 Functional Correctness | 🔵 Trivial | ⚖️ Poor tradeoffNode selection takes the first two schedulable nodes.
schedulable[0]andschedulable[1]follow API list order. On a multi-zone cluster those two nodes are frequently in the same zone, so the probe passes while cross-zone overlay traffic is broken. The check reports "Node-to-Node Communication: Verified" for a partially broken overlay.Selecting two nodes with different
topology.kubernetes.io/zonelabels when such a pair exists would make the single probe far more informative. Record the chosen pair in the success message so the operator knows what was actually tested.🤖 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 `@src/compute-plane-services/nvca/internal/clustervalidator/checks.go` around lines 1116 - 1117, Update the node selection around nodeA and nodeB so it prefers a pair from different topology.kubernetes.io/zone labels when available, while retaining the existing first-two schedulable nodes as a fallback. Include the selected node names in the successful “Node-to-Node Communication: Verified” message so the tested pair is explicit.src/compute-plane-services/nvca/internal/clustervalidator/checks_controlplane_test.go (5)
270-278: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the
initfunction; it does nothing and its comment is incorrect.The function builds a slice literal and discards it. Constructing
runtime.Objectvalues does not register anything with the fake client's object tracker.fake.NewSimpleClientsetresolves types through the generated scheme ink8s.io/client-go/kubernetes/fake, which registers the built-in types in its own package initialization. The tests above already passstoragev1.StorageClass,corev1.Namespace,corev1.Pod, andcorev1.Servicevalues toNewSimpleClientsetand they work for that reason.The function also does not keep any import alive:
storagev1,corev1, andruntimeare each referenced by the tests directly.The comment states a requirement that does not exist. A future maintainer may copy this pattern into new test files.
🧹 Proposed removal
- -// init is required to register types with the fake client's object tracker. -func init() { - _ = []runtime.Object{ - &storagev1.StorageClass{}, - &corev1.Namespace{}, - &corev1.Pod{}, - &corev1.Service{}, - } -}🤖 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 `@src/compute-plane-services/nvca/internal/clustervalidator/checks_controlplane_test.go` around lines 270 - 278, Remove the no-op init function and its misleading comment from the test file. Leave the existing storagev1, corev1, and runtime imports unchanged where they are still referenced by the tests and NewSimpleClientset calls.
37-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the StorageClass cases into a table-driven test.
The four functions share one shape: seed StorageClasses, run
checkStorageClass, assert the resulting bool and the recommendations. The repository guideline asks for table-driven tests when several scenarios differ only in inputs and expectations.The table also makes the missing branch visible: no test covers the
Listerror path. That path is the subject of thechecks.goLine 824-830 comment, so a case there would pin the corrected behavior.♻️ Proposed table-driven form
func TestCheckStorageClass(t *testing.T) { tests := []struct { name string objects []runtime.Object wantOK bool wantRecommend bool }{ { name: "default annotation present", objects: []runtime.Object{&storagev1.StorageClass{ObjectMeta: metav1.ObjectMeta{ Name: "standard", Annotations: map[string]string{"storageclass.kubernetes.io/is-default-class": "true"}, }}}, wantOK: true, }, { name: "beta annotation accepted", objects: []runtime.Object{&storagev1.StorageClass{ObjectMeta: metav1.ObjectMeta{ Name: "local-path", Annotations: map[string]string{"storageclass.beta.kubernetes.io/is-default-class": "true"}, }}}, wantOK: true, }, { name: "class present but not default", objects: []runtime.Object{&storagev1.StorageClass{ ObjectMeta: metav1.ObjectMeta{Name: "no-annotation-class"}, }}, wantOK: false, wantRecommend: true, }, { name: "no storage classes", wantOK: false, wantRecommend: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { client := fake.NewSimpleClientset(tt.objects...) state := &ValidationState{Log: testLog()} checkStorageClass(context.Background(), client, state) require.NotNil(t, state.DefaultStorageClassOK) assert.Equal(t, tt.wantOK, *state.DefaultStorageClassOK) assert.Equal(t, tt.wantRecommend, len(state.Recommendations) > 0) }) } }As per coding guidelines: "use table-driven tests for multiple scenarios".
🤖 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 `@src/compute-plane-services/nvca/internal/clustervalidator/checks_controlplane_test.go` around lines 37 - 89, Consolidate the four StorageClass tests into a table-driven TestCheckStorageClass using shared setup and assertions, preserving each scenario’s expected DefaultStorageClassOK and recommendation results. Add a List-error case by configuring the fake client to return an error for StorageClass listing, and assert the corrected behavior expected from checkStorageClass, including its recommendation outcome.Source: Coding guidelines
253-268: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the deferred cleanup deletes the probe pods.
TestCheckNodeToNode_ServerPodCreateFailurecovers the create-failure path but does not verify cleanup. Pod cleanup is the fragile part ofcheckNodeToNode: the server pod runs an infinitencloop and only the deferred delete removes it. A test that inspects the recorded actions would pin that contract.Use the fake clientset action log after a run where the server pod is created but never becomes ready.
💚 Proposed additional test
func TestCheckNodeToNode_DeletesProbePodsOnFailure(t *testing.T) { client := fake.NewSimpleClientset( makeNode("node-1", true, 0), makeNode("node-2", true, 0), ) // Creates succeed; the server pod never becomes Ready, so the check // bails out after waitForPodReady and the deferred cleanup must run. state := &ValidationState{Log: testLog()} checkNodeToNode(context.Background(), client, state) var deleted []string for _, a := range client.Actions() { if d, ok := a.(ktesting.DeleteAction); ok && d.GetResource().Resource == "pods" { deleted = append(deleted, d.GetName()) } } assert.NotEmpty(t, deleted, "the deferred cleanup must delete the probe pods") }Confirm the
nodeToNodePodTimeoutof 90 s does not make this test slow; ifwaitForPodReadypolls for the full timeout, inject a shorter duration or stub the wait helper as the file already does for probes elsewhere.🤖 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 `@src/compute-plane-services/nvca/internal/clustervalidator/checks_controlplane_test.go` around lines 253 - 268, Add a cleanup-focused test near TestCheckNodeToNode_ServerPodCreateFailure that lets probe pod creation succeed while pods remain unready, then runs checkNodeToNode and inspects client.Actions() for pod DeleteAction entries. Assert at least one probe pod is deleted, and use the file’s existing timeout or waitForPodReady test seam to keep the test from waiting the full nodeToNodePodTimeout.
144-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd dynamic fake-client coverage for
checkGatewayRoutes.Test the list-error, empty-list, and populated-list branches, including the
HTTPRoutelist kind and all-namespacesNamespace("")call. Add the dynamic fake dependency to theclustervalidator_testBazel target.🤖 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 `@src/compute-plane-services/nvca/internal/clustervalidator/checks_controlplane_test.go` around lines 144 - 151, Extend TestCheckGatewayRoutes coverage with a dynamic fake client for list-error, empty-list, and populated-list cases, verifying HTTPRoute listing uses the HTTPRoute kind and Namespace(""). Add the required dynamic fake dependency to the clustervalidator_test Bazel target.
91-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the populated
FakeDiscoverypaths.Add tests for the all-resources-present and partial-resource cases. Set
Resourceson the embeddedtesting.Fakeand assertGatewayAPICRDsOKfor both outcomes. Addclient-go/discovery/faketoBUILD.bazel.🤖 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 `@src/compute-plane-services/nvca/internal/clustervalidator/checks_controlplane_test.go` around lines 91 - 106, Extend TestCheckGatewayAPICRDs_AbsentOnFakeClient coverage with tests using discovery fake clients whose embedded testing.Fake Resources contain all required Gateway API resources and only a subset, asserting GatewayAPICRDsOK is true and false respectively. Configure Resources on the embedded fake for each case, and add the client-go/discovery/fake dependency to BUILD.bazel.src/compute-plane-services/nvca/internal/clustervalidator/validator_test.go (1)
166-184: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a
NodeToNodeOKcase.
NodeToNodeOKis the second critical control-plane row (validator.goLine 296-299), and no subtest sets it. A change that flips that row to non-critical would pass this suite. TheDefaultStorageClassOKcase already establishes the pattern.💚 Proposed additional subtest
t.Run("node-to-node failure blocks readiness", func(t *testing.T) { fail := false ok := true state := &ValidationState{ Log: testLog(), Role: RoleControlPlane, ControlPlaneHealthy: true, NodesAllReady: true, WebhooksSupported: true, NetworkPoliciesSupported: true, DefaultStorageClassOK: &ok, GatewayAPICRDsOK: &ok, NodeToNodeOK: &fail, K8sVersion: "v1.30.0", TotalNodes: "2", } err := printSummary(state) assert.Error(t, err, "failed node-to-node connectivity must block control-plane readiness") })🤖 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 `@src/compute-plane-services/nvca/internal/clustervalidator/validator_test.go` around lines 166 - 184, Add a `NodeToNodeOK` failure subtest alongside the existing control-plane readiness cases, following the `DefaultStorageClassOK` pattern: keep other critical checks healthy, set `NodeToNodeOK` to false, call `printSummary`, and assert that it returns an error.src/compute-plane-services/nvca/internal/clustervalidator/validator.go (1)
121-131: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a parameter struct for
Run.
Runnow takes four string parameters, one bool, and two clients.configNamespace,configName,summaryNamespace, androleare allstring, so a transposed argument compiles and fails only at runtime. A smallRunOptionsstruct would make each call site self-documenting and prevent silent transposition when the next option is added.🤖 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 `@src/compute-plane-services/nvca/internal/clustervalidator/validator.go` around lines 121 - 131, Introduce a RunOptions struct containing configNamespace, configName, summaryNamespace, emitMetrics, and role, then update Run to accept this options value alongside the context and clients. Update every Run call site to populate fields by name and adjust the implementation to read from the options struct, preserving existing validation behavior.
🤖 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 `@src/compute-plane-services/nvca/cmd/cluster-validator/main.go`:
- Around line 86-108: Update parseRole and its caller to distinguish an unset
VALIDATOR_ROLE from a non-empty unrecognized value, preserving the compute-plane
default for both but logging a warning for the latter. Use the existing logging
mechanism to identify the rejected value, and update the related test
expectations so inputs such as “control_plane” verify the warning behavior.
- Around line 52-56: Declare dynClient as dynamic.Interface before calling
dynamic.NewForConfig, and assign the constructed client only on successful
creation. Preserve the existing warning and nil assignment on failure so
checkGatewayRoutes receives a genuinely nil interface and its guard prevents
List from being called.
- Around line 43-46: Update the Kubernetes client initialization around
internalutil.NewK8sClient so the dynamic client is declared as a
dynamic.Interface and assigned only when client creation succeeds. Preserve the
existing error handling, and ensure the value passed to downstream validation
cannot be a typed-nil dynamic client when initialization fails.
In `@src/compute-plane-services/nvca/internal/clustervalidator/checks.go`:
- Around line 824-830: Update the StorageClasses().List error handling at
src/compute-plane-services/nvca/internal/clustervalidator/checks.go#L824-L830 to
leave DefaultStorageClassOK nil and append a warning that the default
StorageClass status is unknown, omitting the critical row instead of marking it
failed. At
src/compute-plane-services/nvca/internal/clustervalidator/checks.go#L1092-L1098,
update the Nodes().List error handling to leave NodeToNodeOK nil and append a
warning that overlay connectivity is unverified, following the three-state
behavior used by checkControlPlaneHealth and rendered by printSummary.
- Around line 1230-1235: Run gofmt on the composite literal containing the
client container in the clustervalidator checks code, ensuring the contiguous
Name, Image, Command, and Resources fields align to the longest key. Do not
change their values or behavior.
- Around line 1191-1239: Update buildNodeToNodeServerPod and
buildNodeToNodeClientPod so both probe containers use a restricted-compliant
security context: run as non-root, disallow privilege escalation, drop all
capabilities, and use RuntimeDefault seccomp while retaining the non-privileged
port. Also change the managed-by label from nvcf-cli to the cluster-validator
identity used for these pods, consistently in both builders.
- Around line 1119-1129: Update the node-to-node probe setup around the
serverName/clientName generation and pod builders to replace the wrapping
UnixNano suffix with a collision-resistant suffix using
k8s.io/apimachinery/pkg/util/rand, and add the matching Bazel dependency. Set
ActiveDeadlineSeconds on both probe pods so the API server terminates them if
deferred cleanup never runs; preserve the existing cleanup behavior and pod
naming structure.
In `@src/compute-plane-services/nvca/internal/clustervalidator/validator_test.go`:
- Around line 101-130: Replace the non-asserting
TestRun_ControlPlaneRoleSkipsGPUChecks with a test that verifies role dispatch
state: initialize ValidationState with RoleControlPlane, run the relevant
control-plane check using the existing test logger and fake client, then assert
DefaultStorageClassOK is non-nil and GPUAvailable remains false. Alternatively,
add the proposed TestRun_ControlPlaneRoleRunsControlPlaneChecks alongside the
existing test and remove the unused err assignment.
In `@src/compute-plane-services/nvca/internal/clustervalidator/validator.go`:
- Around line 177-192: The control-plane validation flow currently runs the
critical checkNodeToNode probe during preflight, where pod creation may be
unauthorized. Update the Run flow and control-plane branch to skip
checkNodeToNode when emitMetrics is false, while preserving it for normal
in-cluster runs; keep the existing checkNodeToNode behavior unchanged otherwise.
- Around line 80-91: The buildSummary path must propagate all six control-plane
results—DefaultStorageClassOK, GatewayAPICRDsOK, EnvoyGatewayOK,
GatewayRoutesOK, ExternalLBOK, and NodeToNodeOK—into ValidatorSummary.Checks.
Add stable CheckKey constants, include them in AllCheckKeys and
clusterValidatorCheckKeys(), and map each pointer only when non-nil; update the
summary tests to cover these entries.
---
Nitpick comments:
In
`@src/compute-plane-services/nvca/internal/clustervalidator/checks_controlplane_test.go`:
- Around line 270-278: Remove the no-op init function and its misleading comment
from the test file. Leave the existing storagev1, corev1, and runtime imports
unchanged where they are still referenced by the tests and NewSimpleClientset
calls.
- Around line 37-89: Consolidate the four StorageClass tests into a table-driven
TestCheckStorageClass using shared setup and assertions, preserving each
scenario’s expected DefaultStorageClassOK and recommendation results. Add a
List-error case by configuring the fake client to return an error for
StorageClass listing, and assert the corrected behavior expected from
checkStorageClass, including its recommendation outcome.
- Around line 253-268: Add a cleanup-focused test near
TestCheckNodeToNode_ServerPodCreateFailure that lets probe pod creation succeed
while pods remain unready, then runs checkNodeToNode and inspects
client.Actions() for pod DeleteAction entries. Assert at least one probe pod is
deleted, and use the file’s existing timeout or waitForPodReady test seam to
keep the test from waiting the full nodeToNodePodTimeout.
- Around line 144-151: Extend TestCheckGatewayRoutes coverage with a dynamic
fake client for list-error, empty-list, and populated-list cases, verifying
HTTPRoute listing uses the HTTPRoute kind and Namespace(""). Add the required
dynamic fake dependency to the clustervalidator_test Bazel target.
- Around line 91-106: Extend TestCheckGatewayAPICRDs_AbsentOnFakeClient coverage
with tests using discovery fake clients whose embedded testing.Fake Resources
contain all required Gateway API resources and only a subset, asserting
GatewayAPICRDsOK is true and false respectively. Configure Resources on the
embedded fake for each case, and add the client-go/discovery/fake dependency to
BUILD.bazel.
In `@src/compute-plane-services/nvca/internal/clustervalidator/checks.go`:
- Around line 876-877: Update the Gateway API installation recommendation in the
validator checks to replace the moving releases/latest URL with the minimum
supported Gateway API release tag, preserving the standard-install.yaml asset
path.
- Around line 936-949: The pod health count in the gateway validation check
should use each pod’s Ready condition being True instead of Status.Phase ==
corev1.PodRunning. Update the running-count loop near EnvoyGatewayOK to count
ready pods while preserving the existing logging, no-pods message, and
non-critical verdict behavior.
- Around line 1116-1117: Update the node selection around nodeA and nodeB so it
prefers a pair from different topology.kubernetes.io/zone labels when available,
while retaining the existing first-two schedulable nodes as a fallback. Include
the selected node names in the successful “Node-to-Node Communication: Verified”
message so the tested pair is explicit.
In `@src/compute-plane-services/nvca/internal/clustervalidator/validator_test.go`:
- Around line 166-184: Add a `NodeToNodeOK` failure subtest alongside the
existing control-plane readiness cases, following the `DefaultStorageClassOK`
pattern: keep other critical checks healthy, set `NodeToNodeOK` to false, call
`printSummary`, and assert that it returns an error.
In `@src/compute-plane-services/nvca/internal/clustervalidator/validator.go`:
- Around line 121-131: Introduce a RunOptions struct containing configNamespace,
configName, summaryNamespace, emitMetrics, and role, then update Run to accept
this options value alongside the context and clients. Update every Run call site
to populate fields by name and adjust the implementation to read from the
options struct, preserving existing validation 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: b3725ae2-5293-4168-b4c7-6f935a006d4a
📒 Files selected for processing (8)
src/compute-plane-services/nvca/cmd/cluster-validator/BUILD.bazelsrc/compute-plane-services/nvca/cmd/cluster-validator/main.gosrc/compute-plane-services/nvca/cmd/cluster-validator/main_test.gosrc/compute-plane-services/nvca/internal/clustervalidator/BUILD.bazelsrc/compute-plane-services/nvca/internal/clustervalidator/checks.gosrc/compute-plane-services/nvca/internal/clustervalidator/checks_controlplane_test.gosrc/compute-plane-services/nvca/internal/clustervalidator/validator.gosrc/compute-plane-services/nvca/internal/clustervalidator/validator_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/compute-plane-services/nvca/internal/clustervalidator/checks.go`:
- Around line 1199-1207: The node-to-node probe security context lacks an
explicit nonzero user, causing BusyBox containers to be rejected with
RunAsNonRoot. Update nodeToNodeSecurityContext to set RunAsUser to a nonzero
UID, and update both node-to-node pod builders to assert the resulting RunAsUser
and RunAsNonRoot security fields.
🪄 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: 39333b85-811d-4b41-bf40-0d960a82d5ef
📒 Files selected for processing (3)
src/compute-plane-services/nvca/internal/clustervalidator/checks.gosrc/compute-plane-services/nvca/internal/clustervalidator/summary.gosrc/compute-plane-services/nvca/internal/clustervalidator/validator_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- src/compute-plane-services/nvca/internal/clustervalidator/validator_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/compute-plane-services/nvca/internal/clustervalidator/checks.go`:
- Line 1205: Update the inline comment for runAsUser to replace the non-ASCII em
dash with ASCII punctuation, preserving the existing meaning and concise
wording.
🪄 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: d0111a42-f159-43cc-8c72-863aa5283e3d
📒 Files selected for processing (1)
src/compute-plane-services/nvca/internal/clustervalidator/checks.go
1 participant
TL;DR
Extends the cluster-validator binary to support a control-plane role via a new
VALIDATOR_ROLE env var. When set to control-plane, the validator skips GPU and
SMB checks and runs gateway infrastructure and storage checks instead, making
one image serve both cluster types.
Additional Details
The existing validator always ran GPU/SMB checks, which produce false failures
when pointed at a control-plane cluster that has no GPU resources. A role switch
in Run() branches the check set based on the env var value.
New checks when VALIDATOR_ROLE=control-plane:
parseRole in main.go normalizes VALIDATOR_ROLE. dynamic.Interface is built from
the same REST config as the typed client and passed to Run() for HTTPRoute
listing. printSummary gates GPU/SMB summary rows on role so they do not appear
in control-plane run output.
BUILD.bazel updated with runtime/schema and client-go/dynamic deps.
For the Reviewer
internal/clustervalidator/validator.go: Run() gains dynClient and role params, ValidationState gets new pointer-bool fields for control-plane checks, printSummary branches on roleinternal/clustervalidator/checks.go: five new check functions plus node-to-node probe pod builders reusing busybox and the waitForPodReady/getPodIP/waitForPodDone helpers from enforcement.gocmd/cluster-validator/main.go: parseRole, dynamic client construction from the existing REST configinternal/clustervalidator/checks_controlplane_test.go: new file covering all new check functionsFor QA
Tested on k3d cluster with NVCF stack already deployed (single-cluster mode).
Output from control-plane run:
Full end-to-end QA covered by the companion nvcf-cli PR that wires VALIDATOR_ROLE into the Job env.
Issues
NO-REF
Checklist
Summary by CodeRabbit
New Features
Bug Fixes