fix(tests): bound exposing_contextmanager's portforward retry loop - #4273
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 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 #4257). red-hat-data-services#2684
github-actions
Bot
added
the
review-requested
📝 WalkthroughWalkthroughThe Kubernetes utility now accepts a configurable timeout. It bounds each Estimated code review effort: 3 (Moderate) | ~25 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors)
✅ Passed checks (9 passed)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| pf = kubernetes.stream.portforward( | ||
| api_method=core_v1_api.connect_get_namespaced_pod_portforward, | ||
| name=pod.metadata.name, | ||
| namespace=pod.metadata.namespace, |
There was a problem hiding this comment.
🟢 low: Consider using threading.Thread(..., daemon=True) explicitly via keyword argument or adding a brief docstring note about _portforward_with_timeout's thread cleanup semantics. Since the thread is daemonized and abandoned on timeout, it will terminate when the test process exits.
CI status [antigravity]Run: Build Notebooks (pr) #30766603573 — 3/3 complete · 1 passed · 2 skipped No workbench image jobs ran; all matrix jobs were skipped. |
📋 Review SummaryThis PR implements a robust timeout mechanism for 🔍 General Feedback
I have not posted any inline review comments as the changes are well-implemented and correct. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4273 +/- ##
=======================================
Coverage 43.75% 43.75%
=======================================
Files 45 45
Lines 5814 5814
Branches 974 974
=======================================
Hits 2544 2544
Misses 3053 3053
Partials 217 217
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
![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
🧹 Nitpick comments (1)
tests/containers/kubernetes_utils.py (1)
488-493: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the single-port join.
",".join(str(p) for p in [8888])builds a string from a one-element list literal. Use the literal string directly for the same result with less code.♻️ Proposed simplification
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]), + ports="8888", )🤖 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 488 - 493, In the portforward call, replace the one-element join expression in the ports argument with the equivalent literal string "8888", leaving the surrounding Kubernetes stream configuration unchanged.
🤖 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 466-507: Update _portforward_with_timeout so timed-out
port-forward operations cannot accumulate indefinitely across the pytest
process. Replace the abandoned daemon-thread approach with a cancellable or
isolated execution strategy, or enforce a process-wide limit that stops further
attempts after the permitted leak threshold; ensure repeated SocketProxy retries
are bounded and existing success, propagated-error, and timeout behavior remains
intact.
---
Nitpick comments:
In `@tests/containers/kubernetes_utils.py`:
- Around line 488-493: In the portforward call, replace the one-element join
expression in the ports argument with the equivalent literal string "8888",
leaving the surrounding Kubernetes stream configuration unchanged.
🪄 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), Central YAML (inherited), Repository UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0b66bb09-e989-41c7-b5b3-4a18cafadefa
📒 Files selected for processing (2)
tests/containers/kubernetes_utils.pytests/containers/socket_proxy.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
opendatahub-io/kubeflow(manual)opendatahub-io/opendatahub-operator(manual)opendatahub-io/odh-dashboard(manual)
| 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. | ||
| """ | ||
| pf_result: list[kubernetes.stream.ws_client.PortForward] = [] | ||
| error_result: list[Exception] = [] | ||
|
|
||
| def _target() -> None: | ||
| try: | ||
| 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_result.append(pf) | ||
| except Exception as e: # propagated to the caller below, not swallowed | ||
| error_result.append(e) | ||
|
|
||
| thread = threading.Thread(target=_target, daemon=True) | ||
| thread.start() | ||
| thread.join(timeout) | ||
| if error_result: | ||
| raise error_result[0] | ||
| if not pf_result: | ||
| raise TimeoutError( | ||
| f"kubernetes.stream.portforward() to {pod.metadata.name} did not return within {timeout:.1f}s" | ||
| ) | ||
| return pf_result[0] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether the test invocation stops on first failure (limits leak accumulation).
set -euo pipefail
rg -n '\-x\b|--maxfail|exitfirst' Makefile tests/containers/pytest.ini pyproject.toml 2>/dev/null || true
fd -e ini -e cfg pytest --exec cat {} \;Repository: opendatahub-io/notebooks
Length of output: 3239
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- CI pytest invocations ---'
rg -n -i --glob '.github/**' --glob '.tekton/**' --glob 'Makefile*' --glob '*.yml' --glob '*.yaml' --glob '*.sh' --glob '*.mk' 'pytest|maxfail|exitfirst| -x([[:space:]]|$)' . || true
printf '%s\n' '--- helper call sites and lifecycle ---'
rg -n '_portforward_with_timeout|portforward|class SocketProxy|def deploy\(' tests/containers tests 2>/dev/null | head -250
printf '%s\n' '--- relevant source context ---'
sed -n '430,540p' tests/containers/kubernetes_utils.pyRepository: opendatahub-io/notebooks
Length of output: 9058
🏁 Script executed:
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for p in Path(".").rglob("*"):
if p.is_file() and (".github" in p.parts or ".tekton" in p.parts or p.name in {"Makefile", "pytest.ini", "pyproject.toml"}):
try:
s = p.read_text()
except (UnicodeDecodeError, OSError):
continue
if "pytest" in s.lower() or "maxfail" in s.lower() or "exitfirst" in s.lower():
print(f"{p}:")
for i, line in enumerate(s.splitlines(), 1):
if any(x in line.lower() for x in ("pytest", "maxfail", "exitfirst")):
print(f" {i}: {line}")
PYRepository: opendatahub-io/notebooks
Length of output: 5267
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- container-test workflow execution and failure policy ---'
sed -n '120,175p' .github/workflows/test-containers.yaml
sed -n '215,245p' .github/workflows/test-containers.yaml
printf '%s\n' '--- template workflow execution and failure policy ---'
sed -n '475,505p' .github/workflows/build-notebooks-TEMPLATE.yaml
sed -n '552,580p' .github/workflows/build-notebooks-TEMPLATE.yaml
printf '%s\n' '--- deployment and cleanup call graph ---'
sed -n '90,260p' tests/containers/kubernetes_utils.py
sed -n '500,555p' tests/containers/kubernetes_utils.py
sed -n '1,180p' tests/containers/socket_proxy.pyRepository: opendatahub-io/notebooks
Length of output: 25256
🏁 Script executed:
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
needles = (
"exposing_contextmanager(",
"SocketProxy(",
".close(",
"yield",
)
for path in [Path("tests/containers/kubernetes_utils.py"), Path("tests/containers/socket_proxy.py")]:
print(f"--- {path} ---")
lines = path.read_text().splitlines()
for i, line in enumerate(lines, 1):
if any(n in line for n in needles):
lo, hi = max(1, i - 3), min(len(lines), i + 5)
print("\n".join(f"{j}: {lines[j-1]}" for j in range(lo, hi + 1)))
print()
PYRepository: opendatahub-io/notebooks
Length of output: 6036
Bound abandoned port-forward attempts across the pytest process. CI runs pytest without -x or --maxfail, so tests continue after failures. Each timeout leaves a daemon thread, and the blocked client may retain sockets for the rest of the process. Repeated SocketProxy retries can accumulate these resources across tests (CWE-400). Use a cancellable or isolated operation, or enforce a process-wide leak limit that stops the session. Logging alone does not release resources.
🤖 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 466 - 507, Update
_portforward_with_timeout so timed-out port-forward operations cannot accumulate
indefinitely across the pytest process. Replace the abandoned daemon-thread
approach with a cancellable or isolated execution strategy, or enforce a
process-wide limit that stops further attempts after the permitted leak
threshold; ensure repeated SocketProxy retries are bounded and existing success,
propagated-error, and timeout behavior remains intact.
Source: Path instructions
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ysok The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
Description
Implements "Fix #2" from the suggested (not-yet-implemented) hardening ideas in
red-hat-data-services/notebooks#2684:
bound
exposing_contextmanager()'skubernetes.stream.portforward()retry loop intests/containers/kubernetes_utils.py.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 straight port of
red-hat-data-services/notebooks#2691
(
rhoai-2.25), where it was implemented and CI-verified first, and where a CodeRabbitreview caught the single-hung-call gap described above (fixed there first, then ported
here). Per this repo's usual sync direction this would normally go upstream first, but
this specific enhancement was sequenced
rhoai-2.25→main→rhoai-3.5since, atthe time this work started,
rhoai-2.25had the only CI coverage of this exact codepath (
openshift-container-tests, from red-hat-data-services#2685) andmaindidn't (#4257). #4257 has since been independently resolved by #4259 while thisPR was in progress, so
main'sopenshift-container-testsjob will also exercisethis change directly in this PR's own CI.
How Has This Been Tested?
ast.parse,uv run ruff format --check,uv run ruff check,uv run pyrightall clean on both changed files.
exposing_contextmanager/SocketProxy(confirmed via grep) -- real coverage is via the
openshift-container-testsCI job,which exercises this exact code path here and already went green on the
rhoai-2.25port (PR fix-permissions script exits successfully on missing target, potentially masking errors #2691, all 5 matrix legs) both before and after theCodeRabbit-driven fix.
portforward()call that hangs forever -> correctly bounded, raisesTimeoutErrorat the configured timeout instead of hangingon the loop's own deadline as before
Self checklist (all need to be checked):
make test(gmakeon macOS) before asking for reviewDockerfile.konfluxfiles should be done inodh/notebooksand automatically synced torhds/notebooks. For Konflux-specific changes, modifyDockerfile.konfluxfiles directly inrhds/notebooksas these require special attention in the downstream repository and flow to the upcoming RHOAI release.Merge criteria: