Reconcile startWorkspace (#18673)

* [server] Reconcile workspace starts using redlock

* [server] Check abortSignal before updating the DB state

* [server] Add feature flag workspace_start_controller

review comments

Author: geropl

components/gitpod-db/src/typeorm/workspace-db-impl.ts

@@ -379,45 +379,6 @@ export class TypeORMWorkspaceDBImpl extends TransactionalDBImpl<WorkspaceDB> imp
         return qb.getOne();
     }
 
-    public async findAllWorkspaceInstances(
-        offset: number,
-        limit: number,
-        orderBy: keyof WorkspaceInstance,
-        orderDir: "ASC" | "DESC",
-        ownerId?: string,
-        minCreationTime?: Date,
-        maxCreationTime?: Date,
-        onlyRunning?: boolean,
-        type?: WorkspaceType,
-    ): Promise<{ total: number; rows: WorkspaceInstance[] }> {
-        const workspaceInstanceRepo = await this.getWorkspaceInstanceRepo();
-        const queryBuilder = workspaceInstanceRepo
-            .createQueryBuilder("wsi")
-            .leftJoinAndMapOne("wsi.workspace", DBWorkspace, "ws", "wsi.workspaceId = ws.id")
-            .skip(offset)
-            .take(limit)
-            .orderBy("wsi." + orderBy, orderDir)
-            .where("ws.type = :type", { type: type ? type.toString() : "regular" }); // only regular workspaces by default
-        if (ownerId) {
-            queryBuilder.andWhere("wsi.ownerId = :ownerId", { ownerId });
-        }
-        if (minCreationTime) {
-            queryBuilder.andWhere("wsi.creationTime >= :minCreationTime", {
-                minCreationTime: minCreationTime.toISOString(),
-            });
-        }
-        if (maxCreationTime) {
-            queryBuilder.andWhere("wsi.creationTime < :maxCreationTime", {
-                maxCreationTime: maxCreationTime.toISOString(),
-            });
-        }
-        if (onlyRunning) {
-            queryBuilder.andWhere("wsi.phasePersisted != 'stopped'").andWhere("wsi.deleted != TRUE");
-        }
-        const [rows, total] = await queryBuilder.getManyAndCount();
-        return { total, rows };
-    }
-
     public async getInstanceCount(type?: string): Promise<number> {
         const workspaceInstanceRepo = await this.getWorkspaceInstanceRepo();
         const queryBuilder = workspaceInstanceRepo
@@ -1096,13 +1057,17 @@ export class TypeORMWorkspaceDBImpl extends TransactionalDBImpl<WorkspaceDB> imp
         return <WorkspaceAndInstance>res;
     }
 
-    async findInstancesByPhaseAndRegion(phase: string, region: string): Promise<WorkspaceInstance[]> {
+    async findInstancesByPhase(phases: string[]): Promise<WorkspaceInstance[]> {
+        if (phases.length < 0) {
+            throw new Error("At least one phase must be provided");
+        }
+
         const repo = await this.getWorkspaceInstanceRepo();
-        // uses index: ind_phasePersisted_region
+        // uses index: ind_phasePersisted
         const qb = repo
             .createQueryBuilder("wsi")
-            .where("wsi.phasePersisted = :phase", { phase })
-            .andWhere("wsi.region = :region", { region });
+            .where("wsi.deleted != TRUE")
+            .andWhere("wsi.phasePersisted IN (:phases)", { phases });
         return qb.getMany();
     }
 

components/gitpod-db/src/workspace-db.ts

@@ -130,24 +130,12 @@ export interface WorkspaceDB {
         query?: AdminGetWorkspacesQuery,
     ): Promise<{ total: number; rows: WorkspaceAndInstance[] }>;
     findWorkspaceAndInstance(id: string): Promise<WorkspaceAndInstance | undefined>;
-    findInstancesByPhaseAndRegion(phase: string, region: string): Promise<WorkspaceInstance[]>;
+    findInstancesByPhase(phases: string[]): Promise<WorkspaceInstance[]>;
 
     getWorkspaceCount(type?: String): Promise<Number>;
     getWorkspaceCountByCloneURL(cloneURL: string, sinceLastDays?: number, type?: string): Promise<number>;
     getInstanceCount(type?: string): Promise<number>;
 
-    findAllWorkspaceInstances(
-        offset: number,
-        limit: number,
-        orderBy: keyof WorkspaceInstance,
-        orderDir: "ASC" | "DESC",
-        ownerId?: string,
-        minCreationTime?: Date,
-        maxCreationTime?: Date,
-        onlyRunning?: boolean,
-        type?: WorkspaceType,
-    ): Promise<{ total: number; rows: WorkspaceInstance[] }>;
-
     findRegularRunningInstances(userId?: string): Promise<WorkspaceInstance[]>;
     findRunningInstancesWithWorkspaces(
         workspaceClusterName?: string,

components/gitpod-protocol/src/util/timeutil.ts

@@ -77,6 +77,10 @@ export function rightBefore(date: string): string {
     return new Date(new Date(date).getTime() - 1).toISOString();
 }
 
+export function durationLongerThanSeconds(time: number, durationSeconds: number, now: number = Date.now()): boolean {
+    return (now - time) / 1000 > durationSeconds;
+}
+
 export function millisecondsToHours(milliseconds: number): number {
     return milliseconds / 1000 / 60 / 60;
 }

components/server/src/container-module.ts

@@ -133,6 +133,7 @@ import { GitpodTokenService } from "./user/gitpod-token-service";
 import { EnvVarService } from "./user/env-var-service";
 import { ScmService } from "./projects/scm-service";
 import { RelationshipUpdateJob } from "./authorization/relationship-updater-job";
+import { WorkspaceStartController } from "./workspace/workspace-start-controller";
 
 export const productionContainerModule = new ContainerModule(
     (bind, unbind, isBound, rebind, unbindAsync, onActivation, onDeactivation) => {
@@ -170,6 +171,7 @@ export const productionContainerModule = new ContainerModule(
         bind(WorkspaceService).toSelf().inSingletonScope();
         bind(WorkspaceFactory).toSelf().inSingletonScope();
         bind(WorkspaceStarter).toSelf().inSingletonScope();
+        bind(WorkspaceStartController).toSelf().inSingletonScope();
         bind(ImageSourceProvider).toSelf().inSingletonScope();
 
         bind(ServerFactory).toAutoFactory(GitpodServerImpl);

components/server/src/jobs/runner.ts

@@ -9,7 +9,6 @@ import { repeat } from "@gitpod/gitpod-protocol/lib/util/repeat";
 import { inject, injectable } from "inversify";
 import { RedisMutex } from "../redis/mutex";
 import { log } from "@gitpod/gitpod-protocol/lib/util/logging";
-import { ExecutionError, ResourceLockedError } from "redlock";
 import { jobsDurationSeconds, reportJobCompleted, reportJobStarted } from "../prometheus-metrics";
 import { DatabaseGarbageCollector } from "./database-gc";
 import { OTSGarbageCollector } from "./ots-gc";
@@ -19,6 +18,7 @@ import { WorkspaceGarbageCollector } from "./workspace-gc";
 import { SnapshotsJob } from "./snapshots";
 import { RelationshipUpdateJob } from "../authorization/relationship-updater-job";
 import { runWithContext } from "../util/log-context";
+import { WorkspaceStartController } from "../workspace/workspace-start-controller";
 
 export const Job = Symbol("Job");
 
@@ -40,6 +40,7 @@ export class JobRunner {
         @inject(WorkspaceGarbageCollector) private readonly workspaceGC: WorkspaceGarbageCollector,
         @inject(SnapshotsJob) private readonly snapshotsJob: SnapshotsJob,
         @inject(RelationshipUpdateJob) private readonly relationshipUpdateJob: RelationshipUpdateJob,
+        @inject(WorkspaceStartController) private readonly workspaceStartController: WorkspaceStartController,
     ) {}
 
     public start(): DisposableCollection {
@@ -53,6 +54,7 @@ export class JobRunner {
             this.workspaceGC,
             this.snapshotsJob,
             this.relationshipUpdateJob,
+            this.workspaceStartController,
         ];
 
         for (const job of jobs) {
@@ -106,7 +108,7 @@ export class JobRunner {
                 });
             });
         } catch (err) {
-            if (err instanceof ResourceLockedError || err instanceof ExecutionError) {
+            if (RedisMutex.isLockError(err)) {
                 log.debug(
                     `Failed to acquire lock for job ${job.name}. Likely another instance already holds the lock.`,
                     err,

components/server/src/redis/mutex.ts

@@ -6,7 +6,7 @@
 
 import { inject, injectable } from "inversify";
 import { Redis } from "ioredis";
-import Redlock, { RedlockAbortSignal } from "redlock";
+import Redlock, { RedlockAbortSignal, ResourceLockedError, ExecutionError, Settings } from "redlock";
 
 @injectable()
 export class RedisMutex {
@@ -40,7 +40,12 @@ export class RedisMutex {
         resources: string[],
         duration: number,
         routine: (signal: RedlockAbortSignal) => Promise<T>,
+        settings: Partial<Settings> = {},
     ): Promise<T> {
-        return this.client().using(resources, duration, routine);
+        return this.client().using(resources, duration, settings, routine);
+    }
+
+    public static isLockError(err: any): boolean {
+        return err instanceof ResourceLockedError || err instanceof ExecutionError;
     }
 }

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

@@ -122,7 +122,7 @@ describe("WorkspaceService", async () => {
         const workspace = await createTestWorkspace(workspaceService, org, owner, project);
 
         const result = await workspaceService.startWorkspace({}, owner, workspace.id);
-        expect(result.workspaceURL).to.equal(`https://${workspace.id}.ws.gitpod.io`);
+        expect(result.instanceID).to.not.be.undefined;
     });
 
     it("owner can start own workspace - shared", async () => {
@@ -131,7 +131,7 @@ describe("WorkspaceService", async () => {
         await workspaceService.controlAdmission(owner.id, workspace.id, "everyone");
 
         const result = await workspaceService.startWorkspace({}, owner, workspace.id);
-        expect(result.workspaceURL).to.equal(`https://${workspace.id}.ws.gitpod.io`);
+        expect(result.instanceID).to.not.be.undefined;
     });
 
     it("stanger cannot start owner workspace", async () => {
@@ -164,7 +164,7 @@ describe("WorkspaceService", async () => {
         const workspaceService = container.get(WorkspaceService);
         const workspace = await createTestWorkspace(workspaceService, org, member, project);
         const result = await workspaceService.startWorkspace({}, member, workspace.id);
-        expect(result.workspaceURL).to.equal(`https://${workspace.id}.ws.gitpod.io`);
+        expect(result.instanceID).to.not.be.undefined;
     });
 
     it("stanger cannot start org member workspace", async () => {

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

@@ -53,7 +53,6 @@ import { getExperimentsClientForBackend } from "@gitpod/gitpod-protocol/lib/expe
 import { WorkspaceRegion, isWorkspaceRegion } from "@gitpod/gitpod-protocol/lib/workspace-cluster";
 import { RegionService } from "./region-service";
 import { ProjectsService } from "../projects/projects-service";
-import { EnvVarService } from "../user/env-var-service";
 import { WorkspaceManagerClientProvider } from "@gitpod/ws-manager/lib/client-provider";
 import { SupportedWorkspaceClass } from "@gitpod/gitpod-protocol/lib/workspace-class";
 import { Config } from "../config";
@@ -80,7 +79,6 @@ export class WorkspaceService {
         @inject(WorkspaceManagerClientProvider) private readonly clientProvider: WorkspaceManagerClientProvider,
         @inject(WorkspaceDB) private readonly db: WorkspaceDB,
         @inject(EntitlementService) private readonly entitlementService: EntitlementService,
-        @inject(EnvVarService) private readonly envVarService: EnvVarService,
         @inject(ProjectsService) private readonly projectsService: ProjectsService,
         @inject(OrganizationService) private readonly orgService: OrganizationService,
         @inject(RedisPublisher) private readonly publisher: RedisPublisher,
@@ -475,12 +473,6 @@ export class WorkspaceService {
             throw new ApplicationError(ErrorCodes.NOT_FOUND, "Workspace not found!");
         }
 
-        const envVarsPromise = this.envVarService.resolveEnvVariables(
-            user.id,
-            workspace.projectId,
-            workspace.type,
-            workspace.context,
-        );
         const projectPromise = workspace.projectId
             ? ApplicationError.notFoundToUndefined(this.projectsService.getProject(user.id, workspace.projectId))
             : Promise.resolve(undefined);
@@ -495,14 +487,7 @@ export class WorkspaceService {
         );
 
         // at this point we're about to actually start a new workspace
-        const result = await this.workspaceStarter.startWorkspace(
-            ctx,
-            workspace,
-            user,
-            await projectPromise,
-            await envVarsPromise,
-            options,
-        );
+        const result = await this.workspaceStarter.startWorkspace(ctx, workspace, user, await projectPromise, options);
         return result;
     }
 

components/server/src/workspace/workspace-start-controller.ts

@@ -0,0 +1,80 @@
+/**
+ * Copyright (c) 2023 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 { TraceContext } from "@gitpod/gitpod-protocol/lib/util/tracing";
+import { Job } from "../jobs/runner";
+import { DBWithTracing, TracedWorkspaceDB, UserDB, WorkspaceDB } from "@gitpod/gitpod-db/lib";
+import { durationLongerThanSeconds } from "@gitpod/gitpod-protocol/lib/util/timeutil";
+import { WorkspaceStarter } from "./workspace-starter";
+import { getExperimentsClientForBackend } from "@gitpod/gitpod-protocol/lib/experiments/configcat-server";
+import { log } from "@gitpod/gitpod-protocol/lib/util/logging";
+
+@injectable()
+export class WorkspaceStartController implements Job {
+    public readonly name: string = "workspace-start-controller";
+    public readonly frequencyMs: number = 1000 * 10; // 10s
+
+    constructor(
+        @inject(TracedWorkspaceDB) private readonly workspaceDB: DBWithTracing<WorkspaceDB>,
+        @inject(UserDB) private readonly userDB: UserDB,
+        @inject(WorkspaceStarter) private readonly workspaceStarter: WorkspaceStarter,
+    ) {}
+
+    public async run(): Promise<void> {
+        const span = TraceContext.startSpan("controlStartingWorkspaces");
+        const ctx = { span };
+
+        try {
+            const instances = await this.workspaceDB.trace(ctx).findInstancesByPhase(WorkspaceStarter.STARTING_PHASES);
+            for (const instance of instances) {
+                try {
+                    const phase = instance.status.phase;
+                    if (
+                        phase === "preparing" ||
+                        phase === "building" ||
+                        // !!! Note: during pending, the handover between app-cluster and ws-manager happens. !!!
+                        // This means:
+                        //  - there is a control loop on ws-manager-bridge that checks for an upper limit a instance may be in pending phase
+                        //  - it will take some time after calling ws-manager to see the first status update
+                        // In 99.9% this is due to timing, so we need to be a bit cautious here not to spam ourselves.
+                        (phase === "pending" && durationLongerThanSeconds(Date.parse(instance.creationTime), 30))
+                    ) {
+                        // this.reconcileWorkspaceStart(ctx, instance.id);
+                        const workspace = await this.workspaceDB.trace(ctx).findById(instance.workspaceId);
+                        if (!workspace) {
+                            throw new Error("cannot find workspace for instance");
+                        }
+                        const user = await this.userDB.findUserById(workspace.ownerId);
+                        if (!user) {
+                            throw new Error("cannot find owner for workspace");
+                        }
+
+                        const isEnabled = await getExperimentsClientForBackend().getValueAsync(
+                            "workspace_start_controller",
+                            false,
+                            {
+                                user,
+                                projectId: workspace.projectId,
+                            },
+                        );
+                        if (!isEnabled) {
+                            continue;
+                        }
+
+                        await this.workspaceStarter.reconcileWorkspaceStart(ctx, instance.id, user, workspace);
+                    }
+                } catch (err) {
+                    log.warn({ instanceId: instance.id }, "error while reconciling workspace start", err);
+                }
+            }
+        } catch (err) {
+            TraceContext.setError(ctx, err);
+        } finally {
+            span.finish();
+        }
+    }
+}