fix(validator): separate an aborted run from a deployment that never completed

Two residual issues in waitForDeclaredTrainer, both the same class this PR
exists to fix — reporting one condition under another's verdict.

pollCtx.Done() fires for two reasons that mean opposite things: the 2m poll
deadline, and cancellation of the parent context. Only the first means the
delivered Trainer never became complete. The second means the run was aborted —
catalog timeout, canceled phase, killed Job — and reporting that as a customer
deployment defect is the misclassification the Unavailable-to-NotFound swap
fixed, reintroduced one level down. The deadline branch now checks ctx.Err()
first and returns Timeout for a canceled run.

The loop also probed with pollCtx. getTrainerObject checks ctx.Err() at the top
of every read and returns Timeout, so a deadline landing while a probe was in
flight surfaced as a bare timeout rather than the NotFound-plus-diagnosis this
function was built to produce. The probe runs on the parent context now, and the
select owns the deadline exclusively, so the same cluster state cannot produce
two different verdicts depending on where in the loop the clock ran out.

That window was small but widening: the probe makes roughly six sequential Gets,
each with its own DiagnosticTimeout, so on a loaded apiserver it can run for
seconds.

Adds a test for the cancellation path, and notes on the table that the
not-declared-and-absent row is covered by e2e rather than being an oversight —
it downloads and kustomize-builds the upstream archive.

Refs #2297

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

Author: yuanchen8911

validators/performance/trainer_ensure_test.go

@@ -247,9 +247,9 @@ func TestFoldCleanupError_PreservesCleanupCode(t *testing.T) {
 // 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.
+// The not-declared + missing row is deliberately absent rather than overlooked: it
+// reaches installTrainer, which downloads and kustomize-builds the upstream release
+// archive, so it is covered by e2e rather than being unit-testable here.
 func TestEnsureTrainerInstalled_RecipeDrivenLifecycle(t *testing.T) {
 	tests := []struct {
 		name            string
@@ -333,3 +333,32 @@ func withShortTrainerWait(t *testing.T) func() {
 		trainerInstallWaitTimeout, trainerInstallPollInterval = oldTimeout, oldInterval
 	}
 }
+
+// TestWaitForDeclaredTrainer_CanceledRunIsNotADeploymentDefect pins the distinction
+// the poll deadline and parent cancellation would otherwise blur.
+//
+// Both expire the poll context, but they mean opposite things: the deadline means
+// the delivered Trainer never became complete, which is the customer's deployment
+// defect this PR exists to surface; cancellation means the run was aborted — a
+// catalog timeout, a canceled phase, a killed Job — which is not. Reporting the
+// second as the first is the same misclassification that made ErrCodeUnavailable
+// wrong for the deadline case.
+func TestWaitForDeclaredTrainer_CanceledRunIsNotADeploymentDefect(t *testing.T) {
+	defer withShortTrainerWait(t)()
+	client := newTrainerFakeClient()
+
+	ctx, cancel := context.WithCancel(context.Background())
+	cancel()
+
+	_, err := waitForDeclaredTrainer(ctx, client)
+	if err == nil {
+		t.Fatal("expected an error when the run is canceled")
+	}
+	if stderrors.Is(err, aicrErrors.New(aicrErrors.ErrCodeNotFound, "")) {
+		t.Errorf("canceled run reported as NotFound (%v); an aborted run is not a "+
+			"failed deployment and must not be filed as one", err)
+	}
+	if !stderrors.Is(err, aicrErrors.New(aicrErrors.ErrCodeTimeout, "")) {
+		t.Errorf("error code = %v, want ErrCodeTimeout", err)
+	}
+}

validators/performance/trainer_lifecycle.go

@@ -563,7 +563,12 @@ func waitForDeclaredTrainer(ctx context.Context, dynamicClient dynamic.Interface
 	defer cancel()
 
 	for {
-		install, ok, err := isTrainerInstalled(pollCtx, dynamicClient)
+		// Probe with the parent context, not pollCtx: getTrainerObject checks
+		// ctx.Err() at the top of every read and returns Timeout, so a deadline
+		// landing mid-probe would surface as a bare timeout instead of the
+		// NotFound-plus-diagnosis this function exists to produce. Letting the
+		// select below own the deadline keeps the two conditions separable.
+		install, ok, err := isTrainerInstalled(ctx, dynamicClient)
 		if err != nil {
 			return trainerInstall{}, aicrErrors.PropagateOrWrap(err, aicrErrors.ErrCodeInternal,
 				"failed to check Kubeflow Trainer installation")
@@ -575,6 +580,16 @@ func waitForDeclaredTrainer(ctx context.Context, dynamicClient dynamic.Interface
 
 		select {
 		case <-pollCtx.Done():
+			// pollCtx expires for two different reasons and they mean opposite
+			// things. A canceled parent means the run was aborted — catalog timeout,
+			// phase cancellation, the Job killed — and reporting that as a customer
+			// deployment defect is the same misclassification the NotFound code
+			// above exists to avoid, one level down.
+			if ctx.Err() != nil {
+				return trainerInstall{}, aicrErrors.Wrap(aicrErrors.ErrCodeTimeout,
+					"canceled while waiting for the recipe-declared Kubeflow Trainer", ctx.Err())
+			}
+
 			// 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