fix(validators): pair the TCPXO sidecar with the installer AICR ships - #2379
fix(validators): pair the TCPXO sidecar with the installer AICR ships#2379yuanchen8911 wants to merge 3 commits into
Conversation
The performance validator's GKE H100 TrainingRuntime pinned tcpgpudmarxd-dev v1.0.20, which Google pairs with plugin installer v1.0.14. AICR's recipe deploys installer v1.0.15, whose partner is v1.0.21. That runtime is rendered and run on a live cluster during the performance phase, so the benchmark executed against a pair Google documents as unsupported. Renovate excludes **/testdata/** so the pin could never be updated automatically, and ignoreDeps listed only the installer half of the pair, leaving the daemon free to move on its own. Add the daemon so the coupling is symmetric. Also move the demo workload from the v1.0.14/v1.0.20 pair to v1.0.15/v1.0.21 so it matches the installer the recipe deploys, and update the two doc notes that described it as pinned to the earlier pair. Signed-off-by: Yuan Chen <yuanchen97@gmail.com>
yuanchen8911
added
theme/validation
|
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 (2)
Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review. 📝 WalkthroughWalkthroughThe PR updates TCPXO daemon images to Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change aligns the TCPXO runtime and demo images with the installer’s supported version pair and updates related guidance; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation The PR restores the v1.0.15/v1.0.21 pair, updates the validator fixture and documentation, and adds the daemon to Renovate ignoreDeps. However, issue Full details: Out of Scope Changes checkExplanation All changes support the linked issue: they update TCPXO versions, Renovate configuration, documentation, the demo workload, and the performance validator fixture. No unrelated code changes are present. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-fix-tcpxo-sidecar-version-pair.docs.buildwithfern.com/aicr |
The manifest header told the reader to update only the plugin tag when their cluster runs a different installer version. Following it leaves both daemon references untouched, recreating the mismatched pair this change exists to fix. State the coupling, name the pinned pair, and say to update both images, so the manifest carries the same guidance the integrator doc already gives. Also correct the lookup: the installer image lives in initContainers, not containers, so the previous jsonpath returned empty. Signed-off-by: Yuan Chen <yuanchen97@gmail.com>
njhensley
left a comment
njhensley
left a comment
There was a problem hiding this comment.
✅ Approve — TCPXO sidecar/installer pairing
Method: independent multi-persona review (correctness, operability / CI-DX, docs) + domain verification against Google's gpudirect-tcpxo release table + an adversarial senior meta-reviewer that re-derived each finding from the resolved code and ran the affected tests.
Legend: 🔴 Blocker · 🟠 Major · 🟡 Minor · 🔵 Nitpick
This is a tight, correct, low-risk fix. Independently confirmed:
- Pairing is correct. Google's release table pairs installer v1.0.15 ↔ daemon v1.0.21 and v1.0.14 ↔ v1.0.20 — exactly this PR's mapping. The recipe ships installer
v1.0.15digest-pinned (nccl-tcpxo-installer.yaml:101), so the benchmark previously ran an unsupported pair. runtime.yamlcorrectly bumps only the daemon — the plugin is installed cluster-side by the recipe DaemonSet, not carried in the fixture. It's both template and golden innccl_test.go;go test ./validators/performance/...passes with no golden-output regression.- The jsonpath fix is a real bug fix. The installer lives under
initContainers(containersholds onlypause), so the oldcontainers[?(@.name=="nccl-tcpxo-installer")]lookup returned empty; the newinitContainers[...]path resolves and matches the integrator doc. - Completeness: repo-wide grep shows zero stale
v1.0.14/v1.0.20refs remaining; the BOM is untouched (daemon isn't in a Helm chart, so nomake bom-docsneeded).
No blocker / major / minor findings. Three optional 🔵 nitpicks are inline — none blocking.
Tally: 🔴 0 | 🟠 0 | 🟡 0 | 🔵 3 — Recommendation: Approve
| // the same stack version skew from the opposite direction. | ||
| ignoreDeps: [ | ||
| "602401143452.dkr.ecr.us-west-2.amazonaws.com/eks/aws-efa-k8s-device-plugin", | ||
| "us-docker.pkg.dev/gce-ai-infra/gpudirect-tcpxo/nccl-plugin-gpudirecttcpx-dev", |
There was a problem hiding this comment.
🔵 Nitpick — ignoreDeps daemon entry is pre-emptive, not an active drift guard today
Worth keeping, but the comment (:366-370) and the PR body frame this entry as symmetric with the installer entry, and today it isn't in effect. Renovate never extracts tcpgpudmarxd-dev: the image appears only in demos/** (not covered by the kubernetes manager's managerFilePatterns, which is narrowed to ^recipes/components/[^/]+/manifests/[^/]+\.ya?ml$) and in validators/**/testdata/** (excluded by ignorePaths). The sibling nccl-plugin-gpudirecttcpx-dev entry IS load-bearing because that image lives in the scanned recipe manifest nccl-tcpxo-installer.yaml:101.
Blast radius: None functionally — the entry is harmless and correctly becomes load-bearing if the daemon ever lands in a scanned manifest. This is a wording accuracy note, not a defect.
Fix: Consider softening the comment, e.g. "pre-emptive symmetry — the daemon isn't in a Renovate-scanned path today, but this keeps the pair coupled if it ever lands in recipes/components/*/manifests/."
There was a problem hiding this comment.
You're right, and my reasoning was wrong. I had assumed demos/ was scannable because it isn't in ignorePaths — but managerFilePatterns is the actual gate, and the kubernetes manager is narrowed to ^recipes/components/[^/]+/manifests/. The daemon appears only in demos/ and validators/**/testdata/, so Renovate never extracts it.
Fixed in 9035485. The comment now says the entry is pre-emptive rather than an active guard, explains why (manager scope, not ignorePaths), and states what it does buy: a future widening of those patterns can't silently let the daemon move alone. PR body corrected to match — it made the same overstatement.
| # Then update the nccl-plugin-gpudirecttcpx-dev tag below to match. | ||
| # -o jsonpath='{.spec.template.spec.initContainers[?(@.name=="nccl-tcpxo-installer")].image}' | ||
| # then update BOTH images below to that installer's pair, per Google's | ||
| # release table: https://github.com/GoogleCloudPlatform/container-engine-accelerators/blob/master/gpudirect-tcpxo/README.md |
There was a problem hiding this comment.
🔵 Nitpick — Wording drift for the same URL
This header calls the linked Google page "release table" while docs/integrator/gke-tcpxo-networking.md:203 calls the same URL "Google's release notes." Both are defensible; harmless terminology drift for the same target.
Fix: Unify the phrasing if you touch the file.
There was a problem hiding this comment.
Unified in 9035485 — the demo header now says "release notes", matching gke-tcpxo-networking.md's wording for the same URL.
| # plugin version. Check with: | ||
| # Google ships the plugin installer and the tcpxo-daemon as a coupled release | ||
| # pair; running a mismatched pair is unsupported. This manifest is pinned to | ||
| # v1.0.15 (nccl-plugin-gpudirecttcpx-dev) with v1.0.21 (tcpgpudmarxd-dev), |
There was a problem hiding this comment.
🔵 Nitpick — Version pair now also stated in prose
Adds the v1.0.15 / v1.0.21 pair in prose alongside the two image lines, growing the hand-maintained-literal surface. It also helps the reader by naming the pinned pair where they'll edit it, and the single-source-of-truth follow-up is already tracked in #2378.
Fix: No action needed here; noting for completeness.
There was a problem hiding this comment.
Agreed on both halves. It does grow the hand-maintained surface, and that's the tradeoff — but naming the pair where the reader is about to edit it is the point of the header, since the whole failure mode is updating one image and not the other. #2378 tracks collapsing these literals to a single source of truth; its drift-surface inventory counts every site, this prose included.
Review correction: the entry is not an active drift guard today. The kubernetes manager is narrowed to recipes/components/*/manifests, and tcpgpudmarxd-dev appears only in demos/ and validators/**/testdata/, so Renovate never extracts it. The previous comment claimed symmetry with the installer entry, which overstates what the config does now. Keep the entry -- it prevents a future widening of those patterns from letting the daemon move alone -- but say what it actually is. Also unify the wording for the same Google URL: the demo header called it a 'release table' while the integrator doc calls it 'release notes'. Signed-off-by: Yuan Chen <yuanchen97@gmail.com>
njhensley
left a comment
njhensley
left a comment
There was a problem hiding this comment.
Re-review — Approve ✅
All three nitpicks from my prior review are resolved (2 addressed, 1 reasonably kept by design and tracked in #2378), and the fix commit 90354855 introduces no new issues.
I independently re-derived every load-bearing claim:
- Version pairing — Google's published README table confirms installer
v1.0.15 ↔ daemon v1.0.21andv1.0.14 ↔ v1.0.20, exactly as this PR states. runtime.yamlcarries only the daemon sidecar (v1.0.21), correctly paired with the recipe's on-cluster installerv1.0.15— no stray plugin left atv1.0.14.- jsonpath fix — the installer image lives under
initContainers:(nccl-tcpxo-installer.yaml:71,100), so the oldcontainers[...]lookup returned empty; the corrected path is right. - renovate comment — verified the
kubernetesmanager is narrowed torecipes/components/*/manifests(L131-132) and**/testdata/**is inignorePaths(L392), so "pre-emptive rather than an active guard today" is accurate.
Clean, correct, well-documented fix. 🔴 0 · 🟠 0 · 🟡 0 · 🔵 0 (new).
2 participants
Summary
Pairs the TCPXO
tcpgpudmarxd-devsidecar with the plugin installer AICRactually deploys: the performance validator's GKE H100 runtime moves from
v1.0.20tov1.0.21, and the demo workload moves from thev1.0.14/v1.0.20pair to
v1.0.15/v1.0.21.Motivation / Context
Google ships the TCPXO plugin installer and the workload-side daemon as a
coupled release pair and states that a mismatched pair is unsupported. Their
release table pairs
v1.0.15withv1.0.21, andv1.0.14withv1.0.20.AICR's recipe deploys installer
v1.0.15(
recipes/components/gke-nccl-tcpxo/manifests/nccl-tcpxo-installer.yaml), butvalidators/performance/testdata/h100/gke/runtime.yamlpinned the daemon atv1.0.20. That file is not an inert fixture —nccl_benchmark_runtime.gorenders it and runs it on a live cluster during the performance phase, so the
NCCL benchmark executed against an unsupported pair.
Two things let it drift unnoticed:
.github/renovate.json5excludes**/testdata/**, so the pin could never beupdated automatically.
ignoreDepslisted onlynccl-plugin-gpudirecttcpx-dev. The daemon half wasabsent, so the installer was frozen while its partner was free to move — the
same stack version skew the surrounding comment warns about, from the
opposite direction.
Fixes: #2378
Related: #2290, #2377
Type of Change
Component(s) Affected
pkg/validator)docs/,examples/).github/renovate.json5,demos/Implementation Notes
gke-nccl-test-tcpxo.yamlcarried aself-consistent
v1.0.14/v1.0.20pair, documented as deliberate in twoplaces. It is moved to
v1.0.15/v1.0.21so the demo matches what the recipedeploys rather than requiring the reader to reconcile two pairs; the two doc
notes are updated to match, keeping the "update both images to your cluster's
pair" guidance.
page as "release notes" to match the integrator doc's wording for the same
URL. It previously told the
reader to update only the plugin tag, so following it on a cluster running a
different installer would have left the daemon pinned and recreated the exact
mismatch this PR fixes. The header also looked the installer up under
containers, but the image lives ininitContainers(
nccl-tcpxo-installer.yaml:71,100-101), so the old jsonpath returned empty;it now matches the working lookup in the integrator doc.
that manifest with a digest. The daemon images are not: the GAR repository is
auth-gated (anonymous manifest reads return 401), so a digest cannot be
resolved here. Tags match the existing convention for this image.
ignoreDepsentry is pre-emptive, not an active guard. Reviewestablished that Renovate does not extract
tcpgpudmarxd-devtoday: thekubernetesmanager is narrowed torecipes/components/*/manifests, and thedaemon appears only in
demos/andvalidators/**/testdata/. The entry iskept so a future widening of those patterns cannot silently let the daemon
move alone, and the comment now says exactly that. It does not create a
single source of truth for the pair — the versions remain hand-maintained
literals across several files, which gke-nccl-tcpxo: installer/sidecar version pair has drifted and is not enforced #2378 tracks as the remaining
structural work.
v1.0.17/v1.0.23requires GPU driver 595.71.05, GKE 1.33.5-gke.1125000+, and CUDA13.2 — prerequisites AICR does not meet today, and which interact with the
GKE driver-installer rework in feat(recipes)!: GKE bundle-installer replaces driver-installer #2360. This PR restores a supported pair at the
version already shipped.
Testing
make qualifywas not run in full: the change touches no Go source (0.gofiles in the diff — YAML, Markdown, and JSON5 only), so the Go test, coverage,
api-diff, and scan lanes cannot regress from it. The scoped checks above cover
every file kind in the diff.
lint-goadditionally fails on this machine for apre-existing toolchain reason unrelated to this change (golangci-lint built
against go1.26, repo targets go1.27).
The rendered-runtime path is exercised by
validators/performanceunit tests,which pass. End-to-end confirmation that the
v1.0.21daemon runs against thev1.0.15installer requires a GKEa3-megagpu-8gcluster and has not been run;the pairing is taken from Google's published release table.
Risk Assessment
Rollout notes: No API, recipe, or bundle surface changes. The runtime
fixture affects the performance validation phase on GKE H100 only, moving it
from an unsupported image pair to a supported one. Users who copied the demo
manifest are unaffected until they re-copy it.
Checklist
make testwith-race) — scoped to the affected package; see Testingmake lint) —lint-yamlandlint-renovate; see Testing for thelint-gonotegit commit -S)