add tag, cleanup containers at end of dev, rebuild hotkey, lots of refactoring etc. etc. (#9605)

* add tag and set imageName outside of miniflare

* delete containers at the end of dev session

* rebuild containers hotkey

* move build and cleanup into wrangler

* hide hotkey

* fix tests

* set container name to DO class_name not optional `name` field

* pr feedback

* fix tests

* changeset

* Update packages/wrangler/src/api/startDevWorker/LocalRuntimeController.ts

Co-authored-by: Victor Berchet <[email protected]>

* Update packages/containers-shared/src/build.ts

Co-authored-by: Carmen Popoviciu <[email protected]>

---------

Co-authored-by: Carmen Popoviciu <[email protected]>
Co-authored-by: Victor Berchet <[email protected]>

Author: 3 people

.changeset/itchy-rice-sink.md

@@ -0,0 +1,7 @@
+---
+"@cloudflare/containers-shared": patch
+"miniflare": patch
+"wrangler": patch
+---
+
+Add rebuild hotkey for containers local dev, and clean up containers at the end of a dev session.

packages/containers-shared/src/build.ts

@@ -1,6 +1,10 @@
 import { spawn } from "child_process";
 import { readFileSync } from "fs";
-import { BuildArgs, Logger } from "./types";
+import path from "path";
+import { dockerImageInspect } from "./inspect";
+import { MF_DEV_CONTAINER_PREFIX } from "./registry";
+import { BuildArgs, ContainerDevOptions, Logger } from "./types";
+import { verifyDockerInstalled } from "./utils";
 
 export async function constructBuildCommand(
 	options: BuildArgs,
@@ -65,3 +69,58 @@ export function dockerBuild(
 		});
 	});
 }
+
+/**
+ *
+ * Builds (or pulls - TODO) the container images for local development. This
+ * will be called before starting the local development server, and by a rebuild
+ * hotkey during development.
+ *
+ * Because this runs when local dev starts, we also do some validation here,
+ * such as checking if the Docker CLI is installed, and if the container images
+ * expose any ports.
+ */
+export async function prepareContainerImagesForDev(
+	dockerPath: string,
+	containerOptions: ContainerDevOptions[]
+) {
+	if (process.platform === "win32") {
+		throw new Error(
+			"Local development with containers is currently not supported on Windows. You should use WSL instead. You can also set `enable_containers` to false if you do not need to develop the container as part of your application."
+		);
+	}
+	await verifyDockerInstalled(dockerPath);
+	for (const options of containerOptions) {
+		await buildContainer(dockerPath, options);
+		await checkExposedPorts(dockerPath, options.imageTag);
+	}
+}
+
+async function buildContainer(
+	dockerPath: string,
+	options: ContainerDevOptions
+) {
+	// just let the tag default to latest
+	const { buildCmd, dockerfile } = await constructBuildCommand({
+		tag: options.imageTag,
+		pathToDockerfile: options.image,
+		buildContext: options.imageBuildContext ?? path.dirname(options.image),
+		args: options.args,
+		platform: "linux/amd64",
+	});
+
+	await dockerBuild(dockerPath, { buildCmd, dockerfile });
+}
+
+async function checkExposedPorts(dockerPath: string, imageTag: string) {
+	const output = await dockerImageInspect(dockerPath, {
+		imageTag,
+		formatString: "{{ len .Config.ExposedPorts }}",
+	});
+	if (output === "0" && process.platform !== "linux") {
+		throw new Error(
+			`The container "${imageTag.replace(MF_DEV_CONTAINER_PREFIX + "/", "")}" does not expose any ports.\n` +
+				"To develop containers locally on non-Linux platforms, you must expose any ports that you call with `getTCPPort()` in your Dockerfile."
+		);
+	}
+}

packages/containers-shared/src/knobs.ts

@@ -1,4 +1,11 @@
+import { MF_DEV_CONTAINER_PREFIX } from "./registry";
+
 // default cloudflare managed registry, can be overriden with the env var - CLOUDFLARE_CONTAINER_REGISTRY
 export const getCloudflareContainerRegistry = () => {
 	return process.env.CLOUDFLARE_CONTAINER_REGISTRY ?? "registry.cloudflare.com";
 };
+
+/** Prefixes with the cloudflare-dev namespace. The name should be the container's DO classname, and the tag a build uuid. */
+export const getDevContainerImageName = (name: string, tag: string) => {
+	return `${MF_DEV_CONTAINER_PREFIX}/${name.toLowerCase()}:${tag}`;
+};

packages/containers-shared/src/registry.ts

@@ -9,3 +9,5 @@ export const getCloudflareRegistryWithAccountNamespace = (
 ): string => {
 	return `${getCloudflareContainerRegistry()}/${accountID}/${tag}`;
 };
+
+export const MF_DEV_CONTAINER_PREFIX = "cloudflare-dev";

packages/containers-shared/src/types.ts

@@ -18,3 +18,14 @@ export type BuildArgs = {
 	/** sets --network=host at build time. only used by workers CI. */
 	setNetworkToHost?: boolean;
 };
+
+/** build/pull agnostic container options */
+export type ContainerDevOptions = {
+	/** may be dockerfile or registry link */
+	image: string;
+	/** formatted as cloudflare-dev/workername-DOclassname:build-id */
+	imageTag: string;
+	imageBuildContext?: string;
+	/** build time args */
+	args?: Record<string, string>;
+};

packages/containers-shared/src/utils.ts

@@ -1,4 +1,4 @@
-import { spawn, StdioOptions } from "child_process";
+import { execFile, spawn, StdioOptions } from "child_process";
 import { existsSync, statSync } from "fs";
 
 /** helper for simple docker command call that don't require any io handling */
@@ -29,6 +29,25 @@ export const runDockerCmd = async (
 	});
 };
 
+export const runDockerCmdWithOutput = async (
+	dockerPath: string,
+	args: string[]
+): Promise<string> => {
+	return new Promise((resolve, reject) => {
+		execFile(dockerPath, args, (error, stdout) => {
+			if (error) {
+				return reject(
+					new Error(
+						`Failed running docker command: ${error.message}. Command: ${dockerPath} ${args.join(" ")}`
+					)
+				);
+			}
+			return resolve(stdout.trim());
+		});
+	});
+};
+
+/** throws when docker is not installed */
 export const verifyDockerInstalled = async (dockerPath: string) => {
 	try {
 		await runDockerCmd(dockerPath, ["info"], ["inherit", "pipe", "pipe"]);
@@ -47,6 +66,7 @@ export function isDir(path: string) {
 	return stats.isDirectory();
 }
 
+/** returns true if it is a dockerfile, false if it is a registry link, throws if neither */
 export const isDockerfile = (image: string): boolean => {
 	// TODO: move this into config validation
 	if (existsSync(image)) {
@@ -86,3 +106,50 @@ export const isDockerfile = (image: string): boolean => {
 	}
 	return false;
 };
+
+/**
+ * Kills and removes any containers which come from the given image tag
+ */
+export const cleanupContainers = async (
+	dockerPath: string,
+	imageTags: Set<string>
+) => {
+	try {
+		// Find all containers (stopped and running) for each built image
+		const containerIds: string[] = [];
+		for (const imageTag of imageTags) {
+			containerIds.push(
+				...(await getContainerIdsFromImage(dockerPath, imageTag))
+			);
+		}
+
+		if (containerIds.length === 0) {
+			return true;
+		}
+
+		// Workerd should have stopped all containers, but clean up any in case. Sends a sigkill.
+		await runDockerCmd(
+			dockerPath,
+			["rm", "--force", ...containerIds],
+			["inherit", "pipe", "pipe"]
+		);
+		return true;
+	} catch (error) {
+		return false;
+	}
+};
+
+const getContainerIdsFromImage = async (
+	dockerPath: string,
+	ancestorImage: string
+) => {
+	const output = await runDockerCmdWithOutput(dockerPath, [
+		"ps",
+		"-a",
+		"--filter",
+		`ancestor=${ancestorImage}`,
+		"--format",
+		"{{.ID}}",
+	]);
+	return output.split("\n").filter((line) => line.trim());
+};

packages/miniflare/src/index.ts

@@ -38,8 +38,6 @@ import {
 	Response,
 } from "./http";
 import {
-	ContainerController,
-	ContainerOptions,
 	D1_PLUGIN_NAME,
 	DURABLE_OBJECTS_PLUGIN_NAME,
 	DurableObjectClassNames,
@@ -906,7 +904,6 @@ export class Miniflare {
 	#socketPorts?: SocketPorts;
 	#runtimeDispatcher?: Dispatcher;
 	#proxyClient?: ProxyClient;
-	#containerController?: ContainerController;
 
 	#cfObject?: Record<string, any> = {};
 
@@ -1592,10 +1589,6 @@ export class Miniflare {
 			innerBindings: Worker_Binding[];
 		}[] = [];
 
-		let containerOptions: {
-			[className: string]: ContainerOptions;
-		} = {};
-
 		for (const [key, plugin] of PLUGIN_ENTRIES) {
 			const pluginExtensions = await plugin.getExtensions?.({
 				// @ts-expect-error `CoreOptionsSchema` has required options which are
@@ -1627,14 +1620,6 @@ export class Miniflare {
 				workerOpts.assets.assets.workerName = workerOpts.core.name;
 			}
 
-			if (workerOpts.containers.containers) {
-				// we don't care which worker this container belongs to, as they are id'd already by the DO classname
-				containerOptions = {
-					...containerOptions,
-					...workerOpts.containers.containers,
-				};
-			}
-
 			// Collect all bindings from this worker
 			const workerBindings: Worker_Binding[] = [];
 			allWorkerBindings.set(workerName, workerBindings);
@@ -1892,25 +1877,6 @@ export class Miniflare {
 			}
 		}
 
-		if (
-			Object.keys(containerOptions).length &&
-			sharedOpts.containers.enableContainers
-		) {
-			if (this.#containerController === undefined) {
-				this.#containerController = new ContainerController(
-					containerOptions,
-					this.#sharedOpts.containers,
-					this.#log
-				);
-				await this.#containerController.buildAllContainers();
-			} else {
-				this.#containerController.updateConfig(
-					containerOptions,
-					this.#sharedOpts.containers
-				);
-			}
-		}
-
 		const globalServices = getGlobalServices({
 			sharedOptions: sharedOpts.core,
 			allWorkerRoutes,
@@ -2669,6 +2635,7 @@ export class Miniflare {
 			// Cleanup as much as possible even if `#init()` threw
 			await this.#proxyClient?.dispose();
 			await this.#runtime?.dispose();
+
 			await this.#stopLoopbackServer();
 			// `rm -rf ${#tmpPath}`, this won't throw if `#tmpPath` doesn't exist
 			await fs.promises.rm(this.#tmpPath, { force: true, recursive: true });