From b7342675006524c6a3acc09b85e2b77ad757def6 Mon Sep 17 00:00:00 2001 From: Mike Cook Date: Thu, 20 Aug 2026 22:46:29 -0700 Subject: [PATCH 01/11] fix(validator): tolerate all taints on Trainer/JobSet controller Deployments The Kubeflow Trainer/JobSet controller-manager Deployments ship with no tolerations. On a cluster where every node pool carries a taint (e.g. an arch-tainted GPU pool plus a system pool GKE reserves for its own managed components once no untainted pool remains), the controllers have nowhere to schedule and installTrainer times out waiting for a Deployment that can never become Ready. applyControllerTolerations stamps a blanket tolerate-all onto the Trainer and JobSet controller-manager Deployments specifically (by name) when either has no existing tolerations; a Deployment that already declares tolerations, or any other Deployment in the manifest set, is left untouched. Scoping by name rather than by Kind alone matters here: this is called for every Deployment decoded from the installer's manifest set, and a future addition to that set must not silently inherit a blanket {operator: Exists} it never asked for. Extract the repeated "operator" toleration-key literal into keyOperator to satisfy golangci-lint's goconst threshold across the package. Signed-off-by: Mike Cook --- validators/performance/consts.go | 1 + .../performance/inference_perf_constraint.go | 2 +- .../nccl_all_reduce_bw_constraint.go | 4 +- validators/performance/trainer_lifecycle.go | 61 ++++++++ .../performance/trainer_lifecycle_test.go | 140 ++++++++++++++++++ 5 files changed, 205 insertions(+), 3 deletions(-) diff --git a/validators/performance/consts.go b/validators/performance/consts.go index 097cdac24..8bdd93169 100644 --- a/validators/performance/consts.go +++ b/validators/performance/consts.go @@ -21,6 +21,7 @@ const ( versionV1alpha1 = "v1alpha1" versionV1beta1 = "v1beta1" keyName = "name" + keyOperator = "operator" checkNameNCCLAllReduceBW = "nccl-all-reduce-bw" // nodeJobName is the name of both the NCCL worker replicatedJob and its diff --git a/validators/performance/inference_perf_constraint.go b/validators/performance/inference_perf_constraint.go index d4fa5f883..9da3fab94 100644 --- a/validators/performance/inference_perf_constraint.go +++ b/validators/performance/inference_perf_constraint.go @@ -1823,7 +1823,7 @@ func tolerationsToUnstructured(tolerations []v1.Toleration) []any { tolList := make([]any, 0, len(tolerations)) for _, t := range tolerations { tolMap := map[string]any{ - "operator": string(t.Operator), + keyOperator: string(t.Operator), } if t.Key != "" { tolMap["key"] = t.Key diff --git a/validators/performance/nccl_all_reduce_bw_constraint.go b/validators/performance/nccl_all_reduce_bw_constraint.go index 8d2297ba8..92365f519 100644 --- a/validators/performance/nccl_all_reduce_bw_constraint.go +++ b/validators/performance/nccl_all_reduce_bw_constraint.go @@ -1451,7 +1451,7 @@ func applyNCCLWorkerScheduling(obj *unstructured.Unstructured, nodeSelector map[ tolList := make([]any, 0, len(tolerations)) for _, t := range tolerations { tolMap := map[string]any{ - "operator": string(t.Operator), + keyOperator: string(t.Operator), } if t.Key != "" { tolMap["key"] = t.Key @@ -1479,7 +1479,7 @@ func applyNCCLWorkerScheduling(obj *unstructured.Unstructured, nodeSelector map[ return unstructured.SetNestedSlice(obj.Object, replicatedJobs, "spec", "template", "spec", "replicatedJobs") } -// nestedMap navigates a chain of string keys through nested map[string]interface{} values. +// nestedMap navigates a chain of string keys through nested map[string]any values. // Returns the target map and true if found, nil and false otherwise. func nestedMap(m map[string]any, keys ...string) (map[string]any, bool) { current := m diff --git a/validators/performance/trainer_lifecycle.go b/validators/performance/trainer_lifecycle.go index d51ec0b5d..9d47b7979 100644 --- a/validators/performance/trainer_lifecycle.go +++ b/validators/performance/trainer_lifecycle.go @@ -32,8 +32,10 @@ import ( "strings" "time" + "github.com/NVIDIA/aicr/pkg/component" "github.com/NVIDIA/aicr/pkg/defaults" aicrErrors "github.com/NVIDIA/aicr/pkg/errors" + corev1 "k8s.io/api/core/v1" k8serrors "k8s.io/apimachinery/pkg/api/errors" apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -67,6 +69,11 @@ const ( // trainerControllerDeployment is the Deployment name for the Trainer controller-manager. trainerControllerDeployment = "kubeflow-trainer-controller-manager" + // jobSetControllerDeployment is the JobSet controller-manager Deployment name + // emitted by this package's kustomize overlay (see jobSetNameLabel for why + // the Helm chart's release-derived name doesn't apply here). + jobSetControllerDeployment = "jobset-controller-manager" + // trainerControllerService is the Service fronting the controller-manager's // webhook port. Without it the admission webhooks have no endpoints and every // TrainJob create is rejected. @@ -159,6 +166,57 @@ const ( jobSetPromotedImageRepo = "registry.k8s.io/jobset/jobset" ) +// controllerTolerateAll lets a Trainer/JobSet controller-manager Deployment +// schedule on any node pool, regardless of taints. Built through +// component.TolerationsToPodSpec, the same converter used for Helm-values +// toleration overrides, so there is one canonical place that knows the +// toleration-to-map shape. +var controllerTolerateAll = tolerationsToAnySlice( + component.TolerationsToPodSpec([]corev1.Toleration{{Operator: corev1.TolerationOpExists}}), +) + +// tolerationsToAnySlice widens []map[string]any to []any: unstructured pod +// specs (podSpec["tolerations"]) must hold []any, not []map[string]any, to +// match how NestedSlice reads and how JSON round-tripping serializes it. +func tolerationsToAnySlice(tolerations []map[string]any) []any { + result := make([]any, len(tolerations)) + for i, t := range tolerations { + result[i] = t + } + return result +} + +// applyControllerTolerations stamps controllerTolerateAll onto the Trainer and +// JobSet controller-manager Deployments' pod template, unless one already +// declares tolerations. Scoped to those two names so an unrelated Deployment +// in the manifest set never gets a blanket toleration it didn't ask for. +func applyControllerTolerations(obj *unstructured.Unstructured) error { + if obj.GroupVersionKind().Kind != "Deployment" { + return nil + } + switch obj.GetName() { + case trainerControllerDeployment, jobSetControllerDeployment: + default: + return nil + } + + if existing, found, err := unstructured.NestedSlice(obj.Object, "spec", "template", "spec", "tolerations"); err != nil { + return aicrErrors.Wrap(aicrErrors.ErrCodeInternal, + fmt.Sprintf("failed to read tolerations from Deployment %q", obj.GetName()), err) + } else if found && len(existing) > 0 { + return nil + } + + podSpec, found := nestedMap(obj.Object, "spec", "template", "spec") + if !found { + return aicrErrors.New(aicrErrors.ErrCodeInternal, + fmt.Sprintf("pod spec not found in Deployment %q", obj.GetName())) + } + podSpec["tolerations"] = controllerTolerateAll + slog.Info("Applying blanket toleration to controller Deployment", "name", obj.GetName()) + return nil +} + // GVRs for the objects the Trainer lifecycle probes and waits on. var ( trainerCRDGVR = schema.GroupVersionResource{ @@ -807,6 +865,9 @@ func decodeTrainerObjects(resources []*resource.Resource) ([]*unstructured.Unstr if obj.GroupVersionKind().Kind == "" { continue } + if tolErr := applyControllerTolerations(obj); tolErr != nil { + return nil, tolErr + } objs = append(objs, obj) } return objs, nil diff --git a/validators/performance/trainer_lifecycle_test.go b/validators/performance/trainer_lifecycle_test.go index b03cfe7ef..71a63221c 100644 --- a/validators/performance/trainer_lifecycle_test.go +++ b/validators/performance/trainer_lifecycle_test.go @@ -17,6 +17,8 @@ package main import ( "strings" "testing" + + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) func TestRewriteJobSetStagingImage(t *testing.T) { @@ -76,3 +78,141 @@ func TestRewriteJobSetStagingImage_PreservesTag(t *testing.T) { t.Errorf("got %q, want %q", got, want) } } + +// deploymentFixture returns a minimal unstructured Deployment, optionally with an +// existing tolerations list, for exercising applyControllerTolerations. +func deploymentFixture(name string, existingTolerations []any) *unstructured.Unstructured { + podSpec := map[string]any{ + "containers": []any{ + map[string]any{"name": "manager", "image": "example/manager:latest"}, + }, + } + if existingTolerations != nil { + podSpec["tolerations"] = existingTolerations + } + return &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "apps/v1", + "kind": "Deployment", + "metadata": map[string]any{"name": name}, + "spec": map[string]any{ + "template": map[string]any{ + "spec": podSpec, + }, + }, + }} +} + +// TestApplyControllerTolerations covers both controller names, the two +// mutation-failure paths, and that an unrelated Deployment is left untouched. +func TestApplyControllerTolerations(t *testing.T) { + tests := []struct { + name string + obj *unstructured.Unstructured + wantErr bool + // wantTolerations is checked only when wantErr is false. nil means "the + // tolerations field must not be present at all" (untouched, not merely + // empty). + wantTolerations []any + }{ + { + name: "Trainer controller Deployment with no tolerations gets tolerate-all", + obj: deploymentFixture(trainerControllerDeployment, nil), + wantTolerations: []any{ + map[string]any{"operator": "Exists"}, + }, + }, + { + name: "JobSet controller Deployment with no tolerations gets tolerate-all", + obj: deploymentFixture(jobSetControllerDeployment, nil), + wantTolerations: []any{ + map[string]any{"operator": "Exists"}, + }, + }, + { + name: "Deployment with existing tolerations is left untouched", + obj: deploymentFixture(trainerControllerDeployment, []any{ + map[string]any{"key": "dedicated", "operator": "Equal", "value": "trainer", "effect": "NoSchedule"}, + }), + wantTolerations: []any{ + map[string]any{"key": "dedicated", "operator": "Equal", "value": "trainer", "effect": "NoSchedule"}, + }, + }, + { + name: "non-controller Deployment is left untouched", + obj: deploymentFixture("some-other-deployment", nil), + wantTolerations: nil, + }, + { + name: "non-Deployment resource is left untouched", + obj: &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "v1", + "kind": "Service", + "metadata": map[string]any{"name": trainerControllerDeployment}, + "spec": map[string]any{}, + }}, + wantTolerations: nil, + }, + { + name: "missing pod spec fails closed", + obj: &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "apps/v1", + "kind": "Deployment", + "metadata": map[string]any{"name": trainerControllerDeployment}, + "spec": map[string]any{}, + }}, + wantErr: true, + }, + { + name: "malformed tolerations field fails closed", + obj: &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "apps/v1", + "kind": "Deployment", + "metadata": map[string]any{"name": trainerControllerDeployment}, + "spec": map[string]any{ + "template": map[string]any{ + "spec": map[string]any{ + // A string, not a slice: NestedSlice's type assertion fails. + "tolerations": "not-a-slice", + }, + }, + }, + }}, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := applyControllerTolerations(tt.obj) + if (err != nil) != tt.wantErr { + t.Fatalf("error = %v, wantErr %v", err, tt.wantErr) + } + if tt.wantErr { + return + } + + got, found, _ := unstructured.NestedSlice(tt.obj.Object, "spec", "template", "spec", "tolerations") + if tt.wantTolerations == nil { + if found { + t.Errorf("expected no tolerations field, got %v", got) + } + return + } + if !found { + t.Fatalf("expected tolerations %v, found none", tt.wantTolerations) + } + if len(got) != len(tt.wantTolerations) { + t.Fatalf("got %d toleration(s) %v, want %d %v", len(got), got, len(tt.wantTolerations), tt.wantTolerations) + } + for i := range got { + gotTol, _ := got[i].(map[string]any) + wantTol, _ := tt.wantTolerations[i].(map[string]any) + for k, v := range wantTol { + if gotTol[k] != v { + t.Errorf("toleration[%d][%q] = %v, want %v", i, k, gotTol[k], v) + } + } + } + }) + } +} From d9f20d03be78c6c9ff0954a3e561caedd02f5f41 Mon Sep 17 00:00:00 2001 From: Mike Cook Date: Fri, 21 Aug 2026 13:47:42 -0700 Subject: [PATCH 02/11] fix(validator): widen Trainer controller readiness timeout On cold start, the Kubeflow Trainer controller-manager's cert-controller sidecar provisions its webhook cert via a get-or-create against the API server; racing that against a not-yet-synced informer cache produces a resourceVersion conflict on the update. This is expected behavior under cert-controller's optimistic-concurrency retry, not a defect in Trainer or in this validator. The sidecar's own reconcile loop retries and self-heals unassisted. Each retry adds latency, though, and on a slow cold start the cumulative delay can push first-ready past the old 2-minute budget, failing the validator's readiness wait for a controller that was already recovering on its own. Widen to 3 minutes so the wait accommodates the expected retry latency instead of racing it. Signed-off-by: Mike Cook --- pkg/defaults/timeouts.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/defaults/timeouts.go b/pkg/defaults/timeouts.go index daeea3b9b..a8ddd1ed4 100644 --- a/pkg/defaults/timeouts.go +++ b/pkg/defaults/timeouts.go @@ -667,7 +667,7 @@ const ( // TrainerControllerReadyTimeout is the time to wait for the Kubeflow Trainer // controller-manager Deployment to have at least one ready replica after installation. - TrainerControllerReadyTimeout = 2 * time.Minute + TrainerControllerReadyTimeout = 3 * time.Minute // TrainerInstallPollInterval is the sleep between checks that a // recipe-declared Kubeflow Trainer installation has become complete. The From efd453267cd421331626d4fe6172fd260e4c865b Mon Sep 17 00:00:00 2001 From: Mike Cook Date: Fri, 28 Aug 2026 13:39:55 -0700 Subject: [PATCH 03/11] fix(validator): warn when a controller Deployment name is unmatched njhensley flagged that applyControllerTolerations matches the Trainer and JobSet controller-manager Deployments by exact name: a future Trainer archive bump (or the JobSet overlay variant) that renames either emitted Deployment would make both switch cases miss silently, reverting that controller to unschedulable on an all-tainted cluster with no signal beyond a bare readiness timeout downstream. decodeTrainerObjects now tracks which of the two expected controller names it actually saw among the decoded Deployments and logs a warning for any that are missing, so a future rename fails loudly instead of surfacing only as an opaque readiness timeout. Signed-off-by: Mike Cook --- validators/performance/trainer_lifecycle.go | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/validators/performance/trainer_lifecycle.go b/validators/performance/trainer_lifecycle.go index 9d47b7979..03e2c0977 100644 --- a/validators/performance/trainer_lifecycle.go +++ b/validators/performance/trainer_lifecycle.go @@ -849,6 +849,7 @@ func installTrainer(ctx context.Context, dynamicClient dynamic.Interface, discov // before the first apply so a malformed manifest cannot leave a partial install. func decodeTrainerObjects(resources []*resource.Resource) ([]*unstructured.Unstructured, error) { objs := make([]*unstructured.Unstructured, 0, len(resources)) + seenControllers := make(map[string]bool, 2) for _, res := range resources { // Convert to unstructured via YAML round-trip (guarantees plain Go types). yamlBytes, err := res.AsYAML() @@ -868,8 +869,24 @@ func decodeTrainerObjects(resources []*resource.Resource) ([]*unstructured.Unstr if tolErr := applyControllerTolerations(obj); tolErr != nil { return nil, tolErr } + if obj.GroupVersionKind().Kind == "Deployment" { + seenControllers[obj.GetName()] = true + } objs = append(objs, obj) } + + // A future Trainer archive bump (or a JobSet overlay variant) that renames + // either controller Deployment makes applyControllerTolerations's name + // switch miss it silently, reverting that controller to unschedulable on + // an all-tainted cluster. Surface the mismatch here instead of letting it + // resurface only as a bare readiness timeout downstream. + for _, name := range []string{trainerControllerDeployment, jobSetControllerDeployment} { + if !seenControllers[name] { + slog.Warn("Expected controller Deployment not found in Trainer manifest set; "+ + "it will not receive the blanket toleration and may be unschedulable on all-tainted clusters", + "deployment", name) + } + } return objs, nil } From a150d8c6a6ee0c6e1dafa5bf41f7c5e50a546272 Mon Sep 17 00:00:00 2001 From: Mike Cook Date: Fri, 28 Aug 2026 13:41:41 -0700 Subject: [PATCH 04/11] docs(defaults): capture the 2m->3m Trainer readiness rationale in-code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit njhensley noted the value changed 2m->3m but the doc comment still described only "at least one ready replica after installation," with the cert-controller cold-start / retry reasoning living only in the PR body — lost on squash-merge. Fold the cert-controller cold-start retry rationale into the TrainerControllerReadyTimeout comment, including that the resourceVersion conflict it retries through is expected behavior under cert-controller's optimistic-concurrency retry, not a defect in Trainer or this validator, so a future engineer does not tune the value back down and reintroduce the flake. Signed-off-by: Mike Cook --- pkg/defaults/timeouts.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/pkg/defaults/timeouts.go b/pkg/defaults/timeouts.go index a8ddd1ed4..9dd457a28 100644 --- a/pkg/defaults/timeouts.go +++ b/pkg/defaults/timeouts.go @@ -666,7 +666,14 @@ const ( TrainerCRDEstablishedTimeout = 2 * time.Minute // TrainerControllerReadyTimeout is the time to wait for the Kubeflow Trainer - // controller-manager Deployment to have at least one ready replica after installation. + // controller-manager Deployment to have at least one ready replica after + // installation. Widened from 2m to 3m: on cold start the cert-controller + // sidecar's webhook-cert get-or-create can race a not-yet-synced informer + // cache, producing a resourceVersion conflict that the sidecar's own + // reconcile loop retries and self-heals from unassisted. This is expected + // behavior under cert-controller's optimistic-concurrency retry, not a + // defect in Trainer or in this validator — but each retry adds latency + // that could otherwise push first-ready past a tighter budget. TrainerControllerReadyTimeout = 3 * time.Minute // TrainerInstallPollInterval is the sleep between checks that a From e40070b37e6f3923bba30458f74fabb83567a91a Mon Sep 17 00:00:00 2001 From: Mike Cook Date: Fri, 28 Aug 2026 13:48:39 -0700 Subject: [PATCH 05/11] fix(validator): stop aliasing controllerTolerateAll into live Deployments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit njhensley flagged that controllerTolerateAll is a package-level []any holding one shared map[string]any, assigned by reference into both the Trainer and JobSet Deployments' pod specs. No live bug today — both apply paths DeepCopy() before Create/Update, and nothing mutates tolerations in place — but a future in-place edit of one controller's tolerations would silently corrupt the other Deployment and the process-wide global. Add cloneControllerTolerateAll and stamp a fresh copy per call instead of the shared slice/map, and pin the isolation with a test that mutates one Deployment's stamped toleration and asserts it does not leak into the other Deployment or the shared global. Signed-off-by: Mike Cook --- validators/performance/trainer_lifecycle.go | 20 ++++++++++++- .../performance/trainer_lifecycle_test.go | 29 +++++++++++++++++++ 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/validators/performance/trainer_lifecycle.go b/validators/performance/trainer_lifecycle.go index 03e2c0977..5721d556a 100644 --- a/validators/performance/trainer_lifecycle.go +++ b/validators/performance/trainer_lifecycle.go @@ -186,6 +186,24 @@ func tolerationsToAnySlice(tolerations []map[string]any) []any { return result } +// cloneControllerTolerateAll returns a fresh copy of controllerTolerateAll. +// It is stamped onto both the Trainer and JobSet Deployments, so a per-call +// copy keeps a future in-place edit of one controller's tolerations from +// silently corrupting the other Deployment's live object, or the shared +// process-wide global itself. +func cloneControllerTolerateAll() []any { + cloned := make([]any, len(controllerTolerateAll)) + for i, t := range controllerTolerateAll { + m, _ := t.(map[string]any) + clonedMap := make(map[string]any, len(m)) + for k, v := range m { + clonedMap[k] = v + } + cloned[i] = clonedMap + } + return cloned +} + // applyControllerTolerations stamps controllerTolerateAll onto the Trainer and // JobSet controller-manager Deployments' pod template, unless one already // declares tolerations. Scoped to those two names so an unrelated Deployment @@ -212,7 +230,7 @@ func applyControllerTolerations(obj *unstructured.Unstructured) error { return aicrErrors.New(aicrErrors.ErrCodeInternal, fmt.Sprintf("pod spec not found in Deployment %q", obj.GetName())) } - podSpec["tolerations"] = controllerTolerateAll + podSpec["tolerations"] = cloneControllerTolerateAll() slog.Info("Applying blanket toleration to controller Deployment", "name", obj.GetName()) return nil } diff --git a/validators/performance/trainer_lifecycle_test.go b/validators/performance/trainer_lifecycle_test.go index 71a63221c..6c801f923 100644 --- a/validators/performance/trainer_lifecycle_test.go +++ b/validators/performance/trainer_lifecycle_test.go @@ -104,6 +104,35 @@ func deploymentFixture(name string, existingTolerations []any) *unstructured.Uns // TestApplyControllerTolerations covers both controller names, the two // mutation-failure paths, and that an unrelated Deployment is left untouched. +// TestApplyControllerTolerations_Isolation pins that the two controllers do +// not share a live toleration slice: mutating one Deployment's stamped +// tolerations in place must not affect the other, or the shared +// controllerTolerateAll package-level global. +func TestApplyControllerTolerations_Isolation(t *testing.T) { + trainerObj := deploymentFixture(trainerControllerDeployment, nil) + jobSetObj := deploymentFixture(jobSetControllerDeployment, nil) + + if err := applyControllerTolerations(trainerObj); err != nil { + t.Fatalf("applyControllerTolerations(trainer) error = %v", err) + } + if err := applyControllerTolerations(jobSetObj); err != nil { + t.Fatalf("applyControllerTolerations(jobset) error = %v", err) + } + + trainerTols, _, _ := unstructured.NestedSlice(trainerObj.Object, "spec", "template", "spec", "tolerations") + trainerTol, _ := trainerTols[0].(map[string]any) + trainerTol["key"] = "mutated-for-trainer-only" + + jobSetTols, _, _ := unstructured.NestedSlice(jobSetObj.Object, "spec", "template", "spec", "tolerations") + jobSetTol, _ := jobSetTols[0].(map[string]any) + if _, mutated := jobSetTol["key"]; mutated { + t.Errorf("mutating the Trainer Deployment's toleration leaked into the JobSet Deployment: %v", jobSetTol) + } + if _, mutated := controllerTolerateAll[0].(map[string]any)["key"]; mutated { + t.Errorf("mutating a stamped toleration leaked into the shared controllerTolerateAll global: %v", controllerTolerateAll[0]) + } +} + func TestApplyControllerTolerations(t *testing.T) { tests := []struct { name string From b6ed346f9b3e4e0cb8405395d0023975e01e2ff8 Mon Sep 17 00:00:00 2001 From: Mike Cook Date: Fri, 28 Aug 2026 13:49:58 -0700 Subject: [PATCH 06/11] fix(validator): log when a controller Deployment already has tolerations njhensley noted the apply branch logs "Applying blanket toleration...", but the "already tolerated" skip branch returned silently. An operator debugging why a controller Deployment is still Pending on a tainted pool got no signal that the blanket toleration was deliberately withheld because the manifest already declared its own. Add a slog.Debug before the early return so the skip is observable alongside the apply-path log. Signed-off-by: Mike Cook --- validators/performance/trainer_lifecycle.go | 1 + 1 file changed, 1 insertion(+) diff --git a/validators/performance/trainer_lifecycle.go b/validators/performance/trainer_lifecycle.go index 5721d556a..ab1e43701 100644 --- a/validators/performance/trainer_lifecycle.go +++ b/validators/performance/trainer_lifecycle.go @@ -222,6 +222,7 @@ func applyControllerTolerations(obj *unstructured.Unstructured) error { return aicrErrors.Wrap(aicrErrors.ErrCodeInternal, fmt.Sprintf("failed to read tolerations from Deployment %q", obj.GetName()), err) } else if found && len(existing) > 0 { + slog.Debug("Controller Deployment already declares tolerations; leaving untouched", "name", obj.GetName()) return nil } From 71c1d755093e089399b76ddf52280dce36449b5f Mon Sep 17 00:00:00 2001 From: Mike Cook Date: Fri, 28 Aug 2026 13:50:37 -0700 Subject: [PATCH 07/11] fix(validator): include namespace on applied-toleration log njhensley noted the applied-toleration log was name-only while the surrounding Deployment-lifecycle logs in this file include namespace. The overlay sets namespace, so it is available at decode time. Add namespace to the log for symmetry with the rest of the file. Signed-off-by: Mike Cook --- validators/performance/trainer_lifecycle.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/validators/performance/trainer_lifecycle.go b/validators/performance/trainer_lifecycle.go index ab1e43701..5b7724385 100644 --- a/validators/performance/trainer_lifecycle.go +++ b/validators/performance/trainer_lifecycle.go @@ -232,7 +232,7 @@ func applyControllerTolerations(obj *unstructured.Unstructured) error { fmt.Sprintf("pod spec not found in Deployment %q", obj.GetName())) } podSpec["tolerations"] = cloneControllerTolerateAll() - slog.Info("Applying blanket toleration to controller Deployment", "name", obj.GetName()) + slog.Info("Applying blanket toleration to controller Deployment", "name", obj.GetName(), "namespace", obj.GetNamespace()) return nil } From f744ce246ad12d231960cde19b1370ccef4c476d Mon Sep 17 00:00:00 2001 From: Mike Cook Date: Fri, 28 Aug 2026 13:52:26 -0700 Subject: [PATCH 08/11] fix(validator): also gate controller toleration match on apps group njhensley noted the Kind check in applyControllerTolerations matched the "Deployment" Kind string alone, so a Deployment-kind resource in a non-apps group whose name collided with a controller name would be stamped. Harmless for the kustomize overlay this runs over today (only the two standard apps/v1 Deployments), but purely defensive. Assert GroupVersionKind().Group == "apps" alongside the Kind check, and add a test case for a Deployment-kind resource in a non-apps group. Signed-off-by: Mike Cook --- validators/performance/trainer_lifecycle.go | 2 +- validators/performance/trainer_lifecycle_test.go | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/validators/performance/trainer_lifecycle.go b/validators/performance/trainer_lifecycle.go index 5b7724385..0f17f267b 100644 --- a/validators/performance/trainer_lifecycle.go +++ b/validators/performance/trainer_lifecycle.go @@ -209,7 +209,7 @@ func cloneControllerTolerateAll() []any { // declares tolerations. Scoped to those two names so an unrelated Deployment // in the manifest set never gets a blanket toleration it didn't ask for. func applyControllerTolerations(obj *unstructured.Unstructured) error { - if obj.GroupVersionKind().Kind != "Deployment" { + if gvk := obj.GroupVersionKind(); gvk.Kind != "Deployment" || gvk.Group != "apps" { return nil } switch obj.GetName() { diff --git a/validators/performance/trainer_lifecycle_test.go b/validators/performance/trainer_lifecycle_test.go index 6c801f923..7422e3e1b 100644 --- a/validators/performance/trainer_lifecycle_test.go +++ b/validators/performance/trainer_lifecycle_test.go @@ -181,6 +181,16 @@ func TestApplyControllerTolerations(t *testing.T) { }}, wantTolerations: nil, }, + { + name: "Deployment-kind resource in a non-apps group is left untouched", + obj: &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "example.com/v1", + "kind": "Deployment", + "metadata": map[string]any{"name": trainerControllerDeployment}, + "spec": map[string]any{}, + }}, + wantTolerations: nil, + }, { name: "missing pod spec fails closed", obj: &unstructured.Unstructured{Object: map[string]any{ From 7a8f40eb7e6e30ce6ec1397b66a7fe096a2d9ab2 Mon Sep 17 00:00:00 2001 From: Mike Cook Date: Fri, 28 Aug 2026 13:52:52 -0700 Subject: [PATCH 09/11] test(validator): pin present-but-empty tolerations boundary njhensley noted the guard `found && len(existing) > 0` means a Deployment shipping `tolerations: []` falls through and gets stamped (the desired behavior), but the existing tests covered only nil and non-empty tolerations, leaving this boundary unpinned against a future refactor that flips the guard to `found` alone. Add a deploymentFixture case with an explicit empty tolerations slice asserting it still receives the blanket toleration. Signed-off-by: Mike Cook --- validators/performance/trainer_lifecycle_test.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/validators/performance/trainer_lifecycle_test.go b/validators/performance/trainer_lifecycle_test.go index 7422e3e1b..1503d4102 100644 --- a/validators/performance/trainer_lifecycle_test.go +++ b/validators/performance/trainer_lifecycle_test.go @@ -171,6 +171,17 @@ func TestApplyControllerTolerations(t *testing.T) { obj: deploymentFixture("some-other-deployment", nil), wantTolerations: nil, }, + { + // found && len(existing) > 0 is the guard: an explicit empty slice is + // "present but empty," which must fall through to being stamped, not + // be treated the same as "already tolerated." Pins this boundary + // against a future refactor that flips the guard to `found` alone. + name: "Deployment with present-but-empty tolerations gets tolerate-all", + obj: deploymentFixture(trainerControllerDeployment, []any{}), + wantTolerations: []any{ + map[string]any{"operator": "Exists"}, + }, + }, { name: "non-Deployment resource is left untouched", obj: &unstructured.Unstructured{Object: map[string]any{ From bdad2511c60138213585e72d8e29124f9e65c3b6 Mon Sep 17 00:00:00 2001 From: Mike Cook Date: Fri, 28 Aug 2026 13:56:54 -0700 Subject: [PATCH 10/11] test(validator): add end-to-end coverage for decodeTrainerObjects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit njhensley noted applyControllerTolerations is unit-tested directly (including both error paths), but its only call site — decodeTrainerObjects — had no test. The wiring (toleration actually lands on the returned objects, decode-error propagation, and the Kind=="" skip ordering just above the applyControllerTolerations call) was unverified. Feed decodeTrainerObjects a small kustomize-resource fixture with a controller Deployment plus an unrelated ConfigMap and assert the toleration lands only on the controller; add a second case asserting a malformed tolerations field aborts decoding with a non-nil error. Signed-off-by: Mike Cook --- .../performance/trainer_lifecycle_test.go | 93 +++++++++++++++++++ 1 file changed, 93 insertions(+) diff --git a/validators/performance/trainer_lifecycle_test.go b/validators/performance/trainer_lifecycle_test.go index 1503d4102..6e85f7fd1 100644 --- a/validators/performance/trainer_lifecycle_test.go +++ b/validators/performance/trainer_lifecycle_test.go @@ -15,10 +15,13 @@ package main import ( + "fmt" "strings" "testing" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "sigs.k8s.io/kustomize/api/hasher" + "sigs.k8s.io/kustomize/api/resource" ) func TestRewriteJobSetStagingImage(t *testing.T) { @@ -266,3 +269,93 @@ func TestApplyControllerTolerations(t *testing.T) { }) } } + +// TestDecodeTrainerObjects exercises decodeTrainerObjects end to end: the +// toleration lands only on a controller Deployment resource, not on an +// unrelated resource in the same manifest set, and the Kind=="" skip ordering +// above the applyControllerTolerations call does not interfere. +func TestDecodeTrainerObjects(t *testing.T) { + rf := resource.NewFactory(&hasher.Hasher{}) + + manifest := fmt.Sprintf(`apiVersion: apps/v1 +kind: Deployment +metadata: + name: %s +spec: + template: + spec: + containers: + - name: manager + image: example/trainer:latest +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: some-config +data: + foo: bar +`, trainerControllerDeployment) + + resources, err := rf.SliceFromBytes([]byte(manifest)) + if err != nil { + t.Fatalf("SliceFromBytes() error = %v", err) + } + + objs, err := decodeTrainerObjects(resources) + if err != nil { + t.Fatalf("decodeTrainerObjects() error = %v", err) + } + if len(objs) != 2 { + t.Fatalf("got %d decoded object(s), want 2", len(objs)) + } + + var trainerObj, unrelatedObj *unstructured.Unstructured + for _, o := range objs { + switch o.GetName() { + case trainerControllerDeployment: + trainerObj = o + case "some-config": + unrelatedObj = o + } + } + if trainerObj == nil { + t.Fatal("Trainer controller Deployment missing from decoded objects") + } + tols, found, _ := unstructured.NestedSlice(trainerObj.Object, "spec", "template", "spec", "tolerations") + if !found || len(tols) != 1 { + t.Errorf("Trainer Deployment tolerations = %v, want a single blanket tolerate-all entry", tols) + } + + if unrelatedObj == nil { + t.Fatal("unrelated ConfigMap missing from decoded objects") + } + if _, found, _ := unstructured.NestedSlice(unrelatedObj.Object, "spec", "template", "spec", "tolerations"); found { + t.Error("unrelated ConfigMap must not receive a toleration") + } +} + +// TestDecodeTrainerObjects_PropagatesTolerationError verifies a malformed +// resource that fails applyControllerTolerations aborts decoding rather than +// leaving a partial object list. +func TestDecodeTrainerObjects_PropagatesTolerationError(t *testing.T) { + rf := resource.NewFactory(&hasher.Hasher{}) + + manifest := fmt.Sprintf(`apiVersion: apps/v1 +kind: Deployment +metadata: + name: %s +spec: + template: + spec: + tolerations: not-a-slice +`, trainerControllerDeployment) + + resources, err := rf.SliceFromBytes([]byte(manifest)) + if err != nil { + t.Fatalf("SliceFromBytes() error = %v", err) + } + + if _, err := decodeTrainerObjects(resources); err == nil { + t.Fatal("decodeTrainerObjects() expected error for a malformed tolerations field, got nil") + } +} From 4ae436aa6116a272d8e6d114e0b172d3e631b794 Mon Sep 17 00:00:00 2001 From: Mike Cook Date: Fri, 28 Aug 2026 14:00:37 -0700 Subject: [PATCH 11/11] test(validator): pin the operator key in NCCL toleration assertions njhensley noted the tolerationsToUnstructured / applyNCCLWorkerScheduling tests asserted key/value/effect but never read tol["operator"], so the const-keyed output key (keyOperator) was compiler-checked but not test-pinned. Add an operator assertion to the existing toleration checks in TestApplyNCCLWorkerScheduling_Tolerations and _Both to harden the refactor. Signed-off-by: Mike Cook --- validators/performance/nccl_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/validators/performance/nccl_test.go b/validators/performance/nccl_test.go index 5db706ac9..17c1c1e3a 100644 --- a/validators/performance/nccl_test.go +++ b/validators/performance/nccl_test.go @@ -142,8 +142,8 @@ func TestApplyNCCLWorkerScheduling_Tolerations(t *testing.T) { t.Fatalf("tolerations count = %d, want 1", len(tolsRaw)) } tol, _ := tolsRaw[0].(map[string]any) - if tol["key"] != "gpu-type" || tol["value"] != "h100" || tol["effect"] != "NoSchedule" { - t.Errorf("toleration = %v, want gpu-type=h100:NoSchedule", tol) + if tol["key"] != "gpu-type" || tol["value"] != "h100" || tol["effect"] != "NoSchedule" || tol["operator"] != "Equal" { + t.Errorf("toleration = %v, want gpu-type=h100:NoSchedule operator=Equal", tol) } } } @@ -209,8 +209,8 @@ func TestApplyNCCLWorkerScheduling_Both(t *testing.T) { t.Fatalf("tolerations count = %d, want 1", len(tolsRaw)) } tol, _ := tolsRaw[0].(map[string]any) - if tol["key"] != "custom-taint" || tol["value"] != "true" || tol["effect"] != "NoSchedule" { - t.Errorf("toleration = %v, want custom-taint=true:NoSchedule", tol) + if tol["key"] != "custom-taint" || tol["value"] != "true" || tol["effect"] != "NoSchedule" || tol["operator"] != "Equal" { + t.Errorf("toleration = %v, want custom-taint=true:NoSchedule operator=Equal", tol) } } }