refactor(janitor): drop drift handling now that node-level locking is in place

The cross-controller node lock prevents two ExtRRs (or any other
maintenance CR) from acting on the same node concurrently, so the
"foreign taint at our key" scenario is unreachable in production. The
ownership check was useful before the lock landed; it's now dead code
that bloats the state machine.

Removed:
* The value-match drift detection in reconcileApply (Released=False
  terminal failure path).
* The drift-safe branch in reconcileCleanup — under the lock, any taint
  at our key is ours, so unconditionally remove it.
* `transitionToReleaseFailure` helper (no callers).
* `ReasonReleaseTaintFailed`, `eventReasonReleaseTaintFailed`,
  `ExtRRResultFailure`, `ExtRROpenStateFailed` constants (no users).
* Three envtests covering the drift paths (apply, cleanup, observability).
* TestExtRRForeignTaintDrift e2e — the scenario can't occur under the
  lock contract.

Updated:
* reconcileApply doc — no longer mentions "drift → terminal False"; the
  only remaining transient failure is "Node not found → requeue".
* reconcileCleanup doc — explains the lock invariant that justifies
  the simplified taint-removal.
* ExtRRTotal phase comment — released and external_response are
  success-only now.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: jtschelling

janitor/pkg/controller/externalremediationrequest_controller.go

@@ -61,10 +61,8 @@ const (
 	ReleaseTaintKey = "nvsentinel.dgxc.nvidia.com/external-remediation"
 
 	ReasonReleaseTaintApplied = "ReleaseTaintApplied"
-	ReasonReleaseTaintFailed  = "ReleaseTaintFailed"
 
 	eventReasonReleaseTaintApplied   = "ReleaseTaintApplied"
-	eventReasonReleaseTaintFailed    = "ReleaseTaintFailed"
 	eventReasonReleaseTaintRemoved   = "ReleaseTaintRemoved"
 	eventReasonOperatorDeleteRequest = "OperatorDeleteRequested"
 
@@ -275,16 +273,14 @@ const nodeMissingRequeue = 30 * time.Second
 const nodeLockRequeue = 2 * time.Second
 
 // reconcileApply applies the release taint + managed=false in one PATCH then
-// transitions NVSentinelOwnershipReleased. Failure modes per ADR-040: drift
-// → terminal False; Node not found → transient requeue; already-applied →
-// idempotent fast path. Other API errors propagate and controller-runtime
-// requeues with backoff.
+// transitions NVSentinelOwnershipReleased=True. Node not found → transient
+// requeue; already-applied → idempotent fast path. Other API errors propagate
+// and controller-runtime requeues with backoff.
 //
 // Acquires the cross-controller node-level lock before any node mutation so a
 // concurrent RebootNode / TerminateNode / GPUReset can't act on the same node.
-// The lock is released either explicitly on drift (terminal failure) and on
-// Complete=True cleanup, or implicitly via the lease's ownerReference when the
-// ExtRR is deleted.
+// The lock is released on Complete=True cleanup (branch 4) and on operator
+// delete (branch 2); the lease's ownerReference catches the failure case.
 func (r *ExternalRemediationRequestReconciler) reconcileApply(
 	ctx context.Context, extrrObj *nvsentinelv1.ExternalRemediationRequest,
 ) (ctrl.Result, error) {
@@ -309,29 +305,16 @@ func (r *ExternalRemediationRequestReconciler) reconcileApply(
 		return ctrl.Result{RequeueAfter: nodeLockRequeue}, nil
 	}
 
-	if existing := findTaintByKey(node.Spec.Taints, ReleaseTaintKey); existing != nil {
-		if existing.Value != extrrObj.Name {
-			msg := fmt.Sprintf(
-				"node %q already tainted by ExternalRemediationRequest %q; another ExtRR owns this node",
-				nodeName, existing.Value)
-			slog.WarnContext(ctx, "release taint drift detected",
-				"extrr", extrrObj.Name, "node", nodeName, "existing_owner", existing.Value)
-			// Release the lock — drift is terminal and we never owned the taint.
-			r.NodeLock.CheckUnlock(ctx, extrrObj, nodeName)
-
-			return ctrl.Result{}, r.transitionToReleaseFailure(ctx, extrrObj, msg)
-		}
-		// Idempotent fast path — no PATCH needed if both taint and label match.
-		if node.Labels[managed.ManagedLabelKey] == managed.ManagedLabelValueFalse {
-			slog.InfoContext(ctx, "release taint and managed=false label already in place; transitioning condition",
-				"extrr", extrrObj.Name, "node", nodeName)
+	// We hold the lock, so any taint at our key is ours.
+	if findTaintByKey(node.Spec.Taints, ReleaseTaintKey) != nil &&
+		node.Labels[managed.ManagedLabelKey] == managed.ManagedLabelValueFalse {
+		slog.InfoContext(ctx, "release taint and managed=false label already in place; transitioning condition",
+			"extrr", extrrObj.Name, "node", nodeName)
 
-			msg := fmt.Sprintf("release taint %s=%s and managed=false label already present on node %q",
-				ReleaseTaintKey, extrrObj.Name, nodeName)
+		msg := fmt.Sprintf("release taint %s=%s and managed=false label already present on node %q",
+			ReleaseTaintKey, extrrObj.Name, nodeName)
 
-			return ctrl.Result{}, r.transitionToReleaseSuccess(ctx, extrrObj, msg)
-		}
-		// Taint matches but label is missing — fall through to patch the label.
+		return ctrl.Result{}, r.transitionToReleaseSuccess(ctx, extrrObj, msg)
 	}
 
 	nodeToUpdate := node.DeepCopy()
@@ -385,26 +368,6 @@ func (r *ExternalRemediationRequestReconciler) transitionToReleaseSuccess(
 	return nil
 }
 
-// transitionToReleaseFailure records terminal drift failure. Skips ExtRROpen
-// — these ExtRRs aren't in-flight, just counted under released{failure}.
-func (r *ExternalRemediationRequestReconciler) transitionToReleaseFailure(
-	ctx context.Context, extrrObj *nvsentinelv1.ExternalRemediationRequest, message string,
-) error {
-	changed, err := r.transitionReleased(ctx, extrrObj, metav1.ConditionFalse, ReasonReleaseTaintFailed, message)
-	if err != nil {
-		return err
-	}
-
-	if !changed {
-		return nil
-	}
-
-	metrics.GlobalMetrics.IncExtRRTotal(metrics.ExtRRPhaseReleased, metrics.ExtRRResultFailure)
-	r.emitEvent(extrrObj, corev1.EventTypeWarning, eventReasonReleaseTaintFailed, message)
-
-	return nil
-}
-
 // reconcileCleanupAfterComplete scrubs the Node when Complete=True; the ExtRR
 // stays as a historical record until an operator deletes it.
 func (r *ExternalRemediationRequestReconciler) reconcileCleanupAfterComplete(
@@ -491,11 +454,9 @@ func (r *ExternalRemediationRequestReconciler) recordClose(
 		fmt.Sprintf("release taint and managed=false label removed (%s)", closeReason))
 }
 
-// reconcileCleanup removes the release taint only when its value matches this
-// ExtRR (drift-safe — a foreign taint is left alone) and deletes the managed
-// label. Returns true when the PATCH mutated the Node.
-//
-//nolint:cyclop // dispatch over drift cases; inline switch reads cleaner.
+// reconcileCleanup removes the release taint and the managed label from the
+// Node. Returns true when the PATCH mutated the Node. The node-level lock
+// guarantees we own the taint at our key, so no value-match check is needed.
 func (r *ExternalRemediationRequestReconciler) reconcileCleanup(
 	ctx context.Context, extrrObj *nvsentinelv1.ExternalRemediationRequest,
 ) (bool, error) {
@@ -516,16 +477,9 @@ func (r *ExternalRemediationRequestReconciler) reconcileCleanup(
 	nodeToUpdate := node.DeepCopy()
 	changed := false
 
-	if existing := findTaintByKey(nodeToUpdate.Spec.Taints, ReleaseTaintKey); existing != nil {
-		switch existing.Value {
-		case extrrObj.Name:
-			nodeToUpdate.Spec.Taints = removeTaintByKey(nodeToUpdate.Spec.Taints, ReleaseTaintKey)
-			changed = true
-		default:
-			// Drift: foreign ExtRR owns the taint; its own cleanup will remove it.
-			slog.WarnContext(ctx, "release taint owned by a different ExtRR; leaving in place during cleanup",
-				"extrr", extrrObj.Name, "node", nodeName, "existing_owner", existing.Value)
-		}
+	if findTaintByKey(nodeToUpdate.Spec.Taints, ReleaseTaintKey) != nil {
+		nodeToUpdate.Spec.Taints = removeTaintByKey(nodeToUpdate.Spec.Taints, ReleaseTaintKey)
+		changed = true
 	}
 
 	if _, ok := nodeToUpdate.Labels[managed.ManagedLabelKey]; ok {

janitor/pkg/controller/externalremediationrequest_controller_test.go

@@ -395,41 +395,6 @@ var _ = Describe("ExternalRemediationRequest Controller resolution paths (branch
 				"Node ResourceVersion must not advance after the cleanup PATCH settles")
 		})
 
-		It("leaves a foreign taint in place when another ExtRR claims the node (drift on cleanup)", func() {
-			nodeName := "node-true-drift-1"
-			key := prepareReleased(ctx, r, "true-drift-extrr-1", nodeName)
-			DeferCleanup(forceFinalizerRemovalByKey, ctx, r, key)
-			DeferCleanup(deleteNodeForCleanup, ctx, r, nodeName)
-
-			// Rewrite the taint value to simulate a foreign owner.
-			var node corev1.Node
-			Expect(r.Client.Get(ctx, ctrlclient.ObjectKey{Name: nodeName}, &node)).To(Succeed())
-
-			origNode := node.DeepCopy()
-			for i := range node.Spec.Taints {
-				if node.Spec.Taints[i].Key == ReleaseTaintKey {
-					node.Spec.Taints[i].Value = "foreign-err"
-					break
-				}
-			}
-
-			Expect(r.Client.Patch(ctx, &node, ctrlclient.StrategicMergeFrom(origNode))).To(Succeed())
-
-			setExternalRemediationComplete(ctx, r.Client,
-				&nvsentinelv1.ExternalRemediationRequest{ObjectMeta: metav1.ObjectMeta{
-					Name: key.Name, Namespace: key.Namespace,
-				}}, "True", "ExternalRemediationSucceeded")
-
-			_, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: key})
-			Expect(err).NotTo(HaveOccurred())
-
-			Expect(r.Client.Get(ctx, ctrlclient.ObjectKey{Name: nodeName}, &node)).To(Succeed())
-			taint := findTaintByKey(node.Spec.Taints, ReleaseTaintKey)
-			Expect(taint).NotTo(BeNil(), "foreign taint must NOT be removed by our cleanup")
-			Expect(taint.Value).To(Equal("foreign-err"))
-			Expect(node.Labels).NotTo(HaveKey(managed.ManagedLabelKey),
-				"label removal is unconditional (cluster-wide semantics)")
-		})
 	})
 
 	Context("branch 2: deletionTimestamp set (operator-driven release)", func() {
@@ -792,36 +757,6 @@ var _ = Describe("ExternalRemediationRequest Controller apply path (branch 3)",
 
 	// Empty-nodeName rejection is covered by the webhook test suite.
 
-	It("transitions to False when the Node is already tainted by a different ExtRR (drift)", func() {
-		nodeName := "node-drift-1"
-		// Pre-apply a taint with a DIFFERENT ExtRR's name.
-		Expect(r.Client.Create(ctx, newTestNode(nodeName, nil,
-			[]corev1.Taint{{Key: ReleaseTaintKey, Value: "some-other-err", Effect: corev1.TaintEffectNoSchedule}}))).
-			To(Succeed())
-		DeferCleanup(deleteNodeForCleanup, ctx, r, nodeName)
-
-		extrrObj := newTestExtRR("drift-extrr-1", nodeName)
-		Expect(r.Client.Create(ctx, extrrObj)).To(Succeed())
-		DeferCleanup(deleteExtRRForCleanup, ctx, r, extrrObj)
-
-		key := ctrlclient.ObjectKey{Name: extrrObj.Name, Namespace: extrrObj.Namespace}
-		got := reconcileToSteadyState(ctx, r, key, 3)
-
-		released := findExtRRCondition(got, ConditionNVSentinelOwnershipReleased)
-		Expect(released.Status).To(Equal("False"), "drift must transition condition to False")
-		Expect(released.Reason).To(Equal(ReasonReleaseTaintFailed))
-		Expect(released.Message).To(ContainSubstring("some-other-err"),
-			"drift message must identify the existing taint owner")
-		Expect(released.Message).To(ContainSubstring(nodeName))
-
-		var node corev1.Node
-		Expect(r.Client.Get(ctx, ctrlclient.ObjectKey{Name: nodeName}, &node)).To(Succeed())
-		taint := findTaintByKey(node.Spec.Taints, ReleaseTaintKey)
-		Expect(taint).NotTo(BeNil())
-		Expect(taint.Value).To(Equal("some-other-err"), "drift case must NOT overwrite the existing taint")
-		Expect(node.Labels).NotTo(HaveKey(managed.ManagedLabelKey), "drift case must NOT set managed=false")
-	})
-
 	It("requeues without acting when another maintenance resource holds the node lock", func() {
 		nodeName := "node-locked-1"
 		Expect(r.Client.Create(ctx, newTestNode(nodeName, nil, nil))).To(Succeed())
@@ -1087,32 +1022,6 @@ var _ = Describe("ExternalRemediationRequest Controller observability", func() {
 		Expect(events).To(ContainElement(ContainSubstring(eventReasonReleaseTaintApplied)))
 	})
 
-	It("increments released{failure} + emits ReleaseTaintFailed on drift", func() {
-		nodeName := "node-obs-drift-1"
-		// Pre-existing taint owned by a foreign ExtRR.
-		Expect(r.Client.Create(ctx, newTestNode(nodeName, nil,
-			[]corev1.Taint{{Key: ReleaseTaintKey, Value: "foreign-owner", Effect: corev1.TaintEffectNoSchedule}}))).
-			To(Succeed())
-		DeferCleanup(deleteNodeForCleanup, ctx, r, nodeName)
-
-		failureBefore := testutil.ToFloat64(janitormetrics.ExtRRTotal.WithLabelValues(
-			janitormetrics.ExtRRPhaseReleased, janitormetrics.ExtRRResultFailure))
-
-		extrrObj := newTestExtRR("obs-drift-1", nodeName)
-		Expect(r.Client.Create(ctx, extrrObj)).To(Succeed())
-		DeferCleanup(forceFinalizerRemoval, ctx, r, extrrObj)
-
-		key := ctrlclient.ObjectKey{Name: extrrObj.Name, Namespace: extrrObj.Namespace}
-		reconcileToSteadyState(ctx, r, key, 3)
-
-		Expect(testutil.ToFloat64(janitormetrics.ExtRRTotal.WithLabelValues(
-			janitormetrics.ExtRRPhaseReleased, janitormetrics.ExtRRResultFailure)) - failureBefore).
-			To(BeNumerically("==", 1.0))
-
-		events := drainEvents(r)
-		Expect(events).To(ContainElement(ContainSubstring(eventReasonReleaseTaintFailed)))
-	})
-
 	It("increments closed{success} + external_response{success} + observes age on True cleanup", func() {
 		nodeName := "node-obs-close-success-1"
 		key := prepareReleased(ctx, r, "obs-close-success-1", nodeName)

janitor/pkg/metrics/extrr_metrics.go

@@ -30,22 +30,20 @@ const (
 
 const (
 	ExtRRResultSuccess         = "success"
-	ExtRRResultFailure         = "failure"
 	ExtRRResultOperatorDeleted = "operator_deleted"
 	// ExtRRResultNone is the explicit sentinel for phases without an outcome.
 	ExtRRResultNone = ""
 )
 
 const (
 	ExtRROpenStateAwaiting = "awaiting"
-	ExtRROpenStateFailed   = "failed"
 )
 
 var (
 	// ExtRRTotal phases:
 	//   created           — fresh ExtRR initialised.
-	//   released          — NVSentinelOwnershipReleased flipped. result=success|failure.
-	//   external_response — ExternalRemediationComplete observed. result=success|failure.
+	//   released          — NVSentinelOwnershipReleased=True. result=success.
+	//   external_response — ExternalRemediationComplete=True observed. result=success.
 	//   closed            — cleanup ran. result=success | operator_deleted.
 	ExtRRTotal = prometheus.NewCounterVec(prometheus.CounterOpts{
 		Name: "nvsentinel_external_remediation_total",

tests/external_remediation_test.go

@@ -363,79 +363,6 @@ func TestExtRROperatorDeleteEscape(t *testing.T) {
 	testEnv.Test(t, feature.Feature())
 }
 
-// TestExtRRForeignTaintDrift: a fresh ExtRR for a Node already tainted by
-// another ExtRR transitions to Released=False and leaves the foreign taint.
-func TestExtRRForeignTaintDrift(t *testing.T) {
-	feature := features.New("TestExtRRForeignTaintDrift").
-		WithLabel("suite", "drift").
-		WithLabel("component", "janitor")
-
-	var (
-		nodeName       string
-		foreignOwnerCR = "extrr-foreign-owner"
-		freshCR        = "extrr-drift-victim"
-	)
-
-	feature.Setup(func(ctx context.Context, t *testing.T, c *envconf.Config) context.Context {
-		client, err := c.NewClient()
-		require.NoError(t, err)
-
-		nodeName, err = helpers.GetRealNodeName(ctx, client)
-		require.NoError(t, err)
-
-		// Land the foreign-owner taint first.
-		_, err = helpers.CreateExtRRCR(ctx, client, foreignOwnerCR, nodeName, "foreign")
-		require.NoError(t, err)
-		helpers.WaitForExtRRCondition(ctx, t, client, foreignOwnerCR,
-			"NVSentinelOwnershipReleased", "True")
-
-		return ctx
-	})
-
-	feature.Assess("fresh ExtRR for an already-tainted node transitions to False",
-		func(ctx context.Context, t *testing.T, c *envconf.Config) context.Context {
-			client, err := c.NewClient()
-			require.NoError(t, err)
-
-			_, err = helpers.CreateExtRRCR(ctx, client, freshCR, nodeName, "victim")
-			require.NoError(t, err)
-
-			got := helpers.WaitForExtRRCondition(ctx, t, client, freshCR,
-				"NVSentinelOwnershipReleased", "False")
-			require.NotNil(t, got)
-
-			cond := helpers.GetCRCondition(got, "NVSentinelOwnershipReleased")
-			require.NotNil(t, cond)
-			assert.Equal(t, "ReleaseTaintFailed", cond["reason"],
-				"drift case must surface ReleaseTaintFailed reason")
-
-			node, err := helpers.GetNodeByName(ctx, client, nodeName)
-			require.NoError(t, err)
-			assertNodeHasReleaseTaint(t, node, foreignOwnerCR)
-
-			return ctx
-		})
-
-	feature.Teardown(func(ctx context.Context, t *testing.T, c *envconf.Config) context.Context {
-		client, err := c.NewClient()
-		if err != nil {
-			return ctx
-		}
-
-		_ = helpers.DeleteAllCRs(ctx, t, client, helpers.ExternalRemediationRequestGVK)
-		// Belt-and-suspenders in case the finalizer-driven cleanup didn't complete.
-		if nodeName != "" {
-			if err := helpers.ScrubExtRRStateFromNode(ctx, client, nodeName); err != nil {
-				t.Logf("ScrubExtRRStateFromNode(%s): %v", nodeName, err)
-			}
-		}
-
-		return ctx
-	})
-
-	testEnv.Test(t, feature.Feature())
-}
-
 func assertNodeHasReleaseTaint(t *testing.T, node *corev1.Node, expectedOwner string) {
 	t.Helper()