test(validator): make the fall-through guard actually reach the branch

The regression row added last round could not fail. It seeded the fake with
completeTrainerInstall(), so the first probe in ensureTrainerInstalled succeeded,
waitForDeclaredTrainer was never entered, and the row exercised the same path as
the one above it. A guard that passes whether or not the bug is present is worse
than no guard, because it reports coverage it does not have.

Replaced with a stateful fake reproducing the state the fall-through actually
lived in: incomplete on the first probe, complete on a later one, which is what a
chart still landing looks like. The test asserts no resources were claimed for
cleanup and that the probe ran more than once, so it fails if the wait is never
entered.

Verified by control: reintroducing the fall-through makes it fail, and restoring
the fix makes it pass.

Also carries discovery's own reason out of isTrainerInstalled instead of
flattening it to a bare false. 'No admission configuration at all' and 'the
configuration exists but names no Service' are different fixes, and both are
reachable during the rollout this PR now waits through.

Corrects the doc comment that still claimed the same recipe behaves identically
regardless of live state. What the recipe determines is what a missing
installation *means*; execution still differs across the undeclared rows, which
is why that wording was withdrawn in #2297.

Refs #2297

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

Author: yuanchen8911

validators/performance/trainer_ensure_test.go

@@ -18,13 +18,15 @@ import (
 	"context"
 	stderrors "errors"
 	"strings"
+	"sync/atomic"
 	"testing"
 	"time"
 
 	aicrErrors "github.com/NVIDIA/aicr/pkg/errors"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	k8stesting "k8s.io/client-go/testing"
 )
 
@@ -277,15 +279,6 @@ func TestEnsureTrainerInstalled_RecipeDrivenLifecycle(t *testing.T) {
 			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(),
-		},
 	}
 
 	for _, tt := range tests {
@@ -362,3 +355,51 @@ func TestWaitForDeclaredTrainer_CanceledRunIsNotADeploymentDefect(t *testing.T)
 		t.Errorf("error code = %v, want ErrCodeTimeout", err)
 	}
 }
+
+// TestEnsureTrainerInstalled_DeclaredRolloutDoesNotFallThrough pins the branch the
+// fall-through actually lived in.
+//
+// A table row seeded with completeTrainerInstall() cannot reach it: the first probe
+// in ensureTrainerInstalled succeeds, so waitForDeclaredTrainer is never entered and
+// the row exercises the same path as the one above it. The supported rollout state —
+// initially incomplete, complete on a later poll — is the one that enters the wait
+// and then must return rather than continuing into the install path.
+func TestEnsureTrainerInstalled_DeclaredRolloutDoesNotFallThrough(t *testing.T) {
+	defer withShortTrainerWait(t)()
+
+	client := newTrainerFakeClient()
+	var probes int32
+	// Report incomplete on the first probe and complete afterwards, which is what a
+	// chart still landing looks like. The reactor answers only the first read the
+	// probe makes; a NotFound there is enough to make the whole probe incomplete.
+	client.PrependReactor("get", "customresourcedefinitions",
+		func(k8stesting.Action) (bool, runtime.Object, error) {
+			if atomic.AddInt32(&probes, 1) == 1 {
+				return true, nil, apierrors.NewNotFound(
+					schema.GroupResource{Group: "apiextensions.k8s.io", Resource: "customresourcedefinitions"},
+					"trainjobs.trainer.kubeflow.org")
+			}
+			return false, nil, nil // fall through to the tracker
+		})
+	for _, obj := range completeTrainerInstall() {
+		if err := client.Tracker().Add(obj); err != nil {
+			t.Fatalf("seeding fake: %v", err)
+		}
+	}
+
+	// A nil discovery client makes installTrainer panic or error immediately, so if
+	// the code falls through to the install path this test fails loudly rather than
+	// silently passing.
+	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 Trainer the recipe delivered must never be "+
+			"claimed for cleanup, and a non-empty result means the install path ran", len(refs))
+	}
+	if atomic.LoadInt32(&probes) < 2 {
+		t.Errorf("probes = %d, want >= 2: the wait was never entered, so this test "+
+			"does not cover the branch it claims to", probes)
+	}
+}

validators/performance/trainer_lifecycle.go

@@ -250,9 +250,16 @@ func isTrainerInstalled(ctx context.Context, dynamicClient dynamic.Interface) (t
 	// makes this work for both the self-install overlay and the Helm chart.
 	install, found, err := discoverTrainerInstall(ctx, dynamicClient,
 		trainerValidatingWebhookGVR, trainerValidatingWebhookConfig, trainerValidatingWebhookName)
-	if err != nil || !found {
+	if err != nil {
 		return trainerInstall{}, false, err
 	}
+	if !found {
+		// Carry discovery's own reason out rather than flattening it to a bare
+		// false: it is the difference between "no admission configuration at all"
+		// and "the configuration is there but names no Service", and the operator
+		// needs to know which to fix.
+		return install, false, nil
+	}
 
 	ok, err := hasTrainerWebhook(ctx, dynamicClient,
 		trainerMutatingWebhookGVR, trainerMutatingWebhookConfig, trainerMutatingWebhookName)
@@ -456,8 +463,11 @@ func trainerResourceClient(dynamicClient dynamic.Interface,
 //   - 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.
+// This is deliberately keyed on the recipe rather than on live cluster state: what a
+// missing installation *means* is now a property of the recipe, not of whatever
+// happens to be on the cluster. Execution still differs across the undeclared rows —
+// a pre-existing Trainer is reused, an absent one is installed — which is why the
+// earlier "behaves identically regardless of live state" wording was withdrawn.
 func ensureTrainerInstalled(ctx context.Context, dynamicClient dynamic.Interface,
 	discoveryClient discovery.DiscoveryInterface, recipeDeclaresTrainer bool) ([]trainerResourceRef, error) {