fix(fault-quarantine): keep applied taints whenever a ValidationRequest is created

Address review feedback: drop the validation.retainTaints setting. When
fault-quarantine creates a ValidationRequest it now keeps every taint it
applied during the quarantine session, the same way it keeps the cordon,
by clearing taintsToBeRemoved next to isUnCordon in
triggerValidationOnUnquarantine. lifecycle-manager lifts them through
schedulingGate.taints when validation passes.

The docs state that every taint a rule-set applies must be listed in
schedulingGate.taints with remove set to true.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Harsha Kalalbandi <hkalalbandi@voltagepark.com>

Author: HarshavardhanK

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

@@ -82,7 +82,6 @@ data:
       resource = {{ printf "%ss" (.Values.validation.kind | lower) | quote }}
       templateFileName = {{ .Values.validation.templateFileName | quote }}
       templateMountPath = "/etc/config"
-      retainTaints = {{ .Values.validation.retainTaints | default false }}
 
     {{- range .Values.validation.ruleSets }}
     [[validation.ruleSets]]

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

@@ -270,7 +270,9 @@ ruleSets:
 
 # The validation section configures post-remediation validation from the fault-quarantine
 # module. If enabled, fault-quarantine will preserve the scheduling gate on a given node
-# when an unquarantine event occurs, and it will create the given validation resource
+# (the cordon and the taints it applied) when an unquarantine event occurs, so every taint
+# a ruleSet above applies must be listed in lifecycle-manager config.schedulingGate.taints
+# with remove set to true. fault-quarantine will also create the given validation resource
 # for the requested tests. The list of test is derived from the unhealthy events which
 # occurred during the quarantine session and these tests are derived from the ruleSets
 # section below.
@@ -280,8 +282,6 @@ validation:
   version: "v1alpha1"
   kind: "ValidationRequest"
   templateFileName: "validation-request-template.yaml"
-  # Keep quarantine taints during validation; list them in lifecycle-manager schedulingGate.taints.
-  retainTaints: false
   templates:
     "validation-request-template.yaml": |
       apiVersion: nvsentinel.nvidia.com/v1alpha1

docs/configuration/validation.md

@@ -343,7 +343,8 @@ status:
 | templateFileName | string | The filename of the Go text/template, resolved from templates, rendered to build the ValidationRequest |
 | templates | map[string]string | Inline template content, keyed by filename |
 | ruleSets | []RuleSet | Maps HealthEvents from a quarantine session to the tests they require |
-| retainTaints | bool | Keep the session's quarantine taints when a ValidationRequest is created, as the cordon is kept. List each taint in lifecycle-manager schedulingGate.taints with remove set to true so it is lifted when validation passes, and keep the not-under-quarantine readiness criterion so a new quarantine fails the pending validation instead of releasing the node. Use remove set to true only for taints that fault-quarantine alone applies: a taint that was on the node before the quarantine is not fault-quarantine's, and lifecycle-manager would lift it too. Default false |
+
+When fault-quarantine creates a ValidationRequest, it keeps the cordon and every taint it applied during the quarantine session, and it stops tracking them. lifecycle-manager releases them when validation passes. Every taint that a fault-quarantine rule-set applies must therefore be listed in lifecycle-manager schedulingGate.taints with remove set to true. A taint that is not listed is not tolerated by the test pods, so validation cannot run on the node, and it is never removed. Keep the not-under-quarantine readiness criterion so a new quarantine fails the pending validation instead of releasing the node. Use remove set to true only for taints that fault-quarantine alone applies: lifecycle-manager matches on key, value, and effect, so it also lifts a matching taint that was on the node before the quarantine. When no ValidationRequest is created, fault-quarantine removes its taints itself.
 
 ### fault-quarantine.validation.ruleSets
 

fault-quarantine/pkg/config/config.go

@@ -90,8 +90,6 @@ type ValidationConfig struct {
 	TemplateMountPath string              `toml:"templateMountPath"`
 	TemplateFileName  string              `toml:"templateFileName"`
 	RuleSets          []ValidationRuleSet `toml:"ruleSets"`
-	// RetainTaints leaves quarantine taints for lifecycle-manager schedulingGate.taints to remove.
-	RetainTaints bool `toml:"retainTaints"`
 }
 
 type TomlConfig struct {

fault-quarantine/pkg/reconciler/reconciler.go

@@ -1945,14 +1945,13 @@ func (r *Reconciler) performUncordon(
 		return false, nil
 	}
 
-	annotationsToBeRemoved, isUnCordon, validationRequestCreated, err := r.triggerValidationOnUnquarantine(
-		ctx, span, event, annotations, annotationsToBeRemoved, isUnCordon)
+	annotationsToBeRemoved, taintsToBeRemoved, isUnCordon, validationRequestCreated, err :=
+		r.triggerValidationOnUnquarantine(ctx, span, event, annotations, annotationsToBeRemoved, taintsToBeRemoved,
+			isUnCordon)
 	if err != nil {
 		return true, err
 	}
 
-	taintsToBeRemoved = r.taintsToRemoveOnUncordon(span, taintsToBeRemoved, validationRequestCreated)
-
 	if !r.breakerActive() {
 		slog.InfoContext(ctx, "Circuit breaker is disabled, proceeding with unquarantine action for node",
 			"node", event.NodeName)
@@ -1990,19 +1989,6 @@ func (r *Reconciler) performUncordon(
 	return false, nil
 }
 
-// taintsToRemoveOnUncordon returns no taints when a ValidationRequest was created and
-// validation.retainTaints is set, so the taints stay until validation completes.
-func (r *Reconciler) taintsToRemoveOnUncordon(span trace.Span, taints []config.Taint,
-	validationRequestCreated bool) []config.Taint {
-	if !validationRequestCreated || !r.config.TomlConfig.Validation.RetainTaints {
-		return taints
-	}
-
-	span.SetAttributes(attribute.Int("fault_quarantine.taints.retained", len(taints)))
-
-	return nil
-}
-
 func (r *Reconciler) buildUncordonLabelsToRemove(ruleLabelsToRemove []config.Label,
 	validationRequestCreated bool) []string {
 	labelsToRemove := []string{statemanager.NVSentinelStateLabelKey}
@@ -2021,15 +2007,16 @@ func (r *Reconciler) buildUncordonLabelsToRemove(ruleLabelsToRemove []config.Lab
 // annotation present when it is being unquarantined. Note that we also require that the node was fully drained
 // as part of its quarantine session so it's possible that ValidationRequest creation is skipped even if the
 // annotation present. If a ValidationRequest is created, we will skip removing the cordon, skip removing the
-// cordon-by labels, and skip adding the uncordon-by labels. Taints are still removed unless validation.retainTaints
-// is set.
+// taints fault-quarantine applied, skip removing the cordon-by labels, and skip adding the uncordon-by labels.
+// lifecycle-manager removes the cordon and the taints listed in schedulingGate.taints when validation succeeds.
 //
 // If a ValidationRequest creation fails, the node will not be uncordoned or untainted and all fault-quarantine labels
 // and annotations will be preserved.
 func (r *Reconciler) triggerValidationOnUnquarantine(ctx context.Context, span trace.Span, event *protos.HealthEvent,
-	annotations map[string]string, annotationsToBeRemoved []string, isUnCordon bool) ([]string, bool, bool, error) {
+	annotations map[string]string, annotationsToBeRemoved []string, taintsToBeRemoved []config.Taint,
+	isUnCordon bool) ([]string, []config.Taint, bool, bool, error) {
 	if _, exists := annotations[common.QuarantineValidationHealthEventAnnotationKey]; !exists {
-		return annotationsToBeRemoved, isUnCordon, false, nil
+		return annotationsToBeRemoved, taintsToBeRemoved, isUnCordon, false, nil
 	}
 
 	validationRequestCreated, err := r.createValidationRequestIfRequested(ctx, event, annotations)
@@ -2043,18 +2030,19 @@ func (r *Reconciler) triggerValidationOnUnquarantine(ctx context.Context, span t
 			attribute.String("fault_quarantine.error.message", err.Error()),
 		)
 
-		return annotationsToBeRemoved, isUnCordon, false, err
+		return annotationsToBeRemoved, taintsToBeRemoved, isUnCordon, false, err
 	}
 
 	annotationsToBeRemoved = append(annotationsToBeRemoved, common.QuarantineValidationHealthEventAnnotationKey)
 
 	if validationRequestCreated {
+		taintsToBeRemoved = nil
 		isUnCordon = false
 
 		span.SetAttributes(attribute.Bool("fault_quarantine.validation_request.created", true))
 	}
 
-	return annotationsToBeRemoved, isUnCordon, validationRequestCreated, nil
+	return annotationsToBeRemoved, taintsToBeRemoved, isUnCordon, validationRequestCreated, nil
 }
 
 func (r *Reconciler) createValidationRequestIfRequested(ctx context.Context, event *protos.HealthEvent,

fault-quarantine/pkg/reconciler/reconciler_e2e_test.go

@@ -1813,11 +1813,13 @@ func TestE2E_ValidationRequestCreatedWhenEventDrained(t *testing.T) {
 	}, eventuallyTimeout, eventuallyPollInterval, "A ValidationRequest should be created")
 	assert.ElementsMatch(t, []string{"dcgm-diag-test", "nccl-test"}, listValidationRequestTests(ctx, t, nodeName)[0])
 
-	t.Log("Verify the node stays cordoned pending validation, but its taints are removed")
+	t.Log("Verify the node stays cordoned and tainted pending validation")
 	node, err := e2eTestClient.CoreV1().Nodes().Get(ctx, nodeName, metav1.GetOptions{})
 	require.NoError(t, err)
 	assert.True(t, node.Spec.Unschedulable, "Node should stay unschedulable pending validation")
-	verifyFQTaintAbsent(t, node, "nvidia.com/gpu-xid-error")
+	verifyFQTaintPresent(t, node, "nvidia.com/gpu-xid-error")
+	assert.Empty(t, node.Annotations[common.QuarantineHealthEventAppliedTaintsAnnotationKey],
+		"Applied-taints annotation should be cleared once the ValidationRequest is created")
 	assert.Empty(t, node.Annotations[common.QuarantineValidationHealthEventAnnotationKey],
 		"Validation-session annotation should be cleared once the ValidationRequest is created")
 
@@ -1832,21 +1834,21 @@ func TestE2E_ValidationRequestCreatedWhenEventDrained(t *testing.T) {
 		"nvsentinel-state label should be removed")
 }
 
-func TestE2E_RetainTaints(t *testing.T) {
+func TestE2E_ValidationRequestCreationKeepsTaints(t *testing.T) {
 	tests := []struct {
 		name    string
 		drained bool
 	}{
-		{name: "validation requested keeps taints", drained: true},
-		{name: "validation skipped removes taints", drained: false},
+		{name: "ValidationRequest created keeps taints", drained: true},
+		{name: "ValidationRequest skipped removes taints", drained: false},
 	}
 
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
 			ctx, cancel := context.WithTimeout(e2eTestContext, 20*time.Second)
 			defer cancel()
 
-			nodeName := "e2e-retain-taints-" + generateShortTestID()
+			nodeName := "e2e-validation-taints-" + generateShortTestID()
 			createE2ETestNode(ctx, t, nodeName, nil, nil, nil, false)
 			defer func() {
 				_ = e2eTestClient.CoreV1().Nodes().Delete(ctx, nodeName, metav1.DeleteOptions{})
@@ -1866,7 +1868,6 @@ func TestE2E_RetainTaints(t *testing.T) {
 					Tests: []string{"dcgm-diag-test"},
 				},
 			})
-			validation.RetainTaints = true
 
 			tomlConfig := config.TomlConfig{
 				LabelPrefix: "k8s.nvidia.com/",