This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-8947-e4e324a207b4e6bce37e8845689c702a6b0cdc02 in repository https://gitbox.apache.org/repos/asf/texera.git
commit d27b7b49877bcc52d80602e57f5b0e5f86291736 Author: Kunwoo (Chris) <[email protected]> AuthorDate: Mon Oct 12 03:15:54 2026 +0000 feat(frontend): add admin computing unit terminate button (#8947) ### What changes were proposed in this PR? Problem. An admin can list every computing unit on Admin > Computing Units (#8888) but cannot act on one. Freeing a stuck or idle unit on a full Kubernetes cluster still means finding its pod and deleting it with `kubectl`. The backend already lets an ADMIN terminate any unit (`DELETE /computing-unit/{cuid}/terminate`, #7337), so only the UI is missing. Fix. Adds a delete-icon button to every row, in a new Actions column. It calls `ComputingUnitActionsService.confirmAndTerminate`, the confirmation the user's own Computing Units page already uses, so the dialog follows the unit type: a "results will be lost" warning for a Kubernetes unit and "Disconnect" for a local one. Cancelling the dialog or a failed request leaves the row alone. `confirmAndTerminate` gains an optional `onTerminated` callback that runs only after a successful termination, and the page uses it to drop the row at once instead of showing the unit as `Running` until the next 5 second poll. Two details follow from an admin seeing other users' units. A poll that started before the `DELETE` can answer after it with the unit still listed, so the page remembers the units it terminated and filters them out of every poll response. And ng-zorro renders the dialog and toast text through `[innerHTML]` while any user can rename their unit, so `confirmAndTerminate` now HTML-escapes the unit name for every caller. ``` Before: admin finds a stuck unit on the page -> finds its pod -> kubectl delete pod After: admin finds a stuck unit on the page -> clicks its delete icon -> confirms -> row disappears ``` Not included. Telling the owner their unit was terminated (#6483) and the cluster capacity panels (#6481, #6482) are separate issues. There is no backend change. ### Any related issues, documentation, discussions? Closes #6480. Part of #6476. Follows #8888, which added the page. ### How was this PR tested? Tests were added for the button, the hand-off to the dialog and the name escaping. They cover a button on every row (Kubernetes and local), the click confirming the clicked row's unit, the row leaving only after a successful termination and staying on cancel or failure, a stale in-flight poll not bringing the row back, the detail row's `colspan`, a unit name holding markup in the dialog and the toast, and `onTerminated` firing on success only. The tests were written first and watched fail, and breaking the code on purpose (name left unescaped, callback on every outcome, row dropped on click, poll responses left unfiltered) fails exactly the seven tests meant to catch it. ``` cd frontend && npx ng test --watch=false --include='**/admin-computing-unit.component.spec.ts' --include='**/computing-unit-actions.service.spec.ts' cd frontend && npx ng test --watch=false ``` The first command passes 53 tests and the second passes 6,622 (1 skipped). `ngc -p src/tsconfig.app.json --noEmit`, `prettier-eslint` and `eslint` ran clean on the changed files. https://github.com/user-attachments/assets/a33ffa6f-439a-4f71-ae8f-7efe5d94daac ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code, Claude Sonnet 5.5 --- .../computing-unit-actions.service.spec.ts | 46 ++++++++ .../computing-unit-actions.service.ts | 8 +- .../admin-computing-unit.component.html | 24 +++- .../admin-computing-unit.component.spec.ts | 121 +++++++++++++++++++-- .../admin-computing-unit.component.ts | 34 +++++- 5 files changed, 215 insertions(+), 18 deletions(-) diff --git a/frontend/src/app/common/service/computing-unit/computing-unit-actions/computing-unit-actions.service.spec.ts b/frontend/src/app/common/service/computing-unit/computing-unit-actions/computing-unit-actions.service.spec.ts index 94fd340d3a..54f2573708 100644 --- a/frontend/src/app/common/service/computing-unit/computing-unit-actions/computing-unit-actions.service.spec.ts +++ b/frontend/src/app/common/service/computing-unit/computing-unit-actions/computing-unit-actions.service.spec.ts @@ -193,5 +193,51 @@ describe("ComputingUnitActionsService", () => { expect(notificationService.error).toHaveBeenCalledWith("Failed to terminate computing unit: kaboom"); }); + + // ng-zorro renders string content as HTML, and an admin sees other users' unit names, so a name must reach the + // modal and the toast as text. + describe("a unit name that holds markup", () => { + const markup = '<a href="https://evil.example";>Confirm</a> & <b>x</b>'; + const escaped = "<a href="https://evil.example">Confirm</a> & <b>x</b>"; + + it.each(["kubernetes", "local"])("is escaped in the confirmation modal of a %s unit", type => { + service.confirmAndTerminate(7, unit({ type, name: markup })); + + expect(modalService.confirm.mock.calls[0][0].nzContent).toContain(escaped); + }); + + it("is escaped in the success message", () => { + service.confirmAndTerminate(7, unit({ name: markup })); + + modalService.confirm.mock.calls[0][0].nzOnOk(); + + expect(notificationService.success).toHaveBeenCalledWith(`Terminated Computing Unit: ${escaped}`); + }); + }); + + describe("onTerminated callback", () => { + it("runs once the confirmed termination succeeds, and not before the modal is confirmed", () => { + const onTerminated = vi.fn(); + service.confirmAndTerminate(7, unit(), onTerminated); + expect(onTerminated).not.toHaveBeenCalled(); + + modalService.confirm.mock.calls[0][0].nzOnOk(); + + expect(onTerminated).toHaveBeenCalledTimes(1); + }); + + it.each([ + ["reports failure", of(false)], + ["observable errors", throwError(() => new Error("kaboom"))], + ])("does not run when the termination %s", (_outcome, termination) => { + const onTerminated = vi.fn(); + statusService.terminateComputingUnit.mockReturnValue(termination); + service.confirmAndTerminate(7, unit(), onTerminated); + + modalService.confirm.mock.calls[0][0].nzOnOk(); + + expect(onTerminated).not.toHaveBeenCalled(); + }); + }); }); }); diff --git a/frontend/src/app/common/service/computing-unit/computing-unit-actions/computing-unit-actions.service.ts b/frontend/src/app/common/service/computing-unit/computing-unit-actions/computing-unit-actions.service.ts index 2b67a8b2ba..49047b299a 100644 --- a/frontend/src/app/common/service/computing-unit/computing-unit-actions/computing-unit-actions.service.ts +++ b/frontend/src/app/common/service/computing-unit/computing-unit-actions/computing-unit-actions.service.ts @@ -18,6 +18,7 @@ */ import { Injectable } from "@angular/core"; +import { escape as escapeHtml } from "lodash-es"; import { Observable } from "rxjs"; import { NzModalService } from "ng-zorro-antd/modal"; import { ShareAccessComponent } from "../../../../dashboard/component/user/share-access/share-access.component"; @@ -88,13 +89,15 @@ export class ComputingUnitActionsService { throw new Error("Unsupported computing unit type"); } - confirmAndTerminate(cuid: number, unit: DashboardWorkflowComputingUnit): void { + /** Runs `onTerminated` once the unit is terminated, and not if the modal is cancelled or the termination fails. */ + confirmAndTerminate(cuid: number, unit: DashboardWorkflowComputingUnit, onTerminated?: () => void): void { if (!unit.computingUnit.uri) { this.notificationService.error("Invalid computing unit."); return; } - const unitName = unit.computingUnit.name; + // ng-zorro renders this string as HTML and any user can name their unit, so the name is escaped. + const unitName = escapeHtml(unit.computingUnit.name); const unitType = unit?.computingUnit.type || "kubernetes"; // fallback const templates = unitTypeMessageTemplate[unitType]; @@ -118,6 +121,7 @@ export class ComputingUnitActionsService { next: (success: boolean) => { if (success) { this.notificationService.success(`Terminated Computing Unit: ${unitName}`); + onTerminated?.(); } else { this.notificationService.error("Failed to terminate computing unit"); } diff --git a/frontend/src/app/dashboard/component/admin/computing-unit/admin-computing-unit.component.html b/frontend/src/app/dashboard/component/admin/computing-unit/admin-computing-unit.component.html index 49f5a60520..d0faa5b6f4 100644 --- a/frontend/src/app/dashboard/component/admin/computing-unit/admin-computing-unit.component.html +++ b/frontend/src/app/dashboard/component/admin/computing-unit/admin-computing-unit.component.html @@ -49,19 +49,19 @@ <th nzWidth="40px"></th> <th [nzSortFn]="sortByName" - nzWidth="20%"> + nzWidth="18%"> Name </th> <th [nzSortFn]="sortByOwner" - nzWidth="20%"> + nzWidth="18%"> Owner </th> <th [nzSortFn]="sortByType" [nzFilters]="typeFilters" [nzFilterFn]="filterByType" - nzWidth="12%"> + nzWidth="10%"> Type </th> <th @@ -76,7 +76,8 @@ nzWidth="16%"> Created </th> - <th nzWidth="20%">Resources</th> + <th nzWidth="18%">Resources</th> + <th nzWidth="8%">Actions</th> </tr> </thead> <tbody> @@ -106,10 +107,23 @@ {{ formatRelativeTime(unit.computingUnit.creationTime) }} </td> <td>{{ resourceSummary(unit) }}</td> + <td> + <button + nz-button + nzType="text" + nzDanger + [nz-tooltip]="terminateTooltip(unit)" + [attr.aria-label]="terminateTooltip(unit)" + (click)="terminate(unit)"> + <span + nz-icon + nzType="delete"></span> + </button> + </td> </tr> <tr *ngIf="expandedCuids.has(unit.computingUnit.cuid)"> <td></td> - <td colspan="6"> + <td colspan="7"> <dl class="spec-detail"> <div *ngFor="let field of specFields"> <dt>{{ field.label }}</dt> diff --git a/frontend/src/app/dashboard/component/admin/computing-unit/admin-computing-unit.component.spec.ts b/frontend/src/app/dashboard/component/admin/computing-unit/admin-computing-unit.component.spec.ts index ce692f4d88..88a6ee5a08 100644 --- a/frontend/src/app/dashboard/component/admin/computing-unit/admin-computing-unit.component.spec.ts +++ b/frontend/src/app/dashboard/component/admin/computing-unit/admin-computing-unit.component.spec.ts @@ -21,8 +21,10 @@ import { HttpErrorResponse } from "@angular/common/http"; import { ComponentFixture, TestBed } from "@angular/core/testing"; import { of, Subject, throwError } from "rxjs"; import { NzMessageService } from "ng-zorro-antd/message"; +import { NzModalService } from "ng-zorro-antd/modal"; import { AdminComputingUnitComponent } from "./admin-computing-unit.component"; import { WorkflowComputingUnitManagingService } from "../../../../common/service/computing-unit/workflow-computing-unit/workflow-computing-unit-managing.service"; +import { ComputingUnitActionsService } from "../../../../common/service/computing-unit/computing-unit-actions/computing-unit-actions.service"; import { DashboardWorkflowComputingUnit, WorkflowComputingUnit, @@ -81,8 +83,8 @@ function withResource(over: Partial<WorkflowComputingUnitResourceLimit>): Partia return withCu({ resource: { ...makeUnit().computingUnit.resource, ...over } }); } -function localUnit(): DashboardWorkflowComputingUnit { - return makeUnit(withCu({ type: "local", resource: NAN_RESOURCE })); +function localUnit(cuid = 1): DashboardWorkflowComputingUnit { + return makeUnit(withCu({ cuid, type: "local", resource: NAN_RESOURCE })); } describe("AdminComputingUnitComponent", () => { @@ -92,7 +94,7 @@ describe("AdminComputingUnitComponent", () => { beforeEach(async () => { await TestBed.configureTestingModule({ - providers: [{ provide: UserService, useClass: StubUserService }, ...commonTestProviders], + providers: [NzModalService, { provide: UserService, useClass: StubUserService }, ...commonTestProviders], imports: [AdminComputingUnitComponent, ...commonTestImports], }).compileComponents(); @@ -108,6 +110,23 @@ describe("AdminComputingUnitComponent", () => { fixture.destroy(); }); + // `nz-table` adds a hidden measure row, so match on the owner cell every data row has. + const dataRows = () => + Array.from<HTMLElement>(fixture.nativeElement.querySelectorAll("tbody tr")).filter( + row => row.querySelector("texera-user-avatar") !== null + ); + + // Stubs the shared confirmation, so no modal opens. `confirmed` stands for the admin confirming it and the + // termination succeeding. + const stubConfirm = (confirmed = false) => + vi + .spyOn(TestBed.inject(ComputingUnitActionsService), "confirmAndTerminate") + .mockImplementation((_cuid, _unit, onTerminated) => { + if (confirmed) { + onTerminated?.(); + } + }); + it("should create", () => { expect(component).toBeTruthy(); }); @@ -119,12 +138,9 @@ describe("AdminComputingUnitComponent", () => { fixture.detectChanges(); - // `nz-table` adds a hidden measure row, so match on the owner cell every data row has. - const dataRows = Array.from<HTMLElement>(fixture.nativeElement.querySelectorAll("tbody tr")).filter( - row => row.querySelector("texera-user-avatar") !== null - ); - expect(dataRows.length).toBe(1); - expect(dataRows[0].textContent).toContain("alice"); + const rows = dataRows(); + expect(rows.length).toBe(1); + expect(rows[0].textContent).toContain("alice"); }); describe("loading and polling", () => { @@ -257,6 +273,28 @@ describe("AdminComputingUnitComponent", () => { }); }); + // A poll that started before a termination can answer after it, with a list that still has the unit. + it("does not bring a terminated row back when a poll that was in flight answers", () => { + const first = makeUnit(); + const second = makeUnit(withCu({ cuid: 2 })); + const inFlight = new Subject<DashboardWorkflowComputingUnit[]>(); + vi.mocked(service.listAllComputingUnits) + .mockReturnValueOnce(of([first, second])) + .mockReturnValueOnce(inFlight); + stubConfirm(true); + const cuids = () => component.computingUnits.map(u => u.computingUnit.cuid); + + component.ngOnInit(); + // The next poll is now waiting on the backend. + vi.advanceTimersByTime(5000); + component.terminate(first); + expect(cuids()).toEqual([2]); + + inFlight.next([first, second]); + inFlight.complete(); + expect(cuids()).toEqual([2]); + }); + // `switchMap` would cancel the slow response on every tick, so the table would never refresh. it("lets a response slower than the interval finish instead of cancelling it", () => { const requests: Subject<DashboardWorkflowComputingUnit[]>[] = []; @@ -348,6 +386,71 @@ describe("AdminComputingUnitComponent", () => { expect(specDetail()).not.toBeNull(); }); + describe("terminating a unit", () => { + // The row's only `nz-button`: the expander is a plain button. + const terminateButtons = () => + Array.from<HTMLButtonElement>(fixture.nativeElement.querySelectorAll("button[nz-button]")); + const first = makeUnit(); + const second = makeUnit(withCu({ cuid: 2, name: "second" })); + + it("gives every row a terminate button, labelled for its unit type", () => { + vi.mocked(service.listAllComputingUnits).mockReturnValue(of([first, localUnit(2)])); + + fixture.detectChanges(); + + expect(terminateButtons().map(b => b.getAttribute("aria-label"))).toEqual([ + "Terminate this computing unit", + "Disconnect from this computing unit", + ]); + }); + + it("asks to confirm the termination of the unit on the clicked row", () => { + const confirm = stubConfirm(); + vi.mocked(service.listAllComputingUnits).mockReturnValue(of([first, second])); + fixture.detectChanges(); + + terminateButtons()[1].click(); + + expect(confirm).toHaveBeenCalledTimes(1); + expect(confirm).toHaveBeenCalledWith(2, second, expect.any(Function)); + }); + + it("drops only that row once the termination succeeds, without waiting for the next poll", () => { + stubConfirm(true); + vi.mocked(service.listAllComputingUnits).mockReturnValue(of([first, second])); + fixture.detectChanges(); + + terminateButtons()[0].click(); + fixture.detectChanges(); + + expect(component.computingUnits.map(u => u.computingUnit.cuid)).toEqual([2]); + expect(dataRows()).toHaveLength(1); + }); + + // The modal can be cancelled and the request can fail, and in both the unit lives on. + it("keeps the row until the termination has succeeded", () => { + stubConfirm(); + vi.mocked(service.listAllComputingUnits).mockReturnValue(of([first, second])); + fixture.detectChanges(); + + terminateButtons()[0].click(); + fixture.detectChanges(); + + expect(dataRows()).toHaveLength(2); + }); + + // The actions column is the seventh after the expander, and the detail row must span all of them. + it("spans the expanded detail row across every column, the actions one included", () => { + vi.mocked(service.listAllComputingUnits).mockReturnValue(of([first])); + component.expandedCuids.add(first.computingUnit.cuid); + + fixture.detectChanges(); + + const detailCell = specDetail()!.closest("td")!; + expect(detailCell.getAttribute("colspan")).toBe("7"); + }); + }); + describe("resourceSummary", () => { it("joins CPU, memory and GPU with a middot and labels", () => { const unit = makeUnit(withResource({ gpuLimit: "1" })); diff --git a/frontend/src/app/dashboard/component/admin/computing-unit/admin-computing-unit.component.ts b/frontend/src/app/dashboard/component/admin/computing-unit/admin-computing-unit.component.ts index 88a9f84b11..3ff5f5c901 100644 --- a/frontend/src/app/dashboard/component/admin/computing-unit/admin-computing-unit.component.ts +++ b/frontend/src/app/dashboard/component/admin/computing-unit/admin-computing-unit.component.ts @@ -35,7 +35,9 @@ import { NzTableFilterFn, } from "ng-zorro-antd/table"; import { NzAlertComponent } from "ng-zorro-antd/alert"; +import { NzButtonComponent } from "ng-zorro-antd/button"; import { NzCardComponent } from "ng-zorro-antd/card"; +import { NzIconDirective } from "ng-zorro-antd/icon"; import { NzBadgeComponent } from "ng-zorro-antd/badge"; import { NzTooltipDirective } from "ng-zorro-antd/tooltip"; import { NzSpinComponent } from "ng-zorro-antd/spin"; @@ -45,7 +47,12 @@ import { DashboardWorkflowComputingUnit, WorkflowComputingUnitResourceLimit, } from "../../../../common/type/workflow-computing-unit"; -import { getComputingUnitBadgeColor, getComputingUnitStatusTooltip } from "../../../../common/util/computing-unit.util"; +import { ComputingUnitActionsService } from "../../../../common/service/computing-unit/computing-unit-actions/computing-unit-actions.service"; +import { + getComputingUnitBadgeColor, + getComputingUnitStatusTooltip, + unitTypeMessageTemplate, +} from "../../../../common/util/computing-unit.util"; import { formatRelativeTime } from "../../../../common/util/format.util"; import { extractErrorMessage } from "../../../../common/util/error"; import { UserAvatarComponent } from "../../user/user-avatar/user-avatar.component"; @@ -68,7 +75,9 @@ type SpecKey = Exclude<keyof WorkflowComputingUnitResourceLimit, "nodeAddresses" styleUrls: ["./admin-computing-unit.component.scss"], imports: [ NzAlertComponent, + NzButtonComponent, NzCardComponent, + NzIconDirective, NzTableComponent, NzTheadComponent, NzTbodyComponent, @@ -94,6 +103,9 @@ export class AdminComputingUnitComponent implements OnInit { pollFailing = false; lastUpdated?: Date; readonly expandedCuids = new Set<number>(); + // Units terminated from this page. A poll that started before the `DELETE` can still answer with them, so every + // response is filtered by this set. A `cuid` is never reused, so there is no need to prune it. + private readonly terminatedCuids = new Set<number>(); readonly getBadgeColor = getComputingUnitBadgeColor; readonly getStatusTooltip = getComputingUnitStatusTooltip; @@ -133,6 +145,7 @@ export class AdminComputingUnitComponent implements OnInit { constructor( private computingUnitService: WorkflowComputingUnitManagingService, + private computingUnitActionsService: ComputingUnitActionsService, private messageService: NzMessageService ) {} @@ -162,7 +175,7 @@ export class AdminComputingUnitComponent implements OnInit { .subscribe(units => { this.pollFailing = false; this.lastUpdated = new Date(); - this.computingUnits = units; + this.computingUnits = this.withoutTerminated(units); }); } @@ -183,6 +196,23 @@ export class AdminComputingUnitComponent implements OnInit { return unit.computingUnit.type === "local"; } + /** Confirms the termination, then drops the row at once instead of at the next poll. */ + terminate(unit: DashboardWorkflowComputingUnit): void { + const cuid = unit.computingUnit.cuid; + this.computingUnitActionsService.confirmAndTerminate(cuid, unit, () => { + this.terminatedCuids.add(cuid); + this.computingUnits = this.withoutTerminated(this.computingUnits); + }); + } + + terminateTooltip(unit: DashboardWorkflowComputingUnit): string { + return unitTypeMessageTemplate[unit.computingUnit.type].terminateTooltip; + } + + private withoutTerminated(units: ReadonlyArray<DashboardWorkflowComputingUnit>): DashboardWorkflowComputingUnit[] { + return units.filter(u => !this.terminatedCuids.has(u.computingUnit.cuid)); + } + /** The Resources column, e.g. "2 CPU · 4Gi · 1 GPU", with GPU left out when it is 0. */ resourceSummary(unit: DashboardWorkflowComputingUnit): string { if (this.isLocal(unit)) {
