fix(evidence): propagate CNCF section failures#1730
Conversation
|
Welcome to AICR, @hogeheer499-commits! Thanks for your first pull request. Before review, please ensure:
A maintainer will review this soon. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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)
📝 WalkthroughWalkthroughThe evidence script now parses authoritative PASS, SKIP, and FAIL results, removes stale artifacts before collection, and returns failure for failed evidence sections. Multiple prerequisite and connectivity paths now emit explicit SKIP or FAIL results. A new table-driven Go test exercises collector propagation across success, skip, failure, invalid-result, and unavailable-component scenarios. Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
yuanchen8911
left a comment
yuanchen8911
left a comment
There was a problem hiding this comment.
Issue 1 should be fixed. Items 2 and 3 are notes/follow-ups. Everything else reviewed (verdict regex, main() tally, sourcing guard, consumer propagation path, emit-claim/claim_shape_test.go) is clean.
yuanchen8911
left a comment
yuanchen8911
left a comment
There was a problem hiding this comment.
Requesting changes for the blocking Dynamo evidence failure propagation issue described in the review comments.
The robust-operator section recorded Result: FAIL when the Dynamo operator was present but no DynamoGraphDeployment existed, collapsing a failed API query and a successful zero-row query into the same 0. With section FAILs now propagating to a nonzero exit, a stock Dynamo inference bundle + --cncf-submission failed on the default path: the service-metrics section deploys and cleans up its own DGD before the operator section runs, so the operator section saw zero DGDs. Distinguish a failed DGD query (fail closed) from a successful query with zero rows (absent prerequisite -> SKIP), matching the SKIP treatment given to absent gateway/operator/autoscaler prerequisites. Add a subprocess test for the new branch and assert each section collector emits exactly one column-zero verdict. Fixes the cross-review finding on PR NVIDIA#1730. Signed-off-by: Yuan Chen <yuanchen97@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/evidence/cncf/result_propagation_test.go (1)
174-341: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing test for the DGD query-failure fail-closed branch.
The new
operator-dynamo-no-dgdcase (Line 230-235) covers the "successful query, zero rows → SKIP" branch, but there's no case exercising "query itself fails → FAIL (fail closed)" (collect-evidence.shLine 2490-2491) — the exact branch this PR's fix and the prior critical review comment center on. A future edit to that fail-closed check would go unnoticed by this suite.🧪 Suggested additional case
Harness (add alongside
operator-dynamo-no-dgd):+ operator-dynamo-query-failed) + kubectl() { + if [ "${1:-}" = "cluster-info" ]; then + return 0 + fi + case " $* " in + *"dynamo-platform-dynamo-operator-controller-manager"*) echo "dynamo-operator 1/1" ;; + *" dynamographdeployments "*) return 1 ;; + esac + return 0 + } + SECTION="operator" + ;;Test table:
{ name: "operator DGD query failure fails closed", mode: "operator-dynamo-query-failed", displayName: "Robust AI Operator", wantStatus: "FAIL", singleVerdictFile: "robust-operator.md", wantErr: true, },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/evidence/cncf/result_propagation_test.go` around lines 174 - 341, Add a table-driven case to TestEvidenceResultPropagatesThroughCollector for mode "operator-dynamo-query-failed", asserting Robust AI Operator produces FAIL, sets wantErr, and uses robust-operator.md as singleVerdictFile; ensure the harness triggers the DGD query-error path rather than the successful zero-row SKIP path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/evidence/cncf/result_propagation_test.go`:
- Around line 174-341: Add a table-driven case to
TestEvidenceResultPropagatesThroughCollector for mode
"operator-dynamo-query-failed", asserting Robust AI Operator produces FAIL, sets
wantErr, and uses robust-operator.md as singleVerdictFile; ensure the harness
triggers the DGD query-error path rather than the successful zero-row SKIP path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 423ae348-b308-4f90-9f7b-58a459d23ccd
📒 Files selected for processing (2)
pkg/evidence/cncf/result_propagation_test.gopkg/evidence/cncf/scripts/collect-evidence.sh
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/evidence/cncf/result_propagation_test.go`:
- Around line 251-258: Update the test cases for the operator DGD query failure
scenario in result propagation validation to assert that robust-operator.md
contains exactly the single marker **Result: FAIL**, rather than only checking
the marker count. Apply the same artifact-content assertion to the corresponding
case around the later referenced test block, while preserving the existing
nonzero-error expectation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 7841bea2-db16-4a4d-b51b-9f75aef6f1f3
📒 Files selected for processing (1)
pkg/evidence/cncf/result_propagation_test.go
yuanchen8911
left a comment
yuanchen8911
left a comment
There was a problem hiding this comment.
fixed the three issues
The --cncf-submission behavior change — each section emits exactly one PASS/SKIP/FAIL verdict, a FAIL now propagates to a non-zero exit, and absent optional prerequisites are SKIP — was user-visible but undocumented. Add a short note to docs/user/validation.md and docs/conformance/cncf/index.md. Addresses cross-review docs finding on PR NVIDIA#1730. Signed-off-by: Yuan Chen <yuanchen97@gmail.com>
Record section outcomes as PASS/SKIP/FAIL verdicts and propagate a genuine FAIL to a nonzero exit so the existing Go collector adds it to failed_sections. Each collector writes exactly one authoritative verdict; missing, unknown, conflicting, or malformed verdicts fail closed, and stale artifacts are removed before collection. Absent optional prerequisites emit an explicit SKIP: no inference gateway, no supported operator, an unsupported cluster-autoscaling provider, or a Dynamo operator present with no DynamoGraphDeployment (an absent inference workload is a missing prerequisite, not a failure). A failed DGD query stays FAIL (fail closed); present-but-unhealthy workloads remain failures. Cover the result path with a subprocess test that runs the shipped script without a cluster, including the SKIP branches, the failed-query branch, and a single-verdict-per-section assertion. Document the verdict/exit contract in docs/user/validation.md and docs/conformance/cncf/index.md. Fixes: NVIDIA#1692 Signed-off-by: JS van Dijk <267467744+hogeheer499-commits@users.noreply.github.com>
37c6a6e to
25bb627
Compare
Recipe evidence checkNo leaf overlays affected by this PR. This gate is warning-only and never blocks merge. |
3 participants
Summary
Propagate failed CNCF evidence sections to the existing Go collector instead of returning success with failed submission artifacts. Add explicit skip evidence for absent optional prerequisites and fail closed on missing or malformed verdicts.
Motivation / Context
The evidence script recorded section outcomes only in Markdown, so a genuine
Result: FAILnever reachedfailed_sectionsor the CLI exit status. Optional components also need a distinct skip path so stock clusters do not fail simply because an inference gateway, supported operator, or cloud autoscaler is absent.Fixes: #1692
Related: #1694
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/)pkg/evidence/cncf)Implementation Notes
Each section must now write exactly one authoritative
PASS,SKIP, orFAILverdict. Stale artifacts are removed before collection, and missing, unknown, conflicting, or malformed verdicts fail closed. Gateway, operator, and unsupported cloud-autoscaler prerequisites emit explicit skip artifacts; present but unhealthy workloads remain failures.The parser remains compatible with Bash 3.2. A failed section is retained in the summary and makes the script return nonzero, which lets the existing Go collector add it to
failed_sectionswithout changing the Go interface.Testing
make test: passed with the race detector; repository coverage 77.9% (75% required).golangci-lint2.12.2: zero issues.main.make qualifywas not repeated in the pinned Go container because it did not include the repository's external tool suite. The affected final test, lint, syntax, and diff gates above were rerun.Risk Assessment
Rollout notes: No migration is required. Automation that previously accepted failed evidence will now receive the intended nonzero result; absent optional prerequisites remain successful skips. One behavior change to note: Prometheus port-forward failure in the Dynamo, NIM, and trainer service-metrics paths now records Result: FAIL (previously an uncounted WARNING), so a cluster where Prometheus is unreachable will surface a section failure — confirm submission clusters have a reachable Prometheus. A Dynamo operator present with no DynamoGraphDeployment is treated as SKIP (absent prerequisite), not FAIL.
Checklist
make testwith-race)make lint)git commit -S) — GPG signing info