fix(ci): build+push aiperf-bench image in UAT so inference-perf runs - #1638
Conversation
njhensley
added
the
theme/ci-dx
📝 WalkthroughWalkthroughThis PR updates the AWS and GCP UAT workflows to build, push, and clean up an additional Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related issues
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://cdn.statically.io/img/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 (3)
.github/workflows/uat-gcp.yaml (1)
580-594: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
aiperf-benchis never cleaned up.Same gap as
uat-aws.yaml: the build loop now pushesaiperf-bench:${VALIDATOR_TAG}(Line 241), but this cleanup loop only iteratesdeployment performance conformance agent, leaving theaiperf-benchtag behind on GHCR.🧹 Proposed fix
- for phase in deployment performance conformance agent; do + for phase in deployment performance conformance aiperf-bench agent; do🤖 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 @.github/workflows/uat-gcp.yaml around lines 580 - 594, The cleanup step in the validator image workflow is missing the aiperf-bench container, so its tagged image is left behind in GHCR. Update the Cleanup validator image loop to include aiperf-bench alongside deployment, performance, conformance, and agent, and ensure the gh api delete path uses the same validator tag lookup pattern as the other phases..github/workflows/uat-aws.yaml (1)
622-636: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
aiperf-benchis never cleaned up.The build loop now pushes an
aiperf-bench:${VALIDATOR_TAG}image (Line 245), but this cleanup loop still only iteratesdeployment performance conformance agent— theaiperf-benchtag will never be deleted from GHCR, defeating the "avoid GHCR clutter" intent of this step.🧹 Proposed fix
- for phase in deployment performance conformance agent; do + for phase in deployment performance conformance aiperf-bench agent; do🤖 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 @.github/workflows/uat-aws.yaml around lines 622 - 636, The cleanup step in the UAT workflow misses the new aiperf-bench package, so its tagged image is never deleted from GHCR. Update the Cleanup validator image shell loop in the workflow job to include the aiperf-bench package alongside deployment, performance, conformance, and agent, using the same gh api version lookup and delete flow so the validator cleanup covers every pushed tag.pkg/k8s/pod/wait.go (1)
314-343: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd
logPodPhasetoWaitForPodReady
The readiness watch loop still skips the periodic phase log, so it misses the same waiting signal used byWaitForPodSucceeded. AddlogPodPhase(watchedPod)beforecheckPodReadyfor parity.🤖 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/k8s/pod/wait.go` around lines 314 - 343, The pod readiness watch loop in WaitForPodReady is missing the periodic phase logging used elsewhere. Update the event handling for the watched Pod so it calls logPodPhase(watchedPod) before checkPodReady, matching the behavior in WaitForPodSucceeded and preserving the waiting signal during readiness checks.
🤖 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 @.github/workflows/uat-aws.yaml:
- Around line 622-636: The cleanup step in the UAT workflow misses the new
aiperf-bench package, so its tagged image is never deleted from GHCR. Update the
Cleanup validator image shell loop in the workflow job to include the
aiperf-bench package alongside deployment, performance, conformance, and agent,
using the same gh api version lookup and delete flow so the validator cleanup
covers every pushed tag.
In @.github/workflows/uat-gcp.yaml:
- Around line 580-594: The cleanup step in the validator image workflow is
missing the aiperf-bench container, so its tagged image is left behind in GHCR.
Update the Cleanup validator image loop to include aiperf-bench alongside
deployment, performance, conformance, and agent, and ensure the gh api delete
path uses the same validator tag lookup pattern as the other phases.
In `@pkg/k8s/pod/wait.go`:
- Around line 314-343: The pod readiness watch loop in WaitForPodReady is
missing the periodic phase logging used elsewhere. Update the event handling for
the watched Pod so it calls logPodPhase(watchedPod) before checkPodReady,
matching the behavior in WaitForPodSucceeded and preserving the waiting signal
during readiness checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 410a285b-25e1-451b-a98f-d900962ec890
📒 Files selected for processing (4)
.github/workflows/uat-aws.yaml.github/workflows/uat-gcp.yamlpkg/k8s/pod/wait.gopkg/k8s/pod/wait_internal_test.go
The UAT build step tagged only the deployment/performance/conformance validator images with uat-<run_id>, but the performance phase's inference-perf check resolves the aiperf-bench image through the same AICR_VALIDATOR_IMAGE_TAG=uat-<run_id> override. On from-source (main) cells that tag was never built or pushed, so the AIPerf benchmark pod hit ImagePullBackOff and sat in Pending for the full 15m job timeout, surfacing as '[SERVICE_UNAVAILABLE] pod watch closed before pod reached terminal state'. Only release cells (which resolve aiperf-bench:<version> pushed by on-tag.yaml) worked; the path is only exercised by the inference intent, which is why it surfaced now. Add aiperf-bench to the UAT build loop in both uat-gcp.yaml and uat-aws.yaml, using its distinct Dockerfile path and omitting the unused GO_VERSION build-arg. Also make the failure legible next time: WaitForPodSucceeded/ WaitForPodReady now extract the first waiting container's reason and message (e.g. ImagePullBackOff) into the periodic phase log and the watch-closed error context, instead of logging an opaque run of status=Pending. Fixes NVIDIA#1636 Signed-off-by: Nathan Hensley <nhensley@nvidia.com>
126256f to
1140aac
Compare
|
Addressed all three CodeRabbit findings in the amended commit:
Re-ran |
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/k8s/pod/wait.go`:
- Line 140: The pod phase field is using inconsistent attribute names between
logging and returned error context, making it harder to correlate the same
Status.Phase value. Add a shared key constant in consts.go (alongside
keyNamespace, keyName, keyReason, and keyMessage) for the phase field, then
update both logPodPhase and watchClosedContext to use that same constant instead
of mixing "status" and "phase". Keep the existing structured context behavior,
just make the key name consistent in both places.
🪄 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: bb2f702e-0691-4dc9-bc2e-d8713bd8da5d
📒 Files selected for processing (4)
.github/workflows/uat-aws.yaml.github/workflows/uat-gcp.yamlpkg/k8s/pod/wait.gopkg/k8s/pod/wait_internal_test.go
| // container's reason/message when present so a stuck pull/create is visible in | ||
| // the periodic wait log rather than only after a post-mortem. | ||
| func logPodPhase(p *corev1.Pod) { | ||
| attrs := []any{keyName, p.Name, "status", p.Status.Phase} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Inconsistent naming for the pod-phase field across log vs. error context.
logPodPhase logs the phase under "status" (Line 140) while watchClosedContext stores it under "phase" (Line 154). Since both were added in this PR specifically to make failures easier to diagnose, using the same key/attr name for the same underlying Status.Phase value would make it easier to correlate periodic log lines with the returned structured error. Consider adding a keyPhase constant alongside keyNamespace/keyName/keyReason/keyMessage in consts.go and using it in both places.
♻️ Suggested consistency fix
const (
keyNamespace = "namespace"
keyName = "name"
keyReason = "reason"
keyMessage = "message"
keyURI = "uri"
+ keyPhase = "phase"
) func logPodPhase(p *corev1.Pod) {
- attrs := []any{keyName, p.Name, "status", p.Status.Phase}
+ attrs := []any{keyName, p.Name, keyPhase, p.Status.Phase} func watchClosedContext(namespace, name string, p *corev1.Pod) map[string]any {
- ctxMap := map[string]any{keyNamespace: namespace, keyName: name, "phase": string(p.Status.Phase)}
+ ctxMap := map[string]any{keyNamespace: namespace, keyName: name, keyPhase: string(p.Status.Phase)}Also applies to: 154-154
🤖 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/k8s/pod/wait.go` at line 140, The pod phase field is using inconsistent
attribute names between logging and returned error context, making it harder to
correlate the same Status.Phase value. Add a shared key constant in consts.go
(alongside keyNamespace, keyName, keyReason, and keyMessage) for the phase
field, then update both logPodPhase and watchClosedContext to use that same
constant instead of mixing "status" and "phase". Keep the existing structured
context behavior, just make the key name consistent in both places.
There was a problem hiding this comment.
Skipping this one deliberately. The status log attr is pre-existing — the original line was slog.Info("pod current phase", ..., "status", watchedPod.Status.Phase), and status=Pending is exactly the field that appears in the failure logs this PR diagnoses. Renaming it to phase would break any log grep/dashboard keyed on status=, which isn't worth a cosmetic gain. The phase key in the structured error is a distinct, internal consumer (error context, not log stream), so I've left the two as-is.
There was a problem hiding this comment.
@njhensley, that makes sense — the log stream stability tradeoff outweighs the cosmetic naming alignment here, especially since status=Pending is the exact grep target this PR is meant to make more diagnosable. Thanks for the context, I'll leave it as-is.
✏️ Learnings added
Learnt from: njhensley
Repo: NVIDIA/aicr PR: 1638
File: pkg/k8s/pod/wait.go:140-140
Timestamp: 2026-07-07T07:48:10.420Z
Learning: In NVIDIA/aicr, `pkg/k8s/pod/wait.go`'s `logPodPhase` function intentionally logs the pod phase under the `"status"` slog attribute key (not `"phase"`), because this is pre-existing behavior (`slog.Info("pod current phase", ..., "status", watchedPod.Status.Phase)`) relied upon by log-based grep/dashboards keyed on `status=`. This is deliberately kept distinct from the `"phase"` key used in `watchClosedContext`'s structured error context map, which is a separate internal consumer. Do not suggest unifying these two key names for consistency.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: njhensley
Repo: NVIDIA/aicr PR: 595
File: pkg/recipe/loader.go:56-60
Timestamp: 2026-04-16T18:20:53.204Z
Learning: In this repository’s Go code, if an error was already created/wrapped with `pkg/errors` (via `aicrerrors.New(...)` or `aicrerrors.WrapWithContext(...)`) and includes the correct structured error code (e.g., `ErrCodeInvalidRequest`, `ErrCodeTimeout`, `ErrCodeInternal`), then returning it should use the bare form `return nil, err` (or propagate the error as-is). Do not require “missing context” when the source already has proper codes; do not double-wrap it. Avoid changing the error by using `errors.Wrapf` (which can override the structured code) or `fmt.Errorf("%w", err)` in these cases, which is documented as an anti-pattern in `CLAUDE.md`.
Learnt from: mchmarny
Repo: NVIDIA/aicr PR: 728
File: pkg/recipe/criteria.go:801-820
Timestamp: 2026-05-01T23:59:30.754Z
Learning: In NVIDIA/aicr, use the idiomatic helper `errors.PropagateOrWrap(err, errCode, message)` from `pkg/errors` when handling errors in Go code. This helper should be preferred over manual `errors.As` + conditional return logic: it propagates `err` as-is if it already carries a `*StructuredError` code, and otherwise wraps `err` using the provided fallback `errCode` and `message`. Apply this pattern especially when calling functions from `pkg/serializer`, `pkg/recipe`, or other packages that may already return coded structured errors.
Learnt from: njhensley
Repo: NVIDIA/aicr PR: 844
File: pkg/fingerprint/from_measurements.go:200-220
Timestamp: 2026-05-11T21:09:20.802Z
Learning: In this repo’s Go code, do not flag or request changes for `int64`→`int` casts when the value originates from a Go `int` and remains in-process (e.g., it round-trips via `measurement.Reading.Any()` but never crosses a system boundary like serialization/network/DB). Only validate/guard numeric ranges at system boundaries; adding `math.MaxInt32/MinInt32` clamping for such internal-only casts is an anti-pattern (per CLAUDE.md), since the scenario is guaranteed by internal/framework behavior and 64-bit Linux targets keep `int` effectively 64 bits.
Learnt from: mchmarny
Repo: NVIDIA/aicr PR: 1126
File: pkg/api/doc.go:105-105
Timestamp: 2026-05-30T16:28:11.381Z
Learning: In NVIDIA/aicr, timeout deadlines are intentionally layered: `defaults.ServerHandlerTimeout` (90s) is the outer HTTP/server ceiling, while individual handlers may set tighter per-operation deadlines using `context.WithTimeout` (e.g., 30s) to bound specific I/O or sub-operations. During code review, do not flag this as a conflict or redundancy: documenting both values is correct and intended. The outer timeout prevents net/http from closing connections mid-handler, and the inner deadlines constrain only the relevant sub-operation(s).
Learnt from: yuanchen8911
Repo: NVIDIA/aicr PR: 1580
File: tools/bom/freshness_test.go:157-168
Timestamp: 2026-07-01T19:44:36.763Z
Learning: In this repo’s Go code, calls to `pkg/errors.New` must include an error code argument (e.g., `errors.New(errors.ErrCodeInvalidRequest, msg)`). Do not use `pkg/errors.New` without a code. However, in `_test.go` files, using `fmt.Errorf` is acceptable for errors created inside test-only helper functions that are purely local—i.e., they are consumed immediately by `t.Fatalf`/`t.Errorf` (or otherwise handled within the test) and never returned to a production caller—because such local errors carry no meaningful error code (see patterns in `tools/bom/freshness_test.go` like the duplicate-row check helpers).
…VIDIA#1638) Signed-off-by: Nathan Hensley <nhensley@nvidia.com> Co-authored-by: Mark Chmarny <mchmarny@users.noreply.github.com>
2 participants
Summary
Build and push the
aiperf-benchvalidator image with theuat-<run_id>tag in the UAT workflows, so theinference-perfperformance check can actually pull it instead of hanging onImagePullBackOff.Motivation / Context
On from-source (main) UAT cells the AIPerf benchmark pod sat in
Pendingfor the full 15 min job timeout and the run failed with:Root cause: the UAT "Build and push validator images" step tagged only
deployment,performance, andconformancewithuat-<run_id>, but the performance phase'sinference-perfcheck resolves theaiperf-benchimage through the sameAICR_VALIDATOR_IMAGE_TAG=uat-<run_id>override (isReleaseVersion("main")is false, so the override rewrites the tag). That tag was never built or pushed → the pod 404'd on pull →ImagePullBackOff(phasePending) → 15 min timeout.on-push.yaml/on-tag.yamlVALIDATOR_PHASESalready includeaiperf-bench, so release cells worked; only from-source main cells broke, and the path is only exercised by the inference intent (DC2, #1275), which is why it surfaced now.The
SERVICE_UNAVAILABLE / pod watch closedmessage is a misleading surface symptom — the watch channel closed ~9 s before the context deadline, sopkg/k8s/pod/wait.goreported watch-closed rather than timeout.Fixes: #1636
Related: #1275
Type of Change
Component(s) Affected
pkg/errors,pkg/k8s).github/workflows/uat-{gcp,aws}.yaml)Implementation Notes
aiperf-benchto the UAT build loop inuat-gcp.yamlanduat-aws.yaml, using acaseso it builds from its distinct Dockerfile path (validators/performance/aiperf-bench.Dockerfile) and omits the unusedGO_VERSIONbuild-arg. Kept the phase set in sync withVALIDATOR_PHASES.WaitForPodSucceeded/WaitForPodReadynow extract the first init/regular container'sWaitingreason+message (e.g.ImagePullBackOff) into the periodicpod current phaselog and the watch-closed error context, so this class of failure is legible instead of an opaque run ofstatus=Pending. No control-flow change.Testing
New table-driven tests cover
podWaitingReason,logPodPhase, andwatchClosedContext(all three helpers at 100%); package coverage 80.1% → 81.2%. The build loop was dry-run-verified to expand toaiperf-bench:uat-<run_id>correctly.Risk Assessment
Rollout notes: N/A — takes effect on the next UAT run.
Checklist
make testwith-race) — for affected packagemake lint) — for affected packagegit commit -S)