diff --git a/components/server/src/bitbucket-server/bitbucket-server-repository-provider.ts b/components/server/src/bitbucket-server/bitbucket-server-repository-provider.ts index 2ef68b01570d71..50ae8e642e0abb 100644 --- a/components/server/src/bitbucket-server/bitbucket-server-repository-provider.ts +++ b/components/server/src/bitbucket-server/bitbucket-server-repository-provider.ts @@ -190,7 +190,7 @@ export class BitbucketServerRepositoryProvider implements RepositoryProvider { owner, repoKind, repositorySlug: repo, - query: { shaOrRevision: ref, limit: 1000 }, + query: { shaOrRevision: ref, limit: 1000 }, // ft: why do we limit to 1000 and not maxDepth? }); const commits = commitsResult.values || []; diff --git a/components/server/src/bitbucket/bitbucket-repository-provider.ts b/components/server/src/bitbucket/bitbucket-repository-provider.ts index e0c3f2a2be6c3d..f6748fb6bdb91f 100644 --- a/components/server/src/bitbucket/bitbucket-repository-provider.ts +++ b/components/server/src/bitbucket/bitbucket-repository-provider.ts @@ -143,7 +143,7 @@ export class BitbucketRepositoryProvider implements RepositoryProvider { ): Promise { const api = await this.apiFactory.create(user); // TODO(janx): To get more results than Bitbucket API's max pagelen (seems to be 100), pagination should be handled. - // The additional property 'page' may be helfpul. + // The additional property 'page' may be helpful. const result = await api.repositories.listCommitsAt({ workspace: owner, repo_slug: repo, diff --git a/components/server/src/github/github-repository-provider.ts b/components/server/src/github/github-repository-provider.ts index 5b6f5030602bf5..7d15259c1df3ce 100644 --- a/components/server/src/github/github-repository-provider.ts +++ b/components/server/src/github/github-repository-provider.ts @@ -130,7 +130,7 @@ export class GithubRepositoryProvider implements RepositoryProvider { } // TODO(janx): To get more results than GitHub API's max page size (seems to be 100), pagination should be handled. - // These additional history properties may be helfpul: + // These additional history properties may be helpful: // totalCount, // pageInfo { // haxNextPage, diff --git a/components/server/src/prebuilds/incremental-workspace-service.ts b/components/server/src/prebuilds/incremental-workspace-service.ts index 74df001724ee7f..60952674ee76d6 100644 --- a/components/server/src/prebuilds/incremental-workspace-service.ts +++ b/components/server/src/prebuilds/incremental-workspace-service.ts @@ -16,13 +16,17 @@ import { } from "@gitpod/gitpod-protocol"; import { log } from "@gitpod/gitpod-protocol/lib/util/logging"; import { PrebuiltWorkspaceState, WithCommitHistory } from "@gitpod/gitpod-protocol/lib/protocol"; -import { WorkspaceDB } from "@gitpod/gitpod-db/lib"; +import { PrebuildWithWorkspace, WorkspaceDB } from "@gitpod/gitpod-db/lib"; import { Config } from "../config"; import { HostContextProvider } from "../auth/host-context-provider"; import { ImageSourceProvider } from "../workspace/image-source-provider"; +import { ApplicationError, ErrorCodes } from "@gitpod/gitpod-protocol/lib/messaging/error"; +import { TrustedValue } from "@gitpod/gitpod-protocol/lib/util/scrubbing"; const MAX_HISTORY_DEPTH = 100; +type IncrementalWorkspaceMatch = "none" | "incremental" | "exact"; + @injectable() export class IncrementalWorkspaceService { @inject(Config) protected readonly config: Config; @@ -45,6 +49,7 @@ export class IncrementalWorkspaceService { context.revision, maxDepth, ); + history.commitHistory.unshift(context.revision); if (context.additionalRepositoryCheckoutInfo && context.additionalRepositoryCheckoutInfo.length > 0) { const histories = context.additionalRepositoryCheckoutInfo.map(async (info) => { @@ -78,42 +83,89 @@ export class IncrementalWorkspaceService { return; } - const imageSourcePromise = 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.findPrebuildsWithWorkspace(projectId); - const imageSource = await imageSourcePromise; - for (const recentPrebuild of recentPrebuilds) { - if ( - this.isGoodBaseforIncrementalBuild( - history, - config, - imageSource, - recentPrebuild.prebuild, - recentPrebuild.workspace, - includeUnfinishedPrebuilds, - ) - ) { - return recentPrebuild.prebuild; + const [recentPrebuilds, imageSource] = await Promise.allSettled([ + // 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) + this.workspaceDB.findPrebuildsWithWorkspace(projectId), + this.imageSourceProvider.getImageSource({}, user, context, config), + ]); + if (imageSource.status === "rejected") { + log.error("Image source promise was rejected", { reason: imageSource.reason, userId: user.id }); + throw new ApplicationError( + ErrorCodes.INTERNAL_SERVER_ERROR, + "Something went wrong when looking up prebuilds", + ); + } + if (recentPrebuilds.status === "rejected") { + log.error("Prebuild lookup promise was rejected", { reason: recentPrebuilds.reason, userId: user.id }); + throw new ApplicationError( + ErrorCodes.INTERNAL_SERVER_ERROR, + "Something went wrong when looking up prebuilds", + ); + } + + // traverse prebuilds by commit history instead of their creationTime, so that we don't match prebuilds created for older revisions but triggered later + const candidates: { candidate: PrebuildWithWorkspace; index: number }[] = []; + for (const recentPrebuild of recentPrebuilds.value) { + const { prebuild, workspace } = recentPrebuild; + const { match, index } = this.isMatchForIncrementalBuild( + history, + config, + imageSource.value, + prebuild, + workspace, + includeUnfinishedPrebuilds, + ); + if (match === "exact") { + log.info("Found base for incremental build", { + prebuildId: prebuild.id, + match: new TrustedValue({ + exact: true, + distanceFromContext: 0, + }), + }); + return prebuild; + } + if (match === "incremental") { + candidates.push({ candidate: recentPrebuild, index: index! }); } } - return undefined; + + if (candidates.length === 0) { + return undefined; + } + + // Sort by index ASC + candidates.sort((a, b) => a.index - b.index); + const { + candidate: { prebuild }, + index, + } = candidates[0]; + + log.info("Found base for incremental build", { + prebuildId: prebuild.id, + match: { + exact: true, + distanceFromContext: index, + }, + }); + return prebuild; } - private isGoodBaseforIncrementalBuild( + private isMatchForIncrementalBuild( history: WithCommitHistory, config: WorkspaceConfig, imageSource: WorkspaceImageSource, candidatePrebuild: PrebuiltWorkspace, candidateWorkspace: Workspace, includeUnfinishedPrebuilds?: boolean, - ): boolean { - if (!history.commitHistory || history.commitHistory.length === 0) { - return false; + ): { match: Omit; index: number } | { match: "none"; index?: undefined } { + // make typescript happy, we know that history.commitHistory is defined + if (!history.commitHistory) { + return { match: "none" }; } if (!CommitContext.is(candidateWorkspace.context)) { - return false; + return { match: "none" }; } const acceptableStates: PrebuiltWorkspaceState[] = ["available"]; @@ -121,36 +173,38 @@ export class IncrementalWorkspaceService { acceptableStates.push("building"); acceptableStates.push("queued"); } - if (!acceptableStates.includes(candidatePrebuild.state)) { - return false; + return { match: "none" }; } - // we are only considering full prebuilds - if (!!candidateWorkspace.basedOnPrebuildId) { - return false; + // we are only considering full prebuilds (we are not building on top of incremental prebuilds) + if (candidateWorkspace.basedOnPrebuildId) { + return { match: "none" }; } + // check if the amount of additional repositories matches the candidate if ( candidateWorkspace.context.additionalRepositoryCheckoutInfo?.length !== history.additionalRepositoryCommitHistories?.length ) { - // different number of repos - return false; + return { match: "none" }; } const candidateCtx = candidateWorkspace.context; - if (!history.commitHistory.some((sha) => sha === candidateCtx.revision)) { - return false; + + // check for overlapping commit history + const commitIndexInHistory = history.commitHistory.indexOf(candidateCtx.revision); + if (commitIndexInHistory === -1) { + return { match: "none" }; } - // check the commits are included in the commit history - for (const subRepo of candidateWorkspace.context.additionalRepositoryCheckoutInfo || []) { - const matchIngRepo = history.additionalRepositoryCommitHistories?.find( + // check for overlapping git history for each additional repo + 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; + if (!matchingRepo || !matchingRepo.commitHistory.some((sha) => sha === subRepo.revision)) { + return { match: "none" }; } } @@ -160,29 +214,41 @@ export class IncrementalWorkspaceService { imageSource, parentImageSource: candidateWorkspace.imageSource, }); - return false; + return { match: "none" }; } // 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); + const prebuildTasks = this.filterPrebuildTasks(config.tasks); + const parentPrebuildTasks = this.filterPrebuildTasks(candidateWorkspace.config.tasks); if (JSON.stringify(prebuildTasks) !== JSON.stringify(parentPrebuildTasks)) { log.debug(`Skipping parent prebuild: Outdated prebuild tasks`, { prebuildTasks, parentPrebuildTasks, }); - return false; + return { match: "none" }; } - return true; + if (commitIndexInHistory === 0) { + return { match: "exact", index: commitIndexInHistory }; + } + + return { match: "incremental", index: commitIndexInHistory }; + } + + /** + * Given an array of tasks returns only the those which are to run during prebuilds, additionally stripping everything besides the prebuild-related configuration from them + */ + private filterPrebuildTasks(tasks: TaskConfig[] = []): Record[] { + return tasks + .map((task) => { + const filteredTask: Record = {}; + for (const key of Object.keys(task)) { + if (["before", "init", "prebuild"].includes(key)) { + filteredTask[key] = task[key as keyof TaskConfig]; + } + } + return filteredTask; + }) + .filter((task) => Object.keys(task).length > 0); } } diff --git a/components/server/src/workspace/context-service.spec.db.ts b/components/server/src/workspace/context-service.spec.db.ts index 54ad017d82fef3..1b77511c4f7587 100644 --- a/components/server/src/workspace/context-service.spec.db.ts +++ b/components/server/src/workspace/context-service.spec.db.ts @@ -22,6 +22,8 @@ import { Repository, RepositoryInfo, WorkspaceConfig, + PrebuiltWorkspaceState, + PrebuiltWorkspace, } from "@gitpod/gitpod-protocol"; import * as chai from "chai"; import { Container } from "inversify"; @@ -130,6 +132,7 @@ class MockRepositoryProvider implements RepositoryProvider { } throw new Error(`ref ${ref} not found`); } + async getCommitInfo(user: User, owner: string, repo: string, ref: string): Promise { return headu(this.branches.get(ref)?.commits); } @@ -541,6 +544,117 @@ describe("ContextService", async () => { expect((ctx.context as any as PrebuiltWorkspaceContext).prebuiltWorkspace.commit).to.equal(revision1); }); + it("should handle triggering prebuilds out of order with respect to commits", async () => { + const commit1 = { + sha: "69420", + author: "some-dude", + commitMessage: `commit 69420`, + }; + const commit2 = { + sha: "69422", + author: "some-dude", + commitMessage: `commit 69422`, + }; + const commit3 = { + sha: "69423", + author: "some-other-dude", + commitMessage: "commit 69423", + }; + const branchName = "branch-2"; + mockRepositoryProvider.addBranch( + { name: branchName, htmlUrl: `https://github.com/gitpod-io/empty/tree/${branchName}` }, + [commit1], + ); + mockRepositoryProvider.pushCommit(branchName, commit2); + mockRepositoryProvider.pushCommit(branchName, commit3); + + // request context for both commits separately + const svc = container.get(ContextService); + let ctx1 = await svc.parseContext(owner, `https://github.com/gitpod-io/empty/commit/${commit1.sha}`, { + projectId: project.id, + organizationId: org.id, + forceDefaultConfig: false, + }); + const ctx2 = await svc.parseContext(owner, `https://github.com/gitpod-io/empty/commit/${commit2.sha}`, { + projectId: project.id, + organizationId: org.id, + forceDefaultConfig: false, + }); + let ctx3 = await svc.parseContext(owner, `https://github.com/gitpod-io/empty/commit/${commit3.sha}`, { + projectId: project.id, + organizationId: org.id, + forceDefaultConfig: false, + }); + + // trigger and "await" prebuilds for all commits in crazy order + const prebuildManager = container.get(PrebuildManager); + const workspaceDb: WorkspaceDB = container.get(WorkspaceDB); + + async function runPrebuild( + commitInfo: CommitInfo, + context: CommitContext, + state: PrebuiltWorkspaceState, + ): Promise { + const prebuildResult = await prebuildManager.startPrebuild( + {}, + { user: owner, project, commitInfo, context }, + ); + const prebuild = await workspaceDb.findPrebuildByID(prebuildResult.prebuildId); + await workspaceDb.storePrebuiltWorkspace({ + ...prebuild!, + state, + }); + const wsAndI = await workspaceDb.findWorkspaceAndInstance(prebuild!.buildWorkspaceId); + await workspaceDb.updateInstancePartial(wsAndI!.instanceId, { status: { phase: "stopped" } }); + + return prebuild!; + } + + const prebuild3 = await runPrebuild(commit3, ctx3.context as CommitContext, "available"); + const prebuild1 = await runPrebuild(commit1, ctx1.context as CommitContext, "available"); + await runPrebuild(commit2, ctx2.context as CommitContext, "available"); + + ctx1 = await svc.parseContext(owner, `https://github.com/gitpod-io/empty/commit/${commit1.sha}`, { + projectId: project.id, + organizationId: org.id, + forceDefaultConfig: false, + }); + ctx3 = await svc.parseContext(owner, `https://github.com/gitpod-io/empty/commit/${commit3.sha}`, { + projectId: project.id, + organizationId: org.id, + forceDefaultConfig: false, + }); + const ctxBranch = await svc.parseContext(owner, `https://github.com/gitpod-io/empty/tree/branch-2`, { + projectId: project.id, + organizationId: org.id, + forceDefaultConfig: false, + }); + + expect(ctx1.project?.id).to.equal(project.id); + expect(PrebuiltWorkspaceContext.is(ctx1.context)).to.equal(true); + expect((ctx1.context as any as PrebuiltWorkspaceContext).prebuiltWorkspace.id).to.equal(prebuild1.id); + expect( + (ctx1.context as any as PrebuiltWorkspaceContext).prebuiltWorkspace.commit, + "should point to commit1, ignoring others due to history", + ).to.equal(commit1.sha); + + expect(ctx3.project?.id).to.equal(project.id); + expect(PrebuiltWorkspaceContext.is(ctx3.context)).to.equal(true); + expect((ctx3.context as any as PrebuiltWorkspaceContext).prebuiltWorkspace.id).to.equal(prebuild3.id); + expect( + (ctx3.context as any as PrebuiltWorkspaceContext).prebuiltWorkspace.commit, + "should point to commit3, ignoring more recent prebuilds (1 + 2)", + ).to.equal(commit3.sha); + + expect(ctxBranch.project?.id).to.equal(project.id); + expect(PrebuiltWorkspaceContext.is(ctxBranch.context)).to.equal(true); + expect((ctxBranch.context as any as PrebuiltWorkspaceContext).prebuiltWorkspace.id).to.equal(prebuild3.id); + expect( + (ctxBranch.context as any as PrebuiltWorkspaceContext).prebuiltWorkspace.commit, + "should point to commit3, ingoring more the more recent incremental match prebuild2", + ).to.equal(commit3.sha); + }); + it("should parse snapshot context", async () => { const svc = container.get(ContextService); const ctx = await svc.parseContext(owner, `snapshot/${snapshot.id}`, {