[prebuilds, workspace-gc] Fix and add tests for not showing soft-dele… (#20093)

* [prebuilds, workspace-gc] Fix and add tests for not showing soft-deleted prebuilds anymore

* [workspace-gc] Add more explanatory comments

* Apply suggestions from code review

Co-authored-by: Filip Troníček <[email protected]>

---------

Co-authored-by: Filip Troníček <[email protected]>

Author: geropl

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

@@ -498,7 +498,7 @@ export class TypeORMWorkspaceDBImpl extends TransactionalDBImpl<WorkspaceDB> imp
                        ws.ownerId AS ownerId
                     FROM d_b_workspace AS ws
                     WHERE ws.deleted = 0
-                        AND ws.type= ?
+                        AND ws.type = ?
                         AND ws.softDeleted IS NULL
                         AND ws.softDeletedTime = ''
                         AND ws.pinned = 0
@@ -556,35 +556,6 @@ export class TypeORMWorkspaceDBImpl extends TransactionalDBImpl<WorkspaceDB> imp
         return dbResults as WorkspaceOwnerAndSoftDeleted[];
     }
 
-    /**
-     * @deprecated delete me end of June 2024
-     */
-    public async findPrebuiltWorkspacesForGC(daysUnused: number, limit: number): Promise<WorkspaceAndOwner[]> {
-        const workspaceRepo = await this.getWorkspaceRepo();
-        const dbResults = await workspaceRepo.query(
-            `
-                SELECT ws.id AS id,
-                    ws.ownerId AS ownerId
-                FROM d_b_workspace AS ws,
-                    d_b_prebuilt_workspace AS pb
-                LEFT OUTER JOIN d_b_workspace AS usages ON usages.basedOnPrebuildId = pb.id
-                WHERE
-                        pb.buildworkspaceId = ws.id
-                    AND ws.type = 'prebuild'
-                    AND ws.contentDeletedTime = ''
-                    AND ws.deletionEligibilityTime = ''
-                    AND ws.pinned = 0
-                    AND ws.creationTime < NOW() - INTERVAL ? DAY
-                GROUP BY ws.id, ws.ownerId
-                HAVING
-                    max(usages.creationTime) IS NULL or max(usages.creationTime) < NOW() - INTERVAL ? DAY
-                LIMIT ?;
-            `,
-            [daysUnused, daysUnused, limit],
-        );
-        return dbResults as WorkspaceAndOwner[];
-    }
-
     private async doJoinInstanceWithWorkspace<T>(
         conditions: string[],
         conditionParams: {},
@@ -722,11 +693,22 @@ export class TypeORMWorkspaceDBImpl extends TransactionalDBImpl<WorkspaceDB> imp
 
     public async findPrebuildByWorkspaceID(wsid: string): Promise<PrebuiltWorkspace | undefined> {
         const repo = await this.getPrebuiltWorkspaceRepo();
-        return await repo.createQueryBuilder("pws").where("pws.buildWorkspaceId = :wsid", { wsid }).getOne();
+        return await repo
+            .createQueryBuilder("pws")
+            .innerJoin(DBWorkspace, "ws", "pws.buildWorkspaceId = ws.id")
+            .where("pws.buildWorkspaceId = :wsid", { wsid })
+            .andWhere("ws.contentDeletedTime = ''")
+            .getOne();
     }
+
     public async findPrebuildByID(pwsid: string): Promise<PrebuiltWorkspace | undefined> {
         const repo = await this.getPrebuiltWorkspaceRepo();
-        return await repo.findOne(pwsid);
+        return await repo
+            .createQueryBuilder("pws")
+            .innerJoin(DBWorkspace, "ws", "pws.buildWorkspaceId = ws.id")
+            .where("pws.id = :pwsid", { pwsid })
+            .andWhere("ws.contentDeletedTime = ''")
+            .getOne();
     }
 
     public async findPrebuildsWithWorkspace(projectId: string): Promise<PrebuildWithWorkspace[]> {
@@ -1063,6 +1045,7 @@ export class TypeORMWorkspaceDBImpl extends TransactionalDBImpl<WorkspaceDB> imp
             .innerJoinAndMapOne("pws.project", DBProject, "project", "pws.projectId = project.id")
             .where("project.markedDeleted = false")
             .andWhere("project.id IS NOT NULL")
+            .andWhere("ws.contentDeletedTime = ''")
             .skip(pagination.offset)
             .take(pagination.limit)
             .orderBy("pws.creationTime", sort.order); // todo: take sort field into account
@@ -1130,7 +1113,8 @@ export class TypeORMWorkspaceDBImpl extends TransactionalDBImpl<WorkspaceDB> imp
             .createQueryBuilder("pws")
             .orderBy("pws.creationTime", "DESC")
             .innerJoinAndMapOne("pws.workspace", DBWorkspace, "ws", "pws.buildWorkspaceId = ws.id")
-            .andWhere("pws.id = :id", { id });
+            .andWhere("pws.id = :id", { id })
+            .andWhere("ws.contentDeletedTime = ''");
 
         return query.getOne();
     }

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

@@ -235,42 +235,50 @@ class WorkspaceDBSpec {
     }
 
     @test(timeout(10000))
-    public async testFindPrebuildsForGC_oldPrebuildNoUsage() {
-        await this.createPrebuild(2);
-        const dbResult = await this.db.findPrebuiltWorkspacesForGC(1, 10);
-        expect(dbResult.length).to.eq(1);
-        expect(dbResult[0].id).to.eq("12345");
-        expect(dbResult[0].ownerId).to.eq("1221423");
+    public async testFindEligableWorkspacesForSoftDeletion_markedEligable_Prebuild() {
+        const { ws } = await this.createPrebuild(20, 15);
+        const dbResult = await this.db.findEligibleWorkspacesForSoftDeletion(new Date(), 10, "prebuild");
+        expect(dbResult.length).to.equal(1);
+        expect(dbResult[0].id).to.eq(ws.id);
+        expect(dbResult[0].ownerId).to.eq(ws.ownerId);
     }
 
     @test(timeout(10000))
-    public async testFindPrebuildsForGC_newPrebuildNoUsage() {
-        await this.createPrebuild(0);
-        const dbResult = await this.db.findPrebuiltWorkspacesForGC(1, 10);
+    public async testFindEligableWorkspacesForSoftDeletion_notMarkedEligable_Prebuild() {
+        await this.createPrebuild(20, -7);
+        const dbResult = await this.db.findEligibleWorkspacesForSoftDeletion(new Date(), 10, "prebuild");
         expect(dbResult.length).to.eq(0);
     }
 
     @test(timeout(10000))
-    public async testFindPrebuildsForGC_oldPrebuildOldUsage() {
-        await this.createPrebuild(2, 2);
-        const dbResult = await this.db.findPrebuiltWorkspacesForGC(1, 10);
-        expect(dbResult.length).to.eq(1);
-        expect(dbResult[0].id).to.eq("12345");
-        expect(dbResult[0].ownerId).to.eq("1221423");
-    }
+    public async testPrebuildGarbageCollection() {
+        const { pbws } = await this.createPrebuild(20, 15);
+
+        // mimick the behavior of the Garbage Collector
+        const gcWorkspaces = await this.db.findEligibleWorkspacesForSoftDeletion(new Date(), 10, "prebuild");
+        expect(gcWorkspaces.length).to.equal(1);
+
+        const now = new Date().toISOString();
+        await this.db.updatePartial(gcWorkspaces[0].id, {
+            contentDeletedTime: now,
+            softDeletedTime: now,
+            softDeleted: "gc",
+        });
 
-    @test(timeout(10000))
-    public async testFindPrebuildsForGC_oldPrebuildNewUsage() {
-        await this.createPrebuild(12, 0);
-        const dbResult = await this.db.findPrebuiltWorkspacesForGC(1, 10);
-        expect(dbResult.length).to.eq(0);
+        // next cycle is empty
+        const nextGcCycle = await this.db.findEligibleWorkspacesForSoftDeletion(new Date(), 10, "prebuild");
+        expect(nextGcCycle.length).to.equal(0);
+
+        // prebuild can't be discovered anymore because it's workspace has been GC'ed
+        const prebuild = await this.db.findPrebuildByID(pbws.id);
+        expect(prebuild).to.be.undefined;
     }
 
-    protected async createPrebuild(createdDaysAgo: number, usageDaysAgo?: number) {
+    protected async createPrebuild(createdDaysAgo: number, deletionEligibilityTimeDaysAgo?: number) {
         const now = new Date();
         now.setDate(now.getDate() - createdDaysAgo);
         const creationTime = now.toISOString();
-        await this.db.store({
+        const ws = await this.db.store({
             id: "12345",
             creationTime,
             description: "something",
@@ -283,33 +291,23 @@ class WorkspaceDBSpec {
             config: {},
             type: "prebuild",
         });
-        await this.db.storePrebuiltWorkspace({
+        const pbws = await this.db.storePrebuiltWorkspace({
             id: "prebuild123",
             buildWorkspaceId: "12345",
             creationTime,
-            cloneURL: "",
+            cloneURL: "https://github.com/foo/bar",
             commit: "",
             state: "available",
             statusVersion: 0,
         });
-        if (usageDaysAgo !== undefined) {
-            const now = new Date();
-            now.setDate(now.getDate() - usageDaysAgo);
-            await this.db.store({
-                id: "usage-of-12345",
-                creationTime: now.toISOString(),
-                description: "something",
-                contextURL: "https://github.com/foo/bar",
-                ownerId: "1221423",
-                organizationId: "org123",
-                context: {
-                    title: "my title",
-                },
-                config: {},
-                basedOnPrebuildId: "prebuild123",
-                type: "regular",
-            });
+
+        if (deletionEligibilityTimeDaysAgo !== undefined) {
+            const deletionEligibilityTime = new Date();
+            deletionEligibilityTime.setDate(deletionEligibilityTime.getDate() - deletionEligibilityTimeDaysAgo);
+            await this.db.updatePartial(ws.id, { deletionEligibilityTime: deletionEligibilityTime.toISOString() });
         }
+
+        return { ws, pbws };
     }
 
     @test(timeout(10000))

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

@@ -112,7 +112,6 @@ export interface WorkspaceDB {
         limit: number,
         now: Date,
     ): Promise<WorkspaceAndOwner[]>;
-    findPrebuiltWorkspacesForGC(daysUnused: number, limit: number): Promise<WorkspaceAndOwner[]>;
     findAllWorkspaces(
         offset: number,
         limit: number,

components/server/src/jobs/workspace-gc.ts

@@ -22,9 +22,15 @@ import { SYSTEM_USER_ID } from "../authorization/authorizer";
 import { StorageClient } from "../storage/storage-client";
 
 /**
- * The WorkspaceGarbageCollector has two tasks:
- *  - mark old, unused workspaces as 'softDeleted = "gc"' after a certain period (initially: 21)
- *  - actually delete softDeleted workspaces if they are older than a configured time (initially: 7)
+ * The WorkspaceGarbageCollector is responsible for moving workspaces (also affecting Prebuilds) through the following state machine:
+ *  - every action (create, start, stop, use as prebuild) updates the workspace.deletionEligibilityTime, which is set to some date into the future
+ *  - the GC has multiple sub-tasks to:
+ *    - find _regular_ workspaces "to delete" (with deletionEligibilityTime < now) -> move to "softDeleted"
+ *    - find _any_ workspace "softDeleted" for long enough -> move to "contentDeleted"
+ *    - find _any_ workspace "contentDeleted" for long enough -> move to "purged"
+ *  - prebuilds are special in that:
+ *    - the GC has a dedicated sub-task to move workspace of type "prebuid" from "to delete" (with a different threshold) -> to "contentDeleted" directly
+ *    - the "purging" takes care of all Prebuild-related sub-resources, too
  */
 @injectable()
 export class WorkspaceGarbageCollector implements Job {
@@ -49,31 +55,33 @@ export class WorkspaceGarbageCollector implements Job {
             return;
         }
 
+        // Move eligible "regular" workspace -> softDeleted
         try {
             await this.softDeleteEligibleWorkspaces();
         } catch (error) {
             log.error("workspace-gc: error during eligible workspace deletion", error);
         }
+
+        // Move softDeleted workspaces -> contentDeleted
         try {
             await this.deleteWorkspaceContentAfterRetentionPeriod();
         } catch (error) {
             log.error("workspace-gc: error during content deletion", error);
         }
+
+        // Move eligible "prebuild" workspaces -> contentDeleted (jumping over softDeleted)
+        // At this point, Prebuilds are no longer visible nor usable.
         try {
-            await this.purgeWorkspacesAfterPurgeRetentionPeriod();
-        } catch (err) {
-            log.error("workspace-gc: error during hard deletion of workspaces", err);
-        }
-        try {
-            //TODO (se) delete this end of June 2024
-            await this.deleteOldPrebuilds();
+            await this.deleteEligiblePrebuilds();
         } catch (err) {
-            log.error("workspace-gc: error during prebuild deletion", err);
+            log.error("workspace-gc: error during eligible prebuild deletion", err);
         }
+
+        // Move contentDeleted workspaces -> purged (calling workspaceService.hardDeleteWorkspace)
         try {
-            await this.deleteEligiblePrebuilds();
+            await this.purgeWorkspacesAfterPurgeRetentionPeriod();
         } catch (err) {
-            log.error("workspace-gc: error during eligible prebuild deletion", err);
+            log.error("workspace-gc: error during hard deletion of workspaces", err);
         }
 
         return undefined;
@@ -230,40 +238,6 @@ export class WorkspaceGarbageCollector implements Job {
         }
     }
 
-    private async deleteOldPrebuilds() {
-        const span = opentracing.globalTracer().startSpan("deleteOldPrebuilds");
-        try {
-            const now = new Date();
-            const workspaces = await this.workspaceDB
-                .trace({ span })
-                .findPrebuiltWorkspacesForGC(
-                    this.config.workspaceGarbageCollection.minAgePrebuildDays,
-                    this.config.workspaceGarbageCollection.chunkLimit,
-                );
-            const afterSelect = new Date();
-            log.info(`workspace-gc: about to delete ${workspaces.length} prebuilds`);
-            for (const ws of workspaces) {
-                try {
-                    await this.garbageCollectPrebuild({ span }, ws);
-                } catch (err) {
-                    log.error({ workspaceId: ws.id }, "workspace-gc: failed to delete prebuild", err);
-                }
-            }
-            const afterDelete = new Date();
-
-            log.info(`workspace-gc: successfully deleted ${workspaces.length} prebuilds`, {
-                selectionTimeMs: afterSelect.getTime() - now.getTime(),
-                deletionTimeMs: afterDelete.getTime() - afterSelect.getTime(),
-            });
-            span.addTags({ nrOfCollectedPrebuilds: workspaces.length });
-        } catch (err) {
-            TraceContext.setError({ span }, err);
-            throw err;
-        } finally {
-            span.finish();
-        }
-    }
-
     /**
      * This method garbageCollects a workspace. It deletes its contents and sets the workspaces 'contentDeletedTime'
      * @param ctx