-
Notifications
You must be signed in to change notification settings - Fork 152
fix(tests): bound exposing_contextmanager's portforward retry loop #4273
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -463,28 +463,82 @@ def check_not_none(value: Any, message: str) -> None: | |
| raise ValueError(message) | ||
|
|
||
|
|
||
| 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] | ||
|
Comment on lines
+466
to
+507
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 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 🤖 Prompt for AI AgentsSource: Path instructions |
||
|
|
||
|
|
||
| @contextlib.contextmanager | ||
| def exposing_contextmanager( | ||
| core_v1_api: kubernetes.client.CoreV1Api, | ||
| pod: kubernetes.client.models.V1Pod, | ||
| timeout: float = 30, | ||
| ) -> Generator[socket]: | ||
| # If we e.g., specify the wrong port, the pf = portforward() call succeeds, | ||
| # but pf.connected will later flip to False | ||
| # we need to check that _everything_ works before moving on | ||
| # | ||
| # https://github.com/red-hat-data-services/notebooks/issues/2684: bound this retry loop (and each | ||
| # individual portforward() attempt via _portforward_with_timeout, since a single hung call would | ||
| # otherwise never let this loop's own deadline check run again) so a pod that never becomes | ||
| # reachable can't block the single-threaded SocketProxy forever and starve every later | ||
| # Wait.until retry. | ||
| 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) | ||
| assert s, "Failed to establish connection" | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟢 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.