Remove comparison based on image, release, and service ids

Preparing for the new v3 target state, where the supervisor will make environment
dependent ids optional and rely on using general UUIDs and user known identifiers
for comparison. This PR moves forward in that direction by removing some of those
comparisons for v2 target state.

- imageId to be replaced with imageName
- serviceId to be replace by serviceName
- releaseId to be replaced by commit (future release_uuid)

This is a backwards compatible change, meaning it doesn't completely get rid of
these identifiers (which are still being used by supervisor API and for state
patch), but will not depend on those identifiers for calculating steps to target state.

Change-type: minor

src/compose/app.ts

@@ -31,7 +31,6 @@ export interface AppConstructOpts {
 	appId: number;
 	appName?: string;
 	commit?: string;
-	releaseId?: number;
 	source?: string;
 
 	services: Service[];
@@ -43,7 +42,7 @@ export interface UpdateState {
 	localMode: boolean;
 	availableImages: Image[];
 	containerIds: Dictionary<string>;
-	downloading: number[];
+	downloading: string[];
 }
 
 interface ChangingPair<T> {
@@ -56,7 +55,6 @@ export class App {
 	// When setting up an application from current state, these values are not available
 	public appName?: string;
 	public commit?: string;
-	public releaseId?: number;
 	public source?: string;
 
 	// Services are stored as an array, as at any one time we could have more than one
@@ -69,7 +67,6 @@ export class App {
 		this.appId = opts.appId;
 		this.appName = opts.appName;
 		this.commit = opts.commit;
-		this.releaseId = opts.releaseId;
 		this.source = opts.source;
 		this.services = opts.services;
 		this.volumes = opts.volumes;
@@ -266,34 +263,30 @@ export class App {
 		removePairs: Array<ChangingPair<Service>>;
 		updatePairs: Array<ChangingPair<Service>>;
 	} {
-		const currentByServiceId = _.keyBy(current, 'serviceId');
-		const targetByServiceId = _.keyBy(target, 'serviceId');
+		const currentByServiceName = _.keyBy(current, 'serviceName');
+		const targetByServiceName = _.keyBy(target, 'serviceName');
 
-		const currentServiceIds = Object.keys(currentByServiceId).map((i) =>
-			parseInt(i, 10),
-		);
-		const targetServiceIds = Object.keys(targetByServiceId).map((i) =>
-			parseInt(i, 10),
-		);
+		const currentServiceNames = Object.keys(currentByServiceName);
+		const targetServiceNames = Object.keys(targetByServiceName);
 
-		const toBeRemoved = _(currentServiceIds)
-			.difference(targetServiceIds)
-			.map((id) => ({ current: currentByServiceId[id] }))
+		const toBeRemoved = _(currentServiceNames)
+			.difference(targetServiceNames)
+			.map((id) => ({ current: currentByServiceName[id] }))
 			.value();
 
-		const toBeInstalled = _(targetServiceIds)
-			.difference(currentServiceIds)
-			.map((id) => ({ target: targetByServiceId[id] }))
+		const toBeInstalled = _(targetServiceNames)
+			.difference(currentServiceNames)
+			.map((id) => ({ target: targetByServiceName[id] }))
 			.value();
 
-		const maybeUpdate = _.intersection(targetServiceIds, currentServiceIds);
+		const maybeUpdate = _.intersection(targetServiceNames, currentServiceNames);
 
-		// Build up a list of services for a given service ID, always using the latest created
+		// Build up a list of services for a given service name, always using the latest created
 		// service. Any older services will have kill steps emitted
-		for (const serviceId of maybeUpdate) {
-			const currentServiceContainers = _.filter(current, { serviceId });
+		for (const serviceName of maybeUpdate) {
+			const currentServiceContainers = _.filter(current, { serviceName });
 			if (currentServiceContainers.length > 1) {
-				currentByServiceId[serviceId] = _.maxBy(
+				currentByServiceName[serviceName] = _.maxBy(
 					currentServiceContainers,
 					'createdAt',
 				)!;
@@ -302,13 +295,13 @@ export class App {
 				// be removed
 				const otherContainers = _.without(
 					currentServiceContainers,
-					currentByServiceId[serviceId],
+					currentByServiceName[serviceName],
 				);
 				for (const service of otherContainers) {
 					toBeRemoved.push({ current: service });
 				}
 			} else {
-				currentByServiceId[serviceId] = currentServiceContainers[0];
+				currentByServiceName[serviceName] = currentServiceContainers[0];
 			}
 		}
 
@@ -374,9 +367,9 @@ export class App {
 		 * Filter all the services which should be updated due to run state change, or config mismatch.
 		 */
 		const toBeUpdated = maybeUpdate
-			.map((serviceId) => ({
-				current: currentByServiceId[serviceId],
-				target: targetByServiceId[serviceId],
+			.map((serviceName) => ({
+				current: currentByServiceName[serviceName],
+				target: targetByServiceName[serviceName],
 			}))
 			.filter(
 				({ current: c, target: t }) =>
@@ -456,7 +449,7 @@ export class App {
 		context: {
 			localMode: boolean;
 			availableImages: Image[];
-			downloading: number[];
+			downloading: string[];
 			targetApp: App;
 			containerIds: Dictionary<string>;
 			networkPairs: Array<ChangingPair<Network>>;
@@ -486,7 +479,7 @@ export class App {
 			);
 		}
 
-		if (needsDownload && context.downloading.includes(target?.imageId!)) {
+		if (needsDownload && context.downloading.includes(target?.imageName!)) {
 			// The image needs to be downloaded, and it's currently downloading. We simply keep
 			// the application loop alive
 			return generateStep('noop', {});
@@ -563,7 +556,7 @@ export class App {
 			service.status !== 'Stopping' &&
 			!_.some(
 				changingServices,
-				({ current }) => current?.serviceId !== service.serviceId,
+				({ current }) => current?.serviceName !== service.serviceName,
 			)
 		) {
 			return [generateStep('kill', { current: service })];
@@ -595,11 +588,8 @@ export class App {
 	}
 
 	private generateContainerStep(current: Service, target: Service) {
-		// if the services release/image don't match, then rename the container...
-		if (
-			current.releaseId !== target.releaseId ||
-			current.imageId !== target.imageId
-		) {
+		// if the services release doesn't match, then rename the container...
+		if (current.commit !== target.commit) {
 			return generateStep('updateMetadata', { current, target });
 		} else if (target.config.running !== current.config.running) {
 			if (target.config.running) {
@@ -728,7 +718,7 @@ export class App {
 					!_.some(
 						availableImages,
 						(image) =>
-							image.dockerImageId === dependencyService?.imageId ||
+							image.dockerImageId === dependencyService?.config.image ||
 							imageManager.isSameImage(image, {
 								name: dependencyService?.imageName!,
 							}),
@@ -825,7 +815,6 @@ export class App {
 			{
 				appId: app.appId,
 				commit: app.commit,
-				releaseId: app.releaseId,
 				appName: app.name,
 				source: app.source,
 				services,

src/compose/application-manager.ts

@@ -179,7 +179,7 @@ export async function getRequiredSteps(
 ): Promise<CompositionStep[]> {
 	// get some required data
 	const [downloading, availableImages, currentApps] = await Promise.all([
-		imageManager.getDownloadingImageIds(),
+		imageManager.getDownloadingImageNames(),
 		imageManager.getAvailable(),
 		getCurrentApps(),
 	]);
@@ -199,7 +199,7 @@ export async function inferNextSteps(
 	targetApps: InstancedAppState,
 	{
 		ignoreImages = false,
-		downloading = [] as number[],
+		downloading = [] as string[],
 		availableImages = [] as Image[],
 		containerIdsByAppId = {} as { [appId: number]: Dictionary<string> },
 	} = {},
@@ -674,7 +674,7 @@ function saveAndRemoveImages(
 			(svc) =>
 				_.find(availableImages, {
 					dockerImageId: svc.config.image,
-					imageId: svc.imageId,
+					name: svc.imageName,
 				}) ?? _.find(availableImages, { dockerImageId: svc.config.image }),
 		),
 	) as imageManager.Image[];

src/compose/images.ts

@@ -121,30 +121,30 @@ function createTask(initialContext: Image) {
 	return running(initialContext);
 }
 
-const runningTasks: { [imageId: number]: ImageTask } = {};
+const runningTasks: { [imageName: string]: ImageTask } = {};
 function reportEvent(event: 'start' | 'update' | 'finish', state: Image) {
-	const { imageId } = state;
+	const { name: imageName } = state;
 
 	// Emit by default if a start event is reported
 	let emitChange = event === 'start';
 
 	// Get the current task and update it in memory
 	const currentTask =
-		event === 'start' ? createTask(state) : runningTasks[imageId];
-	runningTasks[imageId] = currentTask;
+		event === 'start' ? createTask(state) : runningTasks[imageName];
+	runningTasks[imageName] = currentTask;
 
 	// TODO: should we assert that the current task exists at this point?
 	// On update, update the corresponding task with the new state if it exists
 	if (event === 'update' && currentTask) {
 		const [updatedTask, changed] = currentTask.update(state);
-		runningTasks[imageId] = updatedTask;
+		runningTasks[imageName] = updatedTask;
 		emitChange = changed;
 	}
 
 	// On update, update the corresponding task with the new state if it exists
 	if (event === 'finish' && currentTask) {
 		[, emitChange] = currentTask.finish();
-		delete runningTasks[imageId];
+		delete runningTasks[imageName];
 	}
 
 	if (emitChange) {
@@ -354,6 +354,12 @@ export function getDownloadingImageIds(): number[] {
 		.map((t) => t.context.imageId);
 }
 
+export function getDownloadingImageNames(): string[] {
+	return Object.values(runningTasks)
+		.filter((t) => t.context.status === 'Downloading')
+		.map((t) => t.context.name);
+}
+
 export async function cleanImageData(): Promise<void> {
 	const imagesToRemove = await withImagesFromDockerAndDB(
 		async (dockerImages, supervisedImages) => {

src/compose/service-manager.ts

@@ -20,7 +20,7 @@ import {
 } from '../lib/errors';
 import * as LogTypes from '../lib/log-types';
 import { checkInt, isValidDeviceName } from '../lib/validation';
-import { Service } from './service';
+import { Service, ServiceStatus } from './service';
 import { serviceNetworksToDockerNetworks } from './utils';
 
 import log from '../lib/supervisor-console';
@@ -88,7 +88,7 @@ export async function get(service: Service) {
 	// Get the container ids for special network handling
 	const containerIds = await getContainerIdMap(service.appId!);
 	const services = (
-		await getAll(`service-id=${service.serviceId}`)
+		await getAll(`service-name=${service.serviceName}`)
 	).filter((currentService) =>
 		currentService.isEqualConfig(service, containerIds),
 	);
@@ -151,7 +151,7 @@ export async function updateMetadata(service: Service, target: Service) {
 	}
 
 	await docker.getContainer(svc.containerId).rename({
-		name: `${service.serviceName}_${target.imageId}_${target.releaseId}`,
+		name: `${service.serviceName}_${target.imageId}_${target.releaseId}_${target.commit}`,
 	});
 }
 
@@ -294,7 +294,7 @@ export async function start(service: Service) {
 		containerId = container.id;
 		logger.logSystemEvent(LogTypes.startService, { service });
 
-		reportNewStatus(containerId, service, 'Starting');
+		reportNewStatus(containerId, service, 'Starting' as ServiceStatus);
 
 		let shouldRemove = false;
 		let err: Error | undefined;
@@ -498,7 +498,7 @@ function reportChange(containerId?: string, status?: Partial<Service>) {
 function reportNewStatus(
 	containerId: string,
 	service: Partial<Service>,
-	status: string,
+	status: ServiceStatus,
 ) {
 	reportChange(
 		containerId,
@@ -611,7 +611,7 @@ async function prepareForHandover(service: Service) {
 	const container = docker.getContainer(svc.containerId);
 	await container.update({ RestartPolicy: {} });
 	return await container.rename({
-		name: `old_${service.serviceName}_${service.imageId}_${service.imageId}_${service.releaseId}`,
+		name: `old_${service.serviceName}_${service.imageId}_${service.releaseId}_${service.commit}`,
 	});
 }
 

src/compose/service.ts

@@ -44,7 +44,8 @@ export class Service {
 	public appId: number;
 	public imageId: number;
 	public config: ServiceConfig;
-	public serviceName: string | null;
+	public serviceName: string;
+	public commit: string;
 	public releaseId: number;
 	public serviceId: number;
 	public imageName: string | null;
@@ -135,12 +136,11 @@ export class Service {
 		delete appConfig.dependsOn;
 		service.createdAt = appConfig.createdAt;
 		delete appConfig.createdAt;
+		service.commit = appConfig.commit;
+		delete appConfig.commit;
 
 		delete appConfig.contract;
 
-		// We don't need this value
-		delete appConfig.commit;
-
 		// Get rid of any extra values and report them to the user
 		const config = sanitiseComposeConfig(appConfig);
 
@@ -600,15 +600,16 @@ export class Service {
 				'Attempt to build Service class from container with malformed labels',
 			);
 		}
-		const nameMatch = container.Name.match(/.*_(\d+)_(\d+)$/);
+		const nameMatch = container.Name.match(/.*_(\d+)_(\d+)(?:_(.*?))?$/);
 		if (nameMatch == null) {
 			throw new InternalInconsistencyError(
-				'Attempt to build Service class from container with malformed name',
+				`Expected supervised container to have name '<serviceName>_<imageId>_<releaseId>_<commit>', got: ${container.Name}`,
 			);
 		}
 
 		svc.imageId = parseInt(nameMatch[1], 10);
 		svc.releaseId = parseInt(nameMatch[2], 10);
+		svc.commit = nameMatch[3];
 		svc.containerId = container.Id;
 		svc.dockerImageId = container.Config.Image;
 
@@ -656,7 +657,7 @@ export class Service {
 			this.config.networkMode = `container:${containerId}`;
 		}
 		return {
-			name: `${this.serviceName}_${this.imageId}_${this.releaseId}`,
+			name: `${this.serviceName}_${this.imageId}_${this.releaseId}_${this.commit}`,
 			Tty: this.config.tty,
 			Cmd: this.config.command,
 			Volumes: volumes,
@@ -862,8 +863,7 @@ export class Service {
 	): boolean {
 		return (
 			this.isEqualConfig(service, currentContainerIds) &&
-			this.releaseId === service.releaseId &&
-			this.imageId === service.imageId
+			this.commit === service.commit
 		);
 	}