OCPBUGS-98643: Add readiness probes to konnectivity proxy sidecars#9051
OCPBUGS-98643: Add readiness probes to konnectivity proxy sidecars#9051hypershift-jira-solve-ci[bot] wants to merge 2 commits into
Conversation
Without readiness probes, Kubernetes considers the konnectivity proxy sidecar containers ready immediately upon start. This causes the parent pod to report Ready before the HTTP/SOCKS5 listener is accepting connections, leading to HostedCluster Available=True being set prematurely while proxy-dependent components cannot reach the guest cluster network. Add TCP socket readiness probes on the serving port so pods are not marked Ready until the proxy listener is operational. Signed-off-by: OpenShift CI Bot <ci-bot@redhat.com>
Use servingPort() for --serving-port CLI arg construction to eliminate duplication with readiness probe port resolution. Add explicit default case to servingPort() for consistency. Replace magic number 8090 with defaultKonnectivityServingPort constant in tests. Add Dual-mode readiness probe assertions covering the OAuth-like custom port scenario. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/valid-reference
|
@hypershift-jira-solve-ci[bot]: This pull request references Jira Issue OCPBUGS-98643, 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. |
📝 WalkthroughWalkthroughKonnectivity container construction now resolves a configured HTTPS or Socks5 serving port, falling back to port 8090. The resolved port is used for the proxy argument and a TCP readiness probe with five-second initial delay and period. Tests cover default and custom ports for single-mode and dual-mode container configurations. Sequence Diagram(s)sequenceDiagram
participant buildContainer
participant servingPort
participant KonnectivityContainer
buildContainer->>servingPort: resolve configured or default port
servingPort-->>buildContainer: return serving port
buildContainer->>KonnectivityContainer: configure serving argument and TCP readiness probe
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/control-plane-operator
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: hypershift-jira-solve-ci[bot] 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
support/controlplane-component/konnectivity-container.go (1)
200-201: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winName the readiness-probe timing contract.
support/controlplane-component/konnectivity-container.go#L200-L201: replace5literals with named timing constants.support/controlplane-component/konnectivity-container_test.go#L197-L198: use named test expectation constants so the asserted values remain explicit.As per coding guidelines, “Avoid magic numbers — use named constants.”
🤖 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 `@support/controlplane-component/konnectivity-container.go` around lines 200 - 201, Replace the readiness probe’s literal InitialDelaySeconds and PeriodSeconds values in support/controlplane-component/konnectivity-container.go:200-201 with named timing constants. Update the corresponding expectations in support/controlplane-component/konnectivity-container_test.go:197-198 to use named test constants, keeping the asserted values explicit and consistent.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@support/controlplane-component/konnectivity-container.go`:
- Around line 200-201: Replace the readiness probe’s literal InitialDelaySeconds
and PeriodSeconds values in
support/controlplane-component/konnectivity-container.go:200-201 with named
timing constants. Update the corresponding expectations in
support/controlplane-component/konnectivity-container_test.go:197-198 to use
named test constants, keeping the asserted values explicit and consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ee64cc1e-a923-4fa3-9fe8-06e5ceff0bc9
📒 Files selected for processing (2)
support/controlplane-component/konnectivity-container.gosupport/controlplane-component/konnectivity-container_test.go
|
@hypershift-jira-solve-ci[bot]: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9051 +/- ##
==========================================
+ Coverage 44.44% 44.46% +0.01%
==========================================
Files 774 774
Lines 96977 96995 +18
==========================================
+ Hits 43105 43127 +22
+ Misses 50897 50895 -2
+ Partials 2975 2973 -2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
You need to regenerate fixtures with |
2 participants
What this PR does / why we need it:
Adds TCP socket readiness probes to the konnectivity proxy sidecar containers. Without readiness probes, Kubernetes considers these containers ready immediately upon start, causing the parent pod to report Ready before the HTTP/SOCKS5 listener is actually accepting connections. This leads to
HostedCluster Available=Truebeing set prematurely while proxy-dependent components cannot yet reach the guest cluster network.The fix adds TCP socket readiness probes on the serving port so pods are not marked Ready until the proxy listener is operational. It also consolidates the serving port logic by reusing
servingPort()for--serving-portCLI argument construction, eliminating duplication with the readiness probe port resolution, and improves test coverage with dual-mode readiness probe assertions.Which issue(s) this PR fixes:
Fixes https://redhat.atlassian.net/browse/OCPBUGS-98643
Special notes for your reviewer:
servingPort()now has an explicit default case for consistency.8090in tests has been replaced with thedefaultKonnectivityServingPortconstant.Checklist:
Always review AI generated responses prior to use.
Generated with Claude Code via openshift-developer plugin
Summary by CodeRabbit