-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Implement a 'Use Last Successful Prebuild' workspace creation mode #13801
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,175 @@ | ||
/** | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jankeromnes Any changes in this file? Or is it a "pure move"? 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. 😕 Something for a follow-up? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @geropl!
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.)
Indeed, you're right about the However, I don't understand what you mean by "disconnect". These two lines still do what was intended, right? (I.e., we fetch the FYI, this was done to fix a bug where, during incremental prebuilds, a prebuild somehow got a I think that bug is still adequately handled in the new version of the code. Does this explanation resolve the "disconnect" you saw? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With "disconnect" I mean that we should be using the exact same There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Maybe, going back to fetching it just once here, and passing it along to |
||
* Copyright (c) 2022 Gitpod GmbH. All rights reserved. | ||
* Licensed under the GNU Affero General Public License (AGPL). | ||
* See License-AGPL.txt in the project root for license information. | ||
*/ | ||
|
||
import { inject, injectable } from "inversify"; | ||
import { | ||
CommitContext, | ||
PrebuiltWorkspace, | ||
TaskConfig, | ||
User, | ||
Workspace, | ||
WorkspaceConfig, | ||
WorkspaceImageSource, | ||
} from "@gitpod/gitpod-protocol"; | ||
import { log } from "@gitpod/gitpod-protocol/lib/util/logging"; | ||
import { WithCommitHistory } from "@gitpod/gitpod-protocol/src/protocol"; | ||
import { WorkspaceDB } from "@gitpod/gitpod-db/lib"; | ||
import { Config } from "../../../src/config"; | ||
import { ConfigProvider } from "../../../src/workspace/config-provider"; | ||
import { HostContextProvider } from "../../../src/auth/host-context-provider"; | ||
import { ImageSourceProvider } from "../../../src/workspace/image-source-provider"; | ||
|
||
@injectable() | ||
export class IncrementalPrebuildsService { | ||
@inject(Config) protected readonly config: Config; | ||
@inject(ConfigProvider) protected readonly configProvider: ConfigProvider; | ||
@inject(HostContextProvider) protected readonly hostContextProvider: HostContextProvider; | ||
@inject(ImageSourceProvider) protected readonly imageSourceProvider: ImageSourceProvider; | ||
@inject(WorkspaceDB) protected readonly workspaceDB: WorkspaceDB; | ||
|
||
public async getCommitHistoryForContext(context: CommitContext, user: User): Promise<WithCommitHistory> { | ||
const maxDepth = this.config.incrementalPrebuilds.commitHistory; | ||
const hostContext = this.hostContextProvider.get(context.repository.host); | ||
const repoProvider = hostContext?.services?.repositoryProvider; | ||
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) { | ||
const histories = context.additionalRepositoryCheckoutInfo.map(async (info) => { | ||
const commitHistory = await repoProvider.getCommitHistory( | ||
user, | ||
info.repository.owner, | ||
info.repository.name, | ||
info.revision, | ||
maxDepth, | ||
); | ||
return { | ||
cloneUrl: info.repository.cloneUrl, | ||
commitHistory, | ||
}; | ||
}); | ||
history.additionalRepositoryCommitHistories = await Promise.all(histories); | ||
} | ||
return history; | ||
} | ||
|
||
public async findGoodBaseForIncrementalBuild( | ||
context: CommitContext, | ||
history: WithCommitHistory, | ||
user: User, | ||
): Promise<PrebuiltWorkspace | undefined> { | ||
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.workspaceDB.findPrebuildsWithWorkpace(context.repository.cloneUrl); | ||
for (const recentPrebuild of recentPrebuilds) { | ||
if ( | ||
await this.isGoodBaseforIncrementalBuild( | ||
history, | ||
config, | ||
imageSource, | ||
recentPrebuild.prebuild, | ||
recentPrebuild.workspace, | ||
) | ||
) { | ||
return recentPrebuild.prebuild; | ||
} | ||
} | ||
} | ||
|
||
protected async isGoodBaseforIncrementalBuild( | ||
history: WithCommitHistory, | ||
config: WorkspaceConfig, | ||
imageSource: WorkspaceImageSource, | ||
candidatePrebuild: PrebuiltWorkspace, | ||
candidateWorkspace: Workspace, | ||
): Promise<boolean> { | ||
if (!history.commitHistory || history.commitHistory.length === 0) { | ||
return false; | ||
} | ||
if (!CommitContext.is(candidateWorkspace.context)) { | ||
return false; | ||
} | ||
|
||
// we are only considering available prebuilds | ||
if (candidatePrebuild.state !== "available") { | ||
return false; | ||
} | ||
|
||
// we are only considering full prebuilds | ||
if (!!candidateWorkspace.basedOnPrebuildId) { | ||
return false; | ||
} | ||
|
||
if ( | ||
candidateWorkspace.context.additionalRepositoryCheckoutInfo?.length !== | ||
history.additionalRepositoryCommitHistories?.length | ||
) { | ||
// different number of repos | ||
return false; | ||
} | ||
|
||
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 candidateWorkspace.context.additionalRepositoryCheckoutInfo || []) { | ||
const matchIngRepo = history.additionalRepositoryCommitHistories?.find( | ||
(repo) => repo.cloneUrl === subRepo.repository.cloneUrl, | ||
); | ||
if (!matchIngRepo || !matchIngRepo.commitHistory.some((sha) => sha === subRepo.revision)) { | ||
return false; | ||
} | ||
} | ||
|
||
// ensure the image source hasn't changed (skips older images) | ||
if (JSON.stringify(imageSource) !== JSON.stringify(candidateWorkspace.imageSource)) { | ||
log.debug(`Skipping parent prebuild: Outdated image`, { | ||
imageSource, | ||
parentImageSource: candidateWorkspace.imageSource, | ||
}); | ||
return false; | ||
} | ||
|
||
// ensure the tasks haven't changed | ||
const filterPrebuildTasks = (tasks: TaskConfig[] = []) => | ||
tasks | ||
.map((task) => | ||
Object.keys(task) | ||
.filter((key) => ["before", "init", "prebuild"].includes(key)) | ||
// @ts-ignore | ||
.reduce((obj, key) => ({ ...obj, [key]: task[key] }), {}), | ||
) | ||
.filter((task) => Object.keys(task).length > 0); | ||
const prebuildTasks = filterPrebuildTasks(config.tasks); | ||
const parentPrebuildTasks = filterPrebuildTasks(candidateWorkspace.config.tasks); | ||
if (JSON.stringify(prebuildTasks) !== JSON.stringify(parentPrebuildTasks)) { | ||
log.debug(`Skipping parent prebuild: Outdated prebuild tasks`, { | ||
prebuildTasks, | ||
parentPrebuildTasks, | ||
}); | ||
return false; | ||
} | ||
|
||
return true; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the current pattern we want to use for Feature Flag names? I thought it'd be
snake_case
? But might be wrong! 🤔There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a debate (internal) about this currently 🙂