[server][dashboard] Allow cancelling Prebuilds #5865

jankeromnes · 2021-09-24T14:41:44Z

Description

Implement a server API method to cancel prebuilds, and point all the Prebuilds controls in the UI to it.

Also show cancelled Prebuilds with a new CANCELED state (in gray).

Related Issue(s)

Fixes #5612
Fixes #6153

How to test

In the Project Configurator, run a Prebuild, and then cancel it. Repeat once.
Trigger a Prebuild, then cancel it from the Prebuilds page.
Same thing, but from the Project a.k.a. Branches page.

Release Notes

Make it possible to cancel pending or running Prebuilds

/uncc

components/server/ee/src/prebuilds/prebuild-manager.ts

jankeromnes · 2021-10-01T13:08:56Z

components/server/ee/src/prebuilds/prebuild-manager.ts

@@ -107,7 +110,8 @@ export class PrebuildManager {

            span.setTag("starting", true);
            await this.workspaceStarter.startWorkspace({ span }, workspace, user, [], {excludeFeatureFlags: ['full_workspace_backup']});
-            return { wsid: workspace.id, done: false };
+            const prebuild = (await prebuildPromise)!;


I'm just slightly worried about this code:

Logically, findPrebuildByWorkspaceID(workspace.id) should always find a prebuild, given that the workspace was created from a StartPrebuildContext context

However, what if there is an edge case, or a new bug in the future 😱

indeed, it seems to lack a contract here, i.e. it seems easy to break because it relies on assumptions, though it's not changed frequently.

perhaps a dedicated workspaceFactory.createPrebuild(..., context): { ws: Workspace, pb: Prebuild } would document the requirements more maintainable? just thinking out loud...

jankeromnes · 2021-10-01T13:09:11Z

components/server/ee/src/prebuilds/prebuild-manager.ts

            if (!workspace) {
                console.error('Unknown workspace id.', { workspaceId });
                throw new Error('Unknown workspace ' + workspaceId);
            }
+            const prebuild = (await prebuildPromise)!;


Same here (see previous comment)

jankeromnes · 2021-10-01T13:10:42Z

components/server/src/workspace/workspace-starter.ts

@@ -265,6 +265,7 @@ export class WorkspaceStarter {
                    await this.workspaceDb.trace({ span }).storePrebuiltWorkspace(prebuild)
                    await this.messageBus.notifyHeadlessUpdate({span}, workspace.ownerId, workspace.id, <HeadlessWorkspaceEvent>{
                        type: HeadlessWorkspaceEventType.Aborted,
+                        // TODO: `workspaceID: workspace.id` not needed here? (found in ee/src/prebuilds/prebuild-queue-maintainer.ts and ee/src/bridge.ts)


@csweichel We've never added a workspaceID here, but I see it in all other occurrences of notifyHeadlessUpdate. Is it really okay to omit it here? (Couldn't find who listens to this.)

AlexTugarev · 2021-10-04T07:03:48Z

/werft run

👍 started the job as gitpod-build-jx-cancel-prebuild.16

AlexTugarev · 2021-10-04T07:20:19Z

components/server/ee/src/prebuilds/prebuild-manager.ts

+            prebuild.state = 'aborted';
+            prebuild.error = 'Cancelled manually';
+            await this.workspaceDB.trace({ span }).storePrebuiltWorkspace(prebuild);
+            await this.messageBus.notifyHeadlessUpdate({ span }, user.id, prebuild.buildWorkspaceId, <HeadlessWorkspaceEvent>{


how does this play together with state transitions driven by the bridge?
the bridge is receiving status updates from ws manager and does update the prebuild workspace "in parallel". there could be races of storePrebuiltWorkspace calls, potentially leading to override the Aborted message?

shouldn't the cancellation be a request to the wsman, for which in turn the bridge would to the notifications and updates from this change?

Good question, thanks! To be honest, I simply copied the prebuild cancellation logic from bridge.ts:

gitpod/components/ws-manager-bridge/ee/src/bridge.ts

Lines 77 to 96 in 6bc80eb

} else if (!!status.conditions!.failed) {

prebuild.state = "aborted";

prebuild.error = status.conditions!.failed;

headlessUpdateType = HeadlessWorkspaceEventType.Aborted;

} else if (!!status.conditions!.headlessTaskFailed) {

prebuild.state = "available";

prebuild.error = status.conditions!.headlessTaskFailed;

prebuild.snapshot = status.conditions!.snapshot;

headlessUpdateType = HeadlessWorkspaceEventType.FinishedButFailed;

} else {

prebuild.state = "available";

prebuild.snapshot = status.conditions!.snapshot;

headlessUpdateType = HeadlessWorkspaceEventType.FinishedSuccessfully;

}

await this.workspaceDB.trace({span}).storePrebuiltWorkspace(prebuild);

await this.messagebus.notifyHeadlessUpdate({span}, userId, workspaceId, <HeadlessWorkspaceEvent>{

type: headlessUpdateType,

workspaceID: workspaceId,

});

and also from how it was implemented in the (now-deleted) PrebuildQueueMaintainer:

gitpod/components/server/ee/src/prebuilds/prebuild-queue-maintainer.ts

Lines 144 to 175 in 2aa2db6

protected async discardPrebuild(ctx: TraceContext, prebuild: PrebuiltWorkspace) {

const span = TraceContext.startSpan("PrebuildQueueMaintainer.discardPrebuild", {})

try {

prebuild.state = 'aborted';

this.workspaceDB.trace({span}).storePrebuiltWorkspace(prebuild);

let userId: string;

const workspace = await this.workspaceDB.trace({span}).findById(prebuild.buildWorkspaceId);

if (workspace) {

userId = workspace.ownerId;

} else {

// this is a bit unorthodox but should still work: the userId is used to build the workspace exchange topic

// on which we send the message. We hardly every (if ever at all) listen for user-specific updates, the prebuild

// status maintainer certainly doesn't. Thus, even if userId is unknown, most parts of the system should still work.

//

// Mind, this branch is really the exception. Other parts of the system will have broken before we get here, i.e.

// see a prebuild workspace without an actual workspace attached.

userId = 'unknown';

}

await this.messagebus.notifyHeadlessUpdate({span}, userId, prebuild.buildWorkspaceId, <HeadlessWorkspaceEvent>{

type: HeadlessWorkspaceEventType.Aborted,

workspaceID: prebuild.buildWorkspaceId,

});

} catch (err) {

TraceContext.logError({span}, err);

throw err;

} finally {

span.finish();

}

}

I don't quite understand the full state machine yet, but it sounds like I should look more deeply into it. 👀

@jankeromnes, first of https://github.com/gitpod-io/gitpod/pull/5835/files

then, you see it's state propagation from the ws-manager-bridge which runs outside of server. which is the whole point: ws-manager running in region A and propagating event from there, but then you want to cancel a prebuild from server in region B. in such cases I'm puzzled about the stream of states a frontend would receive, thus the message for the user would be confusing.

in general, if we want something to happen with the workspace, we're sending requests to the ws-manager of the according region, which is already taken care of by the client connection as far as know. btw, this is what happens for the stop part of this new composite action called cancelPrebuild. I think what needs to be done here is to introduce a CancelPrebuild request in ws-manager, which basically does the stopping but also signals the action to the bridge to do the rest, i.e. update db and push notifications.

After our sync discussion, it seems we'll want to remove the new prebuildManager.cancelPrebuild call, and instead make stopWorkspace also cancel the prebuild (inside the correct ws-manager, e.g. with a new flag on the StopWorkspaceRequest if necessary).

Idea: When stopping a headless workspace manually, set conditions.headlessTaskFailed to something like 'Cancelled manually.

technically that would work.

// headless_task_failed indicates that a headless workspace task failed

just the semantics seems to be different. this value is currently derived from Message regarding the last termination of the container

@csweichel, what's the best way to signal a cancelled prebuild?

@AlexTugarev Note: See also this Slack thread (internal)

The way this builds it out, incoming updates from ws-manager-bridge could overwrite the status.
What about making cancelPrebuild call StopWorkspace on the prebuild workspace and have the supervisor write a message to the termination log when its stopped while the tasks are still running?

If you wanted to provide feedback that cancelation is happening, you could introduce a state to the prebuild that indicates this. This way you could also add a reconciliation loop in bridge that calls StopWorkspace if need be.

AlexTugarev · 2021-10-04T07:36:34Z

components/server/ee/src/workspace/gitpod-server-impl.ts

@@ -1554,6 +1554,28 @@ export class GitpodServerEEImpl extends GitpodServerImpl<GitpodClient, GitpodSer
        return prebuild;
    }

+    async cancelPrebuild(projectId: string, prebuildId: string): Promise<void> {


in theory we could get the projectId from the workspace of that prebuild.
what's the advantage of adding it to the list of args, besides remove a couple of db queries?

what's the advantage of adding it to the list of args, besides remove a couple of db queries?

None. Removing a few unnecessary DB queries sounds great, and this is information we typically already have when making the call, so we might as well not omit it. 🙂 Do you see it otherwise?

AlexTugarev · 2021-10-04T08:33:57Z

Really enjoying to get some control over the running prebuild! That's an awesome feature 🎉

gtsiolis · 2021-10-04T15:17:25Z

/werft run

👍 started the job as gitpod-build-jx-cancel-prebuild.17

jankeromnes · 2021-10-15T13:41:47Z

Okay, ready for review again, now using the conditions.stoppedByRequest added in #6218 ❤️

A few screenshots at random:

@AlexTugarev @gtsiolis please take a look when you have time. 🙏

AlexTugarev · 2021-10-18T07:46:56Z

/werft run

👍 started the job as gitpod-build-jx-cancel-prebuild.24

AlexTugarev · 2021-10-18T08:11:30Z

/lgtm

this works really nicely and improves iterations on the config. 👍🏻

roboquat · 2021-10-18T08:11:34Z

LGTM label has been added.

Git tree hash: 33a115dabe21107b63953cca6e21d038f5902fee

roboquat · 2021-10-18T08:11:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AlexTugarev

Associated issue: #5612

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~components/dashboard/OWNERS~~ [AlexTugarev]
~~components/gitpod-protocol/OWNERS~~ [AlexTugarev]
~~components/server/OWNERS~~ [AlexTugarev]
~~components/ws-manager-bridge/OWNERS~~ [AlexTugarev]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jankeromnes added the approved label Sep 24, 2021

roboquat added do-not-merge/work-in-progress release-note size/M labels Sep 24, 2021

csweichel reviewed Sep 27, 2021

View reviewed changes

components/server/ee/src/prebuilds/prebuild-manager.ts Outdated Show resolved Hide resolved

jankeromnes force-pushed the jx/cancel-prebuild branch 7 times, most recently from 8bc80a1 to b034f03 Compare October 1, 2021 12:32

roboquat added size/L and removed size/M labels Oct 1, 2021

jankeromnes marked this pull request as ready for review October 1, 2021 12:59

roboquat removed the do-not-merge/work-in-progress label Oct 1, 2021

jankeromnes force-pushed the jx/cancel-prebuild branch from b034f03 to 79fd61c Compare October 1, 2021 13:03

jankeromnes commented Oct 1, 2021

View reviewed changes

jankeromnes force-pushed the jx/cancel-prebuild branch 3 times, most recently from f8ca4d1 to 1763be7 Compare October 1, 2021 14:26

jankeromnes requested review from AlexTugarev and gtsiolis October 1, 2021 15:33

AlexTugarev reviewed Oct 4, 2021

View reviewed changes

gtsiolis mentioned this pull request Oct 11, 2021

Mark canceled prebuilds as canceled instead of failed #6153

Closed

AlexTugarev requested a review from csweichel October 14, 2021 13:27

jankeromnes mentioned this pull request Oct 14, 2021

Add new workspace condition "stoppedByRequest" #6213

Closed

3 tasks

jankeromnes marked this pull request as draft October 14, 2021 14:16

roboquat added the do-not-merge/work-in-progress label Oct 14, 2021

This comment has been minimized.

Sign in to view

jankeromnes force-pushed the jx/cancel-prebuild branch from 1763be7 to 9c5a4b9 Compare October 15, 2021 07:47

roboquat added the team: webapp Issue belongs to the WebApp team label Oct 15, 2021

jankeromnes removed the request for review from csweichel October 15, 2021 07:55

jankeromnes force-pushed the jx/cancel-prebuild branch 3 times, most recently from 1d15e1a to 4661ddf Compare October 15, 2021 13:24

jankeromnes marked this pull request as ready for review October 15, 2021 13:24

roboquat removed the do-not-merge/work-in-progress label Oct 15, 2021

jankeromnes force-pushed the jx/cancel-prebuild branch from 4661ddf to 1ccc425 Compare October 15, 2021 13:43

[server][dashboard] Allow cancelling Prebuilds

be09e48

jankeromnes force-pushed the jx/cancel-prebuild branch from 1ccc425 to be09e48 Compare October 15, 2021 13:55

jankeromnes requested review from AlexTugarev and gtsiolis October 15, 2021 13:55

roboquat assigned AlexTugarev Oct 18, 2021

roboquat added the lgtm label Oct 18, 2021

roboquat merged commit bad8372 into main Oct 18, 2021

roboquat deleted the jx/cancel-prebuild branch October 18, 2021 08:12

roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Oct 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[server][dashboard] Allow cancelling Prebuilds #5865

[server][dashboard] Allow cancelling Prebuilds #5865

jankeromnes commented Sep 24, 2021 •

edited

Loading

jankeromnes Oct 1, 2021

AlexTugarev Oct 4, 2021

jankeromnes Oct 1, 2021

jankeromnes Oct 1, 2021

AlexTugarev commented Oct 4, 2021 •

edited by werft-gitpod-dev-com bot

Loading

AlexTugarev Oct 4, 2021

jankeromnes Oct 4, 2021

AlexTugarev Oct 4, 2021

jankeromnes Oct 4, 2021

jankeromnes Oct 14, 2021

AlexTugarev Oct 14, 2021

AlexTugarev Oct 14, 2021

jankeromnes Oct 14, 2021

csweichel Oct 14, 2021

AlexTugarev Oct 4, 2021

jankeromnes Oct 4, 2021

AlexTugarev commented Oct 4, 2021 •

edited by werft-gitpod-dev-com bot

Loading

gtsiolis commented Oct 4, 2021 •

edited by werft-gitpod-dev-com bot

Loading

This comment has been minimized.

jankeromnes commented Oct 15, 2021 •

edited

Loading

AlexTugarev commented Oct 18, 2021 •

edited by werft-gitpod-dev-com bot

Loading

AlexTugarev commented Oct 18, 2021 •

edited by werft-gitpod-dev-com bot

Loading

roboquat commented Oct 18, 2021

roboquat commented Oct 18, 2021

	} else if (!!status.conditions!.failed) {
	prebuild.state = "aborted";
	prebuild.error = status.conditions!.failed;
	headlessUpdateType = HeadlessWorkspaceEventType.Aborted;
	} else if (!!status.conditions!.headlessTaskFailed) {
	prebuild.state = "available";
	prebuild.error = status.conditions!.headlessTaskFailed;
	prebuild.snapshot = status.conditions!.snapshot;
	headlessUpdateType = HeadlessWorkspaceEventType.FinishedButFailed;
	} else {
	prebuild.state = "available";
	prebuild.snapshot = status.conditions!.snapshot;
	headlessUpdateType = HeadlessWorkspaceEventType.FinishedSuccessfully;
	}
	await this.workspaceDB.trace({span}).storePrebuiltWorkspace(prebuild);

	await this.messagebus.notifyHeadlessUpdate({span}, userId, workspaceId, <HeadlessWorkspaceEvent>{
	type: headlessUpdateType,
	workspaceID: workspaceId,
	});

	protected async discardPrebuild(ctx: TraceContext, prebuild: PrebuiltWorkspace) {
	const span = TraceContext.startSpan("PrebuildQueueMaintainer.discardPrebuild", {})

	try {
	prebuild.state = 'aborted';
	this.workspaceDB.trace({span}).storePrebuiltWorkspace(prebuild);

	let userId: string;
	const workspace = await this.workspaceDB.trace({span}).findById(prebuild.buildWorkspaceId);
	if (workspace) {
	userId = workspace.ownerId;
	} else {
	// this is a bit unorthodox but should still work: the userId is used to build the workspace exchange topic
	// on which we send the message. We hardly every (if ever at all) listen for user-specific updates, the prebuild
	// status maintainer certainly doesn't. Thus, even if userId is unknown, most parts of the system should still work.
	//
	// Mind, this branch is really the exception. Other parts of the system will have broken before we get here, i.e.
	// see a prebuild workspace without an actual workspace attached.
	userId = 'unknown';
	}

	await this.messagebus.notifyHeadlessUpdate({span}, userId, prebuild.buildWorkspaceId, <HeadlessWorkspaceEvent>{
	type: HeadlessWorkspaceEventType.Aborted,
	workspaceID: prebuild.buildWorkspaceId,
	});
	} catch (err) {
	TraceContext.logError({span}, err);
	throw err;
	} finally {
	span.finish();
	}
	}

[server][dashboard] Allow cancelling Prebuilds #5865

[server][dashboard] Allow cancelling Prebuilds #5865

Conversation

jankeromnes commented Sep 24, 2021 • edited Loading

Description

Related Issue(s)

How to test

Release Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexTugarev commented Oct 4, 2021 • edited by werft-gitpod-dev-com bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexTugarev commented Oct 4, 2021 • edited by werft-gitpod-dev-com bot Loading

gtsiolis commented Oct 4, 2021 • edited by werft-gitpod-dev-com bot Loading

This comment has been minimized.

jankeromnes commented Oct 15, 2021 • edited Loading

AlexTugarev commented Oct 18, 2021 • edited by werft-gitpod-dev-com bot Loading

AlexTugarev commented Oct 18, 2021 • edited by werft-gitpod-dev-com bot Loading

roboquat commented Oct 18, 2021

roboquat commented Oct 18, 2021

jankeromnes commented Sep 24, 2021 •

edited

Loading

AlexTugarev commented Oct 4, 2021 •

edited by werft-gitpod-dev-com bot

Loading

AlexTugarev commented Oct 4, 2021 •

edited by werft-gitpod-dev-com bot

Loading

gtsiolis commented Oct 4, 2021 •

edited by werft-gitpod-dev-com bot

Loading

jankeromnes commented Oct 15, 2021 •

edited

Loading

AlexTugarev commented Oct 18, 2021 •

edited by werft-gitpod-dev-com bot

Loading

AlexTugarev commented Oct 18, 2021 •

edited by werft-gitpod-dev-com bot

Loading