feat(fault-quarantine): gate events for opted-out nodes via skipNodeLabels (NKX-13877)

Author: cbumb

distros/kubernetes/nvsentinel/charts/fault-quarantine/templates/configmap.yaml

@@ -70,3 +70,9 @@ data:
       [rule-sets.cordon]
         shouldCordon = {{ .cordon.shouldCordon }}
     {{- end }}
+
+    {{- range .Values.skipNodeLabels }}
+    [[skip-node-labels]]
+      key = {{ .key | quote }}
+      value = {{ .value | quote }}
+    {{- end }}

distros/kubernetes/nvsentinel/charts/fault-quarantine/values.yaml

@@ -67,6 +67,14 @@ circuitBreaker:
   # Example: "5m" means if 50% of GPU nodes are cordoned within any 5-minute window, the circuit breaker trips
   duration: "5m"
 
+# Labels that cause fault-quarantine to silently drop events for matching nodes.
+# Any node carrying a label whose key and value match an entry here will not be
+# quarantined, uncordoned, or otherwise acted upon. This covers both healthy and
+# unhealthy events, preventing opted-out nodes from being affected by FQ.
+skipNodeLabels:
+  - key: "nvsentinel.dgxc.nvidia.com/managed"
+    value: "false"
+
 # Rule sets for node quarantine actions
 # Each ruleset defines conditions (match) and actions (taint, label, cordon) to apply when conditions are met
 # Rules are evaluated using CEL (Common Expression Language) expressions

docs/designs/040-external-remediation-request.md

@@ -302,11 +302,13 @@ When `node-labeler` removes those detection labels in response to `managed="fals
 
 `metadata-collector` is included in the list above for completeness: it gates on `driver.installed` AND `nvidia.com/gpu.present`. When `driver.installed` is withheld, the AND fails and `metadata-collector` is evicted too. This is the desired behaviour during external remediation — NVSentinel's collection should also stop on a released node.
 
-#### Cluster-scope monitors: code-level emission gating
+#### Cluster-scope monitors: fault-quarantine skip-node-labels gate
 
-Cluster-scope monitors (`csp-health-monitor`, `kubernetes-object-monitor`, `slurm-drain-monitor`) run as `Deployment`s, not DaemonSets, and target nodes by name from outside the node. They cannot be evicted from a node because they do not run on it. Instead, each cluster-scope monitor reads the target node's `managed` label from its Kubernetes informer cache and skips emission when `managed="false"`. Any other state (label absent, label set to `"true"`, or any other value) means the cluster-scope monitor emits as usual.
+Cluster-scope monitors (`csp-health-monitor`, `kubernetes-object-monitor`, `slurm-drain-monitor`) run as `Deployment`s, not DaemonSets, and target nodes by name from outside the node. They cannot be evicted from a node because they do not run on it. Unlike DaemonSet monitors, gating these monitors at the emission level would require either a node informer per DaemonSet pod (wasteful at scale) or a stale cache that introduces unacceptable latency for opt-out decisions.
 
-A shared helper in `commons/pkg/` provides the lookup so the check is centralized; each cluster-scope monitor calls into it before emitting events for a given node. The check is part of the emission code path, not the scrape/poll loop — monitors keep observing, they just refuse to emit for nodes that are explicitly opted out.
+Instead, the gate is applied centrally in `fault-quarantine` via a configurable `skipNodeLabels` list. Before processing any event (healthy or unhealthy), fault-quarantine looks up the target node in its existing node informer cache and checks whether it carries any label from the configured skip list. If it does, the event is silently dropped: no quarantine, no uncordon, no annotation changes. This covers both new quarantine attempts (unhealthy events) and quarantine-clearing (healthy events for already-quarantined nodes).
+
+The default configuration ships with `nvsentinel.dgxc.nvidia.com/managed=false` in the skip list. Operators can add additional labels via Helm values without code changes. Cluster-scope monitors continue to observe and emit events as usual; the events reach the database and are available for audit, but fault-quarantine refuses to act on them for opted-out nodes. This is consistent with the principle that events should be "observed, recorded, and exported as usual" and the guard applies only to state-changing actions.
 
 #### ERR reconciler interaction
 
@@ -324,7 +326,7 @@ Two timing components contribute to the gap between `managed="false"` being appl
 1. `node-labeler` observes the `managed` write via its informer and runs its own reconcile to remove detection labels. Typical latency: well under a second.
 2. The DaemonSet controller re-evaluates `nodeSelector` matches once detection labels are removed and begins evicting pods. Actual pod termination respects each pod's `terminationGracePeriodSeconds` — typically a few seconds.
 
-The ERR reconciler does **NOT** wait for either component to complete before setting `NVSentinelOwnershipReleased=True`; the condition flips as soon as the PATCH lands. Monitor pods may emit a few last health events for the released node during the eviction window. Those events are caught downstream by the release-taint guard and do not trigger NVSentinel action; they may, however, appear briefly in the event store and observability surfaces. The release taint itself takes effect immediately on patch landing, so destructive scheduling decisions are blocked from the moment the patch is acknowledged. Cluster-scope monitors stop emitting on the next informer-cache observation of the `managed` write — effectively immediately; no pod eviction is involved, so no analogous window exists.
+The ERR reconciler does **NOT** wait for either component to complete before setting `NVSentinelOwnershipReleased=True`; the condition flips as soon as the PATCH lands. Monitor pods may emit a few last health events for the released node during the eviction window. Those events are caught downstream by `fault-quarantine`'s `skipNodeLabels` gate and do not trigger state-changing action; they may, however, appear briefly in the event store and observability surfaces. The release taint itself takes effect immediately on patch landing, so destructive scheduling decisions are blocked from the moment the patch is acknowledged. Cluster-scope monitors continue to emit events as usual (they are not evicted because they run as Deployments), but `fault-quarantine` checks the configured `skipNodeLabels` and silently drops matching events before any quarantine, uncordon, or annotation changes.
 
 #### Trade-offs
 
@@ -481,7 +483,7 @@ Ownership is transferred back to NVSentinel by exactly two events: (1) the exter
 - **External system stops progressing (never sets `ExternalRemediationComplete`, or sets `ExternalRemediationComplete=False`).** No timeout. NVSentinel intentionally does not return the node to service on its own in either case — when the external system has stopped, NVSentinel has no signal about what state the node was left in (mid-RMA, partial repair, hardware swapped but not validated, …). Returning the node to user workloads on that signal would be unsafe. The ERR therefore stays live, the release taint stays applied, `managed` stays `"false"`, and the node remains released to the external system, which may resume work and patch `ExternalRemediationComplete=True` if they end up fixing it. From NVSentinel's side, `err_open{state="awaiting"}` (Unknown) and `err_open{state="failed"}` (False) make the situation visible to operators. Operators alert on persistence (an ERR open longer than the expected external-remediation SLO) and use `kubectl delete err <name>` — backed by the finalizer-driven cleanup path (see *Operator-driven release*) — to force the node back into NVSentinel ownership when they have separately confirmed the node is safe to return.
 - **Node deleted while ERR is open.** ERR reconciler logs and treats taint and label operations as no-ops. The ERR object remains so the external system can still acknowledge completion (which is then a no-op against the missing node). Operators can also reclaim the ERR object itself via `kubectl delete err <name>`; the finalizer-driven cleanup path runs cleanly even with the node already gone (the cleanup PATCH no-ops, the finalizer is removed, and the ERR is garbage-collected).
 - **Multiple distinct faults on a node arriving while an ERR is in flight.** Primary defense is monitor teardown: with `managed=false` on the node, NVSentinel's own monitors aren't emitting events for it. The equivalence-group skip at `fault-remediation` is a defense-in-depth backstop for any event that does slip through (a final emission during the eviction window, or an event from outside the standard pipeline). Either way, only the first event's `HealthEvent` is captured in the ERR spec. Once the ERR is closed — either by `ExternalRemediationComplete=True` driven cleanup or by operator-initiated `kubectl delete err` — the release taint is removed, the `managed` label is removed, node-labeler restamps the detection labels, and monitor pods resume. Any persistent faults are then re-detected and produce a fresh ERR through the standard pipeline.
-- **Health event (healthy or unhealthy) arrives at fault-quarantine while an ERR is in flight.** Defense in depth: with monitor teardown in place, this should be rare (most events from NVSentinel's own monitors are stopped at the source). If one does arrive, fault-quarantine's release-taint guard refuses to act on the release-tainted node, regardless of event polarity. The event is stored as a historical record but causes no state change. The ERR remains the only authority for transitioning the node out of "released" state. After the release taint is removed and monitors resume, the next observation drives fault-quarantine's state transition normally.
+- **Health event (healthy or unhealthy) arrives at fault-quarantine while an ERR is in flight.** Defense in depth: with monitor teardown in place, this should be rare (most events from NVSentinel's own monitors are stopped at the source). If one does arrive, fault-quarantine's `skipNodeLabels` gate checks the node's labels against the configured skip list (which includes `nvsentinel.dgxc.nvidia.com/managed=false` by default) and silently drops the event before any processing. The event is stored as a historical record in the database but causes no state change (no quarantine, no uncordon, no annotation updates). The ERR remains the only authority for transitioning the node out of "released" state. After the managed label is removed and monitors resume, the next observation drives fault-quarantine's state transition normally.
 
 ### Non-goals
 

fault-quarantine/pkg/config/config.go

@@ -67,8 +67,16 @@ type RuleSet struct {
 	Cordon   Cordon `toml:"cordon"`
 }
 
+// SkipNodeLabel defines a label key/value pair that causes fault-quarantine
+// to silently drop events for any node carrying the matching label.
+type SkipNodeLabel struct {
+	Key   string `toml:"key"`
+	Value string `toml:"value"`
+}
+
 type TomlConfig struct {
-	LabelPrefix    string         `toml:"label-prefix"`
-	CircuitBreaker CircuitBreaker `toml:"circuitBreaker"`
-	RuleSets       []RuleSet      `toml:"rule-sets"`
+	LabelPrefix    string          `toml:"label-prefix"`
+	CircuitBreaker CircuitBreaker  `toml:"circuitBreaker"`
+	RuleSets       []RuleSet       `toml:"rule-sets"`
+	SkipNodeLabels []SkipNodeLabel `toml:"skip-node-labels"`
 }

fault-quarantine/pkg/metrics/metrics.go

@@ -25,6 +25,11 @@ const (
 	StatusFailed = "failed"
 )
 
+const (
+	labelNode = "node"
+	labelKey  = "label_key"
+)
+
 var (
 	// Event Processing Metrics
 	TotalEventsReceived = promauto.NewCounter(
@@ -53,36 +58,44 @@ var (
 			Name: "fault_quarantine_nodes_quarantined_total",
 			Help: "Total number of nodes quarantined.",
 		},
-		[]string{"node"},
+		[]string{labelNode},
 	)
 	TotalNodesUnquarantined = promauto.NewCounterVec(
 		prometheus.CounterOpts{
 			Name: "fault_quarantine_nodes_unquarantined_total",
 			Help: "Total number of nodes unquarantined.",
 		},
-		[]string{"node"},
+		[]string{labelNode},
 	)
 	TotalNodesManuallyUncordoned = promauto.NewCounterVec(
 		prometheus.CounterOpts{
 			Name: "fault_quarantine_nodes_manually_uncordoned_total",
 			Help: "Total number of nodes manually uncordoned.",
 		},
-		[]string{"node"},
+		[]string{labelNode},
 	)
 
 	TotalNodesManuallyUntainted = promauto.NewCounterVec(
 		prometheus.CounterOpts{
 			Name: "fault_quarantine_nodes_manually_untainted_total",
 			Help: "Total number of nodes manually untainted",
 		},
-		[]string{"node"},
+		[]string{labelNode},
 	)
 	CurrentQuarantinedNodes = promauto.NewGaugeVec(
 		prometheus.GaugeOpts{
 			Name: "fault_quarantine_current_quarantined_nodes",
 			Help: "Nodes which are currently quarantined and undergoing breakfix",
 		},
-		[]string{"node"},
+		[]string{labelNode},
+	)
+
+	EventsSkippedNodeLabel = promauto.NewCounterVec(
+		prometheus.CounterOpts{
+			Name: "fault_quarantine_events_skipped_node_label_total",
+			Help: "Events skipped because the target node carries a configured skip label.",
+		},
+		[]string{labelKey},
 	)
 
 	// Taint and Cordon Metrics
@@ -105,14 +118,14 @@ var (
 			Name: "fault_quarantine_labels_applied_total",
 			Help: "Total number of quarantine labels applied to nodes.",
 		},
-		[]string{"label_key"},
+		[]string{labelKey},
 	)
 	LabelsRemoved = promauto.NewCounterVec(
 		prometheus.CounterOpts{
 			Name: "fault_quarantine_labels_removed_total",
 			Help: "Total number of quarantine labels removed from nodes.",
 		},
-		[]string{"label_key"},
+		[]string{labelKey},
 	)
 	CordonsApplied = promauto.NewCounter(
 		prometheus.CounterOpts{

fault-quarantine/pkg/reconciler/reconciler.go

@@ -473,6 +473,15 @@ func (r *Reconciler) ProcessEvent(
 		return nil
 	}
 
+	if r.isNodeSkipped(ctx, event.HealthEvent.NodeName) {
+		span.SetAttributes(
+			attribute.String("fault_quarantine.event.processing_status", EventProcessingStatusSkipped),
+			attribute.String("fault_quarantine.skip.reason", "Node carries skip label"),
+		)
+
+		return nil
+	}
+
 	slog.DebugContext(ctx, "Processing event", "checkName", event.HealthEvent.CheckName)
 
 	isNodeQuarantined := r.handleEvent(ctx, event, ruleSetEvals, rulesetsConfig)
@@ -505,6 +514,43 @@ func (r *Reconciler) ProcessEvent(
 	return isNodeQuarantined
 }
 
+// isNodeSkipped checks whether the node carries any label from the configured
+// skip-node-labels list. When a match is found the event should be dropped
+// silently so that opted-out nodes are neither quarantined nor uncordoned.
+//
+// Fail-open: if the node is not in the informer cache (e.g. cache hasn't synced
+// yet on startup, or the node was deleted), the event is processed normally.
+// This is intentional -- a missing cache entry should not block quarantine for
+// nodes that are genuinely unhealthy.
+func (r *Reconciler) isNodeSkipped(ctx context.Context, nodeName string) bool {
+	if len(r.config.TomlConfig.SkipNodeLabels) == 0 {
+		return false
+	}
+
+	node, err := r.k8sClient.NodeInformer.GetNode(nodeName)
+	if err != nil {
+		slog.DebugContext(ctx, "Node not found in informer cache, not skipping", "node", nodeName, "error", err)
+		return false
+	}
+
+	for _, skipLabel := range r.config.TomlConfig.SkipNodeLabels {
+		if skipLabel.Key == "" {
+			slog.WarnContext(ctx, "Ignoring skip-node-label entry with empty key")
+			continue
+		}
+
+		if val, ok := node.Labels[skipLabel.Key]; ok && val == skipLabel.Value {
+			slog.InfoContext(ctx, "Skipping event for node with skip label",
+				"node", nodeName, "labelKey", skipLabel.Key, "labelValue", skipLabel.Value)
+			metrics.EventsSkippedNodeLabel.WithLabelValues(skipLabel.Key).Inc()
+
+			return true
+		}
+	}
+
+	return false
+}
+
 // checkCircuitBreakerAndHalt checks if circuit breaker is tripped and returns true if processing should halt
 func (r *Reconciler) checkCircuitBreakerAndHalt(ctx context.Context) bool {
 	span := tracing.SpanFromContext(ctx)