OCPBUGS-100065: UPSTREAM: <carry>: require all aggregated apiserver endpoints reachable in readyz check - #2730
Conversation
…le in readyz check The api-openshift-apiserver-available and api-openshift-oauth-apiserver-available readyz checks previously latched complete on the first successful connection to any single endpoint of the aggregated apiserver service. On a freshly rebooted master the pod network may still be converging (OVN flow/route programming) when kube-apiserver starts, so a lucky one-off connection could mark the apiserver ready while connectivity to the remaining endpoints was still broken. Once the external load balancer follows readyz and routes traffic to this instance, its aggregator proxies requests over connections established during the blackhole window, and the pinned http2 connections produce 503 'error trying to reach service: http2: client connection lost' plus header timeouts for tens of seconds - observed as 10-15s of oauth-api/openshift-api new-connection disruption on metal-ipi upgrade jobs (~30-50% of master-updating runs). Require every listed ready endpoint address to be reachable on three consecutive polls before reporting the check complete. The existing escape hatches are unchanged: missing endpoints object still completes immediately, and the 60 second overall timeout still reports ready no matter what to avoid a rebootstrapping deadlock. Add unit tests for the new allEndpointsReachable helper. Assisted-By: Claude Fable 5
openshift-ci-robot
added
backports/unvalidated-commits
|
@mkowalski: This pull request references Jira Issue OCPBUGS-100065, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@mkowalski: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
WalkthroughThe SDN aggregated API readiness check now probes all endpoint addresses and requires multiple consecutive successful polls. Tests cover endpoint reachability, arbitrary HTTP responses, failed endpoints, and empty endpoint lists. ChangesSDN readiness validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ReadinessPollLoop
participant allEndpointsReachable
participant AggregatedAPIEndpoint
loop Each poll
ReadinessPollLoop->>allEndpointsReachable: Check all endpoint addresses
allEndpointsReachable->>AggregatedAPIEndpoint: HTTPS GET on port 8443
AggregatedAPIEndpoint-->>allEndpointsReachable: HTTP response or connection failure
allEndpointsReachable-->>ReadinessPollLoop: All reachable or failure
end
ReadinessPollLoop-->>ReadinessPollLoop: Reset or increment consecutive successes
ReadinessPollLoop-->>ReadinessPollLoop: Signal readiness after threshold
🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@mkowalski: This pull request references Jira Issue OCPBUGS-100065, which is invalid:
Comment 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 openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openshift-kube-apiserver/openshiftkubeapiserver/sdn_readyz_wait.go (1)
152-159: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not follow redirects in the readiness probe.
http.Clientwill chase 3xx responses, so a healthy endpoint that redirects to an unavailable target is treated as down and resets the readiness streak. SetCheckRedirectto returnhttp.ErrUseLastResponseand add a 302 regression test.🤖 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 `@openshift-kube-apiserver/openshiftkubeapiserver/sdn_readyz_wait.go` around lines 152 - 159, Update the http.Client construction in the readiness probe to set CheckRedirect so redirects return http.ErrUseLastResponse instead of being followed. Add a regression test covering a 302 response and verify the probe treats the original endpoint as healthy without contacting the redirected target.
🤖 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.
Inline comments:
In `@openshift-kube-apiserver/openshiftkubeapiserver/sdn_readyz_wait_test.go`:
- Around line 61-62: Update the test setup around url.Parse, net.SplitHostPort,
and the server cleanup to handle each returned error explicitly. Fail the test
immediately with useful context when URL parsing or host/port splitting fails,
and ensure server.Close errors are checked and reported so cleanup failures
cannot be ignored.
In `@openshift-kube-apiserver/openshiftkubeapiserver/sdn_readyz_wait.go`:
- Around line 46-48: Update the response handling near the readiness probe to
stop calling httputil.DumpResponse with body dumping; log resp.Status directly
in the existing klog.V(4) message, and check and handle any error returned by
resp.Body.Close().
---
Outside diff comments:
In `@openshift-kube-apiserver/openshiftkubeapiserver/sdn_readyz_wait.go`:
- Around line 152-159: Update the http.Client construction in the readiness
probe to set CheckRedirect so redirects return http.ErrUseLastResponse instead
of being followed. Add a regression test covering a 302 response and verify the
probe treats the original endpoint as healthy without contacting the redirected
target.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1fc8d5ef-ba01-47d5-bb2b-a9c4b62f90c4
📒 Files selected for processing (2)
openshift-kube-apiserver/openshiftkubeapiserver/sdn_readyz_wait.goopenshift-kube-apiserver/openshiftkubeapiserver/sdn_readyz_wait_test.go
| u, _ := url.Parse(server.URL) | ||
| host, port, _ := net.SplitHostPort(u.Host) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 2 'url\.Parse\(server\.URL\)|net\.SplitHostPort\(u\.Host\)|listener\.Close\(\)' \
openshift-kube-apiserver/openshiftkubeapiserver/sdn_readyz_wait_test.goRepository: openshift/kubernetes
Length of output: 819
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,140p' openshift-kube-apiserver/openshiftkubeapiserver/sdn_readyz_wait_test.go | cat -nRepository: openshift/kubernetes
Length of output: 4103
Handle the ignored URL, host/port, and Close errors here. Dropping these return values can turn a setup/cleanup failure into a misleading test result or leave the listener open.
🤖 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 `@openshift-kube-apiserver/openshiftkubeapiserver/sdn_readyz_wait_test.go`
around lines 61 - 62, Update the test setup around url.Parse, net.SplitHostPort,
and the server cleanup to handle each returned error explicitly. Fail the test
immediately with useful context when URL parsing or host/port splitting fails,
and ensure server.Close errors are checked and reported so cleanup failures
cannot be ignored.
Source: Path instructions
| response, dumpErr := httputil.DumpResponse(resp, true) | ||
| klog.V(4).Infof("reached to connect to %q: %v\n%v", url, dumpErr, string(response)) | ||
| resp.Body.Close() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 'DumpResponse|resp\.Body\.Close|connected to' \
openshift-kube-apiserver/openshiftkubeapiserver/sdn_readyz_wait.goRepository: openshift/kubernetes
Length of output: 466
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="openshift-kube-apiserver/openshiftkubeapiserver/sdn_readyz_wait.go"
wc -l "$FILE"
sed -n '1,140p' "$FILE"Repository: openshift/kubernetes
Length of output: 6164
Avoid dumping the full response body here. DumpResponse(resp, true) still drains and allocates the entire body before the V(4) log call, so a slow or large response can delay readiness probes unnecessarily. Log resp.Status instead and handle resp.Body.Close() errors.
🤖 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 `@openshift-kube-apiserver/openshiftkubeapiserver/sdn_readyz_wait.go` around
lines 46 - 48, Update the response handling near the readiness probe to stop
calling httputil.DumpResponse with body dumping; log resp.Status directly in the
existing klog.V(4) message, and check and handle any error returned by
resp.Body.Close().
Source: Path instructions
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mkowalski 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 |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-metal-ipi-upgrade-ovn-ipv6 |
|
@mkowalski: trigger 2 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/18ec7f00-8ab2-11f1-8658-12de802f0584-0 |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-metal-ipi-upgrade-ovn-ipv6 |
|
@mkowalski: trigger 2 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/51fefa00-8abe-11f1-92e8-b43b13b56af5-0 |
|
Payload test results (2 rounds, 3 of 4 runs complete, all rebooted 3/3 masters = valid trials for OCPBUGS-100065):
* both runc failures are Baseline for comparison: ~30-50% of master-updating runs showed 10-15s disruption with episodes starting 39-74s after a master reboot. New code path confirmed active on all masters: 3 clean trials is suggestive but not conclusive for a ~30-50% stochastic race — will run /payload-aggregate for statistical confidence before merge. This comment was generated using AI. Please verify before acting on it. |
|
/payload-aggregate periodic-ci-openshift-release-main-nightly-5.0-e2e-metal-ipi-upgrade-ovn-ipv6 10 |
|
@mkowalski: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/7d56f560-8ae9-11f1-9107-2e8ae1456132-0 |
|
Aggregate results (10 runs + 1 earlier, all valid master-updating trials): oauth-api-new-connections totals: 0, 0, 0, 0, 0, 1, 1, 2, 3, 5, 12 seconds — vs baseline where ~31-50% of runs hit 10-15s. The readiness gate works as designed: in the one 12s outlier (run 2082271087302807552) the check correctly held readiness back through 20+ failed probes ( Net: median disruption drops from ~5-10s to ~1s, >=10s incidence from ~31-50% to 1/11. The residual case confirms the complementary fix (h2 ping-based dead-connection detection on aggregator transports, OCPBUGS-100065 suggestion #2) is required to reach a strict zero. This PR stands on its own as a substantial reduction and is the correct-by-construction half of the fix. This comment was generated using AI. Please verify before acting on it. |
|
@mkowalski: This PR was included in a payload test run from #2732
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/5f31a690-8b50-11f1-893d-e74f5bc1047d-0 |
2 participants
Summary
Fixes OCPBUGS-100065: 10-15s of
oauth-api-new-connections/openshift-api-new-connectionsdisruption during metal-ipi upgrades (~30-50% of master-updating runs).api-openshift-apiserver-available/api-openshift-oauth-apiserver-availablereadyz checks (sdn_readyz_wait.go) latched complete on the first successful connection to any single endpoint of the aggregated apiserver service. On a freshly rebooted master, kube-apiserver starts while OVN is still converging; connectivity flaps, and a lucky one-off connection marks the instance ready (observed:reached openshift-oauth-apiserver via SDN after 13067 millisecondsat 08:30:33 while the availability controller loggedcontext deadline exceededto another endpoint 3s earlier — run 2081616371841503232)./readyzand routes ~1/3 of new connections to the instance; aggregator requests ride http2 connections established during the blackhole window and fail with503 error trying to reach service: http2: client connection lost+ header timeouts for 40-70s until the transport declares the connections dead. Reproduced with identical signature in run 2080091718912315392; 10/10 analyzed runs with >=10s disruption show an episode starting 39-74s after a masterRebootedevent.allEndpointsReachablehelper.Test plan
gofmt,go vet,go build ./openshift-kube-apiserver/...go test ./openshift-kube-apiserver/openshiftkubeapiserver/(new + existing tests pass)This PR was generated using AI. Please verify before acting on it.
Summary by CodeRabbit
Bug Fixes
Tests