fix(ci): harden GKE readiness via /healthcheck probes - #5211
Conversation
Probe GET /healthcheck (not HEAD /) during deploy readiness and again right before Playwright so transient TLS disconnects retry within the existing budget. Also poll the instance health-check spec through transport failures (RHDHBUGS-3508).
PR Summary by QodoHarden CI/GKE readiness by probing GET /healthcheck and polling Playwright health test
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-1.10 #5211 +/- ##
=================================================
+ Coverage 41.03% 69.60% +28.57%
=================================================
Files 121 111 -10
Lines 2220 4702 +2482
Branches 563 537 -26
=================================================
+ Hits 911 3273 +2362
- Misses 1304 1428 +124
+ Partials 5 1 -4
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
46 rules✅ Skills:
e2e-verify-fix, e2e-diagnose-and-fix✅ Cross-repo context Not relevant to this PR:
redhat-developer/rhdh-plugins Not relevant to this PR:
redhat-developer/rhdh-chart Not relevant to this PR:
redhat-developer/rhdh-operator Not relevant to this PR:
redhat-developer/rhdh-local 1. Healthcheck budget unbounded
|
Revert coverage import; shorten pre-Playwright re-probe to warn-only ~30s; drop wait_for_rhdh_healthcheck helper; reduce spec poll to 30s.
|
The container image build workflow finished with status: |
|
|
/test e2e-gke-helm-nightly |
|
/test e2e-gke-helm-nightly |
3 similar comments
|
/test e2e-gke-helm-nightly |
|
/test e2e-gke-helm-nightly |
|
/test e2e-gke-helm-nightly |
zdrapela
changed the title
| # Returns: | ||
| # 0 - HTTP 200 and body contains "status":"ok" | ||
| # 1 - Not ready | ||
| testing::probe_rhdh_healthcheck() { |
There was a problem hiding this comment.
This probe needs to land on main too. origin/main:.ci/pipelines/lib/testing.sh still does curl -I for readiness, and per RHDHBUGS-3508 the flake last hit main on Aug 1.
The ticket says main is waiting on #5083, but that PR has been open since Jul 10, is currently CONFLICTING, and touches 90 files — not a realistic vehicle for a CI flake fix. Could we land the slim extract on main directly, as the ticket comment suggests as the alternative? Otherwise 1.11 branches off main and this fix disappears with it.
| local response http_status body | ||
|
|
||
| # Append http_code on its own line so connect/TLS failures can be retried. | ||
| response=$(curl --insecure -sS -w "\n%{http_code}" "${health_url}" 2> /dev/null || true) |
There was a problem hiding this comment.
No --connect-timeout / --max-time here. If the LB accepts the connection but the backend never answers, curl blocks with no ceiling — that turns the GKE budget of 50 x 30s into unbounded wall clock inside a job with a hard Prow timeout, and makes the pre-Playwright loop open-ended too.
response=$(curl --insecure -sS --connect-timeout 5 --max-time 15 -w "\n%{http_code}" "${health_url}" 2> /dev/null || true)|
|
||
| # Append http_code on its own line so connect/TLS failures can be retried. | ||
| response=$(curl --insecure -sS -w "\n%{http_code}" "${health_url}" 2> /dev/null || true) | ||
| if [[ -z "${response}" ]]; then |
There was a problem hiding this comment.
nit: this branch is effectively unreachable. With -w set, curl still writes the format string on a connect failure, so response comes back as \n000 rather than empty (I checked against a dead port). The retry actually comes from http_status != 200 below, not from here — worth either dropping the guard or rewording the comment above it so the next reader doesn't think it's load-bearing.
Same line above: -S doesn't buy anything with stderr going to /dev/null.
| http_status=$(printf '%s' "${response}" | tail -n 1) | ||
| body=$(printf '%s' "${response}" | sed '$d') | ||
|
|
||
| if [[ "${http_status}" == "200" ]] && [[ "${body}" =~ \"status\"[[:space:]]*:[[:space:]]*\"ok\" ]]; then |
There was a problem hiding this comment.
This substring match passes on a nested hit — {"db":{"status":"ok"},"status":"error"} reads as healthy. jq is already in the CI image (.ci/images/Dockerfile:39), so we can be exact and fail closed on malformed JSON at the same time:
if [[ "${http_status}" == "200" ]] && printf '%s' "${body}" | jq -e '.status == "ok"' > /dev/null 2>&1; then|
|
||
| # Quick re-check after yarn install; do not fail the job — deploy readiness | ||
| # already waited. Warn-only so a brief TLS blip does not abort the suite. | ||
| if [[ -n "${url}" ]]; then |
There was a problem hiding this comment.
Should this use common::retry (.ci/pipelines/lib/common.sh:182) rather than a hand-rolled loop?
if [[ -n "${url}" ]]; then
if common::retry 6 5 testing::probe_rhdh_healthcheck "${url}"; then
log::success "Pre-Playwright /healthcheck OK"
else
log::warn "Pre-Playwright /healthcheck still flaky; continuing to tests"
fi
fiSame behavior, ~15 fewer lines, and consistent logging with the rest of .ci/. It also drops the wasted final sleep 5 — the loop as written sleeps after the 6th failed probe before falling through.
| if [[ -n "${url}" ]]; then | ||
| local ready=false | ||
| local i | ||
| for ((i = 1; i <= 6; i++)); do |
There was a problem hiding this comment.
nit: 6 and 5 are bare literals, and the resulting "~30s" lives only in the warn string below — they'll drift apart the first time someone tunes one. A named local (local health_retries=6 health_backoff_seconds=5) or a one-line comment on why 30s is the right window would keep them honest.
Related: the PR description still says "up to ~2 minutes" while this does ~30s. Looks stale after the slim GKE healthcheck harden commit.
| return 0 | ||
| else | ||
| log::warn "Attempt ${i} of ${max_attempts}: Backstage not yet available (HTTP Status: ${http_status})" | ||
| log::warn "Attempt ${i} of ${max_attempts}: Backstage /healthcheck not yet available" |
There was a problem hiding this comment.
We lost the HTTP status here — this used to print (HTTP Status: 503). That's the one number that separates a TLS disconnect (000) from a real 503 or a 200 whose body isn't ok yet, which is exactly the distinction RHDHBUGS-3508 is about, so dropping it makes the next occurrence harder to triage than the code being replaced.
#5083 keeps that signal on the TS side via its detail string. Could the probe echo the status (or stash it in a namespaced global like _TESTING_LAST_HEALTH_STATUS) so this warn can include it?
| expect(response.status()).toBe(200); | ||
|
|
||
| expect(responseBody).toHaveProperty("status", "ok"); | ||
| // Short poll: one TLS blip should not fail; deploy readiness already waited. |
There was a problem hiding this comment.
Heads up: this spec doesn't run in showcase-rbac-k8s. That project uses testMatch: ["**/playwright/e2e/**/*-rbac.spec.ts"] (playwright.config.ts:151), and instance-health-check.spec.ts doesn't match it — so for one of the two projects the ticket names as affected, this change is a no-op and only the shell probe covers it.
Was the failure there coming through the smoke-test dependency instead? If so, is retries: 10 on smoke-test already absorbing it, or is something still left to fix on that path?
|
|
||
| expect(responseBody).toHaveProperty("status", "ok"); | ||
| // Short poll: one TLS blip should not fail; deploy readiness already waited. | ||
| await expect |
There was a problem hiding this comment.
Two things about this poll.
.toBe(true) means a genuine regression reports Expected: true / Received: false after 30s — no status, no body. The ticket already notes there are no screenshots because nothing ran, so the failure string is the only evidence the next occurrence produces.
And #5083 already solves this on main with probeHealthcheck returning {ok, detail} and waitForRhdhReady rethrowing RHDH not ready within ${timeoutMs}ms: ${lastDetail} — so we'd land two different shapes of the same fix and have to reconcile them at cherry-pick time. Could we mirror it here?
let lastDetail = "no response yet";
await expect
.poll(
async () => {
try {
const response = await request.get("/healthcheck");
if (response.status() !== 200) {
lastDetail = `HTTP ${response.status()}`;
return false;
}
const body: unknown = await response.json();
const ok =
typeof body === "object" &&
body !== null &&
Reflect.get(body, "status") === "ok";
lastDetail = ok ? "status ok" : "HTTP 200 unexpected body";
return ok;
} catch (error) {
lastDetail = `request failed: ${error instanceof Error ? error.message : String(error)}`;
return false;
}
},
{ timeout: 30_000, intervals: [2_000] },
)
.toBe(true);(The try wrapping the whole request is the right call — expect.poll runs await actual() outside its own try/catch, so a transport throw aborts the poll instead of retrying. That's the actual root cause in the ticket.)
| async () => { | ||
| try { | ||
| const response = await request.get("/healthcheck"); | ||
| if (response.status() !== 200) { |
There was a problem hiding this comment.
nit: main's wait-for-rhdh-ready.ts — and #5083's rewrite of it — also checks that content-type contains json before parsing, which guards against a proxy returning an HTML error page with a 200. Worth matching so the 1.10 and main versions don't drift further apart.
2 participants
Summary
GET /healthcheck(notHEAD /) intesting::check_backstage_runningso CI readiness matches what Playwright uses and retries through transient TLS/connect failures./healthcheckfor up to ~2 minutes immediately beforeyarn playwright testafter yarn install.instance-health-check.spec.tswithexpect.pollso a single TLS socket disconnect does not fail the test (RHDHBUGS-3508).5 runs in a row succeeded: https://prow.ci.openshift.org/job-history/gs/test-platform-results/pr-logs/directory/pull-ci-redhat-developer-rhdh-release-1.10-e2e-gke-helm-nightly
Test plan
bash -n .ci/pipelines/lib/testing.sh/healthcheckTLS disconnectsMade with Cursor