fix(evidence): clarify operator SKIP, pin Trainer namespace split - #2269
yuanchen8911 merged 2 commits into
Conversation
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 (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review. 📝 WalkthroughWalkthroughThe change documents recipe-scoped operator detection for CNCF evidence collection. It clarifies that temporary Kubeflow Trainer installations in Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to This localized change clarifies operator evidence wording and documents an existing namespace boundary without changing detection behavior; no actionable merge-blocking risk remains after normal checks and review. Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Recipe evidence checkNo leaf overlays affected by this PR. This gate is warning-only and never blocks merge. |
njhensley
left a comment
njhensley
left a comment
There was a problem hiding this comment.
Review — multi-persona + adversarial meta-review
Method: 3 independent persona reviewers (Domain/evidence-integrity, Correctness, Docs) fanned out in parallel, then every claim re-derived from the resolved code in an adversarial meta-review pass. Pinned to head 338d5085.
Legend: 🔴 Blocker · 🟠 Major · 🟡 Minor · 🔵 Nitpick
Overall: Approve ✅
Comment-only change plus one reworded SKIP string — no logic, no exported surface, no registry/chart/values change. I verified every factual claim in the new comments against the code and all three personas converged: the change is accurate, correctly scoped, and a genuine net-positive. It tightens the accuracy of a signed conformance artifact and fails in the safe direction.
The deepest claim holds: if trainerNamespace were shared with the recipe's kubeflow, the conflict guard at trainer_lifecycle.go:458 (live.Namespace != trainerNamespace) goes false for an incomplete install, which falls through to applyTrainerResources → updateExistingTrainerResource, overwriting the recipe's Helm-managed Deployment/Service/ConfigMap without adding them to the rollback set. The comment is correctly scoped to those non-admission kinds — not overstated.
Confirmed non-issues (examined, cleared)
- SKIP namespace enumeration (
dynamo-system,nvidia-nim,kubeflow) is complete and matches each probe target + registrydefaultNamespace. - Shell safety: all backticks are
\``-escaped in the double-quotedecho` — no command substitution or word-splitting. - No Go test and no committed doc asserts the old SKIP sentence, so the rewrite regresses nothing.
- Committed
docs/conformance/evidence deliberately untouched is correct — those are real PASS records (e.g.trainer-eks-gb200shows a legitimately recipe-deployed Trainer inkubeflow); nothing contradicts the new wording. - "Signed conformance evidence" framing is accurate —
pkg/evidence/attestationattests the evidence tree. - Cross-references (
ensureTrainerInstalled,applyTrainerResources,updateExistingTrainerResource,registry.yaml,#2223) all resolve; the.shand.gocomments are consistent. Comments cite paths/function names (not line numbers) so they won't rot. - Fully implements issue #2223's requested scope.
Summary
| 🔴 Blocker | 🟠 Major | 🟡 Minor | 🔵 Nitpick |
|---|---|---|---|
| 0 | 0 | 0 | 1 |
The one nitpick below is cosmetic and optional.
| write_section_header "Robust AI Operator" | ||
| echo "**Result: SKIP (prerequisite absent)** — no supported Dynamo, NIM, or Kubeflow Trainer operator is installed." >> "${EVIDENCE_FILE}" | ||
| log_info "Robust operator evidence collection skipped — no supported operator found." | ||
| echo "**Result: SKIP (prerequisite absent)** — the recipe/bundle deployed no supported AI operator: no Dynamo operator in \`dynamo-system\`, no NIM operator in \`nvidia-nim\`, and no Kubeflow Trainer in \`kubeflow\`. A Kubeflow Trainer that the performance validator self-installed into \`kubeflow-system\` for the NCCL benchmark is deliberately not counted here — it is torn down at the end of the run and is not part of the deployed bundle." >> "${EVIDENCE_FILE}" |
There was a problem hiding this comment.
🔵 Nitpick — SKIP message is one long ~90-word line
The emitted Result: SKIP string packs the full rationale (three probed namespaces + the kubeflow-system self-install carve-out) into a single ~90-word paragraph in robust-operator.md. Renders fine as Markdown and an auditor benefits from the inline "why", so this is arguably intentional.
Blast radius: Cosmetic only.
Fix: Optional: split the "deliberately not counted…" clause into a second sentence. No change required.
There was a problem hiding this comment.
Taken in 0e59cdc. The carve-out is now emitted as its own paragraph rather than trailing the verdict on one line, so the SKIP result reads on its own and the reason a live kubeflow-system Trainer is not counted sits beside it instead of at the end of a ~90-word block.
Wording is unchanged apart from one em-dash becoming a colon, since the second sentence now stands alone.
You were right that it was intentional — the inline rationale is the point, an auditor seeing a running Trainer needs it. It just should not have been one paragraph. Re-requesting since the approval was dismissed by the push.
njhensley
left a comment
njhensley
left a comment
There was a problem hiding this comment.
Re-review of `0e59cdce` — delta approval.
My prior 🔵 nitpick (SKIP verdict + carve-out packed on one ~90-word line) is ✔️ addressed: 0e59cdce splits it into two echos so the verdict and the kubeflow-system self-install carve-out render as separate Markdown paragraphs in robust-operator.md. Verified on the resolved code — renders as two paragraphs, backticks stay escaped inside the double-quoted echo, nothing emitted after before the function returns.
No new findings; the delta is a 3-line cosmetic split — no logic, no exported surface, no change to signed-evidence semantics. LGTM. Approve.
Section 6 emitted "no supported Dynamo, NIM, or Kubeflow Trainer operator is installed" whenever the recipe deployed none of them. On a cluster where the performance validator's temporary Kubeflow Trainer self-install was live in kubeflow-system, that reads as a false negative: a Trainer was plainly running and healthy at collection time. The probe itself is correct. Section 6 attests what the recipe/bundle deployed, and the validator's self-install is scaffolding for the NCCL benchmark that is torn down at the end of the run, so it must not be collected. Only the wording was wrong. State which namespace each operator was looked for in, and say outright that a self-installed Trainer in kubeflow-system is deliberately not counted. Record why the two namespaces differ on both sides, since the divergence looks like drift and reads as the obvious thing to tidy away. Aligning them would break the collector (a temporary install would be collected as recipe-deployed, so signed evidence would claim an operator the bundle never shipped) and disarm the conflict guard in ensureTrainerInstalled, which compares against trainerNamespace to refuse installing over a Trainer this validator does not own; with one namespace that comparison is always false and applyTrainerResources overwrites the recipe's Helm-managed objects in place, with nothing restoring them afterwards. Refs NVIDIA#2223 Signed-off-by: Yuan Chen <yuanchen97@gmail.com>
The SKIP line emitted the verdict and the self-install carve-out as a single Markdown line, so robust-operator.md rendered them as one ~90-word paragraph. Emit the carve-out as a second paragraph, so the verdict reads on its own and the reason a live kubeflow-system Trainer is not counted sits beside it rather than buried at the end of a block. Wording is unchanged apart from an em-dash to a colon, avoiding two dashes in what is now a short standalone sentence. Refs NVIDIA#2223 Signed-off-by: Yuan Chen <yuanchen97@gmail.com>
0e59cdc to
6cd76f8
Compare
|
@njhensley rebased onto The repo requires branches to be up to date, so this was needed to merge, and it dismissed your approval as a side effect. Nothing to re-read — would you mind a quick re-approve once CI settles? |
2 participants
Summary
Corrects the Section 6 SKIP wording in the CNCF evidence collector so it says which namespaces were probed and why a validator self-installed Kubeflow Trainer is not counted, and records on both sides why the recipe's
kubeflownamespace and the performance validator'skubeflow-systemnamespace must stay different.Motivation / Context
Section 6 (Robust AI Operator) emitted:
On a cluster where the performance validator's temporary Trainer self-install was live in
kubeflow-system, that reads as a false negative — a Trainer was plainly running and healthy at collection time.The probe is correct; only the wording was wrong. Section 6 attests what the recipe/bundle deployed. The validator's self-install is scaffolding for the NCCL benchmark and is torn down at the end of the run, so counting it would make signed conformance evidence claim an operator the bundle never shipped. A SKIP in that situation is the right answer, stated badly.
Fixes: #2223
Related: #2133, #2222
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) —validators/performance(comment only)pkg/errors,pkg/k8s)docs/,examples/)pkg/evidence/cncf(CNCF evidence collector)Implementation Notes
What changed. The emitted SKIP line now names the namespace each operator was looked for in (
dynamo-system,nvidia-nim,kubeflow) and states that a Trainer self-installed intokubeflow-systemby the performance validator is deliberately not counted. The detection block andtrainerNamespaceboth carry a comment explaining the split. No detection logic changed.Why not align the two namespaces. The issue as originally filed also considered moving the validator's self-install onto the chart's
kubeflownamespace for consistency. That divergence turns out to be load-bearing, and collapsing it breaks two things:ensureTrainerInstalled(trainer_lifecycle.go:443) compares the discovered installation's namespace againsttrainerNamespaceto refuse installing over a Trainer it does not own. With one namespace that comparison is always false, so an incomplete recipe install would fall through toapplyTrainerResources, whoseIsAlreadyExistsbranch callsupdateExistingTrainerResourceand overwrites the recipe's Helm-managed Deployment/Service/ConfigMap in place with the vendored upstream kustomize spec. Those objects are not added to the rollback set, so nothing restores them after cleanup.Documenting the invariant on both sides is what stops it being tidied away later.
Testing
Scoped checks rather than a full green
make qualifylocally, because the diff is comments plus one emitted message string — no logic, no exported surface, no registry/chart/values change:shellcheckreports two warnings, both pre-existing and untouched by this diff:SC2034at line 37 (DEPLOY_TIMEOUTunused) andSC2069at line 2341 (redirection order). No new findings.On the local
make qualifyrun. It aborted atlint-goreporting 87 issues, none of them in this branch — every path resolved into unrelated sibling worktrees on the same machine. Re-running the identical full-module lint with an isolatedGOCACHEandGOLANGCI_LINT_CACHEreturns0 issues, confirming those diagnostics were replayed from a shared build cache rather than produced by this code. The linter is v2.12.2, matching the.settings.yamlpin. Because the abort happened at the lint stage, the later local stages (tuning-check, e2e, scan, license-check, api-diff) did not run — CI covers them, and the full Merge Gate is green.CI on
338d5085f: 29 checks pass, 0 fail. That includestests / Lint,tests / Test,tests / E2E,tests / CLI E2E,tests / Security Scan,analyze,grype,malware-scan, and the three GPU lanes (H100 training, H100 inference, L40G smoke). An earlier attempt oftests / Lintwas cancelled after hanging ~9 minutes on an Ubuntu apt mirror before golangci-lint started; it passed on re-run.Coverage: no logic changed, and
validators/is excluded from the coverage floor per.settings.yaml.Risk Assessment
Rollout notes: No behavior change in operator detection, so previously-collected evidence stays valid. Only the SKIP sentence differs in newly collected artifacts. Committed evidence under
docs/conformance/is a historical record of real runs and is deliberately left untouched.Checklist
make testwith-race)make lint)git commit -S)