🐛 Fix flaky generate-demos CI job - #2838
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
pedjak
changed the title
pedjak
changed the title
There was a problem hiding this comment.
🟡 Not ready to approve
The new curl invocation uses -s (silent), which suppresses error output and can negate the intended stderr/diagnostics improvements unless adjusted (e.g., -sS).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR reduces flakiness in the generate-demos E2E/quickstart flow by making catalog content queries more resilient and less resource-intensive when reading large catalogd /api/v1/all JSONL payloads (notably operatorhubio).
Changes:
- Capture and propagate stderr for
bash()command failures by attaching it toexec.ExitError. - Switch catalog queries from
jq -sslurp mode to streamingjqfilters to avoid buffering the full JSONL response in memory. - Add curl retries (and wrap catalog query steps in
waitFor) to tolerate transient port-forward / network issues.
File summaries
| File | Description |
|---|---|
test/e2e/steps/demo_steps.go |
Improves robustness of demo E2E steps by enhancing command diagnostics, switching jq processing to streaming, and adding retry behavior around catalog queries. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| script := fmt.Sprintf( | ||
| `curl -s -k https://%s/catalogs/%s/api/v1/all | jq -s '%s'`, | ||
| `curl -s -k --retry 3 --retry-delay 2 --retry-all-errors https://%s/catalogs/%s/api/v1/all | jq '%s'`, | ||
| addr, catalogName, jqFilter, | ||
| ) |
There was a problem hiding this comment.
Good catch — updated to curl -sS so transport/TLS errors are surfaced on stderr while keeping normal output quiet. This pairs well with the new stderr-to-ExitError propagation in bash().
| waitFor(ctx, func() bool { | ||
| out, err := catalogCurlJq(ctx, catalogName, | ||
| `select(.schema == "olm.package") | .name`) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| return strings.TrimSpace(out) != "" | ||
| }) |
There was a problem hiding this comment.
These changed functions will no longer return an error. Should the context be checked for a timeout so that an error can be returned?
There was a problem hiding this comment.
The waitFor function uses require.Eventually(godog.T(ctx), ..., timeout, tick) which calls t.FailNow() on timeout — the function never returns on failure, it hard-fails the test via runtime.Goexit(). This matches the established pattern used by PodHasContainerCount in the same file (line 209) and dozens of other step functions throughout the test suite. So from a test perspective, the behavior is: either the condition is met within 5 minutes, or the scenario fails immediately.
There was a problem hiding this comment.
🟡 Not ready to approve
The curl | jq pipeline can still mask curl failures without pipefail, which undermines the intended retry/diagnostics improvements and can reintroduce flakiness.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
test/e2e/steps/demo_steps.go:125
- The
curl | jqpipeline is executed underbash -cwithoutpipefail, so a failingcurlcan be masked ifjqexits 0 on empty stdin (common when the connection drops andcurloutputs nothing). That defeats the goal of retrying transient transport failures and can make the step fail later with an unhelpful timeout/empty-output symptom instead of surfacing the real error.
Enable pipefail for this command so a non-zero curl exit status propagates to bash()/waitFor and the captured stderr is preserved.
script := fmt.Sprintf(
`curl -sS -k --retry 3 --retry-delay 2 --retry-all-errors https://%s/catalogs/%s/api/v1/all | jq '%s'`,
addr, catalogName, jqFilter,
)
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
Not a good sign when the update-demos is still failing with this. |
| return "", err | ||
| } | ||
| script := fmt.Sprintf( | ||
| `curl -s -k https://%s/catalogs/%s/api/v1/all | jq -s '%s'`, |
There was a problem hiding this comment.
We need jq -s because the catalog format is in JSONLines (jsonl) format, not monolithic JSON, and slurp-mode forces it to re-represent it inside a monolithic object. Without this, jq will error.
|
Thanks for looking into this @pedjak! |
The generate-demos CI job fails ~45% of the time with jq exit status 5 on the ClusterCatalog Quickstart scenario. Three issues contribute: - jq -s (slurp mode) buffers the entire operatorhubio FBC response in memory before processing, risking system errors on large catalogs - catalog content queries run exactly once with no retry, so any transient port-forward or network hiccup fails the step immediately - bash() does not attach stderr to ExitError, making failures opaque Remove jq slurp mode so each JSON object is processed in constant memory. Wrap CatalogContainsSomePackages, PackageHasSomeChannels, and PackageHasSomeBundles in waitFor for retry on transient errors. Add curl --retry for transport-level resilience. Inject stderr into ExitError in bash() to match k8sClient diagnostics. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Not ready to approve
The new retry loops currently swallow the underlying curl/jq error details, which can still leave CI failures opaque even with improved stderr propagation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (3)
test/e2e/steps/demo_steps.go:158
- The retry loop suppresses the concrete
catalogCurlJqerror by returningfalseon error; if the command keeps failing, the step will time out with a generic message and no visibility into the last stderr/exit status.
Log the last error (and/or empty output) inside the polling closure so failures are diagnosable in CI output.
waitFor(ctx, func() bool {
out, err := catalogCurlJq(ctx, catalogName,
fmt.Sprintf(`select(.schema == "olm.channel") | select(.package == "%s") | .name`, packageName))
if err != nil {
return false
test/e2e/steps/demo_steps.go:170
- Same as the other catalog queries: returning
falseoncatalogCurlJqerrors hides the real failure reason when retries ultimately time out, which makes CI failures hard to debug.
Log the last error/empty output inside the polling closure so the final test output shows what kept failing.
waitFor(ctx, func() bool {
out, err := catalogCurlJq(ctx, catalogName,
fmt.Sprintf(`select(.schema == "olm.bundle") | select(.package == "%s") | .name`, packageName))
if err != nil {
return false
test/e2e/steps/demo_steps.go:149
- The retry loop drops the underlying
catalogCurlJqerror by just returningfalse; if the command keeps failing (e.g., curl/jq non-zero) the eventual timeout failure will be generic and won’t include the real cause, undermining the new stderr propagation inbash().
Log the error/output inside the polling closure so Go test output captures the last observed failure when the step times out.
This issue also appears in the following locations of the same file:
- line 154
- line 166
waitFor(ctx, func() bool {
out, err := catalogCurlJq(ctx, catalogName,
`select(.schema == "olm.package") | .name`)
if err != nil {
return false
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Description
The
generate-demosCI job fails ~45% of the time (9 of last 20 runs) withjqexit status 5 on theClusterCatalog Quickstartscenario. The failure is always at thecatalog "operatorhubio" contains some packagesstep.Three issues contribute to the flakiness:
jq -s(slurp mode) buffers the entire operatorhubio FBC response in memory before processing, risking system errors (exit code 5) on the large operatorhubio catalogbash()does not attach stderr toExitError, making failures opaque in CI logsThis PR:
jq -s(slurp mode) so each JSON object is processed in constant memory via streamingCatalogContainsSomePackages,PackageHasSomeChannels, andPackageHasSomeBundlesinwaitForfor retry on transient errors (matching the pattern used byPodHasContainerCountand dozens of other steps)curl --retry 3 --retry-delay 2 --retry-all-errorsfor transport-level resilienceExitErrorinbash()to match thek8sClientdiagnostics patternTested locally with
make update-demos— all 4 scenarios, 30 steps passed.Reviewer Checklist