fix(validator): decide the Trainer lifecycle from the recipe, not live cluster state

The NCCL benchmark probed the cluster for a Kubeflow Trainer and installed one
when it found none. That made the same recipe behave differently depending on
what happened to be installed, and — more importantly — it masked deployment
failures: a recipe that ships the kubeflow-trainer component but whose Trainer
failed to deploy got an ephemeral fixture installed over the gap, and the
benchmark then reported a passing bandwidth result for a cluster that cannot
run TrainJobs at all.

ensureTrainerInstalled now takes the recipe's declaration and keys on it:

  - declared and present  -> use the delivered installation, unchanged
  - declared but missing  -> fail with ErrCodeUnavailable naming the component
  - not declared          -> install an ephemeral fixture, as before

The third case is the current behavior and stays that way: the recipe never
promised a Trainer, so there is nothing to mask.

Applicability is untouched. It is already recipe-derived through
supportedNCCLCombinations and the nccl-benchmark-profile constraint, and this
change deliberately does not gate on RecipeDeclares — the nccl-all-reduce-bw
constraints are declared on base overlays that do not carry kubeflow-trainer,
so gating applicability on the component would skip the benchmark on every
recipe that actually declares it.

Refs #2297

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

Author: yuanchen8911

validators/performance/nccl_all_reduce_bw_constraint.go

@@ -478,10 +478,14 @@ func runNCCLTrainJob(ctx *validators.Context, gpuConfig *gpuConfiguration,
 
 	dynamicClient := ctx.DynamicClient
 
-	// Ensure a usable Kubeflow Trainer. A complete installation already on the
-	// cluster is left alone and reports no resources; anything we install is ours
-	// to clean up after the test completes.
-	installedResources, err := ensureTrainerInstalled(ctx.Ctx, dynamicClient, ctx.Clientset.Discovery())
+	// 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.
+	recipeDeclaresTrainer := validators.RecipeDeclares(ctx, kubeflowTrainerComponent)
+	installedResources, err := ensureTrainerInstalled(ctx.Ctx, dynamicClient,
+		ctx.Clientset.Discovery(), recipeDeclaresTrainer)
 	if err != nil {
 		return "", err
 	}

validators/performance/trainer_ensure_test.go

@@ -33,7 +33,7 @@ import (
 func TestEnsureTrainerInstalled_CompleteInstallIsLeftAlone(t *testing.T) {
 	client := newTrainerFakeClient(completeTrainerInstall()...)
 
-	refs, err := ensureTrainerInstalled(context.Background(), client, nil)
+	refs, err := ensureTrainerInstalled(context.Background(), client, nil, false)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
@@ -81,7 +81,7 @@ func TestEnsureTrainerInstalled_WaitsOnDiscoveredControllerName(t *testing.T) {
 		return false, nil, nil
 	})
 
-	refs, err := ensureTrainerInstalled(ctx, client, nil)
+	refs, err := ensureTrainerInstalled(ctx, client, nil, false)
 	if err != nil {
 		t.Fatalf("unexpected error waiting on the discovered controller %q (polled %v): %v",
 			releaseDerivedName, polled, err)
@@ -126,7 +126,7 @@ func TestEnsureTrainerInstalled_WaitsForPreexistingController(t *testing.T) {
 	})
 	defer cancel()
 
-	refs, err := ensureTrainerInstalled(ctx, client, nil)
+	refs, err := ensureTrainerInstalled(ctx, client, nil, false)
 	if err == nil {
 		t.Fatal("expected a not-ready pre-existing controller to fail, got nil error")
 	}
@@ -156,7 +156,7 @@ func TestEnsureTrainerInstalled_RefusesToInstallOverForeignNamespace(t *testing.
 			trainerValidatingWebhookName, "kubeflow"),
 	)
 
-	refs, err := ensureTrainerInstalled(context.Background(), client, nil)
+	refs, err := ensureTrainerInstalled(context.Background(), client, nil, false)
 	if err == nil {
 		t.Fatal("expected the installer to refuse installing over an installation in another namespace")
 	}
@@ -180,7 +180,7 @@ func TestEnsureTrainerInstalled_PreservesProbeErrorCode(t *testing.T) {
 		return true, nil, apierrors.NewServiceUnavailable("apiserver is down")
 	})
 
-	_, err := ensureTrainerInstalled(context.Background(), client, nil)
+	_, err := ensureTrainerInstalled(context.Background(), client, nil, false)
 	if err == nil {
 		t.Fatal("expected error, got nil")
 	}
@@ -234,3 +234,45 @@ func TestFoldCleanupError_PreservesCleanupCode(t *testing.T) {
 		t.Errorf("cleanup error code was flattened: %v", got)
 	}
 }
+
+// TestEnsureTrainerInstalled_RecipeDeclaresButMissing verifies the benchmark fails
+// rather than self-installing when the recipe ships Kubeflow Trainer and no complete
+// installation is found.
+//
+// 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)
+	}
+}
+
+// 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()...)
+
+	refs, err := ensureTrainerInstalled(context.Background(), client, nil, true)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if len(refs) != 0 {
+		t.Errorf("refs = %d, want 0 (a recipe-delivered Trainer must never be claimed for cleanup)", len(refs))
+	}
+}

validators/performance/trainer_lifecycle.go

@@ -111,6 +111,11 @@ const (
 	// overwriting a generically-named admission configuration another operator owns.
 	trainerWebhookSuffix = ".trainer.kubeflow.org"
 
+	// kubeflowTrainerComponent is the registry name of the Kubeflow Trainer
+	// component. A recipe declaring it is promising a Trainer installation, which
+	// is what ensureTrainerInstalled keys its behavior on.
+	kubeflowTrainerComponent = "kubeflow-trainer"
+
 	// jobSetCRDName identifies the JobSet dependency. TrainJobs run as JobSets, so
 	// a Trainer whose JobSet controller never becomes ready fails opaquely later.
 	jobSetCRDName = "jobsets.jobset.x-k8s.io"
@@ -431,8 +436,22 @@ func trainerResourceClient(dynamicClient dynamic.Interface,
 // and returns the resources it created, so the caller can delete them when the run
 // finishes. A complete pre-existing installation is left alone and reported as no
 // resources, so the benchmark never deletes a Trainer it does not own.
+//
+// recipeDeclaresTrainer says whether the recipe under validation ships Kubeflow
+// Trainer as a delivered component, and it decides what an incomplete installation
+// means:
+//
+//   - declared, and present: use the delivered installation.
+//   - 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.
+//
+// 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.
 func ensureTrainerInstalled(ctx context.Context, dynamicClient dynamic.Interface,
-	discoveryClient discovery.DiscoveryInterface) ([]trainerResourceRef, error) {
+	discoveryClient discovery.DiscoveryInterface, recipeDeclaresTrainer bool) ([]trainerResourceRef, error) {
 
 	install, installed, err := isTrainerInstalled(ctx, dynamicClient)
 	if err != nil {
@@ -445,6 +464,17 @@ func ensureTrainerInstalled(ctx context.Context, dynamicClient dynamic.Interface
 	}
 
 	if !installed {
+		// The recipe ships Kubeflow Trainer, so a missing or incomplete installation
+		// 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.
+		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))
+		}
+
 		// Before applying anything, check for a live installation somewhere else.
 		// Kustomize applies CRDs and RBAC before webhook configurations, so by the
 		// time the admission-config ownership guard could fire, a shared-name