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 94fd340d3ab..54f2573708f 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 = 'Confirm & x'; + 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 2b67a8b2bae..49047b299ae 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 49f5a60520b..d0faa5b6f4d 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 @@ + nzWidth="18%"> Name + nzWidth="18%"> Owner + nzWidth="10%"> Type Created - Resources + Resources + Actions @@ -106,10 +107,23 @@ {{ formatRelativeTime(unit.computingUnit.creationTime) }} {{ resourceSummary(unit) }} + + + - +
{{ field.label }}
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 ce692f4d885..88a6ee5a081 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): 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(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(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(); + 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[] = []; @@ -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(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 88a9f84b11d..3ff5f5c901f 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(); + // 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(); 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[] { + 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)) {