fix(validator): synthesize NotFound only from a concrete incomplete observation

classifyPollExpiry returned NotFound for any poll-context expiry, including
the case where every probe attempt was a read timeout and no probe ever
observed an incomplete installation. A degraded or slow apiserver was
therefore filed as a customer deployment defect. NotFound now requires a
successful probe that named a missing object; otherwise the verdict keeps the
timeout classification. The rollout allowance is still enforced either way.

hasTrainerWebhook also flattened an absent mutating admission configuration
and one that exists but serves another operator's webhook to the same bare
false, so the caller told an operator to create an object already on the
cluster. It now returns the specific reason, matching how
discoverTrainerInstall carries its own reason out.

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

Author: yuanchen8911

validators/performance/trainer_ensure_test.go

@@ -464,9 +464,17 @@ func TestWaitForDeclaredTrainer_SlowProbeCannotOutrunTheDeadline(t *testing.T) {
 		t.Fatal("wait returned success after its own deadline had passed; the rollout " +
 			"allowance must bound the probe, not only the sleeps between probes")
 	}
-	if !stderrors.Is(err, aicrErrors.New(aicrErrors.ErrCodeNotFound, "")) {
-		t.Errorf("error code = %v, want ErrCodeNotFound: the deadline expired locally "+
-			"with the parent still live, which is a deployment that never completed", err)
+	// Timeout, not NotFound. The allowance is still enforced — the wait fails — but
+	// nothing here was ever read as missing: the installation underneath is complete
+	// and the only thing that went wrong is a read slower than the budget. Filing
+	// that as NotFound would send an operator to fix a deployment that is fine.
+	if stderrors.Is(err, aicrErrors.New(aicrErrors.ErrCodeNotFound, "")) {
+		t.Errorf("slow read reported as NotFound (%v); no probe observed anything "+
+			"incomplete, so there is no deployment defect to file", err)
+	}
+	var se *aicrErrors.StructuredError
+	if !stderrors.As(err, &se) || se.Code != aicrErrors.ErrCodeTimeout {
+		t.Errorf("reported code = %v, want Timeout", err)
 	}
 }
 
@@ -551,9 +559,15 @@ func TestWaitForDeclaredTrainer_LateSuccessDoesNotOutrunTheDeadline(t *testing.T
 		t.Fatal("wait returned success after its allowance had passed; expiry must be " +
 			"rechecked before accepting a probe's result, not only before issuing it")
 	}
-	if !stderrors.Is(err, aicrErrors.New(aicrErrors.ErrCodeNotFound, "")) {
-		t.Errorf("error code = %v, want ErrCodeNotFound: the deadline expired locally "+
-			"with the parent still live", err)
+	// The probe returned complete, so no incomplete installation was ever observed:
+	// the verdict is the timeout that happened, not a deployment defect.
+	if stderrors.Is(err, aicrErrors.New(aicrErrors.ErrCodeNotFound, "")) {
+		t.Errorf("late success reported as NotFound (%v); the probe found the "+
+			"installation complete, so nothing was missing", err)
+	}
+	var se *aicrErrors.StructuredError
+	if !stderrors.As(err, &se) || se.Code != aicrErrors.ErrCodeTimeout {
+		t.Errorf("reported code = %v, want Timeout", err)
 	}
 	// The reason must describe this probe, not the previous one. The installation is
 	// complete here, so blaming a missing object would point the operator at something
@@ -650,3 +664,49 @@ func TestWaitForDeclaredTrainer_LateSuccessClaimsOnlyWhatWasObserved(t *testing.
 		t.Errorf("reason does not state what was actually observed: %v", err)
 	}
 }
+
+// TestWaitForDeclaredTrainer_ReadTimeoutsAloneAreNotADeploymentDefect pins the case
+// where the allowance runs out without any probe ever reaching a conclusion.
+//
+// Every read the probe issues is cut short by the poll deadline, so no probe ever
+// observes an incomplete installation — there is nothing on the cluster the wait can
+// point at. Synthesizing NotFound from that empty observation would file a degraded
+// or slow apiserver as a customer deployment defect, which is the same
+// misclassification the NotFound/Unavailable split exists to prevent. With no
+// observation to stand on, the honest verdict is the timeout that actually happened.
+func TestWaitForDeclaredTrainer_ReadTimeoutsAloneAreNotADeploymentDefect(t *testing.T) {
+	oldTimeout, oldInterval := trainerInstallWaitTimeout, trainerInstallPollInterval
+	trainerInstallWaitTimeout = 20 * time.Millisecond
+	trainerInstallPollInterval = time.Millisecond
+	defer func() {
+		trainerInstallWaitTimeout, trainerInstallPollInterval = oldTimeout, oldInterval
+	}()
+
+	// Seed only the first CRD the probe reads, and stall that read past the
+	// allowance. The read itself succeeds, so the probe learns nothing incomplete;
+	// the deadline is already gone by the time the second read is issued, and
+	// getTrainerObject's pre-read check turns it into a Timeout. That is the only
+	// thing the wait ever hears back.
+	client := newTrainerFakeClient(establishedCRD(trainerCRDTrainJobs))
+	client.PrependReactor("get", "customresourcedefinitions",
+		func(k8stesting.Action) (bool, runtime.Object, error) {
+			time.Sleep(60 * time.Millisecond) // outlives the allowance
+			return false, nil, nil            // then let the tracker answer
+		})
+
+	_, err := waitForDeclaredTrainer(context.Background(), client)
+	if err == nil {
+		t.Fatal("wait returned success after its allowance had passed")
+	}
+	if stderrors.Is(err, aicrErrors.New(aicrErrors.ErrCodeNotFound, "")) {
+		t.Errorf("read timeouts reported as NotFound (%v); no probe ever observed an "+
+			"incomplete installation, so there is no deployment defect to file", err)
+	}
+	var se *aicrErrors.StructuredError
+	if !stderrors.As(err, &se) || se.Code != aicrErrors.ErrCodeTimeout {
+		t.Errorf("reported code = %v, want Timeout", err)
+	}
+	if strings.Contains(err.Error(), "no complete installation was found") {
+		t.Errorf("verdict claims nothing was found, but nothing was ever read: %v", err)
+	}
+}

validators/performance/trainer_lifecycle.go

@@ -261,16 +261,19 @@ func isTrainerInstalled(ctx context.Context, dynamicClient dynamic.Interface) (t
 		return install, false, nil
 	}
 
-	ok, err := hasTrainerWebhook(ctx, dynamicClient,
+	reason, ok, err := hasTrainerWebhook(ctx, dynamicClient,
 		trainerMutatingWebhookGVR, trainerMutatingWebhookConfig, trainerMutatingWebhookName)
 	if err != nil {
 		return trainerInstall{}, false, err
 	}
 	if !ok {
-		slog.Info("Kubeflow Trainer incomplete: admission webhook missing",
-			"configuration", trainerMutatingWebhookConfig, "webhook", trainerMutatingWebhookName)
-		return trainerInstall{Incomplete: fmt.Sprintf(
-			"admission configuration %q is missing", trainerMutatingWebhookConfig)}, false, nil
+		// Carry the specific reason out rather than reporting every failure as a
+		// missing configuration: "create it" and "something else owns this name"
+		// are different jobs for whoever reads the verdict.
+		slog.Info("Kubeflow Trainer incomplete: mutating admission webhook unusable",
+			"configuration", trainerMutatingWebhookConfig, "webhook", trainerMutatingWebhookName,
+			"reason", reason)
+		return trainerInstall{Incomplete: reason}, false, nil
 	}
 
 	// The controller Deployment is found by label: its name is release-derived on
@@ -418,17 +421,28 @@ func trainerAPIErrorCode(err error) aicrErrors.ErrorCode {
 // serves the given Trainer webhook. The name check matters because the upstream
 // manifests use generic, unprefixed configuration names that another operator on
 // the cluster may already own.
+//
+// When the answer is no it returns the reason, because the two ways to get there
+// need different remedies: a configuration that is absent has to be created, while
+// one that exists but serves someone else's webhook has to be reconciled with
+// whatever already owns that name. Reporting both as "missing" — which is what
+// flattening this to a bare false did — sends an operator to create an object that
+// is already on the cluster. The validating-webhook path in discoverTrainerInstall
+// draws the same distinction.
 func hasTrainerWebhook(ctx context.Context, dynamicClient dynamic.Interface,
-	gvr schema.GroupVersionResource, configName, webhookName string) (bool, error) {
+	gvr schema.GroupVersionResource, configName, webhookName string) (string, bool, error) {
 
 	obj, found, err := getTrainerObject(ctx, dynamicClient, gvr, "", configName)
-	if err != nil || !found {
-		return false, err
+	if err != nil {
+		return "", false, err
+	}
+	if !found {
+		return fmt.Sprintf("admission configuration %q is missing", configName), false, nil
 	}
 
 	entries, _, err := unstructured.NestedSlice(obj.Object, "webhooks")
 	if err != nil {
-		return false, aicrErrors.Wrap(aicrErrors.ErrCodeInternal,
+		return "", false, aicrErrors.Wrap(aicrErrors.ErrCodeInternal,
 			fmt.Sprintf("failed to read webhooks from %s %q", gvr.Resource, configName), err)
 	}
 	for _, e := range entries {
@@ -437,10 +451,11 @@ func hasTrainerWebhook(ctx context.Context, dynamicClient dynamic.Interface,
 			continue
 		}
 		if entry[keyName] == webhookName {
-			return true, nil
+			return "", true, nil
 		}
 	}
-	return false, nil
+	return fmt.Sprintf("admission configuration %q exists but does not contain the %q webhook",
+		configName, webhookName), false, nil
 }
 
 // trainerResourceClient returns the namespaced or cluster-scoped client for gvr.
@@ -556,7 +571,19 @@ func ensureTrainerInstalled(ctx context.Context, dynamicClient dynamic.Interface
 // Both paths route through here so the verdict does not depend on whether the clock
 // ran out during a probe or during a sleep: the same cluster state must not produce
 // two different codes.
-func classifyPollExpiry(ctx, pollCtx context.Context, last trainerInstall) error {
+//
+// observed carries a concrete incomplete observation — a probe that succeeded and
+// named the object that is not there — and it is what separates the two failing
+// codes. NotFound may only be synthesized from such an observation. Deriving it from
+// an empty one would report a degraded or slow apiserver, where every probe attempt
+// was a read timeout and nothing was ever read, as a customer deployment defect; and
+// it would report a probe that found the installation complete as one that found
+// nothing. With no observation to stand on, the honest verdict is the timeout that
+// actually happened. The allowance is enforced either way — only the classification
+// differs.
+//
+// unobserved is the reason to report in that case.
+func classifyPollExpiry(ctx, pollCtx context.Context, observed trainerInstall, unobserved string) error {
 	if ctx.Err() != nil {
 		return aicrErrors.Wrap(aicrErrors.ErrCodeTimeout,
 			"canceled while waiting for the recipe-declared Kubeflow Trainer", ctx.Err())
@@ -565,20 +592,25 @@ func classifyPollExpiry(ctx, pollCtx context.Context, last trainerInstall) error
 		return nil
 	}
 
+	verdict := 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)
+
+	if observed.Incomplete == "" {
+		if unobserved == "" {
+			unobserved = "the allowance expired before any probe could tell"
+		}
+		return aicrErrors.New(aicrErrors.ErrCodeTimeout, fmt.Sprintf(verdict, unobserved))
+	}
+
 	// 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 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))
+	return aicrErrors.New(aicrErrors.ErrCodeNotFound, fmt.Sprintf(verdict, observed.Incomplete))
 }
 
 // awaitTrainerController waits for a Trainer the benchmark does not own to finish
@@ -637,7 +669,12 @@ func waitForDeclaredTrainer(ctx context.Context, dynamicClient dynamic.Interface
 			// exactly when the apiserver is degraded that the transport signal is
 			// worth the most.
 			if errors.Is(err, aicrErrors.New(aicrErrors.ErrCodeTimeout, "")) {
-				if verdict := classifyPollExpiry(ctx, pollCtx, last); verdict != nil {
+				// last is the newest concrete observation, and it is the zero value
+				// until some probe completes. When every attempt was cut short by the
+				// deadline there is nothing on the cluster to point at, so this stays
+				// a timeout rather than becoming a deployment defect.
+				if verdict := classifyPollExpiry(ctx, pollCtx, last,
+					"no probe completed before the allowance expired"); verdict != nil {
 					return trainerInstall{}, verdict
 				}
 			}
@@ -660,13 +697,16 @@ func waitForDeclaredTrainer(ctx context.Context, dynamicClient dynamic.Interface
 		// The complete case claims only what was observed. The wait never measures when
 		// the installation became complete, only when it saw that it was, so wording it
 		// as a transition would assert a time nobody read.
-		expired := install
+		//
+		// A complete probe is not an incomplete observation, so it is reported as the
+		// timeout it is: the installation is there, and the only finding is a rollout
+		// or a read slower than the budget.
+		observed, unobserved := install, ""
 		if ok {
-			expired = trainerInstall{
-				Incomplete: "the installation was observed complete only after the allowance expired",
-			}
+			observed = trainerInstall{}
+			unobserved = "the installation was observed complete only after the allowance expired"
 		}
-		if verdict := classifyPollExpiry(ctx, pollCtx, expired); verdict != nil {
+		if verdict := classifyPollExpiry(ctx, pollCtx, observed, unobserved); verdict != nil {
 			return trainerInstall{}, verdict
 		}
 		if ok {
@@ -676,7 +716,8 @@ func waitForDeclaredTrainer(ctx context.Context, dynamicClient dynamic.Interface
 
 		select {
 		case <-pollCtx.Done():
-			if verdict := classifyPollExpiry(ctx, pollCtx, last); verdict != nil {
+			if verdict := classifyPollExpiry(ctx, pollCtx, last,
+				"no probe completed before the allowance expired"); verdict != nil {
 				return trainerInstall{}, verdict
 			}
 			// unreachable: this case fires only once pollCtx is done, and

validators/performance/trainer_probe_test.go

@@ -17,7 +17,9 @@ package main
 import (
 	"context"
 	stderrors "errors"
+	"fmt"
 	"net"
+	"strings"
 	"testing"
 	"time"
 
@@ -721,3 +723,59 @@ func TestTrainerResourceRefString(t *testing.T) {
 		})
 	}
 }
+
+// TestIsTrainerInstalled_MutatingConfigReasonsAreDistinct pins the two shapes of a
+// missing mutating webhook apart.
+//
+// "The configuration does not exist" and "the configuration exists but does not
+// serve our webhook" need different remedies, and flattening both to the first tells
+// an operator to create an object that is already on the cluster — which, on the
+// generic upstream configuration names another operator may own, is the shape most
+// likely to be hit. The validating-webhook discovery path already keeps them apart;
+// this is the same contract on the mutating one.
+func TestIsTrainerInstalled_MutatingConfigReasonsAreDistinct(t *testing.T) {
+	tests := []struct {
+		name         string
+		objs         []runtime.Object
+		wantContains string
+		wantAbsent   string
+	}{
+		{
+			name: "configuration absent",
+			objs: withoutObject(completeTrainerInstall(), dropByName(trainerMutatingWebhookConfig)),
+			wantContains: fmt.Sprintf("admission configuration %q is missing",
+				trainerMutatingWebhookConfig),
+		},
+		{
+			name: "configuration present but serving another operator's webhook",
+			objs: append(
+				withoutObject(completeTrainerInstall(), dropByName(trainerMutatingWebhookConfig)),
+				webhookConfig("MutatingWebhookConfiguration", trainerMutatingWebhookConfig,
+					"defaulter.other.example.com"),
+			),
+			wantContains: fmt.Sprintf("exists but does not contain the %q webhook",
+				trainerMutatingWebhookName),
+			wantAbsent: fmt.Sprintf("admission configuration %q is missing",
+				trainerMutatingWebhookConfig),
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			install, installed, err := isTrainerInstalled(context.Background(), newTrainerFakeClient(tt.objs...))
+			if err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+			if installed {
+				t.Fatal("incomplete Trainer installation reported as installed")
+			}
+			if !strings.Contains(install.Incomplete, tt.wantContains) {
+				t.Errorf("reason = %q, want it to contain %q", install.Incomplete, tt.wantContains)
+			}
+			if tt.wantAbsent != "" && strings.Contains(install.Incomplete, tt.wantAbsent) {
+				t.Errorf("reason %q tells the operator to create a configuration that "+
+					"already exists", install.Incomplete)
+			}
+		})
+	}
+}