From 01661f9a0bed38c6f3fd22621fa389b3f770c713 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Filip=20Tron=C3=AD=C4=8Dek?= Date: Fri, 4 Oct 2024 15:48:03 +0000 Subject: [PATCH 01/13] [ws-gc] Additional logging --- .../gitpod-db/src/typeorm/workspace-db-impl.ts | 14 ++++++++------ components/gitpod-db/src/workspace-db.ts | 6 ++++-- components/server/src/config.ts | 4 ++-- components/server/src/jobs/workspace-gc.ts | 14 +++++++++++++- .../server/src/workspace/workspace-service.ts | 4 ++-- 5 files changed, 29 insertions(+), 13 deletions(-) diff --git a/components/gitpod-db/src/typeorm/workspace-db-impl.ts b/components/gitpod-db/src/typeorm/workspace-db-impl.ts index e4c19b48c73981..8d575b03a869f0 100644 --- a/components/gitpod-db/src/typeorm/workspace-db-impl.ts +++ b/components/gitpod-db/src/typeorm/workspace-db-impl.ts @@ -36,8 +36,9 @@ import { PrebuildWithWorkspace, PrebuildWithWorkspaceAndInstances, PrebuiltUpdatableAndWorkspace, - WorkspaceAndOwner, WorkspaceDB, + WorkspaceOwnerAndContentDeletedTime, + WorkspaceOwnerAndDeletionEligibility, WorkspaceOwnerAndSoftDeleted, WorkspacePortsAuthData, } from "../workspace-db"; @@ -486,7 +487,7 @@ export class TypeORMWorkspaceDBImpl extends TransactionalDBImpl imp cutOffDate: Date = new Date(), limit: number = 100, type: WorkspaceType = "regular", - ): Promise { + ): Promise { // we do not allow to run this with a future date if (cutOffDate > new Date()) { throw new Error("cutOffDate must not be in the future, was: " + cutOffDate.toISOString()); @@ -495,7 +496,8 @@ export class TypeORMWorkspaceDBImpl extends TransactionalDBImpl imp const dbResults = await workspaceRepo.query( ` SELECT ws.id AS id, - ws.ownerId AS ownerId + ws.ownerId AS ownerId, + ws.deletionEligibilityTime AS deletionEligibilityTime FROM d_b_workspace AS ws WHERE ws.deleted = 0 AND ws.type = ? @@ -509,19 +511,19 @@ export class TypeORMWorkspaceDBImpl extends TransactionalDBImpl imp [type, cutOffDate.toISOString(), limit], ); - return dbResults as WorkspaceAndOwner[]; + return dbResults as WorkspaceOwnerAndDeletionEligibility[]; } public async findWorkspacesForPurging( minContentDeletionTimeInDays: number, limit: number, now: Date, - ): Promise { + ): Promise { const minPurgeTime = daysBefore(now.toISOString(), minContentDeletionTimeInDays); const repo = await this.getWorkspaceRepo(); const qb = repo .createQueryBuilder("ws") - .select(["ws.id", "ws.ownerId"]) + .select(["ws.id", "ws.ownerId", "ws.contentDeletedTime"]) .where(`ws.contentDeletedTime != ''`) .andWhere(`ws.contentDeletedTime < :minPurgeTime`, { minPurgeTime }) .andWhere(`ws.deleted = 0`) diff --git a/components/gitpod-db/src/workspace-db.ts b/components/gitpod-db/src/workspace-db.ts index 9336b193fe4888..e42a23186e2cdc 100644 --- a/components/gitpod-db/src/workspace-db.ts +++ b/components/gitpod-db/src/workspace-db.ts @@ -64,6 +64,8 @@ export interface PrebuildWithWorkspaceAndInstances { export type WorkspaceAndOwner = Pick; export type WorkspaceOwnerAndSoftDeleted = Pick; +export type WorkspaceOwnerAndDeletionEligibility = Pick; +export type WorkspaceOwnerAndContentDeletedTime = Pick; export const WorkspaceDB = Symbol("WorkspaceDB"); export interface WorkspaceDB { @@ -102,7 +104,7 @@ export interface WorkspaceDB { cutOffDate?: Date, limit?: number, type?: WorkspaceType, - ): Promise; + ): Promise; findWorkspacesForContentDeletion( minSoftDeletedTimeInDays: number, limit: number, @@ -111,7 +113,7 @@ export interface WorkspaceDB { minContentDeletionTimeInDays: number, limit: number, now: Date, - ): Promise; + ): Promise; findAllWorkspaces( offset: number, limit: number, diff --git a/components/server/src/config.ts b/components/server/src/config.ts index 33d4cf5651be47..c949a156ffba8d 100644 --- a/components/server/src/config.ts +++ b/components/server/src/config.ts @@ -84,10 +84,10 @@ export interface WorkspaceGarbageCollection { /** The minimal age of a workspace before it is marked as 'softDeleted' (= hidden for the user) */ minAgeDays: number; - /** The minimal age of a prebuild (incl. workspace) before it's content is deleted (+ marked as 'softDeleted') */ + /** The minimal age of a prebuild (incl. workspace) before its content is deleted (+ marked as 'softDeleted') */ minAgePrebuildDays: number; - /** The minimal number of days a workspace has to stay in 'softDeleted' before it's content is deleted */ + /** The minimal number of days a workspace has to stay in 'softDeleted' before its content is deleted */ contentRetentionPeriodDays: number; /** The maximum amount of workspaces whose content is deleted in one go */ diff --git a/components/server/src/jobs/workspace-gc.ts b/components/server/src/jobs/workspace-gc.ts index c3cc3f29f825cb..042ef7aba34dd3 100644 --- a/components/server/src/jobs/workspace-gc.ts +++ b/components/server/src/jobs/workspace-gc.ts @@ -29,7 +29,7 @@ import { StorageClient } from "../storage/storage-client"; * - 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 GC has a dedicated sub-task to move workspace of type "prebuild" from "to delete" (with a different threshold) -> to "contentDeleted" directly * - the "purging" takes care of all Prebuild-related sub-resources, too */ @injectable() @@ -55,6 +55,11 @@ export class WorkspaceGarbageCollector implements Job { return; } + log.info("workspace-gc: job started", { + workspaceMinAgeDays: this.config.workspaceGarbageCollection.minAgeDays, + prebuildMinAgeDays: this.config.workspaceGarbageCollection.minAgePrebuildDays, + }); + // Move eligible "regular" workspace -> softDeleted try { await this.softDeleteEligibleWorkspaces(); @@ -115,6 +120,10 @@ export class WorkspaceGarbageCollector implements Job { err, ); } + + log.info({ workspaceId: ws.id }, `workspace-gc: soft deleted a workspace`, { + deletionEligibilityTime: ws.deletionEligibilityTime, + }); } const afterDelete = new Date(); @@ -187,6 +196,9 @@ export class WorkspaceGarbageCollector implements Job { } catch (err) { log.error({ workspaceId: ws.id }, "workspace-gc: failed to purge workspace", err); } + log.info({ workspaceId: ws.id }, `workspace-gc: hard deleted a workspace`, { + contentDeletedTime: ws.contentDeletedTime, + }); } const afterDelete = new Date(); diff --git a/components/server/src/workspace/workspace-service.ts b/components/server/src/workspace/workspace-service.ts index f437d31bf8aaf7..8978c5cae21d6e 100644 --- a/components/server/src/workspace/workspace-service.ts +++ b/components/server/src/workspace/workspace-service.ts @@ -524,8 +524,8 @@ export class WorkspaceService { /** * Sets the deletionEligibilityTime of the workspace, depending on the current state of the workspace and the configuration. * - * @param userId sets the - * @param workspaceId + * @param userId the user to act as + * @param workspaceId the workspace to update * @returns */ async updateDeletionEligabilityTime(userId: string, workspaceId: string, activeNow = false): Promise { From fa953e880700ad8d3f96ac977b26b2ca150edef1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Filip=20Tron=C3=AD=C4=8Dek?= Date: Fri, 4 Oct 2024 15:57:37 +0000 Subject: [PATCH 02/13] typo fix --- components/server/src/workspace/workspace-starter.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/server/src/workspace/workspace-starter.ts b/components/server/src/workspace/workspace-starter.ts index 9ad55f1f4b0fb3..8e14af14bb8cdc 100644 --- a/components/server/src/workspace/workspace-starter.ts +++ b/components/server/src/workspace/workspace-starter.ts @@ -490,7 +490,7 @@ export class WorkspaceStarter { client = await this.clientProvider.get(instanceRegion); } catch (err) { log.error({ instanceId }, "cannot stop workspace instance", err); - // we want to stop a workspace but the region doesn't exist. So we can assume it doesn't run anyymore and there will never be updates coming to bridge. + // we want to stop a workspace but the region doesn't exist. So we can assume it doesn't run anymore and there will never be updates coming to bridge. // let's mark this workspace as stopped if it is not already stopped. const workspace = await this.workspaceDb.trace(ctx).findByInstanceId(instanceId); const instance = await this.workspaceDb.trace(ctx).findInstanceById(instanceId); From c7d01ea30e63b5d2281c5cf566ab4ad2d2e326c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Filip=20Tron=C3=AD=C4=8Dek?= Date: Mon, 7 Oct 2024 07:53:05 +0000 Subject: [PATCH 03/13] test update --- components/gitpod-db/src/workspace-db.spec.db.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/components/gitpod-db/src/workspace-db.spec.db.ts b/components/gitpod-db/src/workspace-db.spec.db.ts index 60669b3e0bcca2..2321c9b360e0c6 100644 --- a/components/gitpod-db/src/workspace-db.spec.db.ts +++ b/components/gitpod-db/src/workspace-db.spec.db.ts @@ -254,7 +254,7 @@ class WorkspaceDBSpec { public async testPrebuildGarbageCollection() { const { pbws } = await this.createPrebuild(20, 15); - // mimick the behavior of the Garbage Collector + // mimic the behavior of the Garbage Collector const gcWorkspaces = await this.db.findEligibleWorkspacesForSoftDeletion(new Date(), 10, "prebuild"); expect(gcWorkspaces.length).to.equal(1); @@ -768,6 +768,7 @@ class WorkspaceDBSpec { { id: "1", ownerId, + contentDeletedTime: d20180131, }, ]); } From c428b1e5e85a3d5dd84ec96b0a4af8aff14b906c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Filip=20Tron=C3=AD=C4=8Dek?= Date: Mon, 7 Oct 2024 11:18:05 +0000 Subject: [PATCH 04/13] Workspace is active now if it just stopped, started or just got created --- .../workspace/workspace-service.spec.db.ts | 6 ++-- .../server/src/workspace/workspace-service.ts | 30 +++++++++++-------- 2 files changed, 21 insertions(+), 15 deletions(-) diff --git a/components/server/src/workspace/workspace-service.spec.db.ts b/components/server/src/workspace/workspace-service.spec.db.ts index 23e55f533e476d..175a2a07b83139 100644 --- a/components/server/src/workspace/workspace-service.spec.db.ts +++ b/components/server/src/workspace/workspace-service.spec.db.ts @@ -701,7 +701,7 @@ describe("WorkspaceService", async () => { workspaceImage: "", }); - await svc.updateDeletionEligabilityTime(owner.id, ws.id); + await svc.updateDeletionEligibilityTime(owner.id, ws.id); const workspace = await svc.getWorkspace(owner.id, ws.id); expect(workspace).to.not.be.undefined; @@ -735,7 +735,7 @@ describe("WorkspaceService", async () => { workspaceImage: "", }); - await svc.updateDeletionEligabilityTime(owner.id, ws.id); + await svc.updateDeletionEligibilityTime(owner.id, ws.id); const workspace = await svc.getWorkspace(owner.id, ws.id); expect(workspace).to.not.be.undefined; @@ -771,7 +771,7 @@ describe("WorkspaceService", async () => { workspaceImage: "", }); - await svc.updateDeletionEligabilityTime(owner.id, ws.id); + await svc.updateDeletionEligibilityTime(owner.id, ws.id); const workspace = await svc.getWorkspace(owner.id, ws.id); expect(workspace).to.not.be.undefined; diff --git a/components/server/src/workspace/workspace-service.ts b/components/server/src/workspace/workspace-service.ts index 8978c5cae21d6e..ae66c2fc56d688 100644 --- a/components/server/src/workspace/workspace-service.ts +++ b/components/server/src/workspace/workspace-service.ts @@ -223,8 +223,8 @@ export class WorkspaceService { ); throw err; } - this.asyncUpdateDeletionEligabilityTime(user.id, workspace.id); - this.asyncUpdateDeletionEligabilityTimeForUsedPrebuild(user.id, workspace); + this.asyncUpdateDeletionEligibilityTime(user.id, workspace.id, true); + this.asyncUpdateDeletionEligibilityTimeForUsedPrebuild(user.id, workspace); if (project && workspace.type === "regular") { this.asyncHandleUpdatePrebuildTriggerStrategy({ ctx, project, workspace, user }); this.asyncStartPrebuild({ ctx, project, workspace, user }); @@ -248,7 +248,7 @@ export class WorkspaceService { const res = await this.db.find({ limit: 20, ...options, - userId, // gpl: We probably want to removed this limitation in the future, butkeeping the old behavior for now due to focus on FGA + userId, // gpl: We probably want to removed this limitation in the future, but keeping the old behavior for now due to focus on FGA includeHeadless: false, }); @@ -375,7 +375,7 @@ export class WorkspaceService { return; } await this.workspaceStarter.stopWorkspaceInstance({}, instance.id, instance.region, reason, policy); - this.asyncUpdateDeletionEligabilityTime(userId, workspaceId); + this.asyncUpdateDeletionEligibilityTime(userId, workspaceId, true); } public async stopRunningWorkspacesForUser( @@ -396,13 +396,13 @@ export class WorkspaceService { reason, policy, ); - this.asyncUpdateDeletionEligabilityTime(userId, info.workspace.id); + this.asyncUpdateDeletionEligibilityTime(userId, info.workspace.id, false); }), ); return infos.map((instance) => instance.workspace); } - private asyncUpdateDeletionEligabilityTimeForUsedPrebuild(userId: string, workspace: Workspace): void { + private asyncUpdateDeletionEligibilityTimeForUsedPrebuild(userId: string, workspace: Workspace): void { (async () => { if (WithPrebuild.is(workspace.context) && workspace.context.prebuildWorkspaceId) { // mark the prebuild active @@ -410,7 +410,7 @@ export class WorkspaceService { workspace.context.prebuildWorkspaceId, ); if (prebuiltWorkspace?.buildWorkspaceId) { - await this.updateDeletionEligabilityTime(userId, prebuiltWorkspace?.buildWorkspaceId, true); + await this.updateDeletionEligibilityTime(userId, prebuiltWorkspace?.buildWorkspaceId, true); } } })().catch((err) => @@ -515,8 +515,8 @@ export class WorkspaceService { ); } - private asyncUpdateDeletionEligabilityTime(userId: string, workspaceId: string): void { - this.updateDeletionEligabilityTime(userId, workspaceId).catch((err) => + private asyncUpdateDeletionEligibilityTime(userId: string, workspaceId: string, activeNow?: boolean): void { + this.updateDeletionEligibilityTime(userId, workspaceId, activeNow).catch((err) => log.error({ userId, workspaceId }, "Failed to update deletion eligibility time", err), ); } @@ -528,7 +528,7 @@ export class WorkspaceService { * @param workspaceId the workspace to update * @returns */ - async updateDeletionEligabilityTime(userId: string, workspaceId: string, activeNow = false): Promise { + async updateDeletionEligibilityTime(userId: string, workspaceId: string, activeNow = false): Promise { try { let daysToLive = this.config.workspaceGarbageCollection?.minAgeDays || 14; const daysToLiveForPrebuilds = this.config.workspaceGarbageCollection?.minAgePrebuildDays || 7; @@ -561,6 +561,12 @@ export class WorkspaceService { daysToLive = daysToLive * 2; } deletionEligibilityTime.setDate(deletionEligibilityTime.getDate() + daysToLive); + if (new Date() > deletionEligibilityTime) { + log.warn({ userId, workspaceId }, "Prevented setting deletion eligibility time in the past", { + deletionEligibilityTime: deletionEligibilityTime.toISOString(), + }); + return; + } await this.db.updatePartial(workspaceId, { deletionEligibilityTime: deletionEligibilityTime.toISOString(), }); @@ -819,7 +825,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, options); - this.asyncUpdateDeletionEligabilityTime(user.id, workspaceId); + this.asyncUpdateDeletionEligibilityTime(user.id, workspaceId, true); return result; } @@ -928,7 +934,7 @@ export class WorkspaceService { const workspace = await this.doGetWorkspace(userId, workspaceId); instance = await this.db.updateInstancePartial(instance.id, { gitStatus }); - await this.updateDeletionEligabilityTime(userId, workspaceId); + await this.updateDeletionEligibilityTime(userId, workspaceId, true); await this.publisher.publishInstanceUpdate({ instanceID: instance.id, ownerID: workspace.ownerId, From 97c09292a9e32774b072875f54609ecd07dde50b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Filip=20Tron=C3=AD=C4=8Dek?= Date: Mon, 7 Oct 2024 11:22:56 +0000 Subject: [PATCH 05/13] Don't ever GC currently running workspaces --- .../src/typeorm/workspace-db-impl.ts | 35 +++++++++---------- 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/components/gitpod-db/src/typeorm/workspace-db-impl.ts b/components/gitpod-db/src/typeorm/workspace-db-impl.ts index 8d575b03a869f0..7e351460dbc032 100644 --- a/components/gitpod-db/src/typeorm/workspace-db-impl.ts +++ b/components/gitpod-db/src/typeorm/workspace-db-impl.ts @@ -493,25 +493,24 @@ export class TypeORMWorkspaceDBImpl extends TransactionalDBImpl imp throw new Error("cutOffDate must not be in the future, was: " + cutOffDate.toISOString()); } const workspaceRepo = await this.getWorkspaceRepo(); - const dbResults = await workspaceRepo.query( - ` - SELECT ws.id AS id, - ws.ownerId AS ownerId, - ws.deletionEligibilityTime AS deletionEligibilityTime - FROM d_b_workspace AS ws - WHERE ws.deleted = 0 - AND ws.type = ? - AND ws.softDeleted IS NULL - AND ws.softDeletedTime = '' - AND ws.pinned = 0 - AND ws.deletionEligibilityTime != '' - AND ws.deletionEligibilityTime < ? - LIMIT ?; - `, - [type, cutOffDate.toISOString(), limit], - ); + const qb = workspaceRepo + .createQueryBuilder("ws") + .select(["ws.id", "ws.ownerId", "ws.deletionEligibilityTime"]) + .where("ws.deleted = :deleted", { deleted: 0 }) + .andWhere("ws.type = :type", { type }) + .andWhere("ws.softDeleted IS NULL") + .andWhere("ws.softDeletedTime = ''") + .andWhere("ws.pinned = :pinned", { pinned: 0 }) + .andWhere("ws.deletionEligibilityTime != ''") + .andWhere("ws.deletionEligibilityTime < :cutOffDate", { cutOffDate: cutOffDate.toISOString() }) + .limit(limit); - return dbResults as WorkspaceOwnerAndDeletionEligibility[]; + const results: WorkspaceOwnerAndDeletionEligibility[] = await qb.getMany(); + return results.filter((ws) => { + // we don't want to delete workspaces that have a running instance + const latestInstance = this.findRunningInstance(ws.id); + return latestInstance === undefined; + }); } public async findWorkspacesForPurging( From 2947014ed31907d844273b848bc99cfe9cc826a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Filip=20Tron=C3=AD=C4=8Dek?= Date: Mon, 7 Oct 2024 11:37:21 +0000 Subject: [PATCH 06/13] Fix tests --- components/gitpod-db/src/typeorm/workspace-db-impl.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/components/gitpod-db/src/typeorm/workspace-db-impl.ts b/components/gitpod-db/src/typeorm/workspace-db-impl.ts index 7e351460dbc032..9b18677bc13a8d 100644 --- a/components/gitpod-db/src/typeorm/workspace-db-impl.ts +++ b/components/gitpod-db/src/typeorm/workspace-db-impl.ts @@ -506,9 +506,9 @@ export class TypeORMWorkspaceDBImpl extends TransactionalDBImpl imp .limit(limit); const results: WorkspaceOwnerAndDeletionEligibility[] = await qb.getMany(); - return results.filter((ws) => { + return results.filter(async (ws) => { // we don't want to delete workspaces that have a running instance - const latestInstance = this.findRunningInstance(ws.id); + const latestInstance = await this.findRunningInstance(ws.id); return latestInstance === undefined; }); } From 440a4c45f094b1a08c930b5f3e297abce5362e13 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Filip=20Tron=C3=AD=C4=8Dek?= Date: Mon, 7 Oct 2024 12:20:28 +0000 Subject: [PATCH 07/13] Fix tests --- .../src/typeorm/workspace-db-impl.ts | 16 ++++-- .../gitpod-db/src/workspace-db.spec.db.ts | 53 +++++++++++++++++-- 2 files changed, 59 insertions(+), 10 deletions(-) diff --git a/components/gitpod-db/src/typeorm/workspace-db-impl.ts b/components/gitpod-db/src/typeorm/workspace-db-impl.ts index 9b18677bc13a8d..5780b039b10e2c 100644 --- a/components/gitpod-db/src/typeorm/workspace-db-impl.ts +++ b/components/gitpod-db/src/typeorm/workspace-db-impl.ts @@ -506,11 +506,17 @@ export class TypeORMWorkspaceDBImpl extends TransactionalDBImpl imp .limit(limit); const results: WorkspaceOwnerAndDeletionEligibility[] = await qb.getMany(); - return results.filter(async (ws) => { - // we don't want to delete workspaces that have a running instance - const latestInstance = await this.findRunningInstance(ws.id); - return latestInstance === undefined; - }); + + // hack to have an async filter predicate + const filtered = await Promise.all( + results.map(async (ws) => { + // we don't want to delete workspaces that have a running instance + const latestInstance = await this.findRunningInstance(ws.id); + return { ws, keep: latestInstance === undefined }; + }), + ); + + return filtered.filter((result) => result.keep).map((result) => result.ws); } public async findWorkspacesForPurging( diff --git a/components/gitpod-db/src/workspace-db.spec.db.ts b/components/gitpod-db/src/workspace-db.spec.db.ts index 2321c9b360e0c6..e7b069494cda44 100644 --- a/components/gitpod-db/src/workspace-db.spec.db.ts +++ b/components/gitpod-db/src/workspace-db.spec.db.ts @@ -26,6 +26,7 @@ class WorkspaceDBSpec { readonly timeWs = new Date(2018, 2, 16, 10, 0, 0).toISOString(); readonly timeBefore = new Date(2018, 2, 16, 11, 5, 10).toISOString(); readonly timeAfter = new Date(2019, 2, 16, 11, 5, 10).toISOString(); + readonly timeAfter2 = new Date(2019, 2, 17, 4, 20, 10).toISOString(); readonly userId = "12345"; readonly projectAID = "projectA"; readonly projectBID = "projectB"; @@ -101,6 +102,30 @@ class WorkspaceDBSpec { deleted: false, usageAttributionId: undefined, }; + readonly wsi3: WorkspaceInstance = { + workspaceId: this.ws.id, + id: "12345", + ideUrl: "example.org", + region: "unknown", + workspaceClass: undefined, + workspaceImage: "abc.io/test/image:123", + creationTime: this.timeAfter2, + startedTime: undefined, + deployedTime: undefined, + stoppingTime: undefined, + stoppedTime: undefined, + status: { + version: 1, + phase: "stopped", + conditions: {}, + }, + configuration: { + theiaVersion: "unknown", + ideImage: "unknown", + }, + deleted: false, + usageAttributionId: undefined, + }; readonly ws2: Workspace = { id: "2", type: "regular", @@ -235,7 +260,7 @@ class WorkspaceDBSpec { } @test(timeout(10000)) - public async testFindEligableWorkspacesForSoftDeletion_markedEligable_Prebuild() { + public async testFindEligibleWorkspacesForSoftDeletion_markedEligible_Prebuild() { const { ws } = await this.createPrebuild(20, 15); const dbResult = await this.db.findEligibleWorkspacesForSoftDeletion(new Date(), 10, "prebuild"); expect(dbResult.length).to.equal(1); @@ -244,7 +269,7 @@ class WorkspaceDBSpec { } @test(timeout(10000)) - public async testFindEligableWorkspacesForSoftDeletion_notMarkedEligable_Prebuild() { + public async testFindEligibleWorkspacesForSoftDeletion_notMarkedEligible_Prebuild() { await this.createPrebuild(20, -7); const dbResult = await this.db.findEligibleWorkspacesForSoftDeletion(new Date(), 10, "prebuild"); expect(dbResult.length).to.eq(0); @@ -311,16 +336,34 @@ class WorkspaceDBSpec { } @test(timeout(10000)) - public async testFindEligableWorkspacesForSoftDeletion_markedEligable() { + public async testFindEligibleWorkspacesForSoftDeletion_markedEligible() { this.ws.deletionEligibilityTime = this.timeWs; - await Promise.all([this.db.store(this.ws), this.db.storeInstance(this.wsi1), this.db.storeInstance(this.wsi2)]); + await Promise.all([ + this.db.store(this.ws), + this.db.storeInstance(this.wsi1), + this.db.storeInstance(this.wsi2), + this.db.storeInstance(this.wsi3), + ]); const dbResult = await this.db.findEligibleWorkspacesForSoftDeletion(new Date(this.timeAfter), 10); expect(dbResult[0].id).to.eq(this.ws.id); expect(dbResult[0].ownerId).to.eq(this.ws.ownerId); } @test(timeout(10000)) - public async testFindEligableWorkspacesForSoftDeletion_notMarkedEligable() { + public async testFindEligibleWorkspacesForSoftDeletion_notMarkedEligible() { + await Promise.all([ + this.db.store(this.ws), + this.db.storeInstance(this.wsi1), + this.db.storeInstance(this.wsi2), + this.db.storeInstance(this.wsi3), + ]); + const dbResult = await this.db.findEligibleWorkspacesForSoftDeletion(new Date(this.timeAfter), 10); + expect(dbResult.length).to.eq(0); + } + + @test(timeout(10000)) + public async testFindEligibleWorkspacesForSoftDeletion_notMarkedEligibleRunningInstance() { + this.ws.deletionEligibilityTime = this.timeWs; await Promise.all([this.db.store(this.ws), this.db.storeInstance(this.wsi1), this.db.storeInstance(this.wsi2)]); const dbResult = await this.db.findEligibleWorkspacesForSoftDeletion(new Date(this.timeAfter), 10); expect(dbResult.length).to.eq(0); From 13e1a76ae9f821d8d29bf84d43e501f11a906daf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Filip=20Tron=C3=AD=C4=8Dek?= Date: Mon, 7 Oct 2024 14:17:58 +0000 Subject: [PATCH 08/13] No more async filter predicates --- .../src/typeorm/workspace-db-impl.ts | 36 +++++++++++-------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/components/gitpod-db/src/typeorm/workspace-db-impl.ts b/components/gitpod-db/src/typeorm/workspace-db-impl.ts index 5780b039b10e2c..4522ebc1616379 100644 --- a/components/gitpod-db/src/typeorm/workspace-db-impl.ts +++ b/components/gitpod-db/src/typeorm/workspace-db-impl.ts @@ -488,14 +488,25 @@ export class TypeORMWorkspaceDBImpl extends TransactionalDBImpl imp limit: number = 100, type: WorkspaceType = "regular", ): Promise { - // we do not allow to run this with a future date if (cutOffDate > new Date()) { throw new Error("cutOffDate must not be in the future, was: " + cutOffDate.toISOString()); } const workspaceRepo = await this.getWorkspaceRepo(); const qb = workspaceRepo .createQueryBuilder("ws") - .select(["ws.id", "ws.ownerId", "ws.deletionEligibilityTime"]) + .leftJoinAndMapOne( + "ws.latestInstance", + DBWorkspaceInstance, + "wsi", + `wsi.id = ( + SELECT i.id + FROM d_b_workspace_instance AS i + WHERE i.workspaceId = ws.id + ORDER BY i.creationTime DESC + LIMIT 1 + )`, + ) + .select(["ws.id", "ws.ownerId", "ws.deletionEligibilityTime", "wsi.id", "wsi.status"]) .where("ws.deleted = :deleted", { deleted: 0 }) .andWhere("ws.type = :type", { type }) .andWhere("ws.softDeleted IS NULL") @@ -503,20 +514,17 @@ export class TypeORMWorkspaceDBImpl extends TransactionalDBImpl imp .andWhere("ws.pinned = :pinned", { pinned: 0 }) .andWhere("ws.deletionEligibilityTime != ''") .andWhere("ws.deletionEligibilityTime < :cutOffDate", { cutOffDate: cutOffDate.toISOString() }) + // we don't want to delete workspaces that are active + .andWhere( + new Brackets((qb) => { + qb.where("wsi.id IS NULL").orWhere("JSON_UNQUOTE(wsi.status->>'$.phase') = :stoppedStatus", { + stoppedStatus: "stopped", + }); + }), + ) .limit(limit); - const results: WorkspaceOwnerAndDeletionEligibility[] = await qb.getMany(); - - // hack to have an async filter predicate - const filtered = await Promise.all( - results.map(async (ws) => { - // we don't want to delete workspaces that have a running instance - const latestInstance = await this.findRunningInstance(ws.id); - return { ws, keep: latestInstance === undefined }; - }), - ); - - return filtered.filter((result) => result.keep).map((result) => result.ws); + return qb.getMany(); } public async findWorkspacesForPurging( From 39065c79c7ef3253fed5b16656208cc279efe7a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Filip=20Tron=C3=AD=C4=8Dek?= Date: Mon, 7 Oct 2024 15:41:43 +0000 Subject: [PATCH 09/13] More prevention logging --- .../server/src/workspace/workspace-service.ts | 22 +++++++++++++------ 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/components/server/src/workspace/workspace-service.ts b/components/server/src/workspace/workspace-service.ts index ae66c2fc56d688..5cd35414d303c7 100644 --- a/components/server/src/workspace/workspace-service.ts +++ b/components/server/src/workspace/workspace-service.ts @@ -553,17 +553,25 @@ export class WorkspaceService { return; } // workspaces with pending changes live twice as long - if ( - (instance?.gitStatus?.totalUncommitedFiles || 0) > 0 || - (instance?.gitStatus?.totalUnpushedCommits || 0) > 0 || - (instance?.gitStatus?.totalUntrackedFiles || 0) > 0 - ) { + const hasGitChanges = + instance?.gitStatus?.totalUncommitedFiles || + 0 > 0 || + instance?.gitStatus?.totalUnpushedCommits || + 0 > 0 || + instance?.gitStatus?.totalUntrackedFiles || + 0 > 0; + if (hasGitChanges) { daysToLive = daysToLive * 2; } deletionEligibilityTime.setDate(deletionEligibilityTime.getDate() + daysToLive); - if (new Date() > deletionEligibilityTime) { - log.warn({ userId, workspaceId }, "Prevented setting deletion eligibility time in the past", { + if ( + workspace.deletionEligibilityTime && + workspace.deletionEligibilityTime > deletionEligibilityTime.toISOString() + ) { + log.warn({ userId, workspaceId }, "Prevented moving deletion eligibility time backwards", { deletionEligibilityTime: deletionEligibilityTime.toISOString(), + hasGitChanges, + lastActive, }); return; } From 00af0fe5f6608ee47d08f69917f0990848ff28a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Filip=20Tron=C3=AD=C4=8Dek?= Date: Tue, 8 Oct 2024 09:49:39 +0000 Subject: [PATCH 10/13] Log all timestamps and don't update `lastActive` when `activeNow === true` --- .../server/src/workspace/workspace-service.ts | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/components/server/src/workspace/workspace-service.ts b/components/server/src/workspace/workspace-service.ts index 5cd35414d303c7..28e33baa9449f6 100644 --- a/components/server/src/workspace/workspace-service.ts +++ b/components/server/src/workspace/workspace-service.ts @@ -535,15 +535,12 @@ export class WorkspaceService { const workspace = await this.doGetWorkspace(userId, workspaceId); const instance = await this.db.findCurrentInstance(workspaceId); - let lastActive = + const lastActive = instance?.stoppingTime || instance?.startedTime || instance?.creationTime || workspace?.creationTime; - if (activeNow) { - lastActive = new Date().toISOString(); - } - if (!lastActive) { + if (!lastActive && !activeNow) { return; } - const deletionEligibilityTime = new Date(lastActive); + const deletionEligibilityTime = activeNow ? new Date() : new Date(lastActive); if (workspace.type === "prebuild") { // set to last active plus daysToLiveForPrebuilds as iso string deletionEligibilityTime.setDate(deletionEligibilityTime.getDate() + daysToLiveForPrebuilds); @@ -570,8 +567,14 @@ export class WorkspaceService { ) { log.warn({ userId, workspaceId }, "Prevented moving deletion eligibility time backwards", { deletionEligibilityTime: deletionEligibilityTime.toISOString(), - hasGitChanges, lastActive, + hasGitChanges, + timestamps: { + instanceStoppingTime: instance?.stoppingTime, + instanceStartedTime: instance?.startedTime, + instanceCreationTime: instance?.creationTime, + workspaceCreationtime: workspace.creationTime, + }, }); return; } From bb26298d6215539216604e4869e01de68dd7ed08 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Filip=20Tron=C3=AD=C4=8Dek?= Date: Wed, 9 Oct 2024 11:41:26 +0000 Subject: [PATCH 11/13] even cooler timestamps --- components/server/src/workspace/workspace-service.ts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/components/server/src/workspace/workspace-service.ts b/components/server/src/workspace/workspace-service.ts index 28e33baa9449f6..ac77c790b282c5 100644 --- a/components/server/src/workspace/workspace-service.ts +++ b/components/server/src/workspace/workspace-service.ts @@ -566,15 +566,16 @@ export class WorkspaceService { workspace.deletionEligibilityTime > deletionEligibilityTime.toISOString() ) { log.warn({ userId, workspaceId }, "Prevented moving deletion eligibility time backwards", { - deletionEligibilityTime: deletionEligibilityTime.toISOString(), - lastActive, hasGitChanges, - timestamps: { + timestamps: new TrustedValue({ + wouldBeDeletionEligibilityTime: deletionEligibilityTime.toISOString(), + currentDeletionEligibilityTime: workspace.deletionEligibilityTime, instanceStoppingTime: instance?.stoppingTime, instanceStartedTime: instance?.startedTime, instanceCreationTime: instance?.creationTime, - workspaceCreationtime: workspace.creationTime, - }, + workspaceCreationTime: workspace.creationTime, + lastActive, + }), }); return; } From 743ae10b969c33b893e6733a45b56b004410d816 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Filip=20Tron=C3=AD=C4=8Dek?= Date: Mon, 14 Oct 2024 15:47:22 +0000 Subject: [PATCH 12/13] Add instance id to log context --- .../server/src/workspace/workspace-service.ts | 28 +++++++++++-------- 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/components/server/src/workspace/workspace-service.ts b/components/server/src/workspace/workspace-service.ts index ac77c790b282c5..defd384e1f3c33 100644 --- a/components/server/src/workspace/workspace-service.ts +++ b/components/server/src/workspace/workspace-service.ts @@ -565,18 +565,22 @@ export class WorkspaceService { workspace.deletionEligibilityTime && workspace.deletionEligibilityTime > deletionEligibilityTime.toISOString() ) { - log.warn({ userId, workspaceId }, "Prevented moving deletion eligibility time backwards", { - hasGitChanges, - timestamps: new TrustedValue({ - wouldBeDeletionEligibilityTime: deletionEligibilityTime.toISOString(), - currentDeletionEligibilityTime: workspace.deletionEligibilityTime, - instanceStoppingTime: instance?.stoppingTime, - instanceStartedTime: instance?.startedTime, - instanceCreationTime: instance?.creationTime, - workspaceCreationTime: workspace.creationTime, - lastActive, - }), - }); + log.warn( + { userId, workspaceId, instanceId: instance?.id }, + "Prevented moving deletion eligibility time backwards", + { + hasGitChanges, + timestamps: new TrustedValue({ + wouldBeDeletionEligibilityTime: deletionEligibilityTime.toISOString(), + currentDeletionEligibilityTime: workspace.deletionEligibilityTime, + instanceStoppingTime: instance?.stoppingTime, + instanceStartedTime: instance?.startedTime, + instanceCreationTime: instance?.creationTime, + workspaceCreationTime: workspace.creationTime, + lastActive, + }), + }, + ); return; } await this.db.updatePartial(workspaceId, { From f2865661dbae4d219aedcfdbcb7fa9e1e4a685ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Filip=20Tron=C3=AD=C4=8Dek?= Date: Mon, 14 Oct 2024 16:00:09 +0000 Subject: [PATCH 13/13] Remove filtering for only non-running workspaces --- .../src/typeorm/workspace-db-impl.ts | 51 +++++++------------ .../gitpod-db/src/workspace-db.spec.db.ts | 8 --- 2 files changed, 19 insertions(+), 40 deletions(-) diff --git a/components/gitpod-db/src/typeorm/workspace-db-impl.ts b/components/gitpod-db/src/typeorm/workspace-db-impl.ts index 4522ebc1616379..46bcfa1de9c072 100644 --- a/components/gitpod-db/src/typeorm/workspace-db-impl.ts +++ b/components/gitpod-db/src/typeorm/workspace-db-impl.ts @@ -36,6 +36,7 @@ import { PrebuildWithWorkspace, PrebuildWithWorkspaceAndInstances, PrebuiltUpdatableAndWorkspace, + WorkspaceAndOwner, WorkspaceDB, WorkspaceOwnerAndContentDeletedTime, WorkspaceOwnerAndDeletionEligibility, @@ -492,39 +493,25 @@ export class TypeORMWorkspaceDBImpl extends TransactionalDBImpl imp throw new Error("cutOffDate must not be in the future, was: " + cutOffDate.toISOString()); } const workspaceRepo = await this.getWorkspaceRepo(); - const qb = workspaceRepo - .createQueryBuilder("ws") - .leftJoinAndMapOne( - "ws.latestInstance", - DBWorkspaceInstance, - "wsi", - `wsi.id = ( - SELECT i.id - FROM d_b_workspace_instance AS i - WHERE i.workspaceId = ws.id - ORDER BY i.creationTime DESC - LIMIT 1 - )`, - ) - .select(["ws.id", "ws.ownerId", "ws.deletionEligibilityTime", "wsi.id", "wsi.status"]) - .where("ws.deleted = :deleted", { deleted: 0 }) - .andWhere("ws.type = :type", { type }) - .andWhere("ws.softDeleted IS NULL") - .andWhere("ws.softDeletedTime = ''") - .andWhere("ws.pinned = :pinned", { pinned: 0 }) - .andWhere("ws.deletionEligibilityTime != ''") - .andWhere("ws.deletionEligibilityTime < :cutOffDate", { cutOffDate: cutOffDate.toISOString() }) - // we don't want to delete workspaces that are active - .andWhere( - new Brackets((qb) => { - qb.where("wsi.id IS NULL").orWhere("JSON_UNQUOTE(wsi.status->>'$.phase') = :stoppedStatus", { - stoppedStatus: "stopped", - }); - }), - ) - .limit(limit); + const dbResults = await workspaceRepo.query( + ` + SELECT ws.id AS id, + ws.ownerId AS ownerId, + ws.deletionEligibilityTime AS deletionEligibilityTime + FROM d_b_workspace AS ws + WHERE ws.deleted = 0 + AND ws.type = ? + AND ws.softDeleted IS NULL + AND ws.softDeletedTime = '' + AND ws.pinned = 0 + AND ws.deletionEligibilityTime != '' + AND ws.deletionEligibilityTime < ? + LIMIT ?; + `, + [type, cutOffDate.toISOString(), limit], + ); - return qb.getMany(); + return dbResults as WorkspaceAndOwner[]; } public async findWorkspacesForPurging( diff --git a/components/gitpod-db/src/workspace-db.spec.db.ts b/components/gitpod-db/src/workspace-db.spec.db.ts index e7b069494cda44..eee3e1063960be 100644 --- a/components/gitpod-db/src/workspace-db.spec.db.ts +++ b/components/gitpod-db/src/workspace-db.spec.db.ts @@ -361,14 +361,6 @@ class WorkspaceDBSpec { expect(dbResult.length).to.eq(0); } - @test(timeout(10000)) - public async testFindEligibleWorkspacesForSoftDeletion_notMarkedEligibleRunningInstance() { - this.ws.deletionEligibilityTime = this.timeWs; - await Promise.all([this.db.store(this.ws), this.db.storeInstance(this.wsi1), this.db.storeInstance(this.wsi2)]); - const dbResult = await this.db.findEligibleWorkspacesForSoftDeletion(new Date(this.timeAfter), 10); - expect(dbResult.length).to.eq(0); - } - @test(timeout(10000)) public async testFindAllWorkspaceAndInstances_workspaceId() { await Promise.all([