Skip to content

fix(pve): serve PVE requests under /api/pve and drop per-gateway rewrites#6130

Open
xuang7 wants to merge 6 commits into
apache:mainfrom
xuang7:fix/single-node-nginx-pve-route
Open

fix(pve): serve PVE requests under /api/pve and drop per-gateway rewrites#6130
xuang7 wants to merge 6 commits into
apache:mainfrom
xuang7:fix/single-node-nginx-pve-route

Conversation

@xuang7

@xuang7 xuang7 commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

The single-node NGINX gateway (bin/single-node/nginx.conf) had no route for the Python Virtual Environment (PVE) REST calls, so they fell through to the catch-all location / and returned 404 from dashboard-service. In the UI this surfaces as a "Could not load Python virtual environments" popup on every Python UDF operator selection.

The root cause is that the frontend requested /pve/... while the backend (PveResource, @Path("/pve"), Jersey mounted under /api/* on the coordinator) serves /api/pve/..., so every gateway had to carry its own /pve to /api/pve rewrite rule, and the single-node config was missing one.

Per review feedback, instead of adding the missing rewrite, this PR removes the mismatch at the source: the frontend now builds PVE URLs with AppSettings.getApiEndpoint() and requests /api/pve/... directly. Every routing layer then becomes a plain routing entry with no path rewriting:

  • bin/single-node/nginx.conf: location /api/pve/ passes through to the coordinator
  • frontend/proxy.config.json: /api/pve entry (ordered before /api), pathRewrite removed
  • bin/k8s/templates/base/gateway/gateway-routes.yaml: /api/pve joins the existing dynamic-backend match list, the URLRewrite filter is removed

The backend is unchanged. Net effect: the 404 is fixed, and the per-gateway rewrite special-casing that allowed the three configs to drift apart is gone.

Any related issues, documentation, discussions?

Related to #5963 (PVE single-node configuration).

How was this PR tested?

  • virtual-environment.service.spec.ts passes with the new URLs (5 specs).
  • On the single-node Docker stack: GET /api/pve/pves?cuid=1 returns 200 JSON from the coordinator, and the old /pve path falls through to the dashboard as expected.
  • helm template renders the updated k8s route with the URLRewrite filter removed.

Was this PR authored or co-authored using generative AI tooling?

Generated-by: Claude Code (Claude Opus 4.8, Claude Fable 5)

@github-actions

github-actions Bot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Automated Reviewer Suggestions

Based on the git blame history of the changed files, we recommend the following reviewers:

  • Contributors with relevant context: @bobbai00, @SarahAsad23, @zyratlo
    You can notify them by mentioning @bobbai00, @SarahAsad23, @zyratlo in a comment.

@xuang7 xuang7 marked this pull request as ready for review July 6, 2026 22:41
xuang7 and others added 2 commits July 6, 2026 15:49
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment thread bin/single-node/nginx.conf Outdated
proxy_set_header Host $host;
}

location /pve/ {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should change this to /api/pve/ in the src code instead of doing a mapping here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. No other comments for me.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, thanks!

Per review: the frontend now calls /api/pve directly via
AppSettings.getApiEndpoint(), so the /pve-to-/api/pve rewrite rules in
single-node nginx, the dev proxy, and the k8s gateway become plain
routing entries.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actions github-actions Bot added the frontend Changes related to the frontend GUI label Jul 7, 2026
@xuang7 xuang7 changed the title fix: add missing /pve route to single-node nginx fix: serve PVE requests under /api/pve and drop per-gateway rewrites Jul 7, 2026
@codecov-commenter

codecov-commenter commented Jul 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 62.50000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.64%. Comparing base (b1c4fd2) to head (e7ee16c).
⚠️ Report is 45 commits behind head on main.

Files with missing lines Patch % Lines
...virtual-environment/virtual-environment.service.ts 62.50% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6130      +/-   ##
============================================
+ Coverage     60.26%   60.64%   +0.38%     
  Complexity     3339     3339              
============================================
  Files          1133     1139       +6     
  Lines         43979    44874     +895     
  Branches       4781     4930     +149     
============================================
+ Hits          26503    27214     +711     
- Misses        16021    16173     +152     
- Partials       1455     1487      +32     
Flag Coverage Δ *Carryforward flag
access-control-service 70.00% <ø> (ø) Carriedforward from b864745
agent-service 44.59% <ø> (ø) Carriedforward from b864745
amber 65.51% <ø> (ø) Carriedforward from b864745
computing-unit-managing-service 0.00% <ø> (ø) Carriedforward from b864745
config-service 52.30% <ø> (ø) Carriedforward from b864745
file-service 63.97% <ø> (ø) Carriedforward from b864745
frontend 53.26% <62.50%> (+1.28%) ⬆️
notebook-migration-service 78.57% <ø> (ø) Carriedforward from b864745
pyamber 91.18% <ø> (ø) Carriedforward from b864745
workflow-compiling-service 55.14% <ø> (ø) Carriedforward from b864745

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

xuang7 added a commit to xuang7/texera that referenced this pull request Jul 7, 2026
v1.2 adaptation of b864745 on main (PR apache#6130): the frontend builds PVE
URLs with AppSettings.getApiEndpoint(), so single-node nginx, the dev
proxy, and the k8s gateway all become plain routes with no rewriting.
Fewer files than on main because this branch predates the StoredPve db
endpoints and the gateway chart reorganization.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@xuang7 xuang7 requested a review from Yicong-Huang July 8, 2026 20:53
Comment thread bin/single-node/nginx.conf Outdated
@xuang7 xuang7 requested review from Yicong-Huang and aglinxinyuan and removed request for bobbai00 July 9, 2026 17:49
@aglinxinyuan aglinxinyuan requested review from Copilot and removed request for aglinxinyuan July 9, 2026 20:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes single-node and gateway routing for Python Virtual Environment (PVE) REST calls by aligning the frontend to call PVE endpoints under the backend’s /api mount (i.e., /api/pve/...) and removing per-gateway /pve -> /api/pve rewrite special-casing across deployment configs.

Changes:

  • Update the frontend PVE service to build REST URLs using AppSettings.getApiEndpoint() (so requests go to /api/pve/...).
  • Update local dev proxying and single-node NGINX to route /api/pve directly to the coordinator service.
  • Update the Kubernetes gateway HTTPRoute to match /api/pve and drop the rewrite filter.

Reviewed changes

Copilot reviewed 4 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
frontend/src/app/workspace/service/virtual-environment/virtual-environment.service.ts Switch PVE REST calls to use AppSettings.getApiEndpoint() so they resolve under /api/pve/*.
frontend/src/app/workspace/service/virtual-environment/virtual-environment.service.spec.ts Update unit tests to match the new base URL construction.
frontend/src/app/workspace/component/power-button/computing-unit-selection.component.ts Update comment to reflect /api/pve/system endpoint path.
frontend/proxy.config.json Add /api/pve proxy target to the coordinator and remove the old /pve rewrite entry.
bin/single-node/nginx.conf Add explicit location /api/pve/ proxying to the workflow-runtime coordinator.
bin/k8s/templates/base/gateway/gateway-routes.yaml Add /api/pve to dynamic backend matches and remove the /pve rewrite rule.
Comments suppressed due to low confidence (1)

frontend/src/app/workspace/service/virtual-environment/virtual-environment.service.spec.ts:85

  • This spec only covers the CRUD /pve/db methods, but this PR also changed the request URLs for getSystemPackages() and fetchPVEs() (and related helpers). Adding unit tests for those methods would prevent regressions in the new /api/pve/* routing (URL + required cuid query param).
  it("deleteUserPve() DELETEs /pve/db/{veid}", () => {
    service.deleteUserPve(9).subscribe();

    const req = httpTestingController.expectOne(`${AppSettings.getApiEndpoint()}/pve/db/9`);
    expect(req.request.method).toBe("DELETE");
    req.flush(null);
  });
});

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

xuang7 added a commit to xuang7/texera that referenced this pull request Jul 10, 2026
…tching

Mirrors the same change on the main PR (apache#6130) to keep the two branches
identical.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tching

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@xuang7 xuang7 force-pushed the fix/single-node-nginx-pve-route branch from 95e2bda to e7ee16c Compare July 10, 2026 23:38
xuang7 added a commit to xuang7/texera that referenced this pull request Jul 10, 2026
…tching

Mirrors the same change on the main PR (apache#6130) to keep the two branches
identical.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@xuang7 xuang7 changed the title fix: serve PVE requests under /api/pve and drop per-gateway rewrites fix(pve): serve PVE requests under /api/pve and drop per-gateway rewrites Jul 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix frontend Changes related to the frontend GUI infra

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants