-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[bridge] Introduce AppClusterWorkspaceInstanceController #13831
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,86 @@ | ||
/** | ||
* Copyright (c) 2022 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 { WorkspaceDB } from "@gitpod/gitpod-db/lib/workspace-db"; | ||
import { Disposable, DisposableCollection } from "@gitpod/gitpod-protocol"; | ||
import { log } from "@gitpod/gitpod-protocol/lib/util/logging"; | ||
import { repeat } from "@gitpod/gitpod-protocol/lib/util/repeat"; | ||
import { TraceContext } from "@gitpod/gitpod-protocol/lib/util/tracing"; | ||
import { inject, injectable } from "inversify"; | ||
import { Configuration } from "./config"; | ||
import { WorkspaceInstanceController } from "./workspace-instance-controller"; | ||
|
||
/** | ||
* The WorkspaceInstance lifecycle is split between application clusters and workspace clusters on the transition from | ||
* pending/building -> starting (cmp. WorkspacePhases here: | ||
* https://github.com/gitpod-io/gitpod/blob/008ea3fadc89d4817cf3effc8a5b30eaf469fb1c/components/gitpod-protocol/src/workspace-instance.ts#L111). | ||
* | ||
* Before the transition, WorkspaceInstances belong to the respective app cluster, denoted by "instance.region === 'eu02'", for exmaple. | ||
* After a WorkspaceInstance has been moved over to a workspace cluster, that moved "ownership" is reflected in said field. | ||
* We maintain a constant connection (called "bridge") to all workspace clusters to be able to keep reality (workspace | ||
* side) in sync with what we have in our DB/forward to clients. | ||
* | ||
* This class is meant to take the same responsibility for all WorkspaceInstances that have not (yet) been passed over | ||
* to a workspace cluster for whatever reason. Here's a list of examples, prefixed by phase: | ||
* - "preparing": failed cleanup after failed call to wsManager.StartWorkspace | ||
* - "building": failed cleanup after failed image-build (which is still controlled by the application cluster, | ||
* although that might change in the future) | ||
*/ | ||
@injectable() | ||
export class AppClusterWorkspaceInstancesController implements Disposable { | ||
@inject(Configuration) protected readonly config: Configuration; | ||
|
||
@inject(WorkspaceDB) protected readonly workspaceDb: WorkspaceDB; | ||
|
||
@inject(WorkspaceInstanceController) protected readonly workspaceInstanceController: WorkspaceInstanceController; | ||
|
||
protected readonly dispoables = new DisposableCollection(); | ||
|
||
public async start() { | ||
const disposable = repeat( | ||
async () => this.controlAppClusterManagedWorkspaceInstances(), | ||
this.config.controllerIntervalSeconds * 1000, | ||
); | ||
this.dispoables.push(disposable); | ||
} | ||
|
||
protected async controlAppClusterManagedWorkspaceInstances() { | ||
const appClusterInstallation = this.config.installation; | ||
|
||
const span = TraceContext.startSpan("controlAppClusterManagedWorkspaceInstances"); | ||
const ctx = { span }; | ||
try { | ||
log.info("Controlling app cluster instances", { installation: appClusterInstallation }); | ||
|
||
const notStoppedInstances = await this.workspaceDb.findRunningInstancesWithWorkspaces( | ||
appClusterInstallation, | ||
undefined, | ||
false, | ||
); | ||
await this.workspaceInstanceController.controlNotStoppedAppClusterManagedInstanceTimeouts( | ||
ctx, | ||
notStoppedInstances, | ||
appClusterInstallation, | ||
); | ||
|
||
log.info("Done controlling app cluster instances", { | ||
installation: appClusterInstallation, | ||
instancesCount: notStoppedInstances.length, | ||
}); | ||
Comment on lines
+56
to
+72
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice. This would be the perfect place to put any metrics which report duration & outcome. Shame we don't have metrics configured for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could "cheat" and attach the duration of the run (and any outcome) to the log fields. |
||
} catch (err) { | ||
log.error("Error controlling app cluster instances", err, { | ||
installation: appClusterInstallation, | ||
}); | ||
TraceContext.setError(ctx, err); | ||
} finally { | ||
span.finish(); | ||
} | ||
} | ||
|
||
public dispose() { | ||
this.dispoables.dispose(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would we eventually move this instance onto this controller also? Seems we've got layers here which may not be adding immediate value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we could! It's basically and artifact of splitting the PRs, and not adjusting after the rebase 😆