[DNM] OCPBUGS-100065: combined payload-testing: readyz gating + aggregator h2 health checking - #2734
[DNM] OCPBUGS-100065: combined payload-testing: readyz gating + aggregator h2 health checking#2734mkowalski wants to merge 2 commits into
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
…ckend connections The aggregator proxies requests to aggregated apiservers over pooled http2 connections. When such a connection is silently broken - for instance when it was established while the pod network on a freshly rebooted control plane node was still converging - the default http2 health check parameters (ReadIdleTimeout=30s, PingTimeout=15s) keep the dead connection pinned for up to ~45 seconds while every request multiplexed onto it fails with 503 'error trying to reach service: http2: client connection lost'. Observed as 10-15s of oauth-api/openshift-api new-connection disruption during metal-ipi upgrade jobs, with a residual episode remaining even after the aggregated apiserver readyz reachability check was strengthened, because connections can break after readiness. Configure the aggregator's backend proxy transport with aggressive http2 connection health checking (ReadIdleTimeout=5s, PingTimeout=5s) so broken connections are detected and dropped within seconds. Aggregated apiservers are same-cluster backends with sub-second round trips, so a connection that cannot answer a ping for a few seconds is broken for practical purposes and re-dialing is cheap. The construction mirrors client-go transport.New (including the config wrappers, so the x509 metrics wrapper still applies) and falls back to transport.New on any unexpected configuration. Assisted-By: Claude Fable 5 (cherry picked from commit 0403774)
openshift-ci-robot
added
the
backports/unvalidated-commits
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
jira/valid-reference
|
@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. |
|
/payload-aggregate periodic-ci-openshift-release-main-nightly-5.0-e2e-metal-ipi-upgrade-ovn-ipv6 10 |
|
@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 |
|
@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/ac8e7790-8c56-11f1-841a-01b252ab71ee-0 |
|
[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 |
WalkthroughThe PR strengthens SDN aggregated API readiness by probing every endpoint across consecutive polls. It also adds a specialized kube-aggregator HTTP/2 backend transport with aggressive health-check timeouts, fallback behavior, wrapper support, and tests. ChangesSDN readiness probing
Aggregated API backend transport
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ReadinessPoller
participant allEndpointsReachable
participant AggregatedAPIEndpoints
ReadinessPoller->>allEndpointsReachable: poll endpoint addresses
allEndpointsReachable->>AggregatedAPIEndpoints: HTTPS GET each endpoint
AggregatedAPIEndpoints-->>allEndpointsReachable: responses or connection failures
allEndpointsReachable-->>ReadinessPoller: complete reachability result
ReadinessPoller->>ReadinessPoller: track consecutive successes
ReadinessPoller-->>ReadinessPoller: signal readiness after threshold
sequenceDiagram
participant updateAPIService
participant newAggregatedAPIBackendRoundTripper
participant HTTP2Transport
participant APIServiceBackend
updateAPIService->>newAggregatedAPIBackendRoundTripper: construct backend RoundTripper
newAggregatedAPIBackendRoundTripper->>HTTP2Transport: configure TLS and health checks
HTTP2Transport-->>newAggregatedAPIBackendRoundTripper: configured transport
newAggregatedAPIBackendRoundTripper-->>updateAPIService: wrapped RoundTripper
updateAPIService->>APIServiceBackend: proxy requests through transport
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (14 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
🧹 Nitpick comments (1)
staging/src/k8s.io/kube-aggregator/pkg/apiserver/patch_backend_transport_test.go (1)
11-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise HTTP/2, not just HTTPS.
This passes even if the transport falls back to HTTP/1.1, so it does not protect the new HTTP/2 health-check path. Start an HTTP/2-enabled test server and assert
resp.ProtoMajor == 2.Proposed test update
- server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + server := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) })) + server.EnableHTTP2 = true + server.StartTLS() defer server.Close() @@ if resp.StatusCode != http.StatusOK { t.Errorf("expected 200, got %d", resp.StatusCode) } + if resp.ProtoMajor != 2 { + t.Errorf("expected HTTP/2, got %s", resp.Proto) + }🤖 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 `@staging/src/k8s.io/kube-aggregator/pkg/apiserver/patch_backend_transport_test.go` around lines 11 - 36, Update TestNewAggregatedAPIBackendRoundTripperServesRequests to use an HTTP/2-enabled TLS test server rather than a generic HTTPS server, then assert the returned response has ProtoMajor equal to 2 in addition to the existing status check. Keep the current request and error-handling coverage intact.
🤖 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: Handle all URL parsing, host/port splitting, and listener
close errors in the down-endpoint fixture setup: at lines 61-62 and 81-82 of
openshift-kube-apiserver/openshiftkubeapiserver/sdn_readyz_wait_test.go, check
each error and call t.Fatalf with context instead of ignoring failures. Ensure
invalid fixture endpoints cannot be used and listener.Close errors are reported.
In `@openshift-kube-apiserver/openshiftkubeapiserver/sdn_readyz_wait.go`:
- Around line 176-182: Update the HTTP transport used by the reachability probe
in allEndpointsReachable to set DisableKeepAlives: true, ensuring each poll
establishes a fresh connection. Add a regression test for the probe that
verifies successive polls force distinct dials.
---
Nitpick comments:
In
`@staging/src/k8s.io/kube-aggregator/pkg/apiserver/patch_backend_transport_test.go`:
- Around line 11-36: Update
TestNewAggregatedAPIBackendRoundTripperServesRequests to use an HTTP/2-enabled
TLS test server rather than a generic HTTPS server, then assert the returned
response has ProtoMajor equal to 2 in addition to the existing status check.
Keep the current request and error-handling coverage intact.
🪄 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: 575daf72-024c-4b8f-9c2d-68a50657238a
📒 Files selected for processing (5)
openshift-kube-apiserver/openshiftkubeapiserver/sdn_readyz_wait.goopenshift-kube-apiserver/openshiftkubeapiserver/sdn_readyz_wait_test.gostaging/src/k8s.io/kube-aggregator/pkg/apiserver/handler_proxy.gostaging/src/k8s.io/kube-aggregator/pkg/apiserver/patch_backend_transport.gostaging/src/k8s.io/kube-aggregator/pkg/apiserver/patch_backend_transport_test.go
| u, _ := url.Parse(server.URL) | ||
| host, port, _ := net.SplitHostPort(u.Host) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle failures while building the down-endpoint fixture.
Ignored parse/split/close errors can leave empty or invalid probe targets and produce misleading test results.
openshift-kube-apiserver/openshiftkubeapiserver/sdn_readyz_wait_test.go#L61-L62: fail witht.Fatalfwhen parsing the test-server URL or splitting its address fails.openshift-kube-apiserver/openshiftkubeapiserver/sdn_readyz_wait_test.go#L81-L82: check bothSplitHostPortandlistener.Closeerrors.
As per path instructions, “Never ignore error returns.”
📍 Affects 1 file
openshift-kube-apiserver/openshiftkubeapiserver/sdn_readyz_wait_test.go#L61-L62(this comment)openshift-kube-apiserver/openshiftkubeapiserver/sdn_readyz_wait_test.go#L81-L82
🤖 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, Handle all URL parsing, host/port splitting, and listener
close errors in the down-endpoint fixture setup: at lines 61-62 and 81-82 of
openshift-kube-apiserver/openshiftkubeapiserver/sdn_readyz_wait_test.go, check
each error and call t.Fatalf with context instead of ignoring failures. Ensure
invalid fixture endpoints cannot be used and listener.Close errors are reported.
Source: Path instructions
| if !allEndpointsReachable(&client, openshiftEndpoints, "8443") { | ||
| consecutiveSuccesses = 0 | ||
| return false, nil | ||
| } | ||
|
|
||
| return false, nil | ||
| consecutiveSuccesses++ | ||
| if consecutiveSuccesses < requiredConsecutiveSuccesses { | ||
| return false, nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- file outline ---'
ast-grep outline openshift-kube-apiserver/openshiftkubeapiserver/sdn_readyz_wait.go --view expanded || true
echo '--- relevant lines ---'
sed -n '1,260p' openshift-kube-apiserver/openshiftkubeapiserver/sdn_readyz_wait.go | cat -n
echo '--- search usages ---'
rg -n "allEndpointsReachable|requiredConsecutiveSuccesses|DisableKeepAlives|Transport: &http.Transport|http.Client" openshift-kube-apiserver -SRepository: openshift/kubernetes
Length of output: 13990
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- test file outline ---'
ast-grep outline openshift-kube-apiserver/openshiftkubeapiserver/sdn_readyz_wait_test.go --view expanded || true
echo '--- relevant test lines ---'
sed -n '1,220p' openshift-kube-apiserver/openshiftkubeapiserver/sdn_readyz_wait_test.go | cat -n
echo '--- connection reuse references ---'
rg -n "KeepAlives|IdleConn|Connection: close|Transport:|httptest|RoundTripper|DialContext|allEndpointsReachable" openshift-kube-apiserver/openshiftkubeapiserver/sdn_readyz_wait_test.go openshift-kube-apiserver/openshiftkubeapiserver/sdn_readyz_wait.go -SRepository: openshift/kubernetes
Length of output: 6719
Disable keep-alives for the reachability probe The single http.Transport can reuse one TLS connection across poll iterations, so three “successful” polls may only prove one socket stayed open. Set DisableKeepAlives: true on this probe transport and add a regression test that forces distinct dials.
🤖 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 176 - 182, Update the HTTP transport used by the reachability probe in
allEndpointsReachable to set DisableKeepAlives: true, ensuring each poll
establishes a fresh connection. Add a regression test for the probe that
verifies successive polls force distinct dials.
|
Payload testing complete (results posted on #2732). Closing the test vehicle. |
|
@mkowalski: This pull request references Jira Issue OCPBUGS-100065. The bug has been updated to no longer 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. |
2 participants
Summary
Draft / DO NOT MERGE — payload-testing vehicle only.
Combines #2730 (readyz: require all aggregated apiserver endpoints reachable) and #2732 (kube-aggregator: fast http2 health checking) to validate both halves of the OCPBUGS-100065 fix together, since
/payload-aggregate-with-prson #2732 is not being processed (aggregator infra issues).Will be closed after results are collected; the individual PRs remain the merge vehicles.
This PR was generated using AI. Please verify before acting on it.
Summary by CodeRabbit
Reliability Improvements
Bug Fixes
Tests