fix(prow-job-executor): retry EV2 gating once for pre-test infra precondition failures - #324
Conversation
…ondition failures AROSLSRE-1721's finished.json-driven retry only covers per-test known-issue failures, since it depends on metadata the aro-hcp-tests binary writes once it runs. A gating job that fails in a shared multi-stage pre-step (e.g. aro-hcp-lease-acquire exhausting the e2e slot pool) never gets that far, so none of its finished.json files ever carry ev2-failed-tests at all. jobAllowsEV2Retry now reports this shape as a new InfraPreconditionEligible class, distinct from KnownIssueEligible, wired to a new EV2InfraRetryableError / ev2-retryable-infra-precondition-failure marker. Same bounded single retry as the existing mechanism (EV2's automatedRetry.maximumRetryCount stays at 1). AROSLSRE-1926
There was a problem hiding this comment.
Pull request overview
This PR extends prow-job-executor’s EV2 gating auto-retry signaling to cover “pre-test infra precondition” failures (where aro-hcp-tests never ran and therefore no finished.json ever contains ev2-failed-tests), in addition to the existing known-issue test-failure retry path.
Changes:
- Introduces a
RetryEligibilityclassification (NotEligible / KnownIssueEligible / InfraPreconditionEligible) returned byjobAllowsEV2Retry. - Adds a distinct infra retry marker + error type (
EV2InfraRetryableMarker/EV2InfraRetryableError) and wiresExecuteAndWaitto emit the correct marker based on eligibility. - Updates and adds unit tests to validate the new eligibility and infra-marker behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tools/prow-job-executor/prowjob/retrymarker.go | Adds RetryEligibility and updates jobAllowsEV2Retry to distinguish infra-precondition eligibility when no step reports test metadata. |
| tools/prow-job-executor/prowjob/retrymarker_test.go | Updates tests for the new return type and adds coverage for the “no step carries metadata” eligibility case. |
| tools/prow-job-executor/prowjob/monitor.go | Adds infra retry marker/error type and updates ExecuteAndWait to return the appropriate retryable error. |
| tools/prow-job-executor/prowjob/monitor_test.go | Adds a new test validating infra-marker behavior and updates existing tests for the new eligibility type. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…bility - add RetryEligibility.String() so logs/test failures show readable names instead of a bare integer - clarify the NotEligible doc comment: it means a step reported results but they weren't eligible (or an error occurred), distinct from InfraPreconditionEligible where no step ever reported results at all
|
Pushed a follow-up commit (d4f2353) addressing the two Copilot review comments on
|
…eview Describes the actual precondition (candidate finished.json carried ev2FailedTestsKey) instead of a nonexistent found parameter.
|
Pushed 77eed39 addressing the latest Copilot comment: reworded the |
Not related to this PR's scope - was a local helper for monitoring CI, added by mistake via a broad git add.
|
Pushed dd19287: removed |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
tools/prow-job-executor/prowjob/monitor.go:72
- This marker comment says the failure happened "before the aro-hcp-tests binary ever ran", but InfraPreconditionEligible also covers cases where aro-hcp-tests may have started but crashed before writing metadata. Reword to match the actual condition (no step's finished.json carried the ev2-failed-tests key).
This issue also appears in the following locations of the same file:
- line 76
- line 236
// EV2InfraRetryableMarker is the fixed, matchable substring prow-job-executor emits when a
// gating job failed in a shared multi-stage pre-step (e.g. aro-hcp-lease-acquire exhausting
// the e2e slot pool) before the aro-hcp-tests binary ever ran, and so is eligible for the
// same bounded, single automatic EV2 retry as a KnownIssueEligible failure - see
// InfraPreconditionEligible. Kept distinct from EV2RetryableMarker (rather than reusing it)
tools/prow-job-executor/prowjob/monitor.go:79
- This comment says the failure happened "before any test could run", but the code classifies InfraPreconditionEligible based on absence of aro-hcp-tests metadata (ev2-failed-tests), which can also happen if aro-hcp-tests crashed before writing metadata. Reword to avoid overstating the cause.
// EV2InfraRetryableError wraps a job failure where no step's finished.json ever reported
// aro-hcp-tests results at all, meaning the failure happened before any test could run.
// Its Error() text always contains EV2InfraRetryableMarker, so EV2's
// automatedRetry.errorContainsAny can match on it and re-run the gating step.
tools/prow-job-executor/prowjob/monitor.go:237
- ExecuteAndWait's comment says EV2InfraRetryableError is used when "no step ever ran the aro-hcp-tests suite at all", but the eligibility condition is actually that no step ever reported aro-hcp-tests results (ev2-failed-tests key absent), which can also include a crash before metadata was written. Consider rewording to avoid overstating the cause.
// EV2InfraRetryableError when no step ever ran the aro-hcp-tests suite at all (a pre-test
// infra failure, e.g. lease-acquire capacity exhaustion). The EV2 gating step's
|
Giulio Frasca (@gmfrasca): changing LGTM is restricted to collaborators DetailsIn response to this:
Instructions 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. |
…; fix stg/int retry gap Adds the new ev2-retryable-infra-precondition-failure marker that prow-job-executor emits (Azure/ARO-Tools#324, AROSLSRE-1926) alongside the existing known-issue marker (AROSLSRE-1721) to the regionalGating step's automatedRetry.errorContainsAny. Also updates the inline comment: sdp-pipelines' generator now honors a step's custom errorContainsAny patterns even in the stg/int fail-fast path (useExclusiveLocks), so these markers take effect there too, not just in prod (AROSLSRE-1764).
|
/lgtm |
|
Venkatesh S (@venkateshsredhat): changing LGTM is restricted to collaborators DetailsIn response to this:
Instructions 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. |
|
/lgtm |
|
Ashish (@ashishmax31): changing LGTM is restricted to collaborators DetailsIn response to this:
Instructions 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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ashishmax31, gmfrasca, janboll, raelga, venkateshsredhat 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 |
…arker (AROSLSRE-1926) Picks up Azure/ARO-Tools#324, which adds a new InfraPreconditionEligible retry class (and EV2InfraRetryableError/ev2-retryable-infra-precondition-failure marker) to prow-job-executor for gating jobs that fail in a shared pre-test step (e.g. aro-hcp-lease-acquire) before the test binary ever runs, so none of the job's finished.json files carry the ev2-failed-tests key. Without this bump, test/cmd/prow-job-executor keeps running the older logic and never emits the new marker.
…; fix stg/int retry gap Adds the new ev2-retryable-infra-precondition-failure marker that prow-job-executor emits (Azure/ARO-Tools#324, AROSLSRE-1926) alongside the existing known-issue marker (AROSLSRE-1721) to the regionalGating step's automatedRetry.errorContainsAny. Also updates the inline comment: sdp-pipelines' generator now honors a step's custom errorContainsAny patterns even in the stg/int fail-fast path (useExclusiveLocks), so these markers take effect there too, not just in prod (AROSLSRE-1764).
…arker (AROSLSRE-1926) Picks up Azure/ARO-Tools#324, which adds a new InfraPreconditionEligible retry class (and EV2InfraRetryableError/ev2-retryable-infra-precondition-failure marker) to prow-job-executor for gating jobs that fail in a shared pre-test step (e.g. aro-hcp-lease-acquire) before the test binary ever runs, so none of the job's finished.json files carry the ev2-failed-tests key. Without this bump, test/cmd/prow-job-executor keeps running the older logic and never emits the new marker.
AROSLSRE-1926
Why
AROSLSRE-1721 added an EV2 gating auto-retry for known-issue test failures, driven by
prow-job-executorreadingfinished.jsonmetadata that thearo-hcp-testsbinary writes once it runs. That mechanism can't help when a gating job fails in a shared multi-stage pre-step (e.g.aro-hcp-lease-acquire) before the test binary ever runs, since none of the job'sfinished.jsonfiles ever carry theev2-failed-testskey at all in that case.Observed case: a stg e2e gating job failed in
aro-hcp-lease-acquirewithno candidate pool for environment "stg" yielded an immediate lease for 1h0m0s across 41 full pass(es)(e2e slot pool capacity exhaustion).What
jobAllowsEV2Retrynow reports a newInfraPreconditionEligibleclass (distinct from the existingKnownIssueEligible) when no step'sfinished.jsonever carriedev2-failed-testsat all.ExecuteAndWaitwires this to a newEV2InfraRetryableError/ev2-retryable-infra-precondition-failuremarker, alongside the existingEV2RetryableError/EV2RetryableMarker. Same bounded single retry as before - EV2's pipeline-levelautomatedRetry.maximumRetryCountstays at 1,prow-job-executornever resubmits jobs itself.Follow-up: a companion sdp-pipelines PR is needed to bump the
prowjobexecutordependency, add the new marker toe2e-pipeline.yaml'sautomatedRetry.errorContainsAny, and fix the AROSLSRE-1764 gap soautomatedRetrytakes effect in stg/int (not just prod) - tracked under the same Jira story.Testing
go build ./...andgo vet ./...fortools/prow-job-executorgo test ./...fortools/prow-job-executor/prowjob- all passing, including newTestExecuteAndWaitFailsWithInfraMarkerWhenNoStepRanTestsand updatedTestJobAllowsEV2RetryNoStepCarriesMetadatagolangci-lint runon the module - 0 issues