🐛 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.
There was a problem hiding this comment.
jq handles JSON Lines (JSONL/NDJSON) natively — each JSON object on a separate line is read as a separate input and the filter is applied to each one independently. No -s needed:
$ echo '{"schema":"olm.package","name":"foo"}
{"schema":"olm.channel","name":"bar"}' | jq 'select(.schema == "olm.package") | .name'
"foo"The filters were adjusted to remove the .[] | prefix that was only needed in slurp mode (where jq wraps all inputs into a single array). Without -s, jq iterates over inputs automatically, so select(...) applies directly to each object.
The benefit of dropping -s is constant-memory processing — jq processes each JSON object as it arrives from the pipe instead of buffering the entire catalog (~100+ MB for operatorhubio) into memory before processing. The original exit status 5 (jq system error) was likely caused by this memory pressure. The full test suite (all 4 scenarios, 30 steps) passes locally with this change.
|
Thanks for looking into this @pedjak! |
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.
There was a problem hiding this comment.
🟡 Not ready to approve
The new curl | jq pipeline can still mask curl failures without pipefail, undermining the intended stderr/error propagation and retry behavior.
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:136
- The
curl | jqpipeline can maskcurlfailures because (withoutpipefail) the pipeline exit status isjq’s. Ifcurlexits non-zero with no/partial output,jqmay still exit 0, causingbash()to return a nil error and losing the curl stderr diagnostics you’re trying to surface/retry on.
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.
There was a problem hiding this comment.
🟡 Not ready to approve
The new curl | jq pipeline should enable pipefail (and ideally curl -f) so upstream curl failures reliably surface as errors with captured stderr rather than being masked by a successful jq exit.
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:136
- The
curl | jqpipeline runs withoutpipefail, so ifcurlfails butjqexits 0 (e.g., empty input), the command can appear successful and you’ll only see an empty result / retries without the underlying transport error. Enablingpipefail(and optionallycurl -f) makes transient HTTP/transport failures reliably surface as an error with captured stderr.
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.
There was a problem hiding this comment.
🟡 Not ready to approve
The new curl-based catalog query path adds retries but lacks explicit per-request timeouts, which can allow a single stalled request to hang far longer than intended and reintroduce CI instability.
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:136
curlincatalogCurlJqhas retries but no explicit connect/overall timeout, so a stalled port-forward/TLS handshake can hang a single attempt for a long time and effectively bypass the per-stepwaitFortimeout. Add--connect-timeoutand--max-time(or similar) to bound each attempt.
script := fmt.Sprintf(
`curl -sS -k --compressed --fail --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.
There was a problem hiding this comment.
🟡 Not ready to approve
The PR description claims curl retry flags were added, but the updated curl invocation in catalogCurlJq currently does not include them, creating a mismatch that should be resolved (by code or description update).
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:136
- PR description says
curluses--retry 3 --retry-delay 2 --retry-all-errors, but the updated command here doesn’t include any retry flags, so transient transport failures will still fail immediately at the curl layer (only the outerwaitForretries). Either update the PR description or add the curl retry options here to match the stated intent.
script := fmt.Sprintf(
`set -o pipefail; curl -sS -k --compressed --fail 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.
There was a problem hiding this comment.
🟡 Not ready to approve
The implementation doesn’t include the curl retry flags promised in the PR description, reducing the intended transport-level resilience.
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:149
- PR description says
curlwas updated to include transport-level retries (--retry 3 --retry-delay 2 --retry-all-errors), but the current command does not include any retry flags. This is a discrepancy with the stated change and reduces resilience to transient network/port-forward failures.
script := fmt.Sprintf(
`set -o pipefail; curl -sS -k --compressed --fail 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.
There was a problem hiding this comment.
🟡 Not ready to approve
The PR description states curl retries were added (--retry 3 --retry-delay 2 --retry-all-errors) but the updated command currently omits these flags.
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:170
- PR description says
curlwas updated with--retry 3 --retry-delay 2 --retry-all-errors, but the current command only uses-sS -k --compressed --fail. Adding the retry flags would match the stated intent and further reduce transient transport failures beforejqruns.
script := fmt.Sprintf(
`set -o pipefail; curl -sS -k --compressed --fail 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.
The generate-demos CI job fails ~45% of the time with jq exit status 5 on the ClusterCatalog Quickstart scenario. Several 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 - with CatalogdHA, kubectl port-forward to the service deterministically picks the same pod via GetFirstPod sorting; if that pod is not the leader, it returns 404 (empty local cache) for every retry Remove jq slurp mode so each JSON object is processed in constant memory, prefixing filters with 'objects' to skip non-object values in the FBC stream. Wrap CatalogContainsSomePackages, PackageHasSomeChannels, and PackageHasSomeBundles in waitFor for retry on transient errors. Add curl --compressed to handle gzip-encoded responses and --fail with pipefail to detect HTTP errors. Resolve the catalogd leader pod via its Lease and port-forward directly to it on the container port (8443), falling back to the service when the lease cannot be read. Reset port-forwards on query failure and re-establish dead ones via liveness checks. Inject stderr into ExitError in bash() to match k8sClient diagnostics. Log catalog query errors at V(0) so CI timeout failures are diagnosable. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Not ready to approve
The implementation currently diverges from the PR description by not including the stated curl --retry ... flags, which is a key part of the proposed flake-reduction approach.
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:173
- PR description says this change adds
curl --retry 3 --retry-delay 2 --retry-all-errors, but the updated command here doesn’t include any retry flags. Without them, transient network/port-forward drops can still fail the step immediately (before the higher-levelwaitForretry kicks in), and the implementation diverges from the stated intent.
script := fmt.Sprintf(
`set -o pipefail; curl -sS -k --compressed --fail 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.
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