fix(validator): use NotFound, wait out the rollout window, fix a fall-through

Review findings on the first commit, all verified.

ErrCodeUnavailable was wrong. validators/applicability.go is the in-repo
authority on exactly this decision and its table reads 'clean read,
absent/empty | recipe DECLARES -> FAIL (NotFound)', reserving Unavailable for
transport failures. The read here succeeds and the answer is 'not deployed', so
filing it under Unavailable would bucket a product defect with apiserver
hiccups — and tell whoever triages the new failures to re-run rather than to fix
their deployment. Swapped to ErrCodeNotFound.

The guard also fired on states the code elsewhere treats as transient.
isTrainerInstalled is an existence probe, not a readiness probe: it reports
incomplete for a CRD that is present but not yet Established, or a controller
Deployment that has not appeared. Both are ordinary rollout states on the
bundle-deploy-validate path, so a validate started while the chart was still
landing would have reported a deployment that had not failed. The declared path
now polls, bounded by TrainerControllerReadyTimeout, before returning a verdict.

The failure discarded a diagnosis it had already computed. isTrainerInstalled
logs which specific object was missing and then returned a bare false; that
reason is now carried out on trainerInstall.Incomplete and named in the error,
so the failure is self-diagnosing rather than sending an operator to the logs.

Restructuring for the poll introduced a fall-through that ineffassign caught: a
successful declared wait set installed = true and then continued into the
install path, which would have reinstalled over a Trainer the recipe delivered.
Both paths now share awaitTrainerController and return. A table case pins it.

Also corrects the comments on the undeclared path, which claimed it always
installs an ephemeral fixture. It reuses a complete pre-existing installation
and installs only when none is present — a supported state #2297 preserves.

Tests are now table-driven over the decision, and assert the error code rather
than only that the message names the component. The not-declared-and-absent row
is deliberately excluded: it reaches installTrainer, which downloads a release
archive, so it belongs in an integration test.

Refs #2297

Signed-off-by: Yuan Chen <yuanchen97@gmail.com>

Author: yuanchen8911

pkg/defaults/timeouts.go

@@ -669,6 +669,13 @@ const (
 	// controller-manager Deployment to have at least one ready replica after installation.
 	TrainerControllerReadyTimeout = 2 * time.Minute
 
+	// TrainerInstallPollInterval is the sleep between checks that a
+	// recipe-declared Kubeflow Trainer installation has become complete. The
+	// benchmark polls rather than failing on the first incomplete read, because a
+	// CRD that is present but not yet Established — or a controller Deployment
+	// that has not appeared — is an ordinary rollout state, not a failed deploy.
+	TrainerInstallPollInterval = 5 * time.Second
+
 	// NCCLTrainJobTimeout is the maximum time to wait for the NCCL all-reduce TrainJob to complete.
 	NCCLTrainJobTimeout = 30 * time.Minute
 

validators/performance/nccl_all_reduce_bw_constraint.go

@@ -481,8 +481,9 @@ func runNCCLTrainJob(ctx *validators.Context, gpuConfig *gpuConfiguration,
 	// Ensure a usable Kubeflow Trainer. Whether an incomplete installation is a
 	// failure or something to install over is decided by the recipe, not by what
 	// happens to be on the cluster: a recipe that ships the component must have a
-	// working one, while a recipe that does not gets an ephemeral fixture. Anything
-	// we install is ours to clean up after the test completes.
+	// working one, while a recipe that does not reuses whatever is present and
+	// installs an ephemeral fixture only when nothing is. Anything we install is
+	// ours to clean up after the test completes.
 	recipeDeclaresTrainer := validators.RecipeDeclares(ctx, kubeflowTrainerComponent)
 	installedResources, err := ensureTrainerInstalled(ctx.Ctx, dynamicClient,
 		ctx.Clientset.Discovery(), recipeDeclaresTrainer)

validators/performance/trainer_ensure_test.go

@@ -19,6 +19,7 @@ import (
 	stderrors "errors"
 	"strings"
 	"testing"
+	"time"
 
 	aicrErrors "github.com/NVIDIA/aicr/pkg/errors"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -235,44 +236,100 @@ func TestFoldCleanupError_PreservesCleanupCode(t *testing.T) {
 	}
 }
 
-// TestEnsureTrainerInstalled_RecipeDeclaresButMissing verifies the benchmark fails
-// rather than self-installing when the recipe ships Kubeflow Trainer and no complete
-// installation is found.
+// TestEnsureTrainerInstalled_RecipeDrivenLifecycle covers the decision the recipe
+// now drives. The rows that matter are the two where the recipe declares the
+// component: a delivered installation is used as-is, and a missing one fails rather
+// than being installed over — which is the whole point, because self-installing
+// there would report a passing benchmark for a cluster whose delivered Trainer is
+// broken.
 //
-// This is the whole point of keying the decision on the recipe: installing an
-// ephemeral Trainer here would let the benchmark report a passing bandwidth result
-// for a cluster whose delivered Trainer is broken — the deployment failure the
-// recipe's own component promised would be masked by the validator working around
-// it.
-func TestEnsureTrainerInstalled_RecipeDeclaresButMissing(t *testing.T) {
-	// An empty cluster: nothing is installed, which is what a failed deployment of
-	// the kubeflow-trainer component looks like to the probe.
-	client := newTrainerFakeClient()
-
-	refs, err := ensureTrainerInstalled(context.Background(), client, nil, true)
-	if err == nil {
-		t.Fatal("expected an error when the recipe declares kubeflow-trainer but none is installed")
-	}
-	if len(refs) != 0 {
-		t.Errorf("refs = %d, want 0 (nothing may be installed on the failure path)", len(refs))
-	}
-	if !strings.Contains(err.Error(), kubeflowTrainerComponent) {
-		t.Errorf("error %q does not name the %s component, so an operator cannot tell which "+
-			"component failed to deploy", err, kubeflowTrainerComponent)
+// The not-declared + complete row is included because the guard could silently
+// break it: a recipe that never claimed a Trainer must still reuse one that happens
+// to be present, exactly as before.
+//
+// The not-declared + missing row is deliberately absent: it reaches installTrainer,
+// which downloads a release archive, so it belongs in an integration test rather
+// than here.
+func TestEnsureTrainerInstalled_RecipeDrivenLifecycle(t *testing.T) {
+	tests := []struct {
+		name            string
+		declared        bool
+		objects         []runtime.Object
+		wantErr         bool
+		wantErrCode     aicrErrors.ErrorCode
+		wantErrContains string
+	}{
+		{
+			name:     "declared and delivered: used as-is, never claimed for cleanup",
+			declared: true,
+			objects:  completeTrainerInstall(),
+		},
+		{
+			name:            "declared but missing: fails instead of installing over it",
+			declared:        true,
+			objects:         nil,
+			wantErr:         true,
+			wantErrCode:     aicrErrors.ErrCodeNotFound,
+			wantErrContains: kubeflowTrainerComponent,
+		},
+		{
+			name:     "not declared but present: reused, as before",
+			declared: false,
+			objects:  completeTrainerInstall(),
+		},
+		{
+			// Regression guard for a fall-through the linter caught once already:
+			// after the declared wait succeeds, the code must take the readiness
+			// path and stop, not continue into the install path and reinstall over
+			// a Trainer the recipe delivered.
+			name:     "declared and delivered: does not fall through to install",
+			declared: true,
+			objects:  completeTrainerInstall(),
+		},
 	}
-}
 
-// TestEnsureTrainerInstalled_RecipeDeclaresAndPresent verifies the delivered
-// installation is used as-is: it is not reinstalled, and it is not claimed for
-// cleanup, because the recipe owns it rather than the benchmark.
-func TestEnsureTrainerInstalled_RecipeDeclaresAndPresent(t *testing.T) {
-	client := newTrainerFakeClient(completeTrainerInstall()...)
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			defer withShortTrainerWait(t)()
+			client := newTrainerFakeClient(tt.objects...)
 
-	refs, err := ensureTrainerInstalled(context.Background(), client, nil, true)
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
+			refs, err := ensureTrainerInstalled(context.Background(), client, nil, tt.declared)
+
+			if (err != nil) != tt.wantErr {
+				t.Fatalf("error = %v, wantErr %v", err, tt.wantErr)
+			}
+			// Nothing is ever claimed for cleanup on these rows: a Trainer the
+			// benchmark did not install must not be deleted by it.
+			if len(refs) != 0 {
+				t.Errorf("refs = %d, want 0", len(refs))
+			}
+			if !tt.wantErr {
+				return
+			}
+			// NotFound, not Unavailable: the read succeeded and the answer was "not
+			// deployed". Unavailable is this package's code for a transport failure
+			// (see the decision table on validators.Require), and filing a product
+			// defect under it tells whoever triages the failure to re-run rather than
+			// to fix their deployment.
+			if !stderrors.Is(err, aicrErrors.New(tt.wantErrCode, "")) {
+				t.Errorf("error code = %v, want %s", err, tt.wantErrCode)
+			}
+			if !strings.Contains(err.Error(), tt.wantErrContains) {
+				t.Errorf("error %q does not name %q, so an operator cannot tell which "+
+					"component failed to deploy", err, tt.wantErrContains)
+			}
+		})
 	}
-	if len(refs) != 0 {
-		t.Errorf("refs = %d, want 0 (a recipe-delivered Trainer must never be claimed for cleanup)", len(refs))
+}
+
+// withShortTrainerWait shrinks the recipe-declared rollout wait for the duration of
+// a test and restores it afterwards.
+func withShortTrainerWait(t *testing.T) func() {
+	t.Helper()
+	oldTimeout, oldInterval := trainerInstallWaitTimeout, trainerInstallPollInterval
+	trainerInstallWaitTimeout = 20 * time.Millisecond
+	trainerInstallPollInterval = time.Millisecond
+	return func() {
+		trainerInstallWaitTimeout, trainerInstallPollInterval = oldTimeout, oldInterval
 	}
 }

validators/performance/trainer_lifecycle.go

@@ -190,6 +190,12 @@ type trainerInstall struct {
 	Namespace  string
 	Service    string
 	Deployment string
+
+	// Incomplete records which specific object was missing when the probe
+	// reported the installation as incomplete. The probe already determines this
+	// to log it; carrying it out makes the resulting failure self-diagnosing
+	// rather than sending an operator to the logs to find out what to fix.
+	Incomplete string
 }
 
 // trainerResourceRef identifies a Kubernetes resource applied during Trainer installation,
@@ -231,11 +237,11 @@ func isTrainerInstalled(ctx context.Context, dynamicClient dynamic.Interface) (t
 		}
 		if !found {
 			slog.Info("Kubeflow Trainer incomplete: CRD missing", "crd", crd)
-			return trainerInstall{}, false, nil
+			return trainerInstall{Incomplete: fmt.Sprintf("CRD %s is missing", crd)}, false, nil
 		}
 		if !isCRDEstablished(obj) {
 			slog.Info("Kubeflow Trainer incomplete: CRD not established", "crd", crd)
-			return trainerInstall{}, false, nil
+			return trainerInstall{Incomplete: fmt.Sprintf("CRD %s is not established", crd)}, false, nil
 		}
 	}
 
@@ -256,7 +262,7 @@ func isTrainerInstalled(ctx context.Context, dynamicClient dynamic.Interface) (t
 	if !ok {
 		slog.Info("Kubeflow Trainer incomplete: admission webhook missing",
 			"configuration", trainerMutatingWebhookConfig, "webhook", trainerMutatingWebhookName)
-		return trainerInstall{}, false, nil
+		return trainerInstall{Incomplete: "the validating admission webhook configuration is missing"}, false, nil
 	}
 
 	// The controller Deployment is found by label: its name is release-derived on
@@ -269,7 +275,7 @@ func isTrainerInstalled(ctx context.Context, dynamicClient dynamic.Interface) (t
 	if !found {
 		slog.Info("Kubeflow Trainer incomplete: controller Deployment missing",
 			"namespace", install.Namespace)
-		return trainerInstall{}, false, nil
+		return trainerInstall{Incomplete: "the controller Deployment was not found"}, false, nil
 	}
 	install.Deployment = controller
 
@@ -279,7 +285,7 @@ func isTrainerInstalled(ctx context.Context, dynamicClient dynamic.Interface) (t
 	} else if !found {
 		slog.Info("Kubeflow Trainer incomplete: controller Service missing",
 			"namespace", install.Namespace, "name", install.Service)
-		return trainerInstall{}, false, nil
+		return trainerInstall{Incomplete: "the controller Service was not found"}, false, nil
 	}
 
 	slog.Info("Kubeflow Trainer installation is complete",
@@ -326,14 +332,14 @@ func discoverTrainerInstall(ctx context.Context, dynamicClient dynamic.Interface
 			// rather than guessing a namespace and reinstalling on top of it.
 			slog.Info("Kubeflow Trainer webhook has no Service reference; cannot locate the installation",
 				"configuration", configName, "webhook", webhookName)
-			return trainerInstall{}, false, nil
+			return trainerInstall{Incomplete: "the admission webhook has no Service reference, so the installation namespace cannot be located"}, false, nil
 		}
 		return trainerInstall{Namespace: namespace, Service: service}, true, nil
 	}
 
 	slog.Info("Kubeflow Trainer incomplete: admission webhook missing",
 		"configuration", configName, "webhook", webhookName)
-	return trainerInstall{}, false, nil
+	return trainerInstall{Incomplete: "the validating admission webhook configuration is missing"}, false, nil
 }
 
 // getTrainerObject fetches one object. NotFound reports found=false with no
@@ -445,8 +451,10 @@ func trainerResourceClient(dynamicClient dynamic.Interface,
 //   - declared, but missing or incomplete: fail. Self-installing here would mask a
 //     broken deployment of a component the recipe promised, and the benchmark would
 //     then report a passing result for a cluster that cannot run TrainJobs at all.
-//   - not declared: install an ephemeral fixture and tear it down, as before. The
-//     recipe never claimed a Trainer, so there is nothing to mask.
+//   - not declared, and present: reuse it, unchanged. The recipe never claimed a
+//     Trainer, so a pre-existing one is not evidence of anything to report.
+//   - not declared, and absent: install an ephemeral fixture and tear it down, as
+//     before. There is nothing to mask, because nothing was promised.
 //
 // This is deliberately keyed on the recipe rather than on live cluster state, so the
 // same recipe behaves the same way regardless of what happens to be installed.
@@ -468,11 +476,23 @@ func ensureTrainerInstalled(ctx context.Context, dynamicClient dynamic.Interface
 		// is a deployment failure, not something to paper over. Installing our own
 		// here would produce a passing benchmark for a cluster whose delivered
 		// Trainer is broken.
+		//
+		// But isTrainerInstalled is an existence probe, not a readiness probe: it
+		// reports incomplete for a CRD that is present but not yet Established, or a
+		// controller Deployment that has not appeared yet. Both are ordinary rollout
+		// states on the bundle-deploy-validate path. Poll before declaring failure,
+		// so a validate that starts while the chart is still landing waits it out
+		// rather than reporting a deployment that has not failed.
 		if recipeDeclaresTrainer {
-			return nil, aicrErrors.New(aicrErrors.ErrCodeUnavailable, fmt.Sprintf(
-				"the recipe declares the %s component but no complete Kubeflow Trainer "+
-					"installation was found; the benchmark will not self-install over a "+
-					"delivered component that failed to deploy", kubeflowTrainerComponent))
+			declared, waitErr := waitForDeclaredTrainer(ctx, dynamicClient)
+			if waitErr != nil {
+				return nil, waitErr
+			}
+			// The delivered installation finished rolling out. Fall through to the
+			// same readiness wait a pre-existing installation gets, and claim no
+			// resources: the recipe owns this Trainer, not the benchmark.
+			install = declared
+			return nil, awaitTrainerController(ctx, dynamicClient, install)
 		}
 
 		// Before applying anything, check for a live installation somewhere else.
@@ -504,15 +524,74 @@ func ensureTrainerInstalled(ctx context.Context, dynamicClient dynamic.Interface
 		return created, nil
 	}
 
-	// The probe confirms every object exists; the controller may still be rolling.
-	// Wait for it here rather than reinstalling over a healthy Trainer we do not own.
+	return nil, awaitTrainerController(ctx, dynamicClient, install)
+}
+
+// awaitTrainerController waits for a Trainer the benchmark does not own to finish
+// starting. The probe confirms every object exists; the controller may still be
+// rolling, and waiting is the alternative to reinstalling over a healthy Trainer.
+func awaitTrainerController(ctx context.Context, dynamicClient dynamic.Interface, install trainerInstall) error {
 	slog.Info("Kubeflow Trainer already installed, waiting for controller readiness",
 		"namespace", install.Namespace, "deployment", install.Deployment)
 	if readyErr := waitForTrainerReady(ctx, dynamicClient, install.Namespace, install.Deployment); readyErr != nil {
-		return nil, aicrErrors.PropagateOrWrap(readyErr, aicrErrors.ErrCodeTimeout,
+		return aicrErrors.PropagateOrWrap(readyErr, aicrErrors.ErrCodeTimeout,
 			"pre-existing Kubeflow Trainer controller is not ready")
 	}
-	return nil, nil
+	return nil
+}
+
+// trainerInstallWaitTimeout and trainerInstallPollInterval bound the wait for a
+// recipe-declared Trainer to finish rolling out. They are variables rather than
+// constants only so tests can exercise the timeout path without waiting for it.
+var (
+	trainerInstallWaitTimeout  = defaults.TrainerControllerReadyTimeout
+	trainerInstallPollInterval = defaults.TrainerInstallPollInterval
+)
+
+// waitForDeclaredTrainer polls until a recipe-declared Kubeflow Trainer reports a
+// complete installation, or fails with the specific object that never appeared.
+//
+// The distinction it draws is the point of the recipe-driven lifecycle: an
+// installation that is still rolling out is not a failed one, but an installation
+// that never completes is — and the benchmark must not install its own Trainer over
+// a delivered component, because that would report a passing result for a cluster
+// whose Trainer is broken.
+func waitForDeclaredTrainer(ctx context.Context, dynamicClient dynamic.Interface) (trainerInstall, error) {
+	var last trainerInstall
+
+	pollCtx, cancel := context.WithTimeout(ctx, trainerInstallWaitTimeout)
+	defer cancel()
+
+	for {
+		install, ok, err := isTrainerInstalled(pollCtx, dynamicClient)
+		if err != nil {
+			return trainerInstall{}, aicrErrors.PropagateOrWrap(err, aicrErrors.ErrCodeInternal,
+				"failed to check Kubeflow Trainer installation")
+		}
+		if ok {
+			return install, nil
+		}
+		last = install
+
+		select {
+		case <-pollCtx.Done():
+			// ErrCodeNotFound, not Unavailable: the read succeeded and the answer was
+			// "not deployed". Unavailable is this package's code for a transport
+			// failure — see the decision table on validators.Require — and using it
+			// here would file a product defect alongside apiserver hiccups, telling
+			// whoever triages it to re-run rather than to fix their deployment.
+			reason := last.Incomplete
+			if reason == "" {
+				reason = "no complete installation was found"
+			}
+			return trainerInstall{}, aicrErrors.New(aicrErrors.ErrCodeNotFound, fmt.Sprintf(
+				"the recipe declares the %s component but its Kubeflow Trainer installation "+
+					"did not become complete within %s: %s. The benchmark will not self-install "+
+					"over a delivered component that failed to deploy",
+				kubeflowTrainerComponent, trainerInstallWaitTimeout, reason))
+		case <-time.After(trainerInstallPollInterval):
+		}
+	}
 }
 
 // foldCleanupError decides the check's verdict when teardown fails. A cleanup