From 59ac05cf65198a1de26c4d704514759ed137c257 Mon Sep 17 00:00:00 2001 From: Aaron Lehmann Date: Mon, 18 Nov 2024 14:13:42 -0800 Subject: [PATCH 1/6] Start workspaces by shelling out to CLI Replace the REST-API-based start flow with one that shells out to the coder CLI. Signed-off-by: Aaron Lehmann --- src/api.ts | 58 ++++++++++++++++++++++++++++++++++++++------------- src/remote.ts | 55 +++++++++++++++++++++++++++++------------------- 2 files changed, 76 insertions(+), 37 deletions(-) diff --git a/src/api.ts b/src/api.ts index fafeaf56..2e574101 100644 --- a/src/api.ts +++ b/src/api.ts @@ -1,3 +1,4 @@ +import { spawn, ChildProcessWithoutNullStreams } from "child_process" import { Api } from "coder/site/src/api/api" import { ProvisionerJobLog, Workspace } from "coder/site/src/api/typesGenerated" import fs from "fs/promises" @@ -122,16 +123,12 @@ export async function makeCoderSdk(baseUrl: string, token: string | undefined, s /** * Start or update a workspace and return the updated workspace. */ -export async function startWorkspaceIfStoppedOrFailed(restClient: Api, workspace: Workspace): Promise { - // If the workspace requires the latest active template version, we should attempt - // to update that here. - // TODO: If param set changes, what do we do?? - const versionID = workspace.template_require_active_version - ? // Use the latest template version - workspace.template_active_version_id - : // Default to not updating the workspace if not required. - workspace.latest_build.template_version_id - +export async function startWorkspaceIfStoppedOrFailed( + restClient: Api, + binPath: string, + workspace: Workspace, + writeEmitter: vscode.EventEmitter, +): Promise { // Before we start a workspace, we make an initial request to check it's not already started const updatedWorkspace = await restClient.getWorkspace(workspace.id) @@ -139,12 +136,43 @@ export async function startWorkspaceIfStoppedOrFailed(restClient: Api, workspace return updatedWorkspace } - const latestBuild = await restClient.startWorkspace(updatedWorkspace.id, versionID) + return new Promise((resolve, reject) => { + const startProcess: ChildProcessWithoutNullStreams = spawn(binPath, [ + "start", + "--yes", + workspace.owner_name + "/" + workspace.name, + ]) + + startProcess.stdout.on("data", (data: Buffer) => { + data + .toString() + .split(/\r*\n/) + .forEach((line: string) => { + if (line !== "") { + writeEmitter.fire(line.toString() + "\r\n") + } + }) + }) + + startProcess.stderr.on("data", (data: Buffer) => { + data + .toString() + .split(/\r*\n/) + .forEach((line: string) => { + if (line !== "") { + writeEmitter.fire(line.toString() + "\r\n") + } + }) + }) - return { - ...updatedWorkspace, - latest_build: latestBuild, - } + startProcess.on("close", (code: number) => { + if (code === 0) { + resolve(restClient.getWorkspace(workspace.id)) + } else { + reject(new Error(`"coder start" process exited with code ${code}`)) + } + }) + }) } /** diff --git a/src/remote.ts b/src/remote.ts index cd0c3918..1977dd85 100644 --- a/src/remote.ts +++ b/src/remote.ts @@ -50,7 +50,11 @@ export class Remote { /** * Try to get the workspace running. Return undefined if the user canceled. */ - private async maybeWaitForRunning(restClient: Api, workspace: Workspace): Promise { + private async maybeWaitForRunning( + restClient: Api, + workspace: Workspace, + binPath: string, + ): Promise { // Maybe already running? if (workspace.latest_build.status === "running") { return workspace @@ -63,6 +67,28 @@ export class Remote { let terminal: undefined | vscode.Terminal let attempts = 0 + function initWriteEmitterAndTerminal(): vscode.EventEmitter { + if (!writeEmitter) { + writeEmitter = new vscode.EventEmitter() + } + if (!terminal) { + terminal = vscode.window.createTerminal({ + name: "Build Log", + location: vscode.TerminalLocation.Panel, + // Spin makes this gear icon spin! + iconPath: new vscode.ThemeIcon("gear~spin"), + pty: { + onDidWrite: writeEmitter.event, + close: () => undefined, + open: () => undefined, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + } as Partial as any, + }) + terminal.show(true) + } + return writeEmitter + } + try { // Show a notification while we wait. return await this.vscodeProposed.window.withProgress( @@ -78,24 +104,7 @@ export class Remote { case "pending": case "starting": case "stopping": - if (!writeEmitter) { - writeEmitter = new vscode.EventEmitter() - } - if (!terminal) { - terminal = vscode.window.createTerminal({ - name: "Build Log", - location: vscode.TerminalLocation.Panel, - // Spin makes this gear icon spin! - iconPath: new vscode.ThemeIcon("gear~spin"), - pty: { - onDidWrite: writeEmitter.event, - close: () => undefined, - open: () => undefined, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as Partial as any, - }) - terminal.show(true) - } + writeEmitter = initWriteEmitterAndTerminal() this.storage.writeToCoderOutputChannel(`Waiting for ${workspaceName}...`) workspace = await waitForBuild(restClient, writeEmitter, workspace) break @@ -103,8 +112,9 @@ export class Remote { if (!(await this.confirmStart(workspaceName))) { return undefined } + writeEmitter = initWriteEmitterAndTerminal() this.storage.writeToCoderOutputChannel(`Starting ${workspaceName}...`) - workspace = await startWorkspaceIfStoppedOrFailed(restClient, workspace) + workspace = await startWorkspaceIfStoppedOrFailed(restClient, binPath, workspace, writeEmitter) break case "failed": // On a first attempt, we will try starting a failed workspace @@ -113,8 +123,9 @@ export class Remote { if (!(await this.confirmStart(workspaceName))) { return undefined } + writeEmitter = initWriteEmitterAndTerminal() this.storage.writeToCoderOutputChannel(`Starting ${workspaceName}...`) - workspace = await startWorkspaceIfStoppedOrFailed(restClient, workspace) + workspace = await startWorkspaceIfStoppedOrFailed(restClient, binPath, workspace, writeEmitter) break } // Otherwise fall through and error. @@ -292,7 +303,7 @@ export class Remote { disposables.push(this.registerLabelFormatter(remoteAuthority, workspace.owner_name, workspace.name)) // If the workspace is not in a running state, try to get it running. - const updatedWorkspace = await this.maybeWaitForRunning(workspaceRestClient, workspace) + const updatedWorkspace = await this.maybeWaitForRunning(workspaceRestClient, workspace, binaryPath) if (!updatedWorkspace) { // User declined to start the workspace. await this.closeRemote() From cdccb52100f1d055a96c9047c2cb0baddd8f19f9 Mon Sep 17 00:00:00 2001 From: Aaron Lehmann Date: Wed, 4 Dec 2024 17:35:58 -0800 Subject: [PATCH 2/6] Remove unneeded type declaration --- src/api.ts | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/api.ts b/src/api.ts index 2e574101..7a9a8f8b 100644 --- a/src/api.ts +++ b/src/api.ts @@ -1,4 +1,4 @@ -import { spawn, ChildProcessWithoutNullStreams } from "child_process" +import { spawn } from "child_process" import { Api } from "coder/site/src/api/api" import { ProvisionerJobLog, Workspace } from "coder/site/src/api/typesGenerated" import fs from "fs/promises" @@ -137,11 +137,7 @@ export async function startWorkspaceIfStoppedOrFailed( } return new Promise((resolve, reject) => { - const startProcess: ChildProcessWithoutNullStreams = spawn(binPath, [ - "start", - "--yes", - workspace.owner_name + "/" + workspace.name, - ]) + const startProcess = spawn(binPath, ["start", "--yes", workspace.owner_name + "/" + workspace.name]) startProcess.stdout.on("data", (data: Buffer) => { data From 8cdbd3a615cf28df48b8491ab4938e3273413101 Mon Sep 17 00:00:00 2001 From: Aaron Lehmann Date: Wed, 4 Dec 2024 17:42:41 -0800 Subject: [PATCH 3/6] Include stderr and full command in workspace start error message --- src/api.ts | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/api.ts b/src/api.ts index 7a9a8f8b..ba43fdb1 100644 --- a/src/api.ts +++ b/src/api.ts @@ -137,7 +137,8 @@ export async function startWorkspaceIfStoppedOrFailed( } return new Promise((resolve, reject) => { - const startProcess = spawn(binPath, ["start", "--yes", workspace.owner_name + "/" + workspace.name]) + const startArgs = ["start", "--yes", workspace.owner_name + "/" + workspace.name] + const startProcess = spawn(binPath, startArgs) startProcess.stdout.on("data", (data: Buffer) => { data @@ -150,6 +151,7 @@ export async function startWorkspaceIfStoppedOrFailed( }) }) + let capturedStderr = "" startProcess.stderr.on("data", (data: Buffer) => { data .toString() @@ -157,6 +159,7 @@ export async function startWorkspaceIfStoppedOrFailed( .forEach((line: string) => { if (line !== "") { writeEmitter.fire(line.toString() + "\r\n") + capturedStderr += line.toString() + "\n" } }) }) @@ -165,7 +168,11 @@ export async function startWorkspaceIfStoppedOrFailed( if (code === 0) { resolve(restClient.getWorkspace(workspace.id)) } else { - reject(new Error(`"coder start" process exited with code ${code}`)) + let errorText = `"${startArgs.join(" ")}" exited with code ${code}` + if (capturedStderr !== "") { + errorText += `: ${capturedStderr}` + } + reject(new Error(errorText)) } }) }) From 437483ea096ec5ec0cfa6d86b00cff0676d897ed Mon Sep 17 00:00:00 2001 From: Aaron Lehmann Date: Wed, 4 Dec 2024 18:21:07 -0800 Subject: [PATCH 4/6] Migrate "session_token" file to "session", add --global-config flag to "coder start" --- src/api.ts | 9 ++++++++- src/remote.ts | 23 ++++++++++++++++++++--- src/storage.ts | 32 ++++++++++++++++++++++++++++++++ 3 files changed, 60 insertions(+), 4 deletions(-) diff --git a/src/api.ts b/src/api.ts index ba43fdb1..217a3d67 100644 --- a/src/api.ts +++ b/src/api.ts @@ -125,6 +125,7 @@ export async function makeCoderSdk(baseUrl: string, token: string | undefined, s */ export async function startWorkspaceIfStoppedOrFailed( restClient: Api, + globalConfigDir: string, binPath: string, workspace: Workspace, writeEmitter: vscode.EventEmitter, @@ -137,7 +138,13 @@ export async function startWorkspaceIfStoppedOrFailed( } return new Promise((resolve, reject) => { - const startArgs = ["start", "--yes", workspace.owner_name + "/" + workspace.name] + const startArgs = [ + "--global-config", + globalConfigDir, + "start", + "--yes", + workspace.owner_name + "/" + workspace.name, + ] const startProcess = spawn(binPath, startArgs) startProcess.stdout.on("data", (data: Buffer) => { diff --git a/src/remote.ts b/src/remote.ts index 1977dd85..b4ac966b 100644 --- a/src/remote.ts +++ b/src/remote.ts @@ -53,6 +53,7 @@ export class Remote { private async maybeWaitForRunning( restClient: Api, workspace: Workspace, + label: string, binPath: string, ): Promise { // Maybe already running? @@ -98,6 +99,7 @@ export class Remote { title: "Waiting for workspace build...", }, async () => { + const globalConfigDir = path.dirname(this.storage.getSessionTokenPath(label)) while (workspace.latest_build.status !== "running") { ++attempts switch (workspace.latest_build.status) { @@ -114,7 +116,13 @@ export class Remote { } writeEmitter = initWriteEmitterAndTerminal() this.storage.writeToCoderOutputChannel(`Starting ${workspaceName}...`) - workspace = await startWorkspaceIfStoppedOrFailed(restClient, binPath, workspace, writeEmitter) + workspace = await startWorkspaceIfStoppedOrFailed( + restClient, + globalConfigDir, + binPath, + workspace, + writeEmitter, + ) break case "failed": // On a first attempt, we will try starting a failed workspace @@ -125,7 +133,13 @@ export class Remote { } writeEmitter = initWriteEmitterAndTerminal() this.storage.writeToCoderOutputChannel(`Starting ${workspaceName}...`) - workspace = await startWorkspaceIfStoppedOrFailed(restClient, binPath, workspace, writeEmitter) + workspace = await startWorkspaceIfStoppedOrFailed( + restClient, + globalConfigDir, + binPath, + workspace, + writeEmitter, + ) break } // Otherwise fall through and error. @@ -167,6 +181,9 @@ export class Remote { const workspaceName = `${parts.username}/${parts.workspace}` + // Migrate "session_token" file to "session", if needed. + this.storage.migrateSessionToken(parts.label) + // Get the URL and token belonging to this host. const { url: baseUrlRaw, token } = await this.storage.readCliConfig(parts.label) @@ -303,7 +320,7 @@ export class Remote { disposables.push(this.registerLabelFormatter(remoteAuthority, workspace.owner_name, workspace.name)) // If the workspace is not in a running state, try to get it running. - const updatedWorkspace = await this.maybeWaitForRunning(workspaceRestClient, workspace, binaryPath) + const updatedWorkspace = await this.maybeWaitForRunning(workspaceRestClient, workspace, parts.label, binaryPath) if (!updatedWorkspace) { // User declined to start the workspace. await this.closeRemote() diff --git a/src/storage.ts b/src/storage.ts index a4f2541f..7f99980d 100644 --- a/src/storage.ts +++ b/src/storage.ts @@ -405,6 +405,20 @@ export class Storage { * The caller must ensure this directory exists before use. */ public getSessionTokenPath(label: string): string { + return label + ? path.join(this.globalStorageUri.fsPath, label, "session") + : path.join(this.globalStorageUri.fsPath, "session") + } + + /** + * Return the directory for the deployment with the provided label to where + * its session token was stored by older code. + * + * If the label is empty, read the old deployment-unaware config instead. + * + * The caller must ensure this directory exists before use. + */ + public getLegacySessionTokenPath(label: string): string { return label ? path.join(this.globalStorageUri.fsPath, label, "session_token") : path.join(this.globalStorageUri.fsPath, "session_token") @@ -488,6 +502,24 @@ export class Storage { } } + /** + * Migrate the session token file from "session_token" to "session", if needed. + */ + public async migrateSessionToken(label: string) { + const oldTokenPath = this.getLegacySessionTokenPath(label) + try { + await fs.stat(oldTokenPath) + const newTokenPath = this.getSessionTokenPath(label) + await fs.rename(oldTokenPath, newTokenPath) + return + } catch (error) { + if ((error as NodeJS.ErrnoException)?.code === "ENOENT") { + return + } + throw error + } + } + /** * Run the header command and return the generated headers. */ From 4a03dc5da4031567c8c249c89692941169114033 Mon Sep 17 00:00:00 2001 From: Aaron Lehmann Date: Thu, 5 Dec 2024 15:39:56 -0800 Subject: [PATCH 5/6] Add await to migrateSessionToken call Co-authored-by: Asher --- src/remote.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/remote.ts b/src/remote.ts index b4ac966b..abe93e1f 100644 --- a/src/remote.ts +++ b/src/remote.ts @@ -182,7 +182,7 @@ export class Remote { const workspaceName = `${parts.username}/${parts.workspace}` // Migrate "session_token" file to "session", if needed. - this.storage.migrateSessionToken(parts.label) + await this.storage.migrateSessionToken(parts.label) // Get the URL and token belonging to this host. const { url: baseUrlRaw, token } = await this.storage.readCliConfig(parts.label) From 1230ee5bebc6584922b035052c2119b03dbbb618 Mon Sep 17 00:00:00 2001 From: Aaron Lehmann Date: Thu, 5 Dec 2024 15:42:01 -0800 Subject: [PATCH 6/6] Avoid unnecessary stat --- src/storage.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/storage.ts b/src/storage.ts index 7f99980d..8039a070 100644 --- a/src/storage.ts +++ b/src/storage.ts @@ -507,11 +507,9 @@ export class Storage { */ public async migrateSessionToken(label: string) { const oldTokenPath = this.getLegacySessionTokenPath(label) + const newTokenPath = this.getSessionTokenPath(label) try { - await fs.stat(oldTokenPath) - const newTokenPath = this.getSessionTokenPath(label) await fs.rename(oldTokenPath, newTokenPath) - return } catch (error) { if ((error as NodeJS.ErrnoException)?.code === "ENOENT") { return