Repository navigation
fix(validator): fail fast on eval errors; nil-safe health checks - #1261
yuanchen8911 merged 1 commit into
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR updates many Kyverno/Chainsaw health-check YAMLs to treat missing Pod status arrays as empty lists before JMESPath waiting-reason filters, changes the dynamo-platform health-check to select the operator by label, and refactors the in-process Chainsaw validator to classify assertion/evaluation failures as ErrCodeInvalidRequest and fast-fail retry loops. A regression test verifies fast-fail behavior. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
mchmarny
left a comment
mchmarny
left a comment
There was a problem hiding this comment.
Three clean, targeted fixes for the deployment-phase hang regression. The error-code reclassification (ErrCodeInternal → ErrCodeInvalidRequest for checkErr) is semantically correct — eval errors are a malformed-expression problem, not an internal one — and the isTerminalAssertErr placement in both retry loops is right (after nil check, before sleep). The || \[]`` JMESPath coalescing is applied consistently across all 22 files. The dynamo label-match is a solid fix that generalizes correctly to any release-name prefix.
One medium concern: the fast-fail path that prevents the 8-minute hang has no unit test. If someone later changes the error code on checkErr, the hang silently returns. Suggest adding a short retry-loop test before this merges. One nit on comment length. CI fully green including on-cluster Tier 1.
9b512cf to
6461d05
Compare
6461d05 to
45c5c1a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@validators/chainsaw/inprocess_test.go`:
- Around line 300-360: The test
TestRunChainsawTestInProcess_TerminalEvalErrorFailsFast only exercises the
named-resource branch via f.addGet; add a second subcase that omits
metadata.name in the YAML produced by makeYAML and instead calls f.addList(...)
with the same healthy resource set so the list-based code path is exercised;
keep the same throwExpr, runChainsawTestInProcess invocation, and the same
assertions about r.Error and elapsed so the list-based (nil-status/List)
terminal-eval behavior is covered alongside the existing named-resource case.
🪄 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: dec03ec3-0707-4379-bac0-dec9a243e8ab
📒 Files selected for processing (24)
recipes/checks/agentgateway/health-check.yamlrecipes/checks/aws-ebs-csi-driver/health-check.yamlrecipes/checks/aws-efa/health-check.yamlrecipes/checks/cert-manager/health-check.yamlrecipes/checks/dynamo-platform/health-check.yamlrecipes/checks/gatekeeper/health-check.yamlrecipes/checks/gpu-operator/health-check.yamlrecipes/checks/grove/health-check.yamlrecipes/checks/k8s-ephemeral-storage-metrics/health-check.yamlrecipes/checks/k8s-nim-operator/health-check.yamlrecipes/checks/kai-scheduler/health-check.yamlrecipes/checks/kube-prometheus-stack/health-check.yamlrecipes/checks/kubeflow-trainer/health-check.yamlrecipes/checks/kueue/health-check.yamlrecipes/checks/network-operator/health-check.yamlrecipes/checks/nfd/health-check.yamlrecipes/checks/nodewright-operator/health-check.yamlrecipes/checks/nvidia-dra-driver-gpu/health-check.yamlrecipes/checks/nvsentinel/health-check.yamlrecipes/checks/prometheus-adapter/health-check.yamlrecipes/checks/slinky-slurm-operator/health-check.yamlrecipes/checks/slinky-slurm/health-check.yamlvalidators/chainsaw/inprocess.govalidators/chainsaw/inprocess_test.go
45c5c1a to
bd466de
Compare
bd466de to
090c858
Compare
Deployment-phase health-check validation regressed in the recent in-process executor + deepened-check stack (NVIDIA#1235/NVIDIA#1245/NVIDIA#1252): 1. inprocess.go: runAssertWithRetry/runErrorWithRetry retried ANY non-nil error, so a permanent JMESPath evaluation error was retried for the entire assert window (6m) instead of failing fast like the old chainsaw binary did. Classify assertion-engine errors as ErrCodeInvalidRequest (terminal) and short-circuit the retry loops on them. Adds a regression test asserting terminal eval errors fail fast (< AssertRetryInterval) rather than retrying to deadline. 2. recipes/checks/*: the (init)containerStatuses[?...] | length(@) projection throws 'invalid type for: <nil>' on any pod without (init) containers (the common case), feeding defect #1. Coalesce to an empty array across all 22 affected component health checks. 3. recipes/checks/dynamo-platform: validate-deployment-exists asserted a Deployment named 'dynamo-operator', but the chart prefixes it with the release name (dynamo-platform-dynamo-operator-controller-manager), so the assert never matched and retried to the deadline. Match by the stable app.kubernetes.io/name=dynamo-operator label instead. Verified on an EKS H100 cluster: deployment phase goes from an 8m timeout (status=other) to PASSED in ~21s; full deployment 4/4 and conformance 11/11 green.
c76ef97 to
598e58e
Compare
Summary
Three regression fixes for deployment-phase health-check validation, introduced by the recent in-process executor + deepened-check stack (#1235/#1245/#1252):
inprocess.go)Motivation / Context
On an EKS H100 cluster,
aicr validate --phase deploymenthung ~8 min and returnedstatus=other("pod not found for Job") instead of real results. Root cause is three interacting issues:runAssertWithRetry/runErrorWithRetryretry on any non-nil error, so a permanent JMESPath evaluation error is retried for the full 6m assert window. The old chainsaw binary treated eval errors as terminal and failed fast.(init)containerStatuses[?...] | length(@)throwsinvalid type for: <nil>on any pod without (init) containers (the common case), which feeds defect fix: remove namespaceOverride from nvidia-dra-driver-gpu values #1.validate-deployment-existsasserts a Deployment nameddynamo-operator, but the chart prefixes it with the release name (dynamo-platform-dynamo-operator-controller-manager), so the assert never matched and retried to the deadline — a false negative against a healthy component.(Defects #1/#2 independently confirmed by Nathan.)
Related: #1235, #1245, #1252
Type of Change
Component(s) Affected
pkg/recipe) —recipes/checks/*pkg/validator) —validators/chainsaw/inprocess.goImplementation Notes
ErrCodeInvalidRequest(terminal) and short-circuit both retry loops via a newisTerminalAssertErrhelper, so a non-evaluable expression fails fast instead of burning the assert deadline. Transient failures (mismatch, NotFound) still retry. Adds a regression test asserting a terminal eval error returns in< AssertRetryIntervalrather than retrying to the deadline.(initContainerStatuses ||+ "[]" +)[?...]— across all 22 affected component health checks (bothinitContainerStatusesandcontainerStatuses).app.kubernetes.io/name=dynamo-operatorlabel (List-and-match) instead of a release-name-dependent hardcoded name.Scope note (
conditions[?...]filters): the remaining(conditions[?type == '...'])filters are intentionally not coalesced — they are projection asserts without| length(@)(so nil.status.conditionsyields an empty match, not a<nil>throw) and target CRDs/Deployments that reliably populate.status.conditions. They are not the same throwing class as thelength()expressions fixed here.Testing
New regression test
TestRunChainsawTestInProcess_TerminalEvalErrorFailsFastreproduces the exactinvalid type for: <nil>throw and asserts fast-fail (returns in 0.00s vs. retrying to deadline).On-cluster (EKS H100): deployment phase went from an 8m timeout (
status=other) to PASSED in ~21s; full run on this branch = deployment 4/4 + conformance 11/11 green. (Performance phase ran a real Qwen3-8B benchmark and reports a true TTFT-p99 SLA miss — a separate cluster/SLA matter, not this change.) Fullmake qualifynot run locally — recommend CI / a maintainer run before merge.Risk Assessment
Rollout notes: Reclassifying assertion-engine errors
ErrCodeInternal → ErrCodeInvalidRequestis a caller-visible semantic shift (CLI exit 8→2, HTTP 500→400) for a malformed bundled-registry expression — intentional and anticipated by the design comment; backwards compatible otherwise.Checklist
golangci-linton changed package — 0 issues)go test ./validators/chainsaw/...), incl. new regression testmake qualify(recommend running in CI / before merge)git commit -S)