Implement a 'Use Last Successful Prebuild' workspace creation mode

[jankeromnes]

Description

Implement a 'Use Last Successful Prebuild' workspace creation mode, and companion button in the "Prebuild In Progress" screen.

Related Issue(s)

A first step towards #12582

How to test

1. Trigger a long-running prebuild, and let it complete successfully
2. Trigger a new long-running prebuild by pushing a new commit (do NOT modify the .gitpod.yml or Dockerfile here!)
3. Without waiting for the 2nd prebuild to complete, open the new commit in Gitpod
4. In the "Prebuild in Progress" screen, you should see a new button: "Use Last Successful Prebuild"
5. Clicking that button should give you a workspace based on the older successful prebuild (fast, no waiting)

Release Notes

Implement a 'Use Last Successful Prebuild' workspace creation mode

Documentation

Werft options:

• /werft with-local-preview
  If enabled this will build install/preview
• /werft with-preview
• /werft with-integration-tests=all
  Valid options are all, workspace, webapp, ide

[werft-gitpod-dev-com]

started the job as gitpod-build-jx-incremental-workspace.1 because the annotations in the pull request description changed
(with .werft/ from main)

[jankeromnes]

Okay, this early proof-of-concept should be ready for testing! 🎉

It adds a Use Last Successful Prebuild button to the Prebuild in Progress... screen.

To get the button and test it:

1. Create a new repository, with a long-running init task. For example, this could be the .gitpod.yml:

tasks:
  - init: sleep 240 && echo "$(date) prebuild commit $(git rev-parse HEAD)" >> /workspace/.init
    command: cat /workspace/.init && echo "$(date) current commit $(git rev-parse HEAD)"

2. Add this repository as a Project in this PR's preview environment
   • for example, create a Team, then add your repository as a new Project
3. This will trigger a new prebuild -- wait for this prebuild to finish successfully (should take ~4-5min)
4. Next, go to the Project overview, and open a new workspace for the main branch
   • here you should get a prebuild, and a checkout, based on the same commit
5. Next, commit a new change to your repository (just don't modify the prebuild task -- e.g. you could add a random README.md or LICENSE file)
6. Check that the new Prebuild has started, but don't wait for it to finish -- instead, immediately open a new workspace for the main branch
   • here, you will see the "Prebuild in Progress" screen, with a "Use Last Successful Prebuild" button
   • click that button -- this should give you a workspace, with a prebuild from an older commit, checked out to the newest commit

Note that this POC will not run any init commands again. It simply gives you an older prebuild, but with a more recent Git checkout.

EDIT:

• Loom video (internal) showing what this PR does
• My test repository used in the video

[geropl]

@jankeromnes A first test just worked nicely! 🧘 🚀

In my opinion we want to quickly be able to try this out in prod, internally. I suggest to do the following:

1. Cleanup the code a little bit (see nits above)
2. Put a FeatureFlag in front
3. Clearly communicate the limitations of this approach (multi-repo)

WDYT?

Further things to follow up with (in later PRs):

1. If I start with use-last-successful-prebuild and there is no prebuild to start from (e.g., within range + no config change), you get thrown back into the running prebuild view, without any further hint. Would it be possible to return an additional marker ala "no ready successful prebuild found", so that we can disable that button and show an explainer on hover (or so)?
2. Make this functionality available from other views (as discussed in earlier syncs)

[jankeromnes]

Alrighty! Addressed most nits by refactoring all common incremental prebuild code into a IncrementalPrebuildsService class, then confirmed everything still worked, then implemented a new feature flag called showUseLastSuccessfulPrebuild. 🙌

Next steps will be to rebase & solve the merge conflicts, but other than that, I guess this is nearing completion. 🏁

EDIT: Done as well now ✅

[mbrevoort]

Feedback from an customer conversation:

Regarding the ability to choose to open a workspace from a previous prebuild:

• would like to see the details of the last prebuild:
  • what was the commit point of the last prebuild, how many commits since, some sense of the delta
  • how long did it take? (oh 40 minutes, the current prebuild is at 3 minutes so it’s going to be a long time if I wait)
• when launching from a previous prebuild they heavily emphasized that they just wanted the prebuild as-is. They didn’t want us to "get cute" and apply the most current git context or incrementally build. They wanted it to be very clear that they weren’t on the latest commit and wanted control over how, if and when they git pull’d or merged and rebased. They valued starting the workspace in a consistent and reliable state much more than having the latest commit.

For the functionality to explicitly start a workspace from a previous prebuild, I suggest we launch the workspace on the old prebuild without syncing the latest git context. And we show a message indicating such and that you are "x commits behind…".

[jankeromnes]

That's very good feedback, many thanks @mbrevoort for sourcing and sharing it! 💯

• would like to see the details of the last prebuild:
  • what was the commit point of the last prebuild, how many commits since, some sense of the delta
  • how long did it take? (oh 40 minutes, the current prebuild is at 3 minutes so it’s going to be a long time if I wait)

Interesting. I wonder how we can represent all this info in a short and sweet format. Does it all fit in a button? 🤔 Here is a very rough attempt at such an "enriched" button (sorry for the bad design):

EDIT: Let's move the discussion to the follow-up issue #13944

---

• when launching from a previous prebuild they heavily emphasized that they just wanted the prebuild as-is. They didn’t want us to "get cute" and apply the most current git context or incrementally build.

Aha. This makes sense, is simple, and also quite risk-free. Maybe I can try to do something similar to #13768 and "force" the checked out commit to be the same as the old prebuild being used. 💭

EDIT: Follow-up issue #13945

---

And we show a message indicating such and that you are "x commits behind…".

Is this really useful? (Also, I wouldn't know where to show this -- e.g. in the terminal on start-up? 👀) My thinking is:

• If we clearly represent which commit the last successful prebuild is based on (i.e. how many commits behind it currently is), and you choose to use it, maybe you're already aware of which commit you're landing in?
• Also, our Git tooling is already naturally good at telling you when you're behind -- it regularly fetches upstream information, and will show you in git status how many commits behind you are (note that this already happens naturally e.g. when you open a workspace and then someone pushes 3 new commits to the upstream branch, leaving you "behind")

[geropl]

What's the current pattern we want to use for Feature Flag names? I thought it'd be snake_case? But might be wrong! 🤔

[jankeromnes]

There is a debate (internal) about this currently 🙂

[geropl]

@jankeromnes Any changes in this file? Or is it a "pure move"? 🤔

[jankeromnes]

This is a new file, composed of 3 pre-existing code sections. Here are the relevant diffs:

getCommitHistoryForContext diff

--- get-commit-history-1        2022-10-18 09:42:59.685989248 +0000
+++ get-commit-history-2        2022-10-18 09:41:19.713994670 +0000
@@ -1,18 +1,18 @@
 const maxDepth = this.config.incrementalPrebuilds.commitHistory;
 const hostContext = this.hostContextProvider.get(context.repository.host);
 const repoProvider = hostContext?.services?.repositoryProvider;
-if (repoProvider) {
-    prebuildContext.commitHistory = await repoProvider.getCommitHistory(
+if (!repoProvider) {
+    return {};
+}
+const history: WithCommitHistory = {};
+history.commitHistory = await repoProvider.getCommitHistory(
         user,
         context.repository.owner,
         context.repository.name,
         context.revision,
         maxDepth,
     );
-    if (
-        context.additionalRepositoryCheckoutInfo &&
-        context.additionalRepositoryCheckoutInfo.length > 0
-    ) {
+if (context.additionalRepositoryCheckoutInfo && context.additionalRepositoryCheckoutInfo.length > 0) {
         const histories = context.additionalRepositoryCheckoutInfo.map(async (info) => {
             const commitHistory = await repoProvider.getCommitHistory(
                 user,
@@ -26,5 +26,6 @@ if (repoProvider) {
                 commitHistory,
             };
         });
-        prebuildContext.additionalRepositoryCommitHistories = await Promise.all(histories);
+    history.additionalRepositoryCommitHistories = await Promise.all(histories);
     }

findGoodBaseForIncrementalBuild diff

--- find-good-base-1    2022-10-18 09:51:32.557961085 +0000
+++ find-good-base-2    2022-10-18 09:41:40.157993561 +0000
@@ -1,23 +1,23 @@
-if (context.commitHistory && context.commitHistory.length > 0) {
+if (!history.commitHistory || history.commitHistory.length < 1) {
+    return;
+}
+
+const { config } = await this.configProvider.fetchConfig({}, user, context);
+const imageSource = await this.imageSourceProvider.getImageSource({}, user, context, config);
+
     // Note: This query returns only not-garbage-collected prebuilds in order to reduce cardinality
     // (e.g., at the time of writing, the Gitpod repository has 16K+ prebuilds, but only ~300 not-garbage-collected)
-    const recentPrebuilds = await this.db.trace({ span }).findPrebuildsWithWorkpace(commitContext.repository.cloneUrl);
-    const loggedContext = filterForLogging(context);
+const recentPrebuilds = await this.workspaceDB.findPrebuildsWithWorkpace(context.repository.cloneUrl);
     for (const recentPrebuild of recentPrebuilds) {
         if (
-            !(await this.isGoodBaseforIncrementalPrebuild(
-                context,
+        await this.isGoodBaseforIncrementalBuild(
+            history,
                 config,
                 imageSource,
                 recentPrebuild.prebuild,
                 recentPrebuild.workspace,
-            ))
+        )
         ) {
-            log.debug({ userId: user.id }, "Not using incremental prebuild base", {
-                candidatePrebuildId: recentPrebuild.prebuild.id,
-                context: loggedContext,
-            });
-            continue;
-        }
+        return recentPrebuild.prebuild;
     }
 }

isGoodBaseForIncrementalBuild diff

--- is-good-base-1      2022-10-18 09:44:50.477983240 +0000
+++ is-good-base-2      2022-10-18 09:42:00.273992470 +0000
@@ -1,7 +1,7 @@
-if (!context.commitHistory || context.commitHistory.length === 0) {
+if (!history.commitHistory || history.commitHistory.length === 0) {
     return false;
 }
-if (!CommitContext.is(candidate.context)) {
+if (!CommitContext.is(candidateWorkspace.context)) {
     return false;
 }
 
@@ -11,23 +11,26 @@ if (candidatePrebuild.state !== "availab
 }
 
 // we are only considering full prebuilds
-if (!!candidate.basedOnPrebuildId) {
+if (!!candidateWorkspace.basedOnPrebuildId) {
     return false;
 }
 
-const candidateCtx = candidate.context;
-if (candidateCtx.additionalRepositoryCheckoutInfo?.length !== context.additionalRepositoryCommitHistories?.length) {
+if (
+    candidateWorkspace.context.additionalRepositoryCheckoutInfo?.length !==
+    history.additionalRepositoryCommitHistories?.length
+) {
     // different number of repos
     return false;
 }
 
-if (!context.commitHistory.some((sha) => sha === candidateCtx.revision)) {
+const candidateCtx = candidateWorkspace.context;
+if (!history.commitHistory.some((sha) => sha === candidateCtx.revision)) {
     return false;
 }
 
 // check the commits are included in the commit history
-for (const subRepo of candidateCtx.additionalRepositoryCheckoutInfo || []) {
-    const matchIngRepo = context.additionalRepositoryCommitHistories?.find(
+for (const subRepo of candidateWorkspace.context.additionalRepositoryCheckoutInfo || []) {
+    const matchIngRepo = history.additionalRepositoryCommitHistories?.find(
         (repo) => repo.cloneUrl === subRepo.repository.cloneUrl,
     );
     if (!matchIngRepo || !matchIngRepo.commitHistory.some((sha) => sha === subRepo.revision)) {
@@ -36,10 +39,10 @@ for (const subRepo of candidateCtx.addit
 }
 
 // ensure the image source hasn't changed (skips older images)
-if (JSON.stringify(imageSource) !== JSON.stringify(candidate.imageSource)) {
+if (JSON.stringify(imageSource) !== JSON.stringify(candidateWorkspace.imageSource)) {
     log.debug(`Skipping parent prebuild: Outdated image`, {
         imageSource,
-        parentImageSource: candidate.imageSource,
+        parentImageSource: candidateWorkspace.imageSource,
     });
     return false;
 }
@@ -55,7 +58,7 @@ const filterPrebuildTasks = (tasks: Task
         )
         .filter((task) => Object.keys(task).length > 0);
 const prebuildTasks = filterPrebuildTasks(config.tasks);
-const parentPrebuildTasks = filterPrebuildTasks(candidate.config.tasks);
+const parentPrebuildTasks = filterPrebuildTasks(candidateWorkspace.config.tasks);
 if (JSON.stringify(prebuildTasks) !== JSON.stringify(parentPrebuildTasks)) {
     log.debug(`Skipping parent prebuild: Outdated prebuild tasks`, {
         prebuildTasks,

[geropl]

Would have been great to see this diff surfaced in the PR, ideally by making the move a separate PR, or at least a separate commit. 😕
Only now I notice that fetchConfig was pulled into findGoodBaseForIncrementalBuild, which feels a bit strange, because now there is a disconnect between these two lines [1, 2]. 🤔

Something for a follow-up?

[jankeromnes]

Thanks @geropl!

Would have been great to see this diff surfaced in the PR, ideally by making the move a separate PR, or at least a separate commit. 😕

True, I agree that code moves and code changes would ideally be made in separate commits. 💯 (But unfortunately, I squashed my commits, to make resolving some merge conflicts easier.)

Only now I notice that fetchConfig was pulled into findGoodBaseForIncrementalBuild, which feels a bit strange, because now there is a disconnect between these two lines [1, 2]. 🤔

Indeed, you're right about the fetchConfig being moved. And this has the unfortunate consequence of calling fetchConfig twice now [1, 2]. This could be resolved by allowing to pass an optional config to findGoodBaseForIncrementalBuild.

However, I don't understand what you mean by "disconnect". These two lines still do what was intended, right? (I.e., we fetch the config for the current context, then we find a potentially older prebuild, but we "fix" the created workspace by forcing its config back to the most recent state.)

FYI, this was done to fix a bug where, during incremental prebuilds, a prebuild somehow got a config with an outdated command, and the new workspace would run the outdated command on start-up even though the .gitpod.yml in the workspace and Git history both had the more recent command.

I think that bug is still adequately handled in the new version of the code. Does this explanation resolve the "disconnect" you saw?

[geropl]

With "disconnect" I mean that we should be using the exact same config being used for the prebuild and for identifying the prebuild.
Now that we're doing it twice that might for some CommitContexts (e.g., PR/branch context) lead to situations where config is out of sync here. This a) makes it harder to reason about the code, and b) opens up possibility for current (or future) bugs.

[jankeromnes]

Thanks for explaining! I don't fully understand how the configs could get out-of-sync (because we only fetch a read-only config, i.e. a .gitpod.yml from the repo, for the single context that we currently want to open, regardless of which prebuild or incremental prebuild we end up using -- we do occasionally fetch the same config multiple times, but we never modify it).

Maybe, going back to fetching it just once here, and passing it along to findGoodBaseForIncrementalBuild would solve your concern? (I.e. then we keep exactly one config object in this flow as before.)

[geropl]

Testing again.

[geropl]

Works like a charm! 🎉

What is a bit off now is the fact that we a) see the "prebuild ran" message, but b) the filesystem is newer. What we want to do about this we can find out in prod. 🙂
✔️

/hold For the nits [1, 2]

[jankeromnes]

I guess this can be merged as-is, and I'm happy to make any adjustment follow-ups!

We already have these two follow-up issues:

• Show more details about the "Last Successful Prebuild" #13944
• Adjust behavior of the new "Use Last Successful Prebuild" button #13945

There are also two open discussions about feature flag capitalization, and incremental build logic diffs. But these discussions can continue async and possibly lead to new follow-up issues if needed. 🌴

Excited about moving this forward! 🛹

/unhold