pkg/steps: retry network-unreachable builds, capture failed build logs - #5363
pkg/steps: retry network-unreachable builds, capture failed build logs#5363kaovilai wants to merge 1 commit into
Conversation
hintsAtInfraReason() classifies a Build's Status.LogSnippet as an infra-side (retryable) failure by matching a fixed set of substrings. "network is unreachable" (ENETUNREACH) and "no route to host" were missing, so a transient dial failure during a Go module/sumdb fetch mid-build (e.g. an IPv6 blip) fell through to "classified as legitimate failure, will not be retried" instead of being retried. Failed builds also had no discrete log artifact: gatherSuccessfulBuildLog only ran on the success path, while the failure path just streamed the build pod log to ci-operator's own stdout, making the failing line hard to find without grepping the whole ci-operator log. Renamed the (already outcome-agnostic) helper to gatherBuildLog and call it from both the success and failure paths, so failed builds get the same build-logs/<name>.log.gz artifact successful ones do. Signed-off-by: Tiger Kaovilai <passawit.kaovilai@gmail.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
📝 WalkthroughWalkthroughThe build steps rename the log-gathering helper, collect logs for failed builds, preserve warning behavior for collection failures, and classify additional network errors as infrastructure failures. ChangesBuild log and retry handling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR improves build flake handling in pkg/steps by (1) recognizing additional transient network failures as infrastructure-side retryable errors and (2) ensuring failed builds also produce a dedicated compressed build log artifact, matching the successful-build behavior.
Changes:
- Extend
hintsAtInfraReason()to classify"network is unreachable"and"no route to host"as retryable infra signals. - Rename
gatherSuccessfulBuildLog→gatherBuildLogand invoke it on both build success and build failure to always emitbuild-logs/<name>.log.gz. - Capture failed build logs as artifacts in
waitForBuildfailure/cancel/error phases (in addition to the existing stdout log dump).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/steps/source.go | Adds new infra log-snippet matches and calls gatherBuildLog for both successful and failed builds. |
| pkg/steps/artifacts.go | Renames/clarifies the build-log artifact helper to be outcome-agnostic. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if err := gatherSuccessfulBuildLog(client, ns, name); err != nil { | ||
| if err := gatherBuildLog(client, ns, name); err != nil { | ||
| // log error but do not fail successful build | ||
| logrus.WithError(err).Warnf("Failed gathering successful build %s logs into artifacts.", name) |
| // api logging capabilities; also, without needing to inject an artifacts container, some of the complexities | ||
| // around download/copy from the artifacts container's volume mount and multiple pods are avoided. | ||
| func gatherSuccessfulBuildLog(buildClient BuildClient, namespace, buildName string) error { | ||
| func gatherBuildLog(buildClient BuildClient, namespace, buildName string) error { |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/steps/artifacts.go (1)
667-672: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument
gatherBuildLogas a Go function.The comment is a sentence fragment and does not start with
gatherBuildLog. State that the function writes the compressed build log for the specified build and returns an error when collection fails.Proposed documentation
-// for gathering build logs (successful or failed) to the artifacts, there is no way to augment the pod spec +// gatherBuildLog writes the compressed build log for buildName to the build artifacts. +// It returns an error when artifact creation or log collection fails.As per coding guidelines, Go documentation on functions should be written properly.
🤖 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/steps/artifacts.go` around lines 667 - 672, Update the documentation immediately above gatherBuildLog to begin with “gatherBuildLog” and state that it writes the compressed build log for the specified build, returning an error if collection fails.Source: Coding guidelines
pkg/steps/source.go (1)
662-664: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd regression tests for the new retry and artifact paths.
Extend
pkg/steps/source_test.gowith table-driven cases that verify:
"network is unreachable"and"no route to host"cause infrastructure retry handling.- A non-infrastructure
DockerBuildFailedresult remains non-retryable.- Failed, cancelled, and errored builds invoke
gatherBuildLog.- Artifact collection failures remain warnings and do not replace the build result.
Also applies to: 751-753
🤖 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/steps/source.go` around lines 662 - 664, The source error classification and build-result handling lack regression coverage. Extend the table-driven tests in source_test.go to verify the new infrastructure retry messages, preserve non-retryability for non-infrastructure DockerBuildFailed results, ensure failed/cancelled/errored builds call gatherBuildLog, and confirm artifact collection failures remain warnings without replacing the build result.
🤖 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.
Nitpick comments:
In `@pkg/steps/artifacts.go`:
- Around line 667-672: Update the documentation immediately above gatherBuildLog
to begin with “gatherBuildLog” and state that it writes the compressed build log
for the specified build, returning an error if collection fails.
In `@pkg/steps/source.go`:
- Around line 662-664: The source error classification and build-result handling
lack regression coverage. Extend the table-driven tests in source_test.go to
verify the new infrastructure retry messages, preserve non-retryability for
non-infrastructure DockerBuildFailed results, ensure failed/cancelled/errored
builds call gatherBuildLog, and confirm artifact collection failures remain
warnings without replacing the build result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: bd4b26a6-b9eb-4524-b8ae-5d045d17f565
📒 Files selected for processing (2)
pkg/steps/artifacts.gopkg/steps/source.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/release(manual)openshift/ci-docs(manual)openshift/release-controller(manual)openshift/ci-chat-bot(manual)
|
/retest |
openshift-ci
Bot
added
lgtm
|
/test images |
|
/test core-valid Note Responses generated with Claude |
|
/test owners Note Responses generated with Claude |
|
/test release-controller-config Note Responses generated with Claude |
|
/test openshift-image-mirror-mappings Note Responses generated with Claude |
|
/test images Note Responses generated with Claude |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deepsm007, kaovilai, Prucek The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test core-valid Note Responses generated with Claude |
|
/test owners Note Responses generated with Claude |
|
/test release-controller-config Note Responses generated with Claude |
|
/test openshift-image-mirror-mappings Note Responses generated with Claude |
|
Apologies for the duplicate Note Responses generated with Claude |
|
Heads up: Can't get the actual compiler error though: Note Responses generated with Claude |
|
Update: root-caused (with help from a local repro on real arm64 hardware). This is not a regression from this PR's diff.
So: looks like an infra-side thin-margin issue on the arm64 builder, not code. Retrying once more; if it keeps failing deterministically, worth flagging to DPTP that the arm64 /test images Note Responses generated with Claude |
|
@kaovilai: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Summary
hintsAtInfraReason()(pkg/steps/source.go) now matches"network is unreachable"and"no route to host", so a transient ENETUNREACH-style dial failure during a build (e.g. a Go module/sumdb fetch mid-Dockerfile-build) is classified as an infra-side retryable failure instead of falling through to "classified as legitimate failure, will not be retried".gatherSuccessfulBuildLogtogatherBuildLog(pkg/steps/artifacts.go) and call it from both the success path and the failure path (waitForBuild'sBuildPhaseFailed/Cancelled/Errorcase, alongside the existingprintBuildLogsstdout dump), so failed builds get the same discretebuild-logs/<name>.log.gzartifact successful builds already do, instead of only being interleaved into ci-operator's own stdout log.No config schema changes. No behavior change for legitimate (non-infra)
DockerBuildFailedfailures — those still won't retry, they just also get a log artifact now.Context
Root-caused from a
DockerBuildFailedflake on anoadp-operatorrehearsal build (oadp-operator-1.5-amd64): a sibling job on the identical commit/Dockerfile succeeded in parallel on a different build pod, confirming a build-farm-node-level transient network flake rather than a real Dockerfile bug. The failing line (dial tcp [...]:443: connect: network is unreachable) was hard to find because it was buried inside the monolithicbuild-log.txtrather than a small dedicated per-build log artifact.Test plan
go build ./pkg/steps/...go vet ./pkg/steps/...gofmt -lclean on both changed filesgo test ./pkg/steps/...— all packages passSummary
pkg/stepsto retry builds that fail with"network is unreachable"or"no route to host".build-logs/<name>.log.gzartifacts for successful, failed, cancelled, and errored builds.DockerBuildFailedfailures.gatherBuildLog.