[server] abort running prebuilds on same branch #10962

svenefftinge · 2022-06-28T08:12:30Z

Description

Cancels all running and queued prebuilds for the same branch when a new commit is pushed.
Adds a project setting to opt-out of this behavior.

Related Issue(s)

Fixes #6569

How to test

Add a project with a long prebuild task
push multipüle times to the same branch
see how the previous prebuilds get cancelled.
push to a different branch and verify that running prebuilds on the other branch are not getting cancelled.
disable the project setting and verify that the cancellation doesn't happen

Release Notes

Outdated prebuilds (i.e. new commits are pushed on a branch) are automatically canceled. This behavior can be disabled in the project's settings.

Documentation

Werft options:

/werft with-preview

werft-gitpod-dev-com · 2022-06-28T08:12:41Z

started the job as gitpod-build-se-playground.11 because the annotations in the pull request description changed
(with .werft/ from main)

jankeromnes · 2022-06-29T11:10:07Z

Many thanks @svenefftinge for continuously improving & optimizing which prebuilds we should be running or not! 💯

I'll give this Pull Request a look. 👀

components/dashboard/src/projects/ProjectSettings.tsx

jankeromnes · 2022-06-29T11:19:42Z

components/dashboard/src/projects/ProjectSettings.tsx

@@ -56,19 +55,14 @@ export default function () {
    const { teams } = useContext(TeamsContext);
    const team = getCurrentTeam(location, teams);

-    const [isLoading, setIsLoading] = useState<boolean>(true);


❤️ Many thanks for removing a isLoading boolean! (which I might have added 🙈)

I agree that maintaining a isLoading boolean is generally bad practice, and creates many opportunities for bugs. (And when a loading state is required, it's always better to explicit wait on what still needs to be loaded, instead of trying to code this into a isLoading boolean.) 👍

components/dashboard/src/projects/ProjectSettings.tsx

jankeromnes

Thanks again! Had an initial pass through the code, and it looks good to me, modulo a few (mostly optional) questions.

Will now give the PR a go, and try to find some edge cases (e.g. image builds?)

jankeromnes · 2022-06-29T11:26:10Z

components/dashboard/src/projects/ProjectSettings.tsx

                onChange={toggleIncrementalPrebuilds}
            />
+            <CheckBox
+                title={<span>Cancel Prebuilds on Outdated Commits </span>}
+                desc={<span>Cancels all pending and running prebuilds on the branch when a new commit is pushed.</span>}


In general, I think that this feature is super valuable and needed, and I'm happy that we're taking a first step towards it. 💯

Without having read the code much further or tested it, one initial thought that occurs is that, if one of your branches (e.g. main) takes a long time to build, and sees very frequent commits (e.g. during working hours), we might be always-cancelling running prebuilds without ever finishing one, for long stretches of time (e.g. maybe we only successfully finish running a prebuild over lunch break or at the end of the day).

components/dashboard/src/projects/ProjectSettings.tsx

jankeromnes · 2022-06-29T11:42:05Z

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

+        const results: Promise<any>[] = [];
+        for (const prebuild of prebuilds) {
+            for (const instance of prebuild.instances) {
+                results.push(this.workspaceStarter.stopWorkspaceInstance({ span }, instance.id, instance.region));


I wonder what happens for instances which are not in a "stoppable" state, e.g. in image build? Will this fail for some not-yet-stoppable instances, or will this wait until it reaches a stoppable state? (I'm guessing the former.)

I don't know either but we should really move the responsibility to the ws-manager.

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

jankeromnes · 2022-06-29T11:44:36Z

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

+        req.setId(instanceId);
+        req.setPolicy(policy || StopWorkspacePolicy.NORMALLY);
+
+        const client = await this.clientProvider.get(instanceRegion);


If I remember correctly, some instances might not have a region yet (e.g. still in image build or so?) and this could cause client to be incorrect or undefined. Is this still the case? 🤔 I guess this needs to be tested.

jankeromnes · 2022-06-29T11:48:53Z

Err, will actually test this PR later today (in ~1.5 hours) as I have upcoming meetings. Please feel free to push any changes if you want to (i.e. no need to necessarily keep the preview env alive until then).

geropl · 2022-06-29T12:30:43Z

@svenefftinge Do you have any plans on announcing this change? This will certainly affect some heavy Gitpod users. 🤔

svenefftinge · 2022-06-29T13:08:36Z

This will certainly affect some heavy Gitpod users. 🤔

How do you think it will affect users? In a negative way you mean?

svenefftinge · 2022-06-29T13:09:56Z

The only path I can imagine where users might be surprised is seeing prebuilds "CANCELED". We should probably tell them why.
I'm adding the status descriptions as a tooltip for the items in the list and also add the information about new commits being pushed on the branch:

jankeromnes · 2022-06-29T13:43:36Z

I'm adding the status descriptions as a tooltip for the items in the list and also add the information about new commits being pushed on the branch:

Good idea! 👍 I wonder if it also makes sense to tell users that they can click on the prebuild in order to know the exact cancellation reason. (It should always be visible in the full error message below the prebuild logs.)

svenefftinge · 2022-06-29T13:45:32Z

Good idea! 👍 I wonder if it also makes sense to tell users that they can click on the prebuild in order to know the exact cancellation reason. (It should always be visible in the full error message below the prebuild logs.)

It is the same message

gtsiolis · 2022-06-29T14:27:11Z

FWIW, auto-cancelling redundant builds is a common practice in CI systems[1][2][...].

In the future, we could introduce a preference option in project settings but going with a sensible default[3] sounds better.

svenefftinge · 2022-06-29T14:37:14Z

In the future, we could introduce a preference option in project settings but going with a sensible default[3] sounds better.

This PR already comes with that preference (default is ☑️ ):

gtsiolis

This PR already comes with that preference (default is ☑️ )

Perfect (Perfect)! ✨

Left one comment for updating the copy used in settings. Feel free to ignore or adjust the suggestion. 🏓

gtsiolis · 2022-06-29T15:32:41Z

components/dashboard/src/projects/ProjectSettings.tsx

                onChange={toggleIncrementalPrebuilds}
            />
+            <CheckBox
+                title={<span>Cancel Prebuilds on Outdated Commits </span>}
+                desc={<span>Cancels all pending and running prebuilds on the branch when a new commit is pushed.</span>}


Suggested change

desc={<span>Cancels all pending and running prebuilds on the branch when a new commit is pushed.</span>}

desc={<span>New prebuilds cancel older pending or running prebuilds on the same branch when new commits are pushed.</span>}

components/dashboard/src/projects/ProjectSettings.tsx

jankeromnes · 2022-06-29T17:20:26Z

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

+            for (const instance of prebuild.instances) {
+                results.push(this.workspaceStarter.stopWorkspaceInstance({ span }, instance.id, instance.region));
+            }
+            prebuild.prebuild.state = "aborted";


Please also specify the cancellation reason by setting a helpful prebuild.error value, similar to the rate-limit error:

Suggested change

prebuild.prebuild.state = "aborted";

prebuild.prebuild.state = "aborted";

prebuild.error = "A newer commit was pushed to the same branch.";

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

jankeromnes

Many thanks! In addition to the code looking good to me earlier, I've tested this PR and it works as advertised. ✨ 🎯

For what it's worth, I also tested the behavior with image builds (by pushing a custom Dockerile and then several commits on top) -- the Prebuilds actually get cancelled, even though the logs continue to receive new updates. 😅 I guess that's fine. 👍

Adding a hold because I'd love the cancellation reason to be explicitly added as a prebuild.error, or in case someone else would like to review.

/hold

svenefftinge · 2022-06-30T06:13:07Z

I'd love the cancellation reason to be explicitly added as a prebuild.error,

Will do in a follow up PR.

roboquat added do-not-merge/work-in-progress do-not-merge/release-note-label-needed labels Jun 28, 2022

roboquat added the size/L label Jun 28, 2022

svenefftinge force-pushed the se/playground branch 3 times, most recently from 3a680c4 to 9aaa1f6 Compare June 28, 2022 10:36

roboquat added release-note and removed do-not-merge/release-note-label-needed labels Jun 28, 2022

svenefftinge force-pushed the se/playground branch from 9aaa1f6 to ec15b00 Compare June 28, 2022 10:50

svenefftinge marked this pull request as ready for review June 28, 2022 10:53

svenefftinge requested a review from a team June 28, 2022 10:53

roboquat removed the do-not-merge/work-in-progress label Jun 28, 2022

github-actions bot added the team: webapp Issue belongs to the WebApp team label Jun 28, 2022

svenefftinge force-pushed the se/playground branch from ec15b00 to d8394ab Compare June 28, 2022 11:15

jankeromnes self-assigned this Jun 29, 2022

jankeromnes reviewed Jun 29, 2022

View reviewed changes

components/dashboard/src/projects/ProjectSettings.tsx Show resolved Hide resolved

jankeromnes reviewed Jun 29, 2022

View reviewed changes

components/dashboard/src/projects/ProjectSettings.tsx Show resolved Hide resolved

jankeromnes reviewed Jun 29, 2022

View reviewed changes

[server] abort running prebuilds on same branch

0b66fda

svenefftinge force-pushed the se/playground branch from d8394ab to 0b66fda Compare June 29, 2022 13:41

gtsiolis approved these changes Jun 29, 2022

View reviewed changes

gtsiolis reviewed Jun 29, 2022

View reviewed changes

components/dashboard/src/projects/ProjectSettings.tsx Show resolved Hide resolved

jankeromnes reviewed Jun 29, 2022

View reviewed changes

jankeromnes approved these changes Jun 29, 2022

View reviewed changes

roboquat added the do-not-merge/hold label Jun 29, 2022

svenefftinge removed the do-not-merge/hold label Jun 30, 2022

roboquat merged commit ba78bd4 into main Jun 30, 2022

roboquat deleted the se/playground branch June 30, 2022 06:10

svenefftinge mentioned this pull request Jun 30, 2022

[prebuilds] store cancel reason #11022

Merged

1 task

This was referenced Jun 30, 2022

Update spacing in project settings #10945

Closed

Update project settings preference copy #11031

Merged

roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Jun 30, 2022

	desc={<span>Cancels all pending and running prebuilds on the branch when a new commit is pushed.</span>}
	desc={<span>New prebuilds cancel older pending or running prebuilds on the same branch when new commits are pushed.</span>}

[server] abort running prebuilds on same branch #10962

[server] abort running prebuilds on same branch #10962

Uh oh!

Conversation

svenefftinge commented Jun 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue(s)

How to test

Release Notes

Documentation

Werft options:

Uh oh!

werft-gitpod-dev-com bot commented Jun 28, 2022

Uh oh!

jankeromnes commented Jun 29, 2022 • edited by werft-gitpod-dev-com bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jankeromnes Jun 29, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jankeromnes left a comment

Choose a reason for hiding this comment

Uh oh!

jankeromnes Jun 29, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jankeromnes Jun 29, 2022

Choose a reason for hiding this comment

Uh oh!

svenefftinge Jun 29, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jankeromnes Jun 29, 2022

Choose a reason for hiding this comment

Uh oh!

jankeromnes commented Jun 29, 2022

Uh oh!

geropl commented Jun 29, 2022

Uh oh!

svenefftinge commented Jun 29, 2022 • edited by werft-gitpod-dev-com bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

svenefftinge commented Jun 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jankeromnes commented Jun 29, 2022 • edited by werft-gitpod-dev-com bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

svenefftinge commented Jun 29, 2022 • edited by werft-gitpod-dev-com bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gtsiolis commented Jun 29, 2022

Uh oh!

svenefftinge commented Jun 29, 2022 • edited by werft-gitpod-dev-com bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gtsiolis left a comment

Choose a reason for hiding this comment

Uh oh!

gtsiolis Jun 29, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jankeromnes Jun 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jankeromnes left a comment

Choose a reason for hiding this comment

Uh oh!

svenefftinge commented Jun 30, 2022

Uh oh!

Uh oh!

svenefftinge commented Jun 28, 2022 •

edited

Loading

jankeromnes commented Jun 29, 2022 •

edited by werft-gitpod-dev-com bot

Loading

svenefftinge commented Jun 29, 2022 •

edited by werft-gitpod-dev-com bot

Loading

svenefftinge commented Jun 29, 2022 •

edited

Loading

jankeromnes commented Jun 29, 2022 •

edited by werft-gitpod-dev-com bot

Loading

svenefftinge commented Jun 29, 2022 •

edited by werft-gitpod-dev-com bot

Loading

svenefftinge commented Jun 29, 2022 •

edited by werft-gitpod-dev-com bot

Loading

jankeromnes Jun 29, 2022 •

edited

Loading