feat(janitor): onboard ExtRR to node-level locking

ExtRR now participates in the cross-controller node-level lock shared by
RebootNode / TerminateNode / GPUReset. Without this, fault-remediation
could create an ExtRR for a node already being acted on by another
maintenance resource (or vice-versa), producing overlapping handoffs.

Behavior:

* reconcileApply acquires the lock after fetching the Node, before any
  Node mutation. Contention requeues with the 2s cadence the siblings
  use. The drift branch explicitly releases the lock (terminal failure;
  we never owned the taint).

* reconcileCleanupAfterComplete (branch 4) releases the lock once
  Complete=True is observed — the external system finished, the Node is
  back in NVSentinel's hands. CheckUnlock is idempotent so subsequent
  no-op cleanup reconciles don't churn it.

* reconcileCleanupOnDeletion (branch 2) releases the lock explicitly
  before finalizer removal. K8s GC via the lease's ownerReference covers
  the failure case, but the explicit unlock matches the sibling pattern
  and shortens the window where the lease could outlive its owner.

Wiring:

* New `NodeLock` + `LockNamespace` fields on the reconciler.
* main.go passes `LockNamespace: podNamespace` (matches siblings).
* SetupWithManager initialises `distributedlock.NewNodeLock(...)` when
  unset, so tests can inject their own.
* New kubebuilder RBAC marker for `coordination.k8s.io/leases`.

Adds an envtest spec that pre-creates a lease with a foreign owner and
asserts the apply path requeues with `nodeLockRequeue` and the Node
stays untouched.

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

Author: jtschelling

janitor/main.go

@@ -240,8 +240,9 @@ func run() error {
 	}
 
 	if err = (&controller.ExternalRemediationRequestReconciler{
-		Client: mgr.GetClient(),
-		Scheme: mgr.GetScheme(),
+		Client:        mgr.GetClient(),
+		Scheme:        mgr.GetScheme(),
+		LockNamespace: podNamespace,
 	}).SetupWithManager(mgr); err != nil {
 		slog.Error("unable to create controller", "controller", "ExternalRemediationRequest", "error", err)
 

janitor/pkg/controller/externalremediationrequest_controller.go

@@ -42,6 +42,7 @@ import (
 	protos "github.com/nvidia/nvsentinel/data-models/pkg/protos"
 	nvsentinelv1 "github.com/nvidia/nvsentinel/janitor/api/v1alpha1"
 	"github.com/nvidia/nvsentinel/janitor/pkg/condition"
+	"github.com/nvidia/nvsentinel/janitor/pkg/distributedlock"
 	"github.com/nvidia/nvsentinel/janitor/pkg/metrics"
 )
 
@@ -78,8 +79,10 @@ const (
 // ExternalRemediationRequestReconciler implements the ADR-040 state machine.
 type ExternalRemediationRequestReconciler struct {
 	client.Client
-	Scheme   *runtime.Scheme
-	Recorder record.EventRecorder
+	Scheme        *runtime.Scheme
+	Recorder      record.EventRecorder
+	NodeLock      distributedlock.NodeLock
+	LockNamespace string
 }
 
 const labelValueUnknown = "unknown"
@@ -112,6 +115,7 @@ func (r *ExternalRemediationRequestReconciler) emitEvent(
 // +kubebuilder:rbac:groups=nvsentinel.dgxc.nvidia.com,resources=externalremediationrequests/status,verbs=get;update;patch
 // +kubebuilder:rbac:groups=nvsentinel.dgxc.nvidia.com,resources=externalremediationrequests/finalizers,verbs=update
 // +kubebuilder:rbac:groups=core,resources=nodes,verbs=get;list;watch;patch
+// +kubebuilder:rbac:groups=coordination.k8s.io,resources=leases,verbs=get;list;watch;create;delete
 
 // Reconcile drives the ExtRR through its lifecycle. The OTEL span is linked to
 // the upstream health-monitor trace via annotations stamped by fault-remediation.
@@ -267,11 +271,20 @@ func (r *ExternalRemediationRequestReconciler) dispatch(
 // nodeMissingRequeue covers the cluster-autoscaler / kubelet-registration race.
 const nodeMissingRequeue = 30 * time.Second
 
+// nodeLockRequeue matches the sibling janitor controllers' lock-retry cadence.
+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.
+//
+// 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.
 func (r *ExternalRemediationRequestReconciler) reconcileApply(
 	ctx context.Context, extrrObj *nvsentinelv1.ExternalRemediationRequest,
 ) (ctrl.Result, error) {
@@ -289,13 +302,22 @@ func (r *ExternalRemediationRequestReconciler) reconcileApply(
 		return ctrl.Result{}, fmt.Errorf("get node %q for ExtRR %q: %w", nodeName, extrrObj.Name, err)
 	}
 
+	if !r.NodeLock.LockNode(ctx, extrrObj, nodeName) {
+		slog.InfoContext(ctx, "another maintenance resource holds the node lock; requeueing",
+			"extrr", extrrObj.Name, "node", nodeName)
+
+		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)
 		}
@@ -400,6 +422,10 @@ func (r *ExternalRemediationRequestReconciler) reconcileCleanupAfterComplete(
 		metrics.GlobalMetrics.IncExtRRTotal(metrics.ExtRRPhaseExternalResponse, metrics.ExtRRResultSuccess)
 	}
 
+	// Release the node-level lock — the external system has finished and the
+	// node is back in NVSentinel's hands. CheckUnlock is idempotent.
+	r.NodeLock.CheckUnlock(ctx, extrrObj, extrrObj.Spec.HealthEvent.NodeName)
+
 	return ctrl.Result{}, nil
 }
 
@@ -425,6 +451,12 @@ func (r *ExternalRemediationRequestReconciler) reconcileCleanupOnDeletion(
 		r.recordClose(extrrObj, metrics.ExtRRResultOperatorDeleted, closeReasonOperatorInitiated)
 	}
 
+	// Release the lock before removing the finalizer. K8s GC via the lease's
+	// ownerReference covers the failure case, but the explicit unlock keeps
+	// us consistent with the sibling controllers and shortens the window
+	// where the lease could outlive its owner.
+	r.NodeLock.CheckUnlock(ctx, extrrObj, extrrObj.Spec.HealthEvent.NodeName)
+
 	updated := extrrObj.DeepCopy()
 	controllerutil.RemoveFinalizer(updated, ExternalRemediationFinalizer)
 
@@ -635,6 +667,10 @@ func (r *ExternalRemediationRequestReconciler) SetupWithManager(mgr ctrl.Manager
 		r.Recorder = mgr.GetEventRecorderFor("externalremediationrequest-controller")
 	}
 
+	if r.NodeLock == nil {
+		r.NodeLock = distributedlock.NewNodeLock(mgr.GetClient(), r.LockNamespace)
+	}
+
 	if err := mgr.GetFieldIndexer().IndexField(
 		context.Background(),
 		&nvsentinelv1.ExternalRemediationRequest{},

janitor/pkg/controller/externalremediationrequest_controller_test.go

@@ -26,6 +26,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/prometheus/client_golang/prometheus/testutil"
 	"google.golang.org/protobuf/types/known/timestamppb"
+	coordinationv1 "k8s.io/api/coordination/v1"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -39,6 +40,7 @@ import (
 	"github.com/nvidia/nvsentinel/commons/pkg/managed"
 	protos "github.com/nvidia/nvsentinel/data-models/pkg/protos"
 	nvsentinelv1 "github.com/nvidia/nvsentinel/janitor/api/v1alpha1"
+	"github.com/nvidia/nvsentinel/janitor/pkg/distributedlock"
 	janitormetrics "github.com/nvidia/nvsentinel/janitor/pkg/metrics"
 )
 
@@ -53,6 +55,7 @@ func newExtRRReconciler() *ExternalRemediationRequestReconciler {
 		Client:   c,
 		Scheme:   scheme.Scheme,
 		Recorder: record.NewFakeRecorder(64),
+		NodeLock: distributedlock.NewNodeLock(c, testExtRRNamespace),
 	}
 }
 
@@ -819,6 +822,55 @@ var _ = Describe("ExternalRemediationRequest Controller apply path (branch 3)",
 		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())
+		DeferCleanup(deleteNodeForCleanup, ctx, r, nodeName)
+
+		// Pre-create a lease as if a sibling maintenance CR (e.g. RebootNode)
+		// already holds the lock for this node.
+		foreignLease := &coordinationv1.Lease{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      nodeName,
+				Namespace: testExtRRNamespace,
+				OwnerReferences: []metav1.OwnerReference{{
+					APIVersion: "janitor.dgxc.nvidia.com/v1alpha1",
+					Kind:       "RebootNode",
+					Name:       "foreign-reboot",
+					UID:        "foreign-uid",
+				}},
+			},
+		}
+		Expect(r.Client.Create(ctx, foreignLease)).To(Succeed())
+		DeferCleanup(func() { _ = r.Client.Delete(ctx, foreignLease) })
+
+		extrrObj := newTestExtRR("locked-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}
+		// Init completes (no Node interaction); second reconcile hits apply,
+		// finds the foreign lock, requeues without acting.
+		_, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: key})
+		Expect(err).NotTo(HaveOccurred())
+
+		result, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: key})
+		Expect(err).NotTo(HaveOccurred(), "lock contention must not propagate as a reconcile error")
+		Expect(result.RequeueAfter).To(Equal(nodeLockRequeue), "lock contention must requeue with the lock-retry cadence")
+
+		// Released stays Unknown — apply never ran.
+		var got nvsentinelv1.ExternalRemediationRequest
+		Expect(r.Client.Get(ctx, key, &got)).To(Succeed())
+		released := findExtRRCondition(&got, ConditionNVSentinelOwnershipReleased)
+		Expect(released.Status).To(Equal("Unknown"))
+
+		// And the Node is untouched.
+		var node corev1.Node
+		Expect(r.Client.Get(ctx, ctrlclient.ObjectKey{Name: nodeName}, &node)).To(Succeed())
+		Expect(findTaintByKey(node.Spec.Taints, ReleaseTaintKey)).To(BeNil())
+		Expect(node.Labels).NotTo(HaveKey(managed.ManagedLabelKey))
+	})
+
 })
 
 func TestExtRRReconciler_NeedsInitialization(t *testing.T) {