fix(validator): tolerate taints, widen Trainer readiness timeout - #2445
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 12 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe Trainer controller readiness timeout increases from two to three minutes. Performance validators use a shared toleration operator key. Trainer and JobSet controller Deployments receive generated tolerations during manifest decoding. Existing tolerations remain unchanged. Lifecycle tests cover mutations, unrelated resources, isolation, and malformed or missing pod specifications. Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change only adds scheduling tolerance for the targeted controller Deployments and extends their readiness wait; no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
mikecook
changed the title
793f445 to
07dcd0f
Compare
njhensley
left a comment
njhensley
left a comment
There was a problem hiding this comment.
Multi-persona review — tolerate taints + widen Trainer readiness timeout
Method: 4 independent persona reviewers (Correctness · Domain/K8s-Architecture · Test-coverage · Operability/CI-DX), every finding then adversarially re-derived from the resolved code by a senior meta-reviewer. Anchored to head 793f445d.
Tier legend: 🔴 Blocker · 🟠 Major · 🟡 Minor · 🔵 Nitpick
Overall assessment
Small, well-scoped, and genuinely clean. It makes the self-install Trainer/JobSet controller-manager Deployments schedulable on all-tainted clusters (blanket {operator: Exists}) and gives the Trainer controller one extra minute to become ready. Nothing survived adversarial re-derivation at 🟠 or above. The two scariest-looking candidates both dissolved under the code:
- The shared-package-var aliasing of
controllerTolerateAllis real but inert — both apply pathsobj.DeepCopy()before Create/Update and nothing mutates tolerations in place. - The name-vs-label asymmetry (toleration matches by name, readiness lookup matches by label) is correct by construction:
applyControllerTolerationsmutates the pre-apply kustomize manifest whose name is guaranteed by the overlay, while the readiness lookup queries the live cluster where a Helm release could rename it.
Routing the toleration shape through component.TolerationsToPodSpec is good hygiene, and the test quality is high — assertions verify content (not just length) and correctly distinguish "field absent" from "field empty."
Recommendation: ✅ Approve with comments. One Minor worth a look; the rest are optional polish. Single most valuable follow-up: make the name-based match observable (warn-log or decode-time assertion) so a future upstream rename fails loudly rather than as a bare readiness timeout.
Confirmed non-issues (examined, refuted or by-design)
- Aliasing → no live bug — DeepCopy at both apply sites; nothing mutates tolerations in place.
- Name-vs-label asymmetry — correct by construction (pre-apply manifest vs. live cluster).
- 3m timeout — well within budget;
NCCLTrainJobTimeout=30mdominates, each wait has its ownctx.WithTimeout, andtimeouts_testbounds it to [1m, 5m]. - Blanket tolerate-all with no nodeSelector — acceptable by design; on an all-tainted cluster the controllers must land somewhere and a hard nodeSelector is infeasible.
- Fix only covers the self-install path, not recipe-declared Helm trainers — correct ownership separation; mutating a recipe-delivered Deployment would be the actual defect.
- Idempotency/re-run — guard reads the fresh render, not the live object; re-install re-stamps and
Updateconverges. - Docs — internal-only ephemeral fixture; no user-facing surface, no docs update required.
Summary
| 🔴 Blocker | 🟠 Major | 🟡 Minor | 🔵 Nitpick |
|---|---|---|---|
| 0 | 0 | 1 | 9 |
Reviewed with a multi-persona panel + adversarial meta-review. Inline comments follow.
| return nil | ||
| } | ||
| switch obj.GetName() { | ||
| case trainerControllerDeployment, jobSetControllerDeployment: |
There was a problem hiding this comment.
🟡 Minor — Name-based match silently no-ops if a future archive bump renames a controller
The two controller Deployments are matched by exact name in this switch. The Kubeflow Trainer archive is downloaded at runtime, so a future pinned-version bump (or the separately-managed JobSet overlay variant) that renames the emitted Deployment makes both cases miss — the function returns nil with no log or error, and that controller reverts to un-schedulable on all-tainted clusters. It resurfaces as a bare (now 3-minute) readiness timeout, precisely the failure the label-based readiness lookup was built to avoid.
Blast radius: Self-install path; the JobSet/Trainer controller silently loses its toleration on a future upstream rename and the benchmark fails at readiness wait rather than with a clear signal.
Fix: Emit a warn-log when a controller-shaped Deployment isn't matched, or add a decode-time assertion that both expected controller names appeared among the decoded objects.
| // 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 |
There was a problem hiding this comment.
🔵 Nitpick — 2m→3m rationale isn't captured in-code
The value changed 2m→3m but the doc comment still describes only "at least one ready replica after installation." The cert-controller cold-start / retry reasoning lives only in the PR body and is lost on squash-merge.
Fix: Fold a one-line why into the comment so the next engineer doesn't tune it back down and reintroduce the flake.
| versionV1alpha1 = "v1alpha1" | ||
| versionV1beta1 = "v1beta1" | ||
| keyName = "name" | ||
| keyOperator = "operator" |
There was a problem hiding this comment.
🔵 Nitpick — keyOperator const wasn't actually required by goconst
The PR justifies this const as needed "to satisfy golangci-lint's goconst." But .golangci.yaml sets goconst: {min-occurrences: 3, ignore-tests: true}, and pre-PR there were exactly 2 non-test occurrences of "operator" (the two source files this PR edits) — goconst was already passing.
Fix: None required — the extraction is a fine readability improvement; just noting the stated CI justification is inaccurate.
| // 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( |
There was a problem hiding this comment.
🔵 Nitpick — controllerTolerateAll is aliased by reference into both Deployments (latent)
controllerTolerateAll is a package-level []any holding one shared map[string]any, assigned by reference to podSpec["tolerations"] for both the Trainer and JobSet Deployments. No live bug today — both apply paths obj.DeepCopy() before Create/Update and nothing mutates the tolerations in place — but a future in-place edit of one controller's tolerations would silently corrupt the other and the process-wide global.
Fix: A per-call copy inside applyControllerTolerations is cheap hygiene, and a two-Deployment isolation test would pin it.
| 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 { |
There was a problem hiding this comment.
🔵 Nitpick — "already tolerated → skip" branch returns silently
The apply branch logs slog.Info("Applying blanket toleration…"), but this found && len(existing) > 0 { return nil } skip branch returns with no log. An operator debugging why a controller is still Pending on a tainted pool gets no signal that the blanket toleration was deliberately withheld because the manifest already had its own.
Fix: Add a slog.Debug("…already declares tolerations; leaving untouched", "name", obj.GetName()) before the early return.
| 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()) |
There was a problem hiding this comment.
🔵 Nitpick — Applied-toleration log omits namespace
This log is name-only; the surrounding Deployment-lifecycle logs in this file include "namespace". The overlay sets namespace, so it's available at decode time.
Fix: Add "namespace", obj.GetNamespace() for symmetry.
| // 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" { |
There was a problem hiding this comment.
🔵 Nitpick — Kind check matches string only, not apps group
obj.GroupVersionKind().Kind != "Deployment" gates on the Kind string alone; a Deployment kind in a non-apps group whose name collided with a controller name would be stamped. Harmless for the kustomize overlay this runs over (only the two standard apps/v1 Deployments), so purely defensive.
Fix: Optionally also assert obj.GroupVersionKind().Group == "apps".
| 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 { |
There was a problem hiding this comment.
🔵 Nitpick — present-but-empty tolerations boundary untested
The guard found && len(existing) > 0 means a Deployment shipping tolerations: [] falls through and gets stamped (the desired behavior). The test covers only nil and non-empty.
Fix: Add a deploymentFixture(trainerControllerDeployment, []any{}) case pinning this boundary against a future refactor that flips the guard to found alone.
| if obj.GroupVersionKind().Kind == "" { | ||
| continue | ||
| } | ||
| if tolErr := applyControllerTolerations(obj); tolErr != nil { |
There was a problem hiding this comment.
🔵 Nitpick — decodeTrainerObjects has no end-to-end test
applyControllerTolerations is unit-tested directly (both error paths included), but its only call site — decodeTrainerObjects — has no test. The wiring (toleration actually lands on returned objects, decode-error propagation, the Kind=="" skip ordering just above this call) is unverified.
Fix: Feed decodeTrainerObjects a small fixture with a controller Deployment + an unrelated object and assert the toleration lands only on the controller (and that a malformed resource yields a non-nil error).
| for _, t := range tolerations { | ||
| tolMap := map[string]any{ | ||
| "operator": string(t.Operator), | ||
| keyOperator: string(t.Operator), |
There was a problem hiding this comment.
🔵 Nitpick — keyOperator output key unpinned by tests
The tolerationsToUnstructured / applyNCCLWorkerScheduling tests assert key/value/effect but never read tol["operator"], so the const-keyed output key is compiler-checked but not test-pinned.
Fix: Add tol["operator"] to the existing key/value/effect assertions to harden the refactor.
…oyments
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 <micook@nvidia.com>
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 <micook@nvidia.com>
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 <micook@nvidia.com>
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 <micook@nvidia.com>
…ents 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 <micook@nvidia.com>
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 <micook@nvidia.com>
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 <micook@nvidia.com>
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 <micook@nvidia.com>
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 <micook@nvidia.com>
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 <micook@nvidia.com>
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 <micook@nvidia.com>
52e9167 to
4ae436a
Compare
njhensley
left a comment
njhensley
left a comment
There was a problem hiding this comment.
Re-review — tolerate taints + widen Trainer readiness timeout
Mode: re-review (delta). My prior review (1 🟡 + 9 🔵, "Approve with comments") stood against 07dcd0f2; 11 commits landed since, each a direct response to a prior finding. Re-anchored to head 4ae436aa.
Method: every prior finding re-verified against the resolved code, plus one adversarial pass for net-new defects. The crux net-new finding (isolation-test false-green) was proven empirically with a throwaway reproduction, not just argued.
Tier legend: 🔴 Blocker · 🟠 Major · 🟡 Minor · 🔵 Nitpick
Overall assessment
A model fix round. All 10 prior findings are resolved, and the two that mattered most — the name-based silent-no-op (🟡) and the shared-var aliasing (🔵) — got the treatment they warranted: a decode-time warn loop, and a real per-call clone. Tests pass, and the new warn loop does not fire spuriously: the kustomize overlay emits both controller Deployments in the one manifest set, so both names are always seen on a healthy run.
One net-new snag: the isolation test added to lock in the aliasing fix reads through unstructured.NestedSlice, which deep-copies — so its mutation never reaches the live object or the global. Reverting the clone leaves the test green (proven empirically): a guard that doesn't guard. The production clone is correct, so there's no live bug.
Approving with comments. Nothing at 🟠+. The one follow-up worth doing is tightening that isolation test (N1).
Prior-feedback status
| # | Prior finding | Disposition | Evidence |
|---|---|---|---|
| P1 🟡 | Name match silent no-op on rename | ✔️ Addressed | decode-time warn loop; both controllers in one manifest set → no spurious fire |
| P2 🔵 | 2m→3m rationale not in-code | ✔️ Addressed | expanded comment |
| P3 🔵 | keyOperator const not required by goconst | ⊘ No code change needed | finding said "none required"; const is fine. The PR body still asserts the goconst justification — inaccurate but harmless |
| P4 🔵 | controllerTolerateAll aliased | ✔️ Prod fixed / ◐ test | clone is correct; the isolation test pinning it is ineffective → N1 inline |
| P5 🔵 | already-tolerated skip silent | ✔️ Addressed | slog.Debug added |
| P6 🔵 | applied-toleration log omits namespace | ✔️ Addressed | namespace added |
| P7 🔵 | Kind check not apps-group gated | ✔️ Addressed | gvk.Group=="apps" + test |
| P8 🔵 | present-but-empty tolerations untested | ✔️ Addressed | boundary case |
| P9 🔵 | decodeTrainerObjects no e2e test | ✔️ Addressed | TestDecodeTrainerObjects(+_PropagatesTolerationError) |
| P10 🔵 | keyOperator output key untested | ✔️ Addressed | asserts operator=Equal |
Confirmed non-issues (net-new pass)
controllerTolerateAllpackage-var init —component.TolerationsToPodSpecis a pure in-memory transform;[{Operator:Exists}]→[{"operator":"Exists"}], non-empty. Safe at init.cloneControllerTolerateAllshallow map copy — sufficient; everyTolerationsToPodSpecvalue is a scalar (no nested maps/slices to alias).- Warn-loop spurious fire — refuted; both controller Deployments ship in the single kustomize manifest set.
- Table-test dispositions —
wantTolerations==nil⇒ field-absent vs.wantErrfor missing-pod-spec / malformed-tolerations are correctly distinguished; length checked before per-key compare.
Summary
| 🔴 Blocker | 🟠 Major | 🟡 Minor | 🔵 Nitpick |
|---|---|---|---|
| 0 | 0 | 1 | 2 |
All prior feedback addressed. Inline comments follow (N1–N3).
| // 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) { |
There was a problem hiding this comment.
🟡 Minor — Isolation test is a false green — NestedSlice deep-copies, so it never exercises aliasing
unstructured.NestedSlice returns runtime.DeepCopyJSONValue(val), so trainerTols[0] is a deep copy — mutating trainerTol["key"] never touches the live Deployment or the controllerTolerateAll global.
Blast radius: Reverting cloneControllerTolerateAll to the aliasing behavior leaves TestApplyControllerTolerations_Isolation passing (verified empirically), so it does not catch the regression it was added (prior P4) to pin. The production clone itself is correct — this is only the guard.
Fix: Read the live map directly (NestedFieldNoCopy, or index into obj.Object[...]) before mutating, or assert the two stamped toleration maps are distinct instances (mutate one via the live pod-spec map and confirm the other Deployment + the global are unchanged).
| }} | ||
| } | ||
|
|
||
| // TestApplyControllerTolerations covers both controller names, the two |
There was a problem hiding this comment.
🔵 Nitpick — Stranded doc comment: opens describing TestApplyControllerTolerations but precedes the _Isolation func
The comment block's first two sentences describe TestApplyControllerTolerations, but the block sits directly above func TestApplyControllerTolerations_Isolation (godoc attributes it there), while the table-driven TestApplyControllerTolerations at :139 has no doc of its own.
Fix: Move the first two sentences down to immediately above func TestApplyControllerTolerations.
| return nil, tolErr | ||
| } | ||
| if obj.GroupVersionKind().Kind == "Deployment" { | ||
| seenControllers[obj.GetName()] = true |
There was a problem hiding this comment.
🔵 Nitpick — seenControllers warn-loop key not apps-group scoped (inert asymmetry)
seenControllers[obj.GetName()] records any Kind=="Deployment" regardless of group, whereas applyControllerTolerations now also gates on gvk.Group=="apps". A non-apps Deployment-kind named like a controller would mark the name "seen" and suppress the warn while not receiving the toleration — the exact silent-miss the warn guards.
Blast radius: Unreachable in the fixed upstream kustomize set; purely defensive.
Fix: Optionally gate the seenControllers write on gvk.Group=="apps" too, to match the apply gate.
2 participants
Summary
Make the Kubeflow Trainer/JobSet controller-manager Deployments schedulable on all-tainted node pools, and widen the Trainer controller readiness timeout to accommodate its cert-controller sidecar's normal cold-start retry latency.
Motivation / Context
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
installTrainertimes out waiting for a Deployment that can never become Ready.Separately, on cold start the 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
resourceVersionconflict that the sidecar's own reconcile loop retries and self-heals from unassisted — but each retry adds latency, and on a slow cold start the cumulative delay could 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.Fixes: N/A
Related: N/A
Type of Change
Component(s) Affected
cmd/aicr,pkg/cli)cmd/aicrd,pkg/server)pkg/recipe)pkg/bundler,pkg/component/*)pkg/collector,pkg/snapshotter)pkg/validator)pkg/errors,pkg/k8s)docs/,examples/)Implementation Notes
applyControllerTolerationsstamps 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 runs 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. Built throughcomponent.TolerationsToPodSpec, the same converter used for Helm-values toleration overrides, so there's one canonical place that knows the toleration-to-map shape.Also extracted the repeated
"operator"toleration-key literal into akeyOperatorconst (used ininference_perf_constraint.goandnccl_all_reduce_bw_constraint.gotoo) to satisfy golangci-lint'sgoconstthreshold across the package — no behavior change there.TrainerControllerReadyTimeoutwidened from 2 to 3 minutes inpkg/defaults/timeouts.go— a single-constant change, independent of the toleration fix.Testing
make qualifypassed clean:make test-coverage: all packages pass with-race;validators/performanceat 62.0% (repo-wide 84.1%, threshold 80%)make lint(golangci-lint + yamllint): cleanmake tuning-check: cleanmake e2e(chainsaw,--no-cluster): 24/24 passed, 0 failed, 0 skippedmake scan(grype): no new vulnerabilities introduced by this changemake license-check: cleanmake api-diff: no incompatible SDK facade / transparent-alias changes since v0.20.0Added
trainer_lifecycle_test.gocoverage forapplyControllerTolerations(existing tolerations preserved, blanket toleration applied when absent, unrelated Deployments/Kinds untouched).Risk Assessment
Rollout notes: N/A — internal validator behavior only. No recipe/API/CLI surface change; the Trainer/JobSet controller Deployments now tolerate all taints and get one extra minute to become ready, nothing else changes.
Checklist
make testwith-race)make lint)git commit -S) — GPG signing info