Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

validators/deployment/expected_resources.go (3)

164-188: ⚠️ Potential issue | 🟠 Major

Wrap namespace lookup in a bounded context.

verifyNamespacesActive calls ctx.Clientset.CoreV1().Namespaces().Get(ctx.Ctx, ...) without a timeout. If the API server stalls, this probe can consume the entire deployment-validation budget and hang the validator job.

In contrast, verifyDRAKubeletPluginReady correctly uses ctx.Timeout(defaults.ResourceVerificationTimeout). For consistency and resilience, apply the same pattern here.

🔧 Suggested fix

 func verifyNamespacesActive(ctx *validators.Context, refs []recipe.ComponentRef) []string {
 	var failures []string
 	seen := make(map[string]bool, len(refs))

+	verifyCtx, cancel := ctx.Timeout(defaults.ResourceVerificationTimeout)
+	defer cancel()
+
 	for _, ref := range refs {
 		if ref.Namespace == "" || seen[ref.Namespace] {
 			continue
 		}
 		seen[ref.Namespace] = true

-		ns, err := ctx.Clientset.CoreV1().Namespaces().Get(ctx.Ctx, ref.Namespace, metav1.GetOptions{})
+		ns, err := ctx.Clientset.CoreV1().Namespaces().Get(verifyCtx, ref.Namespace, metav1.GetOptions{})
 		if err != nil {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@validators/deployment/expected_resources.go` around lines 164 - 188,
verifyNamespacesActive currently calls
ctx.Clientset.CoreV1().Namespaces().Get(ctx.Ctx, ...) without a timeout; wrap
the lookup in a bounded context like the other validator functions by calling
ctx.Timeout(defaults.ResourceVerificationTimeout) to obtain a new context and
cancel function, defer cancel(), and pass that bounded context into
Namespaces().Get; update verifyNamespacesActive to use the new ctx from
ctx.Timeout(defaults.ResourceVerificationTimeout) for each namespace lookup to
prevent hanging.

---

391-409: ⚠️ Potential issue | 🟠 Major

Add bounded context to ClusterPolicy readiness check.

Same issue as verifySkyhookReady: the dynamic Get at line 409 uses ctx.Ctx directly. Use a bounded context for the dynamic client call.

🔧 Suggested fix

 func verifyClusterPolicyReady(ctx *validators.Context) error {
+	verifyCtx, cancel := ctx.Timeout(defaults.ResourceVerificationTimeout)
+	defer cancel()
+
 	gv := clusterPolicyGVR.GroupVersion().String()
 	_, discErr := ctx.Clientset.Discovery().ServerResourcesForGroupVersion(gv)
 	// ... discovery switch ...

 	dynClient, err := getDynamicClient(ctx)
 	if err != nil {
 		return err
 	}

-	clusterPolicy, err := dynClient.Resource(clusterPolicyGVR).Get(ctx.Ctx, clusterPolicyName, metav1.GetOptions{})
+	clusterPolicy, err := dynClient.Resource(clusterPolicyGVR).Get(verifyCtx, clusterPolicyName, metav1.GetOptions{})

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@validators/deployment/expected_resources.go` around lines 391 - 409, The
dynamic Get call using ctx.Ctx for clusterPolicy retrieval is unbounded; create
a bounded context (e.g., boundedCtx, cancel := context.WithTimeout(ctx.Ctx,
defaultTimeout) / defer cancel()) and use boundedCtx in
dynClient.Resource(clusterPolicyGVR).Get(...) instead of ctx.Ctx, ensuring you
import context and define/choose an appropriate timeout constant (or reuse an
existing one) and call cancel() to avoid leaking goroutines; adjust the code
paths around getDynamicClient, dynClient, clusterPolicyGVR and clusterPolicyName
accordingly.

---

249-273: ⚠️ Potential issue | 🟠 Major

Add bounded contexts to Skyhook readiness checks.

The discovery call (line 254) and dynamic Get calls (line 273) use ctx.Ctx directly without a timeout. Apply the same ctx.Timeout(defaults.ResourceVerificationTimeout) pattern used in verifyDRAKubeletPluginReady for consistency.

🔧 Suggested fix

 func verifySkyhookReady(ctx *validators.Context, ref recipe.ComponentRef) error {
 	expectedNames, err := expectedSkyhookNames(ref)
 	if err != nil {
 		return err
 	}
 	if len(expectedNames) == 0 {
 		return errors.New(errors.ErrCodeNotFound,
 			fmt.Sprintf("no Skyhook CR names could be extracted from component %s manifestFiles=%v",
 				ref.Name, ref.ManifestFiles))
 	}

+	verifyCtx, cancel := ctx.Timeout(defaults.ResourceVerificationTimeout)
+	defer cancel()
+
 	gv := skyhookGVR.GroupVersion().String()
-	_, discErr := ctx.Clientset.Discovery().ServerResourcesForGroupVersion(gv)
+	_, discErr := ctx.Clientset.Discovery().ServerResourcesForGroupVersion(gv)  // Note: Discovery doesn't take context
 	// ... rest of function should use verifyCtx for dynamic client calls

Note: The Discovery client doesn't accept a context parameter, but the dynamic client Get calls should use the bounded context:

-		sk, getErr := dynClient.Resource(skyhookGVR).Get(ctx.Ctx, name, metav1.GetOptions{})
+		sk, getErr := dynClient.Resource(skyhookGVR).Get(verifyCtx, name, metav1.GetOptions{})

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@validators/deployment/expected_resources.go` around lines 249 - 273, The
discovery call ServerResourcesForGroupVersion cannot take a context, but the
dynamic client Get calls do need a bounded context like
verifyDRAKubeletPluginReady uses: create a bounded context via
ctx.Timeout(defaults.ResourceVerificationTimeout) (e.g. boundedCtx :=
ctx.Timeout(defaults.ResourceVerificationTimeout) and defer
boundedCtx.Close()/cancel if required by your ctx type) and pass boundedCtx.Ctx
(or the appropriate field) into dynClient.Resource(skyhookGVR).Get(...) instead
of ctx.Ctx; keep the discovery call as-is but use the same timeout pattern
around the loop that calls getDynamicClient and dynClient.Resource(...).Get to
ensure each Get is timeboxed consistent with verifyDRAKubeletPluginReady.

tests/e2e/run.sh (2)

716-720: ⚠️ Potential issue | 🟠 Major

Guard cleanup to avoid deleting pre-existing GPU Operator resources.

cleanup_fake_gpu_operator_fixture() unconditionally deletes the ClusterPolicy and CRD. If this script runs against a cluster with a real GPU Operator installed, teardown would delete production resources.

Consider tracking whether the fixture created these objects and only deleting them if so, or checking for pre-existing resources before setup and skipping the test entirely.

🔧 Suggested approach

+# Track what the fixture created
+CREATED_GPU_OPERATOR_CRD=false
+CREATED_CLUSTER_POLICY=false
+
 setup_fake_gpu_operator_fixture() {
+  # Check if CRD already exists (real gpu-operator installed)
+  if kubectl get crd clusterpolicies.nvidia.com &>/dev/null; then
+    echo "ClusterPolicy CRD already exists (real gpu-operator?), skipping fixture"
+    return 1
+  fi
+
   kubectl create namespace gpu-operator --dry-run=client -o yaml | kubectl apply -f - 2>&1 || true
   # ... CRD apply ...
+  CREATED_GPU_OPERATOR_CRD=true
   # ... ClusterPolicy apply ...
+  CREATED_CLUSTER_POLICY=true
   # ...
 }

 cleanup_fake_gpu_operator_fixture() {
   kubectl delete deployment gpu-operator -n gpu-operator --ignore-not-found 2>&1 || true
-  kubectl delete clusterpolicy cluster-policy --ignore-not-found 2>&1 || true
-  kubectl delete crd clusterpolicies.nvidia.com --ignore-not-found 2>&1 || true
+  if [ "$CREATED_CLUSTER_POLICY" = true ]; then
+    kubectl delete clusterpolicy cluster-policy --ignore-not-found 2>&1 || true
+  fi
+  if [ "$CREATED_GPU_OPERATOR_CRD" = true ]; then
+    kubectl delete crd clusterpolicies.nvidia.com --ignore-not-found 2>&1 || true
+  fi
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/e2e/run.sh` around lines 716 - 720, The cleanup function
cleanup_fake_gpu_operator_fixture unconditionally deletes ClusterPolicy/CRD
which can remove a real GPU Operator; modify setup and cleanup so the script
first checks for pre-existing resources (e.g., run kubectl get for deployment
gpu-operator, clusterpolicy cluster-policy, and crd clusterpolicies.nvidia.com)
and record whether the fixture created them (store a boolean flag or temporary
marker). In setup, if resources already exist, set the flag to skip teardown or
abort the test; in cleanup, only run the kubectl delete lines when the recorded
flag indicates the fixture created those specific objects. Ensure the
flag/marker is uniquely tied to this test run so production resources are never
deleted by mistake.

---

713-714: ⚠️ Potential issue | 🟠 Major

Propagate deployment availability wait failure.

The kubectl wait at line 713 uses || true, which swallows the failure. If the fake gpu-operator deployment never becomes Available, subsequent tests will run against a broken fixture and fail for unrelated reasons.

🔧 Suggested fix

-  kubectl wait --for=condition=available deployment/gpu-operator -n gpu-operator --timeout=60s 2>&1 || true
+  if ! kubectl wait --for=condition=available deployment/gpu-operator -n gpu-operator --timeout=60s 2>&1; then
+    return 1
+  fi
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/e2e/run.sh` around lines 713 - 714, The kubectl wait invocation in
run.sh currently appends "|| true" which swallows failures; remove that fallback
so the non‑zero exit from kubectl wait propagates (or explicitly check its exit
code and exit with a helpful error) to halt the test run when the gpu-operator
deployment (deployment/gpu-operator -n gpu-operator) never becomes Available;
update the block containing the kubectl wait call to either drop "|| true" or
add an explicit error check/exit so downstream tests don't run against a broken
fixture.