[rhoai-2.25] fix(tests): bound exposing_contextmanager's portforward retry loop - #2691
[rhoai-2.25] fix(tests): bound exposing_contextmanager's portforward retry loop#2691jiridanek wants to merge 4 commits 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, well under the outer Wait.until 120s budget) 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. Also adds the s.close()/pf.close()-before-retry cleanup between iterations (previously each failed attempt leaked the prior PortForward/socket). #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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
ChangesPort-forward timeout handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
CI status [antigravity]Run: Build Notebooks (pr) #30764993898 — 2/2 complete · 1 passed · 1 skipped No workbench image jobs ran; all matrix jobs were skipped. |
📋 Review SummaryThis PR successfully implements timeout and cleanup hardening for the Kubernetes port-forward retry loop in 🔍 General Feedback
I have no inline comments to post, as the changes are clean, correct, and well-thought-out. I did not post any inline review comments. |
![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 524-545: The port-forward retry loop can block inside
kubernetes.stream.portforward beyond its deadline. Update the connection setup
around pf and pf.socket in the existing loop to apply a cancellable or
per-connection WebSocket timeout, without modifying process-wide WebSocket
settings, and re-check the deadline after the attempt returns before accepting
the connection.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 099ce4af-2db3-4aed-84ed-6f1992b87747
📒 Files selected for processing (2)
tests/containers/kubernetes_utils.pytests/containers/socket_proxy.py
The log message wrapping the caught exception said "likely due to service not being ready", which was accurate for the pre-existing BrokenPipeError/ ConnectionResetError cases but not necessarily for the new TimeoutError (could also mean a genuinely wedged portforward() call, not just a slow-starting pod). Reworded to state what's actually always true instead: the proxy gives up on this one connection and the client will retry.
CI status [antigravity]Run: Build Notebooks (push) #30765447068 — 2/2 complete · 2 skipped Workflow completed with skipped jobs. |
CI status [antigravity]Run: Build Notebooks (pr) #30765449231 — 2/2 complete · 1 passed · 1 skipped No workbench image jobs ran; all matrix jobs were skipped. |
CodeRabbit correctly flagged that the deadline check only ran between retry loop iterations: a single kubernetes.stream.portforward() call that itself hangs (the primary scenario this fix targets, e.g. a wedged connection to the API server) would never let the loop's own deadline check run again, silently defeating the bound entirely. The kubernetes client doesn't expose a per-call connect timeout for portforward(), and a process-wide websocket.setdefaulttimeout() isn't safe here (multiple SocketProxy instances can call portforward() concurrently from different threads). Run the call in a helper thread and bound it with join(timeout) instead, via a new _portforward_with_timeout() -- on timeout the call is abandoned (thread/connection leaked for the remainder of the test process) rather than joined, which is an acceptable tradeoff for test infrastructure that's about to fail the test and exit soon after anyway. Verified locally against three scenarios: a portforward() call that hangs forever (previously: hung forever; now: bounded correctly), a pod that fails fast repeatedly (still converges to the loop's own deadline as before), and one that succeeds on a later retry (unaffected).
CI status [antigravity]Run: Build Notebooks (push) #30765871047 — 2/2 complete · 2 skipped Workflow completed with skipped jobs. |
CI status [antigravity]Run: Build Notebooks (pr) #30765873737 — 2/2 complete · 1 passed · 1 skipped No workbench image jobs ran; all matrix jobs were skipped. |
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 508-524: Update _portforward_with_timeout so timed-out
port-forward attempts are eventually closed even when callers catch TimeoutError
and retry. Track the helper thread’s late result and close any returned
PortForward, and ensure the abandoned worker or connection has a bounded
lifetime rather than leaking across readiness retries.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a0e43088-22ce-4e99-ac43-1de564e18cec
📒 Files selected for processing (1)
tests/containers/kubernetes_utils.py
| def _portforward_with_timeout( | ||
| core_v1_api: kubernetes.client.CoreV1Api, pod: kubernetes.client.models.V1Pod, timeout: float | ||
| ) -> kubernetes.stream.ws_client.PortForward: | ||
| """Runs kubernetes.stream.portforward() with a wall-clock bound. | ||
|
|
||
| The kubernetes client doesn't expose a per-call connect timeout for portforward() -- it calls | ||
| websocket.connect() with no timeout, so a wedged connection to the API server can hang this call | ||
| forever. The only alternative the library offers is a process-wide `websocket.setdefaulttimeout()`, | ||
| which isn't safe to use here since multiple SocketProxy instances (one per ImageDeployment) can be | ||
| making concurrent portforward() calls from different threads. So we run the call in a helper thread | ||
| and bound it with join(timeout) instead. | ||
|
|
||
| If the call doesn't return in time, it is abandoned rather than joined: the daemon thread and any | ||
| partially-established connection are leaked for the remainder of the test process. That's an | ||
| accepted tradeoff for test infrastructure that's about to raise TimeoutError and fail the test | ||
| (and the process will exit soon after) rather than hang forever with no recovery. | ||
| """ |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- callers of exposing_contextmanager and any outer retry (Wait.until) ---'
rg -n -C 5 'exposing_contextmanager|class Wait\b|def until\b' tests
echo '--- pytest fail-fast configuration (addopts, -x, maxfail) ---'
rg -n 'addopts|maxfail|-x\b' pyproject.toml pytest.ini tox.ini .github 2>/dev/null || trueRepository: red-hat-data-services/notebooks
Length of output: 5171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Wait.until implementation ---'
sed -n '380,455p' tests/containers/kubernetes_utils.py
printf '%s\n' '--- exposing_contextmanager and timeout helper ---'
sed -n '500,595p' tests/containers/kubernetes_utils.py
printf '%s\n' '--- SocketProxy retry loop and cleanup ---'
sed -n '1,135p' tests/containers/socket_proxy.py
printf '%s\n' '--- pytest configuration and workflow invocations ---'
cat pytest.ini
rg -n -C 3 'pytest(\s|$)|pytest\.|make test|make[[:space:]]+test' Makefile .github tests 2>/dev/null | head -200Repository: red-hat-data-services/notebooks
Length of output: 28040
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- remainder of Wait.until ---'
sed -n '430,510p' tests/containers/kubernetes_utils.py
printf '%s\n' '--- all Wait.until call sites ---'
rg -n -C 8 'Wait\.until\(' tests
printf '%s\n' '--- ImageDeployment setup and lifecycle around SocketProxy ---'
sed -n '210,285p' tests/containers/kubernetes_utils.py
printf '%s\n' '--- SocketProxy callers and client retry references ---'
rg -n -C 8 'get_actual_port|listen_and_serve_until_canceled|cancellation_token|remote_socket_factory|Wait\.until' tests/containersRepository: red-hat-data-services/notebooks
Length of output: 28122
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from __future__ import annotations
import ast
from pathlib import Path
utils = Path("tests/containers/kubernetes_utils.py").read_text()
proxy = Path("tests/containers/socket_proxy.py").read_text()
pytest_ini = Path("pytest.ini").read_text()
u = ast.parse(utils)
p = ast.parse(proxy)
def calls(tree: ast.AST, name: str) -> list[int]:
return [
node.lineno
for node in ast.walk(tree)
if isinstance(node, ast.Call)
and isinstance(node.func, ast.Attribute)
and node.func.attr == name
]
wait_until_calls = calls(u, "until")
portforward_calls = [
node.lineno
for node in ast.walk(u)
if isinstance(node, ast.Call)
and isinstance(node.func, ast.Attribute)
and node.func.attr == "portforward"
]
print(f"Wait.until call lines: {wait_until_calls}")
print(f"kubernetes.stream.portforward call lines: {portforward_calls}")
print(f"pytest fail-fast options present: {any(x in pytest_ini for x in ('-x', '--exitfirst', 'maxfail'))}")
for node in ast.walk(p):
if isinstance(node, ast.Try):
caught = [
handler.type.names[0]
for handler in node.handlers
if isinstance(handler.type, ast.Tuple)
for _ in [0]
]
if any(isinstance(handler.type, ast.Name) and handler.type.id == "TimeoutError" for handler in node.handlers):
print(f"SocketProxy catches TimeoutError at line {node.lineno}")
print(f" handler types: {caught or ['TimeoutError']}")
print(f" handler contains continue: {any(isinstance(x, ast.Continue) for h in node.handlers for x in ast.walk(h))}")
for node in ast.walk(u):
if isinstance(node, ast.FunctionDef) and node.name == "_portforward_with_timeout":
thread_starts = [
n.lineno for n in ast.walk(node)
if isinstance(n, ast.Call)
and isinstance(n.func, ast.Attribute)
and n.func.attr == "start"
]
joins = [
n.lineno for n in ast.walk(node)
if isinstance(n, ast.Call)
and isinstance(n.func, ast.Attribute)
and n.func.attr == "join"
]
raises = [
n.lineno for n in ast.walk(node)
if isinstance(n, ast.Raise)
]
print(f"_portforward_with_timeout starts threads at: {thread_starts}")
print(f"_portforward_with_timeout joins at: {joins}")
print(f"_portforward_with_timeout raises at: {raises}")
PYRepository: red-hat-data-services/notebooks
Length of output: 450
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
utils = Path("tests/containers/kubernetes_utils.py").read_text()
proxy = Path("tests/containers/socket_proxy.py").read_text()
pytest_ini = Path("pytest.ini").read_text()
print("Wait.until call count:", len(re.findall(r"\bWait\.until\s*\(", utils)))
print("portforward call count:", len(re.findall(r"\bkubernetes\.stream\.portforward\s*\(", utils)))
print("SocketProxy TimeoutError handling:", bool(re.search(
r"except\s*\([^)]*\bTimeoutError\b[^)]*\).*?:.*?continue",
proxy,
re.S,
)))
print("pytest fail-fast option configured:", bool(re.search(
r"(?m)^\s*(?:addopts\s*=.*(?:^|\s)(?:-x|--exitfirst)|(?:maxfail\s*=))",
pytest_ini,
)))
print("helper starts daemon thread:", bool(re.search(
r"thread\s*=\s*threading\.Thread\(.*daemon=True\).*?thread\.start\(\)",
utils,
re.S,
)))
print("helper joins without a later join:", bool(re.search(
r"thread\.join\(timeout\).*?if not pf_result:.*?raise TimeoutError",
utils,
re.S,
)))
PYRepository: red-hat-data-services/notebooks
Length of output: 376
Close abandoned port-forward attempts.
SocketProxy catches TimeoutError and continues serving clients, so the exception does not fail the test or end the pytest process. The readiness Wait.until retries for up to TIMEOUT_2MIN; each retry can leave a daemon thread or a late-returning, unclosed PortForward. Close abandoned results when they complete, or otherwise bound their lifetime.
🤖 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 508 - 524, Update
_portforward_with_timeout so timed-out port-forward attempts are eventually
closed even when callers catch TimeoutError and retry. Track the helper thread’s
late result and close any returned PortForward, and ensure the abandoned worker
or connection has a bounded lifetime rather than leaking across readiness
retries.
CI status [antigravity]Run: Build Notebooks (push) #30766140573 — 2/2 complete · 2 skipped Workflow completed with skipped jobs. |
CI status [antigravity]Run: Build Notebooks (pr) #30766142345 — 2/2 complete · 1 passed · 1 skipped No workbench image jobs ran; all matrix jobs were skipped. |
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 red-hat-data-services#2691 (rhoai-2.25), where this was implemented and CI-verified first (openshift-container-tests job exercises this exact code path there; no equivalent job exists on main yet, tracked by opendatahub-io#4257). red-hat-data-services#2684
1 participant
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(
kubernetes/stream/ws_client.py) that the underlyingwebsocket.connect(...)callhas no per-call timeout plumbed through
portforward()'s public API -- a singleiteration 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 torecover.
Empirical testing of the two other suggested fixes in #2684 (bigger
listen()backlog, dropping stale queued connections) showed they're no-ops for
Wait.until's actual serial access pattern -- see thefollow-up comment
for the methodology/results. Only this one addresses a real, currently-unmitigated
gap, so it's the only one implemented here.
Separately verified on a real cluster-less repro (remapped container port,
mimicking the port-forward host/pod port mismatch) that this change is orthogonal
to the RStudio
/apiregression fixed in #2685: that failure is an instant(~2ms) client-side
ConnectionErrorto the wrong hardcoded port, happeningafter the portforward tunnel is already established -- outside this function's
code path entirely.
Changes
exposing_contextmanager()gains atimeout: float = 30parameter (well underthe outer
Wait.until(..., TestFrameConstants.TIMEOUT_2MIN, ...)120s budget atthe only call site, so a give-up still leaves room for several fresh retries).
Once exceeded, cleans up any partial
s/pfand raisesTimeoutError.s.close()/pf.close()cleanup between retry iterations. Previously eachfailed attempt leaked the prior
PortForward/socket (this is already howopendatahub-io/notebooks:main's copy of this function behaves -- rhoai-2.25had fallen behind on just this point).
SocketProxy.listen_and_serve_until_canceled()now also catches the newTimeoutErroralongside the existingBrokenPipeError/ConnectionResetError,so the proxy gives up on just that one connection and keeps serving future
retries. Without this, the exception would escape to the outer
except Exceptionhandler and kill the whole proxy thread instead.Verification
ast.parse,uv run ruff check,uv run pyrightall clean on both files.exposing_contextmanager/SocketProxy(confirmed via grep) -- real coverage is via the
openshift-container-testsCI job added in fix(tests): probe / instead of /api for OpenShift port-forward readiness #2685, which exercises this exact code path. Treating its 5
matrix legs going green as the real verification for this PR.
Rollout
Sequenced as one enhancement PR per branch, this one first:
rhoai-2.25(thisPR) →
opendatahub-io/notebooks:main→rhoai-3.5, since this is also the onlybranch with CI coverage of this code path today (tracked for
main/rhoai-3.5separately by opendatahub-io#4257).
Summary by CodeRabbit