[v0ropaev]

kubectl get nvidiadriver keeps printing ready after the Secret named by spec.secretEnv goes missing, and the driver stops being reconciled until something unrelated wakes the controller.

The branch at controllers/nvidiadriver_controller.go:183-194:

	secretName := instance.Spec.SecretEnv
	if len(secretName) > 0 {
		key := client.ObjectKey{Namespace: r.Namespace, Name: secretName}
		if err := r.Get(ctx, key, &corev1.Secret{}); err != nil {
			logger.Error(err, "failed to get secret")
			if condErr := r.conditionUpdater.SetConditionsError(ctx, instance, conditions.ReconcileFailed, err.Error()); condErr != nil {
				logger.Error(condErr, "failed to set condition")
			}
			return reconcile.Result{}, nil
		}
	}

Two things are missing, and they compound.

The state is left stale. Every other error branch in Reconcile assigns it first, at lines 98, 113, 126 (Disabled), 152, 166 and 176. This one does not, and nvDriverUpdater.SetConditionsError cannot make up for it: internal/conditions/nvidiadriver.go:104-107 copies cr.Status.State across and only falls back to NotReady when it is empty, under a comment that says whose job it is.

		// Ensure status.state is not empty when updating the CR status.
		// The caller should set the state appropriately in the CR
		// depending on the error condition.
		instance.Status.State = cr.Status.State
		if instance.Status.State == "" {
			instance.Status.State = nvidiav1alpha1.NotReady
		}

So on a fresh install with the Secret absent from the start the fallback does the right thing, and the case that breaks is the one where the CR has already reconciled once: helm upgrade --set driver.secretEnv=<name> before the Secret exists or into the wrong namespace, a rotation done as delete-then-recreate, a rename. .status.state is a printcolumn (api/nvidia/v1alpha1/nvidiadriver_types.go:522), so that is what the operator, kubectl wait and any CI gate reading it see.

And nothing comes back for it. reconcile.Result{}, nil asks for no requeue, and SetupWithManager watches NVIDIADriver, ClusterPolicy, GPUCluster and Node but not Secrets, so creating the Secret raises no event of its own. The driver stays frozen until a Node label changes, a ClusterPolicy or GPUCluster generation bumps, or the operator restarts.

The ClusterPolicy controller gets the same user error right. createSecretEnvReference at controllers/object_controls.go:3827 returns an error, which reaches clusterpolicy_controller.go:182-189 and both sets NotReady and returns the error, so the request is requeued with backoff.

The change

Set instance.Status.State = nvidiav1alpha1.NotReady the way the siblings do, and requeue after 5 seconds, the interval already used at line 222. time is already imported.

I put both halves in one commit because the stale status on its own is the less interesting symptom; what a user notices is a driver that never picks the Secret up. If you would rather have them separately, the state assignment is the uncontroversial one and I will split it out.

The requeue alone does not cover a Secret deleted after a successful reconcile (pointed out in review), since nothing schedules a run then. So SetupWithManager now also watches Secrets in the operator namespace and enqueues the NVIDIADrivers whose spec.secretEnv names the Secret, on create, update and delete. The manager cache is already limited to the operator namespace and the reconciler already reads that Secret through it, and both the kustomize and the Helm roles grant watch on secrets, so this adds no new informer or RBAC. The 5 second requeue stays as a fallback.

Test

TestEnqueueNVIDIADriversForSecret checks that a Secret event (with the Secret absent from the client, as on delete) enqueues only the driver that references it, and nothing for a Secret of the same name in another namespace.

TestReconcileMissingSecretEnvSetsNotReadyState, a fourth sibling to the three status tests already in controllers/nvidiadriver_controller_test.go, seeded with Status.State: Ready the way TestReconcileConflictSetsNotReadyState is. On main it fails on the state:

Error:      	Not equal:
            	expected: "notReady"
            	actual  : "ready"

go build ./..., go test ./controllers/ (all pass) and golangci-lint run ./controllers/... (0 issues). go test ./... fails only in tests/e2e TestE2E, which needs a live cluster and fails the same way on a clean main.

I have no GPU cluster here, so everything above is from reading the code plus the fake-client test.

---

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[coderabbitai]

📝 Walkthrough

Walkthrough

When the configured driver Secret cannot be retrieved, reconciliation marks the NVIDIADriver as NotReady, records a ReconcileFailed condition, and requests a retry after five seconds. A new test checks the status, condition message, and delayed requeue when the referenced Secret is absent.

---

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 10c8d

A driver whose Secret is missing is now marked NotReady and retried every five seconds. No merge-blocking risk was found; the test could assert more precisely.

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

---

ℹ️ Review info

⚙️ Run configuration

• Configuration used: Repository: NVIDIA/gpu-operator/.coderabbit.yaml
• Review profile: QUIET
• Plan: Enterprise
• Run ID: 840804d8-a9da-446f-8db7-6d197f12adbb

📥 Commits

Reviewing files that changed from the base of the PR and between 9eddbaa and 10c8db4.

📒 Files selected for processing (2)

• controllers/nvidiadriver_controller.go
• controllers/nvidiadriver_controller_test.go

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.

[coderabbitai]

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Assert the observable status and the five-second retry.

LastErrorState only records the in-memory value passed to FakeConditionUpdater. The test can pass even if the driver remains ready in persisted status. A nonzero RequeueAfter can also pass with the wrong retry delay. Add a check that reads the updated driver status through the client, using an updater that persists conditions, and assert RequeueAfter == 5*time.Second.

As per path instructions, “Flag assertions that would still pass if the behavior under test were broken.”

Source: Path instructions

[kvalliyurnatt]

/ok to test 10c8db4

[kvalliyurnatt]

Thanks for your contribution @v0ropaev ! LGTM.

[devin-ai-integration]

Note

Newer findings are available below. Devin Review posted a newer report on this PR, in addition to the findings presented here.

Devin Review found 2 potential issues.

[devin-ai-integration]

🔍 Persisted driver status remains untested

FakeConditionUpdater only records the input state. The test never reads the stored driver, so it cannot catch a status persistence regression.

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[kvalliyurnatt]

@v0ropaev with the watch added do we really need this fallback ?

[v0ropaev]

Agreed, dropped it in 66cc126. The informer relists after watch errors, so a create can't really get lost, and polling every 5s for a secret that may never show up isn't worth it. It's back to returning an empty result like before.

Re review the latest changes

[kvalliyurnatt]

I think this will not work and will break the operator

1. The manager cache uses DefaultNamespaces with two namespaces: the operator namespace and openshift (see cmd/gpu-operator/main.go lines 130-136). For a namespaced type, source.Kind calls multiNamespaceCache.GetInformer, which starts one informer for each cache namespace. This watch therefore also lists and watches Secrets in the openshift namespace.

2. The operator has no RBAC for Secrets in that namespace:

• Helm grants secrets only in the namespaced Role in the release namespace. The ClusterRole does not include secrets.
• The OLM CSV lists secrets only under permissions, not under clusterPermissions.

The controller does not need Secrets from openshift. Reconcile reads only r.Namespace, and enqueueNVIDIADriversForSecret drops other namespaces. To limit Secrets to the operator namespace in main.go:

    cacheOptions := cache.Options{
        DefaultNamespaces: map[string]cache.Config{
            operatorNamespace:  {},
            openshiftNamespace: {},
        },
        ByObject: map[client.Object]cache.ByObject{
            &corev1.Secret{}: {
                Namespaces: map[string]cache.Config{operatorNamespace: {}},
            },
        },
    }

[v0ropaev]

Right, I missed that the cache also covers the openshift namespace. I reproduced it in envtest with a user that only has the namespaced Role for secrets, and the Secret informer never syncs, so the controller wouldn't start. 66cc126 adds a ByObject entry for Secret in main.go limited to the operator namespace, and with that it syncs fine. RBAC needs no change since the Role and the CSV permissions already grant list/watch on secrets there.

[kvalliyurnatt]

I wonder if we should just cache metav1.PartialObjectMetadata here, we don't need anything beyond the name and namespace of the secret right ?

[v0ropaev]

Yes, name and namespace are all we need. The watch and the existence check in Reconcile both use PartialObjectMetadata with the Secret GVK now, so this controller no longer puts secret data in the cache. The ByObject entry still applies because the cache picks the delegate by GVK. The ClusterPolicy path (createSecretEnvReference) still does a full Get, it only needs the name too, so I can switch it the same way in a follow-up if you want.