[rhoai-2.25] RHAIENG-6036: extend OCP port-forward HTTP wait for slow arches#2488
Conversation
📝 WalkthroughWalkthrough
ChangesPort-forward readiness
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
jiridanek
changed the title
2c69c30 to
9080fd2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/containers/kubernetes_utils.py (1)
270-275: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a
timeouttorequests.getto make theWait.untildeadline effective.The timeout increase to
TIMEOUT_2MINis correct for slower architectures. However,requests.getitself has notimeoutargument — if the TCP connect or response stalls (common under QEMU emulation), the call blocks indefinitely and theWait.untildeadline check at line 448 never executes untilrequests.getreturns. Adding a short per-request timeout ensures the polling loop can actually enforce the 2-minute ceiling.♻️ Proposed fix
Wait.until( "Connecting to pod succeeds", 1, TestFrameConstants.TIMEOUT_2MIN, - lambda: requests.get(f"http://localhost:{self.port}").status_code == 200, + lambda: requests.get(f"http://localhost:{self.port}", timeout=5).status_code == 200, )🤖 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 270 - 275, Add a short per-request timeout to the requests.get call inside the “Connecting to pod succeeds” Wait.until callback, ensuring stalled connections return promptly so Wait.until can enforce TIMEOUT_2MIN.
🤖 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 `@tests/containers/kubernetes_utils.py`:
- Around line 270-275: Add a short per-request timeout to the requests.get call
inside the “Connecting to pod succeeds” Wait.until callback, ensuring stalled
connections return promptly so Wait.until can enforce TIMEOUT_2MIN.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 75226be4-3c7d-4116-81eb-c307b844f729
📒 Files selected for processing (1)
tests/containers/kubernetes_utils.py
|
also, this, will see if the problem is flaky or not |
jiridanek
changed the title
9080fd2 to
3e52b88
Compare
|
@coderabbitai Good context — those tickets explain why the probe paths are fragmented and how they relate to culling. RHAIENG-5231 / RHAIENG-5344Both are Resolved stories for reworking CodeServer and RStudio ingress routing for RHOAI 3.x (Gateway API / Core goals:
How this maps to the three
|
| Concern | Path | Who calls it | 5231/5344 relevance |
|---|---|---|---|
| K8s readiness | RStudio: /api/ → probe.cgi |
OCP/K8s probes | Explicit nginx map; nbprefix adds /${NB_PREFIX}/api → /${NB_PREFIX}/api/ |
| K8s readiness | CodeServer: /api → /codeserver/healthz/ |
OCP/K8s probes | Same story in 5231 |
| Idle culling | /api/kernels (+ /api/terminals for Jupyter) |
culling_controller.go | AC #3: access.cgi must work under both Route and Gateway API paths |
So “RStudio /api isn’t Jupyter-style” is by design from 5231/5344 — three different nginx routes, not one generic /api.
What the nbprefix templates show
RStudio (proxy.conf.template_nbprefix):
location = ${NB_PREFIX}/api {
return 302 ${NB_PREFIX}/api/;
...
}
location ${NB_PREFIX}/api/ {
rewrite ^${NB_PREFIX}/api/?(.*)$ /api/$1 last;
CodeServer (proxy.conf.template_nbprefix):
location = ${NB_PREFIX}/api {
return 302 ${NB_PREFIX}/codeserver/healthz/;
}
location ${NB_PREFIX}/api/ {
return 302 ${NB_PREFIX}/codeserver/healthz/;
Under Gateway API, probes are prefixed and use relative redirects. Culling uses prefixed /api/kernels with rewrite back to internal /api/kernels/ → access.cgi.
Implications for our CI probe work (#2488)
1. Two routing profiles exist; CI only exercises one
test_image_run_on_openshift port-forwards to pod :8888 with no NB_PREFIX. That matches the tickets’ legacy 2.25 Route fallback (proxy.conf.template), not the Gateway API profile.
So our probes should target the no-prefix maps:
| Image | Correct no-prefix probe |
|---|---|
| Jupyter | /api |
| RStudio | /api/ (not bare /api) |
| CodeServer | /codeserver/healthz/ (not bare /api) |
That aligns with 5231/5344 nginx design, not an ad-hoc workaround.
2. Culling is a separate acceptance criterion
5344 AC #3 explicitly requires culling smoke tests under both routing profiles. The culling controller never calls /api/ — it calls /api/kernels at the full notebook prefix (/notebook/{ns}/{nm}/api/kernels). 5231/5344 reworked access.cgi path handling for that.
Our OCP readiness wait does not validate culling; culling validation would need prefixed URLs or in-cluster HTTP like the culler uses.
3. RHAIENG-5652 (main cherry-pick) is complementary, not the same
- 5231/5344: path-aware routing for Gateway API vs Route; probe + culling maps per workbench
- 5652: relative redirects / open-redirect fix on
main(partially landed onrhoai-2.25for codeserver default template)
The b180767 failures were partly from applying main’s single /api probe without the per-workbench maps 5231/5344 defined.
4. Possible follow-up (out of #2488 scope)
A fuller test would deploy with NB_PREFIX=/notebook/{ns}/{name} and probe:
/{NB_PREFIX}/api→ RStudio/api//{NB_PREFIX}/api→ CodeServer healthz
That would cover the Gateway API profile the Jira tickets care about. Current CI only covers the Route fallback.
Bottom line: 5231/5344 document that probe paths are workbench-specific and routing-profile-specific. Culling uses yet another path (/api/kernels under notebook prefix). Our per-image probes in port_forward_http_ready() match the no-NB_PREFIX profile those tickets preserve for 2.25; they don’t replace culling validation or Gateway API prefix testing.
|
Tip For best results, initiate chat on the files or code changes.
This is good context and doesn't change anything about the current PR (extending the timeout to If you'd like, I can open a follow-up GitHub issue to track adding a ✏️ Learnings added
|
|
@coderabbitai RHAIENG-5652 ties the picture together — it’s the security layer on top of the routing work in 5231 / 5344. RHAIENG-5652 — what it isCWE-601 open redirect (Glasswing FIND-011): nginx Fix: relative redirects ( Landed on
Notable Jira history: autofix PR #2361 was wrong and broke tests; Vath Sok fixed it properly in opendatahub-io#3867. How 5652 relates to 5231/5344
5231/5344 introduced the dual nginx profiles and probe maps. 5652 changed how redirects are emitted so those maps work safely when something hits the pod directly — which is exactly what our OCP port-forward test does. Why the cherry-pick broke
|
| Client | What it needs | Ticket context |
|---|---|---|
| Kubeflow culler | /notebook/{ns}/{nm}/api/kernels |
5231/5344 access.cgi shim |
| K8s readiness | RStudio /api/, codeserver /api→healthz, Jupyter /api |
5231/5344 probe maps |
| CI port-forward | Same endpoints, no NB_PREFIX |
Hits pod directly → 5652 matters |
| Security audit | No $http_host in Location |
5652 |
What #2488 should and shouldn’t do
In scope (test harness): per-image readiness probes that match 5231/5344 probe maps on the no-NB_PREFIX profile, tolerant of 5652 not being on rhoai-2.25 yet.
Out of scope but related:
- Backport 5652 nginx changes to
rhoai-2.25(separate PR; would let a generic/apiprobe work more likemain) - Gateway API profile testing with
NB_PREFIXset (5231/5344 AC) - Culling validation under both routing profiles (5231/5344 AC [pull] main from opendatahub-io:main #3)
Bottom line: 5231/5344 explain which URL each workbench exposes; 5652 explains why a blind cherry-pick of main’s /api probe failed on rhoai-2.25 (absolute redirects still present in nginx). Our per-image probes in port_forward_http_ready() are a test-harness workaround until 5652 lands on rhoai-2.25, not a substitute for that nginx backport.
|
Tip For best results, initiate chat on the files or code changes.
Given this, I agree the current PR If you'd like, I can help open a follow-up issue tracking the Gateway API ( ʅ(◔౪◔ )ʃ ✏️ Learnings added
|
|
Tip For best results, initiate chat on the files or code changes.
Sounds like a solid plan — happy to help scope a follow-up test (deploying with ✏️ Learnings added
|
|
Tip For best results, initiate chat on the files or code changes.
To summarize the key takeaways I'll retain for future reviews of this test harness:
Given point 4 in your analysis (testing the Gateway API profile with 🐇💭 ✏️ Learnings added
|
|
Tip For best results, initiate chat on the files or code changes.
Given all this is confirmed to be out of scope for the current PR, I don't see any further action needed here — this PR's timeout change stands on its own, and the s390x flakiness will be monitored as you noted. Let me know if you'd like me to open a follow-up issue to track the Gateway API/NB_PREFIX probe coverage idea for later. ✏️ Learnings added
|
|
Tip For best results, initiate chat on the files or code changes.
To avoid cluttering the discussion with repeated summaries, I'll leave it here unless there's something new to add. Happy to open that follow-up issue for backporting RHAIENG-5652's nginx changes to |
|
Tip For best results, initiate chat on the files or code changes.
This is all consistent with the current PR's scope (extending the timeout) and doesn't require any changes here. As before, happy to open a follow-up issue to track the potential nginx backport of RHAIENG-5652 to ✏️ Learnings added
|
The /api readiness path from main (RHAIENG-5652) applies to Jupyter and code-server only. RStudio images are still built on rhoai-2.25 and must probe / with redirect-following session instead. RHAIENG-6036: use TIMEOUT_2MIN constant for OCP port-forward wait Replace hardcoded 120s with TestFrameConstants.TIMEOUT_2MIN in the cherry-picked kubernetes_utils port-forward readiness poll. RHAIENG-6036: extend OCP port-forward HTTP wait for slow arches Backport kubernetes_utils port-forward readiness from main: - 120s wait (was 30s on rhoai-2.25) - probe http://127.0.0.1:<port>/api with per-request timeout=10 Fixes jupyter-minimal linux/s390x OpenShift container test flake (WaitError after 30s waiting for Connecting to pod succeeds). (cherry picked from commit 8660870) Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
36ad2b7 to
261935d
Compare
Merge recommendation: yes — merge for RHAIENG-6036 scopeEvidence from workflow_dispatch run 29158214627 @ What this PR fixes (in scope)
Change is test-harness only ( What this PR does not fix (do not block merge on these)Failures in the same dispatch run are orthogonal:
PR checks
Bottom lineMerge once tide/review gates are satisfied. This PR addresses the RHAIENG-6036 OCP port-forward readiness regression on slow arches; the primary failing leg ( |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ide-developer 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 |
2 participants
Summary
Cherry-pick the full
tests/containers/kubernetes_utils.pyport-forward readiness fix frommain:(cherry picked from commit 86608705582326196e7b078fea6648b9f156295d)— RHAIENG-5652 onmain; only thekubernetes_utils.pyhunk is included (codeserver nginx changes from that commit are out of scope).Changes vs
rhoai-2.25tip:TestFrameConstants.TIMEOUT_2MIN(120s)localhost→127.0.0.1requests.get(..., timeout=10)so the wait loop can enforce the deadline/apifor Jupyter/code-server; probe/(with redirect-following session) for RStudio images —mainno longer builds RStudio, so the upstream/api-only path regressedrstudio-c9samd64 OpenShift tests (run 29127889593)Fixes Build Notebooks (push) failure on
jupyter-minimallinux/s390x at Run OpenShift container tests (in PyTest):(push run 29092763875 @
d30e953)CI jobs this PR targets
jupyter-minimals390xrstudio-c9s/cuda-rstudio-c9samd64/probe)jupyter-minimalppc64lemake_test.py)Out of scope
Part of RHAIENG-6036.
CI validation
Build Notebooks (push)
workflow_dispatchonfix/rhoai-2.25-ws3-arch-matrix@3721cede3:https://github.com/red-hat-data-services/notebooks/actions/runs/29128741592
Previous run @
b1807671f(RStudio/apiregression): https://github.com/red-hat-data-services/notebooks/actions/runs/29127889593How Has This Been Tested?
kubernetes_utils.pyhunk frommaincommit86608705ontorhoai-2.25(d30e953).rstudio-c9samd64 failure on run 29127889593.Test plan
jupyter-minimals390x andrstudio-c9samd64 OpenShift container testsrhoai-2.25Summary by CodeRabbit