Skip to content

Commit 33942e7

Browse files
filiptronicekfiliptronicek
authored
[ws-gc] WS soft deletion improvements (#20271)
* [ws-gc] Additional logging * typo fix * test update * Workspace is active now if it just stopped, started or just got created * Don't ever GC currently running workspaces * Fix tests * Fix tests * No more async filter predicates * More prevention logging * Log all timestamps and don't update `lastActive` when `activeNow === true` * even cooler timestamps * Add instance id to log context * Remove filtering for only non-running workspaces
1 parent 537d9cf commit 33942e7

File tree

8 files changed

+119
-45
lines changed

8 files changed

+119
-45
lines changed

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

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,8 @@ import {
3838
PrebuiltUpdatableAndWorkspace,
3939
WorkspaceAndOwner,
4040
WorkspaceDB,
41+
WorkspaceOwnerAndContentDeletedTime,
42+
WorkspaceOwnerAndDeletionEligibility,
4143
WorkspaceOwnerAndSoftDeleted,
4244
WorkspacePortsAuthData,
4345
} from "../workspace-db";
@@ -486,16 +488,16 @@ export class TypeORMWorkspaceDBImpl extends TransactionalDBImpl<WorkspaceDB> imp
486488
cutOffDate: Date = new Date(),
487489
limit: number = 100,
488490
type: WorkspaceType = "regular",
489-
): Promise<WorkspaceAndOwner[]> {
490-
// we do not allow to run this with a future date
491+
): Promise<WorkspaceOwnerAndDeletionEligibility[]> {
491492
if (cutOffDate > new Date()) {
492493
throw new Error("cutOffDate must not be in the future, was: " + cutOffDate.toISOString());
493494
}
494495
const workspaceRepo = await this.getWorkspaceRepo();
495496
const dbResults = await workspaceRepo.query(
496497
`
497498
SELECT ws.id AS id,
498-
ws.ownerId AS ownerId
499+
ws.ownerId AS ownerId,
500+
ws.deletionEligibilityTime AS deletionEligibilityTime
499501
FROM d_b_workspace AS ws
500502
WHERE ws.deleted = 0
501503
AND ws.type = ?
@@ -516,12 +518,12 @@ export class TypeORMWorkspaceDBImpl extends TransactionalDBImpl<WorkspaceDB> imp
516518
minContentDeletionTimeInDays: number,
517519
limit: number,
518520
now: Date,
519-
): Promise<WorkspaceAndOwner[]> {
521+
): Promise<WorkspaceOwnerAndContentDeletedTime[]> {
520522
const minPurgeTime = daysBefore(now.toISOString(), minContentDeletionTimeInDays);
521523
const repo = await this.getWorkspaceRepo();
522524
const qb = repo
523525
.createQueryBuilder("ws")
524-
.select(["ws.id", "ws.ownerId"])
526+
.select(["ws.id", "ws.ownerId", "ws.contentDeletedTime"])
525527
.where(`ws.contentDeletedTime != ''`)
526528
.andWhere(`ws.contentDeletedTime < :minPurgeTime`, { minPurgeTime })
527529
.andWhere(`ws.deleted = 0`)

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

Lines changed: 43 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ class WorkspaceDBSpec {
2626
readonly timeWs = new Date(2018, 2, 16, 10, 0, 0).toISOString();
2727
readonly timeBefore = new Date(2018, 2, 16, 11, 5, 10).toISOString();
2828
readonly timeAfter = new Date(2019, 2, 16, 11, 5, 10).toISOString();
29+
readonly timeAfter2 = new Date(2019, 2, 17, 4, 20, 10).toISOString();
2930
readonly userId = "12345";
3031
readonly projectAID = "projectA";
3132
readonly projectBID = "projectB";
@@ -101,6 +102,30 @@ class WorkspaceDBSpec {
101102
deleted: false,
102103
usageAttributionId: undefined,
103104
};
105+
readonly wsi3: WorkspaceInstance = {
106+
workspaceId: this.ws.id,
107+
id: "12345",
108+
ideUrl: "example.org",
109+
region: "unknown",
110+
workspaceClass: undefined,
111+
workspaceImage: "abc.io/test/image:123",
112+
creationTime: this.timeAfter2,
113+
startedTime: undefined,
114+
deployedTime: undefined,
115+
stoppingTime: undefined,
116+
stoppedTime: undefined,
117+
status: {
118+
version: 1,
119+
phase: "stopped",
120+
conditions: {},
121+
},
122+
configuration: {
123+
theiaVersion: "unknown",
124+
ideImage: "unknown",
125+
},
126+
deleted: false,
127+
usageAttributionId: undefined,
128+
};
104129
readonly ws2: Workspace = {
105130
id: "2",
106131
type: "regular",
@@ -235,7 +260,7 @@ class WorkspaceDBSpec {
235260
}
236261

237262
@test(timeout(10000))
238-
public async testFindEligableWorkspacesForSoftDeletion_markedEligable_Prebuild() {
263+
public async testFindEligibleWorkspacesForSoftDeletion_markedEligible_Prebuild() {
239264
const { ws } = await this.createPrebuild(20, 15);
240265
const dbResult = await this.db.findEligibleWorkspacesForSoftDeletion(new Date(), 10, "prebuild");
241266
expect(dbResult.length).to.equal(1);
@@ -244,7 +269,7 @@ class WorkspaceDBSpec {
244269
}
245270

246271
@test(timeout(10000))
247-
public async testFindEligableWorkspacesForSoftDeletion_notMarkedEligable_Prebuild() {
272+
public async testFindEligibleWorkspacesForSoftDeletion_notMarkedEligible_Prebuild() {
248273
await this.createPrebuild(20, -7);
249274
const dbResult = await this.db.findEligibleWorkspacesForSoftDeletion(new Date(), 10, "prebuild");
250275
expect(dbResult.length).to.eq(0);
@@ -254,7 +279,7 @@ class WorkspaceDBSpec {
254279
public async testPrebuildGarbageCollection() {
255280
const { pbws } = await this.createPrebuild(20, 15);
256281

257-
// mimick the behavior of the Garbage Collector
282+
// mimic the behavior of the Garbage Collector
258283
const gcWorkspaces = await this.db.findEligibleWorkspacesForSoftDeletion(new Date(), 10, "prebuild");
259284
expect(gcWorkspaces.length).to.equal(1);
260285

@@ -311,17 +336,27 @@ class WorkspaceDBSpec {
311336
}
312337

313338
@test(timeout(10000))
314-
public async testFindEligableWorkspacesForSoftDeletion_markedEligable() {
339+
public async testFindEligibleWorkspacesForSoftDeletion_markedEligible() {
315340
this.ws.deletionEligibilityTime = this.timeWs;
316-
await Promise.all([this.db.store(this.ws), this.db.storeInstance(this.wsi1), this.db.storeInstance(this.wsi2)]);
341+
await Promise.all([
342+
this.db.store(this.ws),
343+
this.db.storeInstance(this.wsi1),
344+
this.db.storeInstance(this.wsi2),
345+
this.db.storeInstance(this.wsi3),
346+
]);
317347
const dbResult = await this.db.findEligibleWorkspacesForSoftDeletion(new Date(this.timeAfter), 10);
318348
expect(dbResult[0].id).to.eq(this.ws.id);
319349
expect(dbResult[0].ownerId).to.eq(this.ws.ownerId);
320350
}
321351

322352
@test(timeout(10000))
323-
public async testFindEligableWorkspacesForSoftDeletion_notMarkedEligable() {
324-
await Promise.all([this.db.store(this.ws), this.db.storeInstance(this.wsi1), this.db.storeInstance(this.wsi2)]);
353+
public async testFindEligibleWorkspacesForSoftDeletion_notMarkedEligible() {
354+
await Promise.all([
355+
this.db.store(this.ws),
356+
this.db.storeInstance(this.wsi1),
357+
this.db.storeInstance(this.wsi2),
358+
this.db.storeInstance(this.wsi3),
359+
]);
325360
const dbResult = await this.db.findEligibleWorkspacesForSoftDeletion(new Date(this.timeAfter), 10);
326361
expect(dbResult.length).to.eq(0);
327362
}
@@ -768,6 +803,7 @@ class WorkspaceDBSpec {
768803
{
769804
id: "1",
770805
ownerId,
806+
contentDeletedTime: d20180131,
771807
},
772808
]);
773809
}

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,8 @@ export interface PrebuildWithWorkspaceAndInstances {
6464

6565
export type WorkspaceAndOwner = Pick<Workspace, "id" | "ownerId">;
6666
export type WorkspaceOwnerAndSoftDeleted = Pick<Workspace, "id" | "ownerId" | "softDeleted">;
67+
export type WorkspaceOwnerAndDeletionEligibility = Pick<Workspace, "id" | "ownerId" | "deletionEligibilityTime">;
68+
export type WorkspaceOwnerAndContentDeletedTime = Pick<Workspace, "id" | "ownerId" | "contentDeletedTime">;
6769

6870
export const WorkspaceDB = Symbol("WorkspaceDB");
6971
export interface WorkspaceDB {
@@ -102,7 +104,7 @@ export interface WorkspaceDB {
102104
cutOffDate?: Date,
103105
limit?: number,
104106
type?: WorkspaceType,
105-
): Promise<WorkspaceAndOwner[]>;
107+
): Promise<WorkspaceOwnerAndDeletionEligibility[]>;
106108
findWorkspacesForContentDeletion(
107109
minSoftDeletedTimeInDays: number,
108110
limit: number,
@@ -111,7 +113,7 @@ export interface WorkspaceDB {
111113
minContentDeletionTimeInDays: number,
112114
limit: number,
113115
now: Date,
114-
): Promise<WorkspaceAndOwner[]>;
116+
): Promise<WorkspaceOwnerAndContentDeletedTime[]>;
115117
findAllWorkspaces(
116118
offset: number,
117119
limit: number,

‎components/server/src/config.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -84,10 +84,10 @@ export interface WorkspaceGarbageCollection {
8484
/** The minimal age of a workspace before it is marked as 'softDeleted' (= hidden for the user) */
8585
minAgeDays: number;
8686

87-
/** The minimal age of a prebuild (incl. workspace) before it's content is deleted (+ marked as 'softDeleted') */
87+
/** The minimal age of a prebuild (incl. workspace) before its content is deleted (+ marked as 'softDeleted') */
8888
minAgePrebuildDays: number;
8989

90-
/** The minimal number of days a workspace has to stay in 'softDeleted' before it's content is deleted */
90+
/** The minimal number of days a workspace has to stay in 'softDeleted' before its content is deleted */
9191
contentRetentionPeriodDays: number;
9292

9393
/** The maximum amount of workspaces whose content is deleted in one go */

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

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ import { StorageClient } from "../storage/storage-client";
2929
* - find _any_ workspace "softDeleted" for long enough -> move to "contentDeleted"
3030
* - find _any_ workspace "contentDeleted" for long enough -> move to "purged"
3131
* - prebuilds are special in that:
32-
* - the GC has a dedicated sub-task to move workspace of type "prebuid" from "to delete" (with a different threshold) -> to "contentDeleted" directly
32+
* - the GC has a dedicated sub-task to move workspace of type "prebuild" from "to delete" (with a different threshold) -> to "contentDeleted" directly
3333
* - the "purging" takes care of all Prebuild-related sub-resources, too
3434
*/
3535
@injectable()
@@ -55,6 +55,11 @@ export class WorkspaceGarbageCollector implements Job {
5555
return;
5656
}
5757

58+
log.info("workspace-gc: job started", {
59+
workspaceMinAgeDays: this.config.workspaceGarbageCollection.minAgeDays,
60+
prebuildMinAgeDays: this.config.workspaceGarbageCollection.minAgePrebuildDays,
61+
});
62+
5863
// Move eligible "regular" workspace -> softDeleted
5964
try {
6065
await this.softDeleteEligibleWorkspaces();
@@ -115,6 +120,10 @@ export class WorkspaceGarbageCollector implements Job {
115120
err,
116121
);
117122
}
123+
124+
log.info({ workspaceId: ws.id }, `workspace-gc: soft deleted a workspace`, {
125+
deletionEligibilityTime: ws.deletionEligibilityTime,
126+
});
118127
}
119128
const afterDelete = new Date();
120129

@@ -187,6 +196,9 @@ export class WorkspaceGarbageCollector implements Job {
187196
} catch (err) {
188197
log.error({ workspaceId: ws.id }, "workspace-gc: failed to purge workspace", err);
189198
}
199+
log.info({ workspaceId: ws.id }, `workspace-gc: hard deleted a workspace`, {
200+
contentDeletedTime: ws.contentDeletedTime,
201+
});
190202
}
191203
const afterDelete = new Date();
192204

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -700,7 +700,7 @@ describe("WorkspaceService", async () => {
700700
workspaceImage: "",
701701
});
702702

703-
await svc.updateDeletionEligabilityTime(owner.id, ws.id);
703+
await svc.updateDeletionEligibilityTime(owner.id, ws.id);
704704

705705
const workspace = await svc.getWorkspace(owner.id, ws.id);
706706
expect(workspace).to.not.be.undefined;
@@ -734,7 +734,7 @@ describe("WorkspaceService", async () => {
734734
workspaceImage: "",
735735
});
736736

737-
await svc.updateDeletionEligabilityTime(owner.id, ws.id);
737+
await svc.updateDeletionEligibilityTime(owner.id, ws.id);
738738

739739
const workspace = await svc.getWorkspace(owner.id, ws.id);
740740
expect(workspace).to.not.be.undefined;
@@ -770,7 +770,7 @@ describe("WorkspaceService", async () => {
770770
workspaceImage: "",
771771
});
772772

773-
await svc.updateDeletionEligabilityTime(owner.id, ws.id);
773+
await svc.updateDeletionEligibilityTime(owner.id, ws.id);
774774

775775
const workspace = await svc.getWorkspace(owner.id, ws.id);
776776
expect(workspace).to.not.be.undefined;

0 commit comments

Comments
 (0)