Skip to content

Commit caa1f74

Browse files
geroplgeropl
committed
[server] Prefer exact matches plus order incremental matches by commit history
1 parent f4eac6d commit caa1f74

File tree

2 files changed

+122
-77
lines changed

2 files changed

+122
-77
lines changed

‎components/server/src/prebuilds/incremental-workspace-service.ts‎

Lines changed: 54 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -16,18 +16,14 @@ import {
1616
} from "@gitpod/gitpod-protocol";
1717
import { log } from "@gitpod/gitpod-protocol/lib/util/logging";
1818
import { PrebuiltWorkspaceState, WithCommitHistory } from "@gitpod/gitpod-protocol/lib/protocol";
19-
import { WorkspaceDB } from "@gitpod/gitpod-db/lib";
19+
import { PrebuildWithWorkspace, WorkspaceDB } from "@gitpod/gitpod-db/lib";
2020
import { Config } from "../config";
2121
import { HostContextProvider } from "../auth/host-context-provider";
2222
import { ImageSourceProvider } from "../workspace/image-source-provider";
2323

2424
const MAX_HISTORY_DEPTH = 100;
2525

26-
enum Match {
27-
None = 0,
28-
Loose = 1,
29-
Exact = 2,
30-
}
26+
type IncrementalWorkspaceMatch = "none" | "incremental" | "exact";
3127

3228
@injectable()
3329
export class IncrementalWorkspaceService {
@@ -93,30 +89,44 @@ export class IncrementalWorkspaceService {
9389
const imageSource = await imageSourcePromise;
9490

9591
// traverse prebuilds by commit history instead of their creationTime, so that we don't match prebuilds created for older revisions but triggered later
96-
for (const commit of history.commitHistory) {
97-
const prebuildsForCommit = recentPrebuilds.filter(({ prebuild }) => prebuild.commit === commit);
98-
for (const entry of prebuildsForCommit) {
99-
const { prebuild, workspace } = entry;
100-
const match = this.isMatchForIncrementalBuild(
101-
history,
102-
config,
103-
imageSource,
92+
const candidates: { candidate: PrebuildWithWorkspace; index: number }[] = [];
93+
for (const recentPrebuild of recentPrebuilds) {
94+
const { prebuild, workspace } = recentPrebuild;
95+
const { match, index } = this.isMatchForIncrementalBuild(
96+
history,
97+
config,
98+
imageSource,
99+
prebuild,
100+
workspace,
101+
includeUnfinishedPrebuilds,
102+
);
103+
if (match === "exact") {
104+
console.log("Found base for incremental build", {
104105
prebuild,
105106
workspace,
106-
includeUnfinishedPrebuilds,
107-
);
108-
if (match > Match.None) {
109-
console.log("Found base for incremental build", {
110-
prebuild,
111-
workspace,
112-
exactMatch: match === Match.Exact,
113-
});
114-
return prebuild;
115-
}
107+
exactMatch: true,
108+
});
109+
return prebuild;
110+
}
111+
if (match === "incremental") {
112+
candidates.push({ candidate: recentPrebuild, index: index! });
116113
}
117114
}
118115

119-
return undefined;
116+
if (candidates.length === 0) {
117+
return undefined;
118+
}
119+
120+
// Sort by index ASC
121+
candidates.sort((a, b) => a.index - b.index);
122+
const { prebuild, workspace } = candidates[0].candidate;
123+
124+
console.log("Found base for incremental build", {
125+
prebuild,
126+
workspace,
127+
exactMatch: false,
128+
});
129+
return prebuild;
120130
}
121131

122132
private isMatchForIncrementalBuild(
@@ -126,13 +136,13 @@ export class IncrementalWorkspaceService {
126136
candidatePrebuild: PrebuiltWorkspace,
127137
candidateWorkspace: Workspace,
128138
includeUnfinishedPrebuilds?: boolean,
129-
): Match {
139+
): { match: Omit<IncrementalWorkspaceMatch, "none">; index: number } | { match: "none"; index?: undefined } {
130140
// make typescript happy, we know that history.commitHistory is defined
131141
if (!history.commitHistory) {
132-
return Match.None;
142+
return { match: "none" };
133143
}
134144
if (!CommitContext.is(candidateWorkspace.context)) {
135-
return Match.None;
145+
return { match: "none" };
136146
}
137147

138148
const acceptableStates: PrebuiltWorkspaceState[] = ["available"];
@@ -141,35 +151,40 @@ export class IncrementalWorkspaceService {
141151
acceptableStates.push("queued");
142152
}
143153
if (!acceptableStates.includes(candidatePrebuild.state)) {
144-
return Match.None;
154+
return { match: "none" };
145155
}
146156

147157
// we are only considering full prebuilds (we are not building on top of incremental prebuilds)
148158
if (candidateWorkspace.basedOnPrebuildId) {
149-
return Match.None;
159+
return { match: "none" };
150160
}
151161

152162
// check if the amount of additional repositories matches the candidate
153163
if (
154164
candidateWorkspace.context.additionalRepositoryCheckoutInfo?.length !==
155165
history.additionalRepositoryCommitHistories?.length
156166
) {
157-
return Match.None;
167+
return { match: "none" };
158168
}
159169

160170
const candidateCtx = candidateWorkspace.context;
161171

162172
// check for overlapping commit history
173+
// TODO(gpl) Isn't "candidateCtx.revision" identical to "candidatePrebuild.commit"? If yes, we could do .indexOf once...
174+
if (candidateCtx.revision !== candidatePrebuild.commit) {
175+
log.warn("Prebuild matching: commits mismatch!", { candidateCtx, candidatePrebuild });
176+
}
163177
if (!history.commitHistory.some((sha) => sha === candidateCtx.revision)) {
164-
return Match.None;
178+
return { match: "none" };
165179
}
180+
166181
// check for overlapping git history for each additional repo
167182
for (const subRepo of candidateWorkspace.context.additionalRepositoryCheckoutInfo ?? []) {
168183
const matchingRepo = history.additionalRepositoryCommitHistories?.find(
169184
(repo) => repo.cloneUrl === subRepo.repository.cloneUrl,
170185
);
171186
if (!matchingRepo || !matchingRepo.commitHistory.some((sha) => sha === subRepo.revision)) {
172-
return Match.None;
187+
return { match: "none" };
173188
}
174189
}
175190

@@ -179,7 +194,7 @@ export class IncrementalWorkspaceService {
179194
imageSource,
180195
parentImageSource: candidateWorkspace.imageSource,
181196
});
182-
return Match.None;
197+
return { match: "none" };
183198
}
184199

185200
// ensure the tasks haven't changed
@@ -190,14 +205,15 @@ export class IncrementalWorkspaceService {
190205
prebuildTasks,
191206
parentPrebuildTasks,
192207
});
193-
return Match.None;
208+
return { match: "none" };
194209
}
195210

196-
if (candidatePrebuild.commit === history.commitHistory[0]) {
197-
return Match.Exact;
211+
const index = history.commitHistory.indexOf(candidatePrebuild.commit);
212+
if (index === 0) {
213+
return { match: "exact", index };
198214
}
199215

200-
return Match.Loose;
216+
return { match: "incremental", index };
201217
}
202218

203219
/**

‎components/server/src/workspace/context-service.spec.db.ts‎

Lines changed: 68 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@ import {
2222
Repository,
2323
RepositoryInfo,
2424
WorkspaceConfig,
25+
PrebuiltWorkspaceState,
26+
PrebuiltWorkspace,
2527
} from "@gitpod/gitpod-protocol";
2628
import * as chai from "chai";
2729
import { Container } from "inversify";
@@ -549,16 +551,22 @@ describe("ContextService", async () => {
549551
commitMessage: `commit 69420`,
550552
};
551553
const commit2 = {
552-
sha: "69421",
554+
sha: "69422",
555+
author: "some-dude",
556+
commitMessage: `commit 69422`,
557+
};
558+
const commit3 = {
559+
sha: "69423",
553560
author: "some-other-dude",
554-
commitMessage: "commit 69421",
561+
commitMessage: "commit 69423",
555562
};
556563
const branchName = "branch-2";
557564
mockRepositoryProvider.addBranch(
558565
{ name: branchName, htmlUrl: `https://github.com/gitpod-io/empty/tree/${branchName}` },
559566
[commit1],
560567
);
561568
mockRepositoryProvider.pushCommit(branchName, commit2);
569+
mockRepositoryProvider.pushCommit(branchName, commit3);
562570

563571
// request context for both commits separately
564572
const svc = container.get(ContextService);
@@ -567,64 +575,85 @@ describe("ContextService", async () => {
567575
organizationId: org.id,
568576
forceDefaultConfig: false,
569577
});
570-
let ctx2 = await svc.parseContext(owner, `https://github.com/gitpod-io/empty/commit/${commit2.sha}`, {
578+
const ctx2 = await svc.parseContext(owner, `https://github.com/gitpod-io/empty/commit/${commit2.sha}`, {
579+
projectId: project.id,
580+
organizationId: org.id,
581+
forceDefaultConfig: false,
582+
});
583+
let ctx3 = await svc.parseContext(owner, `https://github.com/gitpod-io/empty/commit/${commit3.sha}`, {
571584
projectId: project.id,
572585
organizationId: org.id,
573586
forceDefaultConfig: false,
574587
});
575588

576-
// trigger and "await" prebuilds for both commits in push order
589+
// trigger and "await" prebuilds for both commits in crazy order
577590
const prebuildManager = container.get(PrebuildManager);
578591
const workspaceDb: WorkspaceDB = container.get(WorkspaceDB);
579-
const prebuildForCommit2 = await prebuildManager.startPrebuild(
580-
{},
581-
{ user: owner, project, commitInfo: commit2, context: ctx2.context as CommitContext },
582-
);
583-
const prebuild1 = await workspaceDb.findPrebuildByID(prebuildForCommit2.prebuildId);
584-
await workspaceDb.storePrebuiltWorkspace({
585-
...prebuild1!,
586-
state: "available",
587-
});
588-
const wsAndI1 = await workspaceDb.findWorkspaceAndInstance(prebuild1!.buildWorkspaceId);
589-
await workspaceDb.updateInstancePartial(wsAndI1!.instanceId, { status: { phase: "stopped" } });
590592

591-
const prebuildForCommit1 = await prebuildManager.startPrebuild(
592-
{},
593-
{ user: owner, project, commitInfo: commit1, context: ctx1.context as CommitContext },
594-
);
595-
const prebuild2 = await workspaceDb.findPrebuildByID(prebuildForCommit1.prebuildId);
596-
await workspaceDb.storePrebuiltWorkspace({
597-
...prebuild2!,
598-
state: "available",
599-
});
600-
const wsAndI2 = await workspaceDb.findWorkspaceAndInstance(prebuild2!.buildWorkspaceId);
601-
await workspaceDb.updateInstancePartial(wsAndI2!.instanceId, { status: { phase: "stopped" } });
593+
async function runPrebuild(
594+
commitInfo: CommitInfo,
595+
context: CommitContext,
596+
state: PrebuiltWorkspaceState,
597+
): Promise<PrebuiltWorkspace> {
598+
const prebuildResult = await prebuildManager.startPrebuild(
599+
{},
600+
{ user: owner, project, commitInfo, context },
601+
);
602+
const prebuild = await workspaceDb.findPrebuildByID(prebuildResult.prebuildId);
603+
await workspaceDb.storePrebuiltWorkspace({
604+
...prebuild!,
605+
state,
606+
});
607+
const wsAndI = await workspaceDb.findWorkspaceAndInstance(prebuild!.buildWorkspaceId);
608+
await workspaceDb.updateInstancePartial(wsAndI!.instanceId, { status: { phase: "stopped" } });
609+
610+
return prebuild!;
611+
}
602612

603-
// after everything has settled, request context for both commits again
613+
const prebuild3 = await runPrebuild(commit3, ctx3.context as CommitContext, "available");
614+
const prebuild1 = await runPrebuild(commit1, ctx1.context as CommitContext, "available");
615+
await runPrebuild(commit2, ctx2.context as CommitContext, "available");
616+
617+
// should point to commit1, as
604618
ctx1 = await svc.parseContext(owner, `https://github.com/gitpod-io/empty/commit/${commit1.sha}`, {
605619
projectId: project.id,
606620
organizationId: org.id,
607621
forceDefaultConfig: false,
608622
});
609-
ctx2 = await svc.parseContext(owner, `https://github.com/gitpod-io/empty/commit/${commit2.sha}`, {
623+
ctx3 = await svc.parseContext(owner, `https://github.com/gitpod-io/empty/commit/${commit3.sha}`, {
624+
projectId: project.id,
625+
organizationId: org.id,
626+
forceDefaultConfig: false,
627+
});
628+
const ctxBranch = await svc.parseContext(owner, `https://github.com/gitpod-io/empty/tree/branch-2`, {
610629
projectId: project.id,
611630
organizationId: org.id,
612631
forceDefaultConfig: false,
613632
});
614633

615634
expect(ctx1.project?.id).to.equal(project.id);
616635
expect(PrebuiltWorkspaceContext.is(ctx1.context)).to.equal(true);
617-
expect((ctx1.context as any as PrebuiltWorkspaceContext).prebuiltWorkspace.id).to.equal(
618-
prebuildForCommit1.prebuildId,
619-
);
620-
expect((ctx1.context as any as PrebuiltWorkspaceContext).prebuiltWorkspace.commit).to.equal(commit1.sha);
621-
622-
expect(ctx2.project?.id).to.equal(project.id);
623-
expect(PrebuiltWorkspaceContext.is(ctx2.context)).to.equal(true);
624-
expect((ctx2.context as any as PrebuiltWorkspaceContext).prebuiltWorkspace.id).to.equal(
625-
prebuildForCommit2.prebuildId,
626-
);
627-
expect((ctx2.context as any as PrebuiltWorkspaceContext).prebuiltWorkspace.commit).to.equal(commit2.sha);
636+
expect((ctx1.context as any as PrebuiltWorkspaceContext).prebuiltWorkspace.id).to.equal(prebuild1.id);
637+
expect(
638+
(ctx1.context as any as PrebuiltWorkspaceContext).prebuiltWorkspace.commit,
639+
"should point to commit1, ignoring others due to history",
640+
).to.equal(commit1.sha);
641+
642+
expect(ctx3.project?.id).to.equal(project.id);
643+
expect(PrebuiltWorkspaceContext.is(ctx3.context)).to.equal(true);
644+
expect((ctx3.context as any as PrebuiltWorkspaceContext).prebuiltWorkspace.id).to.equal(prebuild3.id);
645+
expect(
646+
(ctx3.context as any as PrebuiltWorkspaceContext).prebuiltWorkspace.commit,
647+
"should point to commit3, ignoring more recent prebuilds (1 + 2)",
648+
).to.equal(commit3.sha);
649+
650+
expect(ctxBranch.project?.id).to.equal(project.id);
651+
expect(PrebuiltWorkspaceContext.is(ctxBranch.context)).to.equal(true);
652+
expect((ctxBranch.context as any as PrebuiltWorkspaceContext).prebuiltWorkspace.id).to.equal(prebuild3.id);
653+
expect(
654+
(ctxBranch.context as any as PrebuiltWorkspaceContext).prebuiltWorkspace.commit,
655+
"should point to commit3, ingoring more the more recent incremental match prebuild2",
656+
).to.equal(commit3.sha);
628657
});
629658

630659
it("should parse snapshot context", async () => {

0 commit comments

Comments
 (0)