[rhoai-3.5] fix(tests): bound exposing_contextmanager's portforward retry loop - #2692
Open
jiridanek wants to merge 1 commit into
Open
[rhoai-3.5] fix(tests): bound exposing_contextmanager's portforward retry loop#2692jiridanek wants to merge 1 commit into
jiridanek wants to merge 1 commit into
Conversation
exposing_contextmanager() retried kubernetes.stream.portforward() forever with no deadline and no give-up condition. Because SocketProxy handles one client synchronously, a single stuck iteration (e.g. a wedged connection to the API server -- the kubernetes client doesn't expose a per-call connect timeout for portforward()) blocks the entire proxy, starving every later Wait.until retry with no way to recover. Add a bounded deadline (default 30s) that raises TimeoutError once exceeded, and catch it in SocketProxy.listen_and_serve_until_canceled() alongside the existing BrokenPipeError/ConnectionResetError handling so the proxy gives up on just that one connection and keeps serving future retries instead of the exception escaping to the outer handler and killing the whole proxy thread. The deadline check alone only runs between retry loop iterations, so a single portforward() call that itself hangs would never let it fire again -- bound that specific call too via a new _portforward_with_timeout() helper, which runs it in a daemon thread bounded by join(timeout). On timeout the call is abandoned (thread/connection leaked for the remainder of the test process) rather than joined; deliberately not using a process-wide websocket.setdefaulttimeout() since multiple SocketProxy instances can call portforward() concurrently from different threads. Straight port of #2691 (rhoai-2.25) and opendatahub-io#4273 (main), where this was implemented and CI-verified first. #2684
|
[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 |
📝 WalkthroughWalkthroughKubernetes port forwarding now has per-attempt and overall deadlines. Resource cleanup occurs when the deadline expires. Socket proxy retry handling now includes ChangesKubernetes timeout handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant exposing_contextmanager
participant _portforward_with_timeout
participant kubernetes_stream
participant listen_and_serve_until_canceled
exposing_contextmanager->>_portforward_with_timeout: request port-forward with remaining timeout
_portforward_with_timeout->>kubernetes_stream: start portforward()
kubernetes_stream-->>_portforward_with_timeout: port-forward or TimeoutError
_portforward_with_timeout-->>exposing_contextmanager: return handle or raise error
listen_and_serve_until_canceled-->>listen_and_serve_until_canceled: log timeout and retry
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
CI status [antigravity]Run: Build Notebooks (pr) #30767060790 — 3/3 complete · 1 passed · 2 skipped No workbench image jobs ran; all matrix jobs were skipped. |
📋 Review SummaryThis PR correctly implements a robustness fix by bounding the 🔍 General Feedback
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/containers/kubernetes_utils.py`:
- Around line 526-543: Update the retry loop around _portforward_with_timeout
and pf.socket(8888) so socket creation failures close the newly assigned pf
before propagating the exception. Preserve the existing retry cleanup and
timeout behavior for successful socket creation.
🪄 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 YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 102fc22e-024e-4409-9175-0c7e4acd2f11
📒 Files selected for processing (2)
tests/containers/kubernetes_utils.pytests/containers/socket_proxy.py
Comment on lines
+526
to
543
| deadline = time.monotonic() + timeout | ||
| pf: kubernetes.stream.ws_client.PortForward | None = None | ||
| s = None | ||
| while not pf or not pf.connected or not s: | ||
| remaining = deadline - time.monotonic() | ||
| if remaining <= 0: | ||
| if s is not None: | ||
| s.close() | ||
| if pf is not None: | ||
| pf.close() | ||
| raise TimeoutError(f"Failed to establish a working portforward to {pod.metadata.name} within {timeout}s") | ||
| if s is not None: | ||
| s.close() | ||
| s = None | ||
| if pf is not None: | ||
| pf.close() | ||
| pf = kubernetes.stream.portforward( | ||
| api_method=core_v1_api.connect_get_namespaced_pod_portforward, | ||
| name=pod.metadata.name, | ||
| namespace=pod.metadata.namespace, | ||
| ports=",".join(str(p) for p in [8888]), | ||
| ) | ||
| pf = _portforward_with_timeout(core_v1_api, pod, remaining) | ||
| s = pf.socket(8888) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Close pf if pf.socket(8888) fails.
This loop closes the previous pf/s before every retry and on deadline expiry, but not if pf.socket(8888) itself raises right after a successful _portforward_with_timeout() call. In that case, the exception propagates out of the generator immediately, before the next iteration's cleanup or the try/finally around yield can run. The newly created pf (and its underlying websocket) leaks for the rest of the test process.
Wrap the socket creation so failure closes pf first.
🔧 Proposed fix
pf = _portforward_with_timeout(core_v1_api, pod, remaining)
- s = pf.socket(8888)
+ try:
+ s = pf.socket(8888)
+ except Exception:
+ pf.close()
+ raise📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| deadline = time.monotonic() + timeout | |
| pf: kubernetes.stream.ws_client.PortForward | None = None | |
| s = None | |
| while not pf or not pf.connected or not s: | |
| remaining = deadline - time.monotonic() | |
| if remaining <= 0: | |
| if s is not None: | |
| s.close() | |
| if pf is not None: | |
| pf.close() | |
| raise TimeoutError(f"Failed to establish a working portforward to {pod.metadata.name} within {timeout}s") | |
| if s is not None: | |
| s.close() | |
| s = None | |
| if pf is not None: | |
| pf.close() | |
| pf = kubernetes.stream.portforward( | |
| api_method=core_v1_api.connect_get_namespaced_pod_portforward, | |
| name=pod.metadata.name, | |
| namespace=pod.metadata.namespace, | |
| ports=",".join(str(p) for p in [8888]), | |
| ) | |
| pf = _portforward_with_timeout(core_v1_api, pod, remaining) | |
| s = pf.socket(8888) | |
| deadline = time.monotonic() + timeout | |
| pf: kubernetes.stream.ws_client.PortForward | None = None | |
| s = None | |
| while not pf or not pf.connected or not s: | |
| remaining = deadline - time.monotonic() | |
| if remaining <= 0: | |
| if s is not None: | |
| s.close() | |
| if pf is not None: | |
| pf.close() | |
| raise TimeoutError(f"Failed to establish a working portforward to {pod.metadata.name} within {timeout}s") | |
| if s is not None: | |
| s.close() | |
| s = None | |
| if pf is not None: | |
| pf.close() | |
| pf = _portforward_with_timeout(core_v1_api, pod, remaining) | |
| try: | |
| s = pf.socket(8888) | |
| except Exception: | |
| pf.close() | |
| raise |
🤖 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 `@tests/containers/kubernetes_utils.py` around lines 526 - 543, Update the
retry loop around _portforward_with_timeout and pf.socket(8888) so socket
creation failures close the newly assigned pf before propagating the exception.
Preserve the existing retry cleanup and timeout behavior for successful socket
creation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements "Fix #2" from the suggested (not-yet-implemented) hardening ideas in
#2684: bound
exposing_contextmanager()'skubernetes.stream.portforward()retry loop intests/containers/kubernetes_utils.py.Why
exposing_contextmanager()retriedportforward()in awhile not pf or not pf.connected or not s:loop with no deadline and no give-upcondition. Verified via the
kubernetesclient library source that the underlyingwebsocket.connect(...)call has no per-call timeout plumbed throughportforward()'spublic API -- a single iteration can, in principle, hang forever (e.g. a wedged
connection to the API server), not just retry quickly.
Because
SocketProxy.listen_and_serve_until_canceled()handles one clientsynchronously, a stuck call here blocks the entire proxy -- no other
Wait.untilretry can ever be serviced again for that pod, with no way to recover.
Changes
exposing_contextmanager()gains atimeout: float = 30parameter. Once exceeded,cleans up any partial
s/pfand raisesTimeoutError.portforward()call that itself hangs would never let it fire again -- added_portforward_with_timeout(), which runs that specific call in a daemon threadbounded by
join(timeout). On timeout the call is abandoned (thread/connectionleaked for the remainder of the test process) rather than joined; deliberately not
using a process-wide
websocket.setdefaulttimeout()since multipleSocketProxyinstances (one per
ImageDeployment) can callportforward()concurrently fromdifferent threads.
SocketProxy.listen_and_serve_until_canceled()now also catches the newTimeoutErroralongside the existingBrokenPipeError/ConnectionResetError, sothe proxy gives up on just that one connection and keeps serving future retries.
This is a mechanical, identical port of
red-hat-data-services/notebooks#2691
(
rhoai-2.25) and opendatahub-io/notebooks#4273(
main) --rhoai-3.5'stests/containers/kubernetes_utils.py/socket_proxy.pywere confirmed byte-identical to
main's pre-change state (modulo one unrelatedlogging.basicConfigline), where this was already implemented and CI-verified,including a CodeRabbit review catching a real gap in the first iteration (the
single-hung-call issue this PR's
_portforward_with_timeout()addresses).Verification
ast.parse,uv run ruff check,uv run ruff format --check,uv run pyrightall clean on both files.
rhoai-2.25(PR fix(tests): probe / instead of /api for OpenShift port-forward readiness #2685) and
main(ci(test-containers): add a job to run the openshift-marked container tests opendatahub-io/notebooks#4259),rhoai-3.5'stest-containers.yamldoes not have anopenshift-container-testsjob -- itwas never ported here. So this PR's own CI does not exercise this exact code
path (confirmed: no
openshift:matrix legs run on this PR, only the regularcontainer-testsjob, which excludesopenshift-marked tests). Verification hereis limited to the static checks above plus local functional testing of three
scenarios against this exact code (a
portforward()call that hangs forever ->correctly bounded; a pod that fails fast repeatedly -> unaffected; one that
succeeds on a later retry -> unaffected). Porting the
openshift-container-testsjob to
rhoai-3.5(mirroring fix(tests): probe / instead of /api for OpenShift port-forward readiness #2685/ci(test-containers): add a job to run the openshift-marked container tests opendatahub-io/notebooks#4259) would be a separate, follow-up PR ifwanted.