Respect update strategies app-wide instead of at the service level

Fixes behavior for release updates which removes a service in current state
and adds a new service in target state.

Change-type: patch
Closes: #2095
Signed-off-by: Christina Ying Wang <christina@balena.io>

src/compose/app.ts

@@ -15,12 +15,13 @@ import {
 import * as targetStateCache from '../device-state/target-state-cache';
 import { getNetworkGateway } from '../lib/docker-utils';
 import * as constants from '../lib/constants';
-
-import { getStepsFromStrategy } from './update-strategies';
-
-import { InternalInconsistencyError, isNotFoundError } from '../lib/errors';
+import {
+	getStepsFromStrategy,
+	getStrategyFromService,
+} from './update-strategies';
+import { isNotFoundError } from '../lib/errors';
 import * as config from '../config';
-import { checkTruthy, checkString } from '../lib/validation';
+import { checkTruthy } from '../lib/validation';
 import { ServiceComposeConfig, DeviceMetadata } from './types/service';
 import { pathExistsOnRoot } from '../lib/host-utils';
 import { isSupervisor } from '../lib/supervisor-metadata';
@@ -133,17 +134,8 @@ export class App {
 			state.containerIds,
 		);
 
-		for (const { current: svc } of removePairs) {
-			// All removes get a kill action if they're not already stopping
-			if (svc!.status !== 'Stopping') {
-				steps.push(generateStep('kill', { current: svc! }));
-			} else {
-				steps.push(generateStep('noop', {}));
-			}
-		}
-
 		// For every service which needs to be updated, update via update strategy.
-		const servicePairs = updatePairs.concat(installPairs);
+		const servicePairs = removePairs.concat(updatePairs, installPairs);
 		steps = steps.concat(
 			servicePairs
 				.map((pair) =>
@@ -511,8 +503,8 @@ export class App {
 		},
 	): Nullable<CompositionStep> {
 		if (current?.status === 'Stopping') {
-			// Theres a kill step happening already, emit a noop to ensure we stay alive while
-			// this happens
+			// There's a kill step happening already, emit a noop to ensure
+			// we stay alive while this happens
 			return generateStep('noop', {});
 		}
 		if (current?.status === 'Dead') {
@@ -521,16 +513,14 @@ export class App {
 			return;
 		}
 
-		const needsDownload = !_.some(
-			context.availableImages,
+		const needsDownload = !context.availableImages.some(
 			(image) =>
 				image.dockerImageId === target?.config.image ||
 				imageManager.isSameImage(image, { name: target?.imageName! }),
 		);
-
 		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
+			// The image needs to be downloaded, and it's currently downloading.
+			// We simply keep the application loop alive
 			return generateStep('noop', {});
 		}
 
@@ -546,47 +536,39 @@ export class App {
 				context.servicePairs,
 			);
 		} else {
-			if (!target) {
-				throw new InternalInconsistencyError(
-					'An empty changing pair passed to generateStepsForService',
-				);
-			}
-
+			// This service is in both current & target so requires an update,
+			// or it's a service that's not in target so requires removal
 			const needsSpecialKill = this.serviceHasNetworkOrVolume(
 				current,
 				context.networkPairs,
 				context.volumePairs,
 			);
-
 			if (
 				!needsSpecialKill &&
+				target != null &&
 				current.isEqualConfig(target, context.containerIds)
 			) {
 				// we're only starting/stopping a service
 				return this.generateContainerStep(current, target);
 			}
 
-			let strategy =
-				checkString(target.config.labels['io.balena.update.strategy']) || '';
-			const validStrategies = [
-				'download-then-kill',
-				'kill-then-download',
-				'delete-then-download',
-				'hand-over',
-			];
-
-			if (!validStrategies.includes(strategy)) {
-				strategy = 'download-then-kill';
+			let strategy: string;
+			let dependenciesMetForStart: boolean;
+			if (target != null) {
+				strategy = getStrategyFromService(target);
+				dependenciesMetForStart = this.dependenciesMetForServiceStart(
+					target,
+					context.targetApp,
+					context.availableImages,
+					context.networkPairs,
+					context.volumePairs,
+					context.servicePairs,
+				);
+			} else {
+				strategy = getStrategyFromService(current);
+				dependenciesMetForStart = false;
 			}
 
-			const dependenciesMetForStart = this.dependenciesMetForServiceStart(
-				target,
-				context.targetApp,
-				context.availableImages,
-				context.networkPairs,
-				context.volumePairs,
-				context.servicePairs,
-			);
 			const dependenciesMetForKill = this.dependenciesMetForServiceKill(
 				context.targetApp,
 				context.availableImages,
@@ -678,7 +660,10 @@ export class App {
 		volumePairs: Array<ChangingPair<Volume>>,
 		servicePairs: Array<ChangingPair<Service>>,
 	): CompositionStep | undefined {
-		if (needsDownload) {
+		if (
+			needsDownload &&
+			this.dependenciesMetForServiceFetch(target, servicePairs)
+		) {
 			// We know the service name exists as it always does for targets
 			return generateStep('fetch', {
 				image: imageManager.imageFromService(target),
@@ -698,6 +683,37 @@ export class App {
 		}
 	}
 
+	private dependenciesMetForServiceFetch(
+		target: Service,
+		servicePairs: Array<ChangingPair<Service>>,
+	) {
+		const [servicePairsWithCurrent, servicePairsWithoutCurrent] = _.partition(
+			servicePairs,
+			(pair) => pair.current != null,
+		);
+
+		// Target services not in current can be safely fetched
+		for (const pair of servicePairsWithoutCurrent) {
+			if (target.serviceName === pair.target!.serviceName) {
+				return true;
+			}
+		}
+
+		// Current services should be killed before target
+		// service fetch depending on update strategy
+		for (const pair of servicePairsWithCurrent) {
+			// Prefer target's update strategy if target service exists
+			const strategy = getStrategyFromService(pair.target ?? pair.current!);
+			if (
+				['kill-then-download', 'delete-then-download'].includes(strategy) &&
+				pair.current!.config.running
+			) {
+				return false;
+			}
+		}
+		return true;
+	}
+
 	// TODO: account for volumes-from, networks-from, links, etc
 	// TODO: support networks instead of only network mode
 	private dependenciesMetForServiceStart(
@@ -751,7 +767,7 @@ export class App {
 		}
 
 		// do not start until all images have been downloaded
-		return this.targetImagesReady(targetApp, availableImages);
+		return this.targetImagesReady(targetApp.services, availableImages);
 	}
 
 	// Unless the update strategy requires an early kill (i.e kill-then-download,
@@ -762,12 +778,14 @@ export class App {
 		targetApp: App,
 		availableImages: Image[],
 	) {
-		// Don't kill any services before all images have been downloaded
-		return this.targetImagesReady(targetApp, availableImages);
+		return this.targetImagesReady(targetApp.services, availableImages);
 	}
 
-	private targetImagesReady(targetApp: App, availableImages: Image[]) {
-		return targetApp.services.every((service) =>
+	private targetImagesReady(
+		targetServices: Service[],
+		availableImages: Image[],
+	) {
+		return targetServices.every((service) =>
 			availableImages.some(
 				(image) =>
 					image.dockerImageId === service.config.image ||

src/compose/update-strategies.ts

@@ -1,12 +1,12 @@
 import * as imageManager from './images';
 import Service from './service';
-
-import { InternalInconsistencyError } from '../lib/errors';
 import { CompositionStep, generateStep } from './composition-steps';
+import { InternalInconsistencyError } from '../lib/errors';
+import { checkString } from '../lib/validation';
 
 export interface StrategyContext {
 	current: Service;
-	target: Service;
+	target?: Service;
 	needsDownload: boolean;
 	dependenciesMetForStart: boolean;
 	dependenciesMetForKill: boolean;
@@ -19,35 +19,35 @@ export function getStepsFromStrategy(
 ): CompositionStep {
 	switch (strategy) {
 		case 'download-then-kill':
-			if (context.needsDownload) {
+			if (context.needsDownload && context.target) {
 				return generateStep('fetch', {
 					image: imageManager.imageFromService(context.target),
-					serviceName: context.target.serviceName!,
+					serviceName: context.target.serviceName,
 				});
 			} else if (context.dependenciesMetForKill) {
 				// We only kill when dependencies are already met, so that we minimize downtime
 				return generateStep('kill', { current: context.current });
 			} else {
-				return { action: 'noop' };
+				return generateStep('noop', {});
 			}
 		case 'kill-then-download':
 		case 'delete-then-download':
 			return generateStep('kill', { current: context.current });
 		case 'hand-over':
-			if (context.needsDownload) {
+			if (context.needsDownload && context.target) {
 				return generateStep('fetch', {
 					image: imageManager.imageFromService(context.target),
-					serviceName: context.target.serviceName!,
+					serviceName: context.target.serviceName,
 				});
 			} else if (context.needsSpecialKill && context.dependenciesMetForKill) {
 				return generateStep('kill', { current: context.current });
-			} else if (context.dependenciesMetForStart) {
+			} else if (context.dependenciesMetForStart && context.target) {
 				return generateStep('handover', {
 					current: context.current,
 					target: context.target,
 				});
 			} else {
-				return { action: 'noop' };
+				return generateStep('noop', {});
 			}
 		default:
 			throw new InternalInconsistencyError(
@@ -55,3 +55,21 @@ export function getStepsFromStrategy(
 			);
 	}
 }
+
+export function getStrategyFromService(svc: Service): string {
+	let strategy =
+		checkString(svc.config.labels['io.balena.update.strategy']) || '';
+
+	const validStrategies = [
+		'download-then-kill',
+		'kill-then-download',
+		'delete-then-download',
+		'hand-over',
+	];
+
+	if (!validStrategies.includes(strategy)) {
+		strategy = 'download-then-kill';
+	}
+
+	return strategy;
+}

test/integration/compose/application-manager.spec.ts

@@ -15,6 +15,8 @@ import {
 	createApps,
 	createCurrentState,
 	DEFAULT_NETWORK,
+	expectSteps,
+	expectNoStep,
 } from '~/test-lib/state-helper';
 import { InstancedAppState } from '~/src/types';
 
@@ -327,6 +329,458 @@ describe('compose/application-manager', () => {
 			.that.deep.includes({ name: 'image-old' });
 	});
 
+	// These cases test whether the Supervisor handles multi-container updates
+	// on a per-app basis rather than a per-service while also respecting update strategies.
+	describe('update strategy compliance for multi-container apps', () => {
+		const createCurrentAndTargetServicesWithLabels = async (labels: {
+			[key: string]: string;
+		}) => {
+			const currentServices = [
+				await createService({
+					image: 'image-main',
+					labels,
+					appId: 1,
+					serviceName: 'main',
+					commit: 'old-release',
+					releaseId: 1,
+				}),
+				await createService({
+					image: 'image-old',
+					labels,
+					appId: 1,
+					serviceName: 'old',
+					commit: 'old-release',
+					releaseId: 1,
+				}),
+			];
+			const targetServices = [
+				await createService({
+					image: 'image-main',
+					labels,
+					appId: 1,
+					serviceName: 'main',
+					commit: 'new-release',
+					releaseId: 2,
+				}),
+				await createService({
+					image: 'image-new',
+					labels,
+					appId: 1,
+					serviceName: 'new',
+					commit: 'new-release',
+					releaseId: 2,
+				}),
+			];
+			return { currentServices, targetServices };
+		};
+
+		it('should not infer a kill step for current service A before target service B download finishes with download-then-kill', async () => {
+			const labels = {
+				'io.balena.update.strategy': 'download-then-kill',
+			};
+			const { currentServices, targetServices } =
+				await createCurrentAndTargetServicesWithLabels(labels);
+
+			/**
+			 * Before target image finishes downloading, do not infer kill step
+			 */
+			let targetApps = createApps(
+				{
+					services: targetServices,
+				},
+				true,
+			);
+			const c1 = createCurrentState({
+				services: currentServices,
+				networks: [DEFAULT_NETWORK],
+				downloading: ['image-new'],
+			});
+			const steps = await applicationManager.inferNextSteps(
+				c1.currentApps,
+				targetApps,
+				{
+					downloading: c1.downloading,
+					availableImages: c1.availableImages,
+					containerIdsByAppId: c1.containerIdsByAppId,
+				},
+			);
+			// There should be two noop steps, one for target service which is still downloading,
+			// and one for current service which is waiting on target download to complete.
+			expectSteps('noop', steps, 2);
+			// No kill step yet
+			expectNoStep('kill', steps);
+
+			/**
+			 * After target image finishes downloading, infer a kill step
+			 */
+			targetApps = createApps(
+				{
+					services: targetServices,
+				},
+				true,
+			);
+			const { currentApps, availableImages, downloading, containerIdsByAppId } =
+				createCurrentState({
+					services: currentServices,
+					networks: [DEFAULT_NETWORK],
+					images: [
+						// Both images have been downloaded or had their metadata updated
+						createImage({
+							name: 'image-main',
+							appId: 1,
+							serviceName: 'main',
+							commit: 'new-release',
+							releaseId: 2,
+						}),
+						createImage({
+							name: 'image-new',
+							appId: 1,
+							serviceName: 'new',
+							commit: 'new-release',
+							releaseId: 2,
+						}),
+					],
+				});
+			const steps2 = await applicationManager.inferNextSteps(
+				currentApps,
+				targetApps,
+				{
+					downloading,
+					availableImages,
+					containerIdsByAppId,
+				},
+			);
+			// Service `old` is safe to kill after download for `new` has completed
+			const [killStep] = expectSteps('kill', steps2, 1);
+			expect(killStep)
+				.to.have.property('current')
+				.that.deep.includes({ serviceName: 'old' });
+			// Service `new` in target release should be started as download has completed
+			const [startStep] = expectSteps('start', steps2, 1);
+			expect(startStep)
+				.to.have.property('target')
+				.that.deep.includes({ serviceName: 'new' });
+			// No noop steps
+			expectNoStep('noop', steps2);
+		});
+
+		it('should infer a fetch step for target service B together with current service A kill with kill-then-download', async () => {
+			const labels = {
+				'io.balena.update.strategy': 'kill-then-download',
+			};
+			const { currentServices, targetServices } =
+				await createCurrentAndTargetServicesWithLabels(labels);
+			const targetApps = createApps(
+				{
+					services: targetServices,
+				},
+				true,
+			);
+
+			/**
+			 * Infer fetch step for new target service together with kill & updateMetadata steps for current services
+			 */
+			const c1 = createCurrentState({
+				services: currentServices,
+				networks: [DEFAULT_NETWORK],
+			});
+			const steps = await applicationManager.inferNextSteps(
+				c1.currentApps,
+				targetApps,
+				{
+					downloading: c1.downloading,
+					// Simulate old images already removed from image table
+					// to avoid removeImage steps
+					availableImages: [],
+					containerIdsByAppId: c1.containerIdsByAppId,
+				},
+			);
+			// Service `new` should be fetched
+			const [fetchStep] = expectSteps('fetch', steps, 1);
+			expect(fetchStep).to.have.property('serviceName').that.equals('new');
+			// Service `old` should be killed
+			const [killStep] = expectSteps('kill', steps, 1);
+			expect(killStep)
+				.to.have.property('current')
+				.that.deep.includes({ serviceName: 'old' });
+			// Service `main` should have its metadata updated
+			const [updateMetadataStep] = expectSteps('updateMetadata', steps, 1);
+			expect(updateMetadataStep)
+				.to.have.property('target')
+				.that.deep.includes({ serviceName: 'main' });
+
+			/**
+			 * Noop while target image is downloading
+			 */
+			const c2 = createCurrentState({
+				// Simulate `kill` and `updateMetadata` steps already executed
+				services: currentServices
+					.filter(({ serviceName }) => serviceName !== 'old')
+					.map((svc) => {
+						svc.releaseId = 2;
+						svc.commit = 'new-release';
+						return svc;
+					}),
+				networks: [DEFAULT_NETWORK],
+			});
+			const steps2 = await applicationManager.inferNextSteps(
+				c2.currentApps,
+				targetApps,
+				{
+					downloading: ['image-new'],
+					availableImages: c2.availableImages,
+					containerIdsByAppId: c2.containerIdsByAppId,
+				},
+			);
+			// Noop while service `new` is downloading
+			expectSteps('noop', steps2, 1);
+			// No start step should be generated until fetch completes
+			expectNoStep('start', steps2);
+			// No other steps
+			expect(steps2).to.have.length(1);
+
+			/**
+			 * Infer start step only after download completes
+			 */
+			const steps3 = await applicationManager.inferNextSteps(
+				c2.currentApps,
+				targetApps,
+				{
+					downloading: c1.downloading,
+					// Both images have been downloaded or had their metadata updated
+					availableImages: [
+						createImage({
+							name: 'image-main',
+							appId: 1,
+							serviceName: 'main',
+							commit: 'new-release',
+							releaseId: 2,
+						}),
+						createImage({
+							name: 'image-new',
+							appId: 1,
+							serviceName: 'new',
+							commit: 'new-release',
+							releaseId: 2,
+						}),
+					],
+					containerIdsByAppId: c1.containerIdsByAppId,
+				},
+			);
+			// Service `new` should be started
+			const [startStep] = expectSteps('start', steps3, 1);
+			expect(startStep)
+				.to.have.property('target')
+				.that.deep.includes({ serviceName: 'new' });
+			// No other steps
+			expect(steps3).to.have.length(1);
+		});
+
+		it('should infer a fetch step for target service B together with current service A removal with delete-then-download', async () => {
+			const labels = {
+				'io.balena.update.strategy': 'delete-then-download',
+			};
+			const { currentServices, targetServices } =
+				await createCurrentAndTargetServicesWithLabels(labels);
+			const targetApps = createApps(
+				{
+					services: targetServices,
+				},
+				true,
+			);
+
+			/**
+			 * Infer fetch step for new target service together with kill & updateMetadata steps for current services
+			 */
+			const c1 = createCurrentState({
+				services: currentServices,
+				networks: [DEFAULT_NETWORK],
+			});
+			const steps = await applicationManager.inferNextSteps(
+				c1.currentApps,
+				targetApps,
+				{
+					downloading: c1.downloading,
+					// Simulate old images already removed from image table
+					// to avoid removeImage steps
+					availableImages: [],
+					containerIdsByAppId: c1.containerIdsByAppId,
+				},
+			);
+			// Service `new` should be fetched
+			const [fetchStep] = expectSteps('fetch', steps, 1);
+			expect(fetchStep).to.have.property('serviceName').that.equals('new');
+			// Service `old` should be killed
+			const [killStep] = expectSteps('kill', steps, 1);
+			expect(killStep)
+				.to.have.property('current')
+				.that.deep.includes({ serviceName: 'old' });
+			// Service `main` should have its metadata updated
+			const [updateMetadataStep] = expectSteps('updateMetadata', steps, 1);
+			expect(updateMetadataStep)
+				.to.have.property('target')
+				.that.deep.includes({ serviceName: 'main' });
+
+			/**
+			 * Noop while target image is downloading
+			 */
+			const c2 = createCurrentState({
+				// Simulate `kill` and `updateMetadata` steps already executed
+				services: currentServices
+					.filter(({ serviceName }) => serviceName !== 'old')
+					.map((svc) => {
+						svc.releaseId = 2;
+						svc.commit = 'new-release';
+						return svc;
+					}),
+				networks: [DEFAULT_NETWORK],
+			});
+			const steps2 = await applicationManager.inferNextSteps(
+				c2.currentApps,
+				targetApps,
+				{
+					downloading: ['image-new'],
+					availableImages: c2.availableImages,
+					containerIdsByAppId: c2.containerIdsByAppId,
+				},
+			);
+			// Noop while service `new` is downloading
+			expectSteps('noop', steps2, 1);
+			// No start step should be generated until fetch completes
+			expectNoStep('start', steps2);
+			// No other steps
+			expect(steps2).to.have.length(1);
+
+			/**
+			 * Infer start step only after download completes
+			 */
+			const steps3 = await applicationManager.inferNextSteps(
+				c2.currentApps,
+				targetApps,
+				{
+					downloading: c1.downloading,
+					// Both images have been downloaded or had their metadata updated
+					availableImages: [
+						createImage({
+							name: 'image-main',
+							appId: 1,
+							serviceName: 'main',
+							commit: 'new-release',
+							releaseId: 2,
+						}),
+						createImage({
+							name: 'image-new',
+							appId: 1,
+							serviceName: 'new',
+							commit: 'new-release',
+							releaseId: 2,
+						}),
+					],
+					containerIdsByAppId: c1.containerIdsByAppId,
+				},
+			);
+			// Service `new` should be started
+			const [startStep] = expectSteps('start', steps3, 1);
+			expect(startStep)
+				.to.have.property('target')
+				.that.deep.includes({ serviceName: 'new' });
+			// No other steps
+			expect(steps3).to.have.length(1);
+		});
+
+		it('should not start target services until all downloads have completed with kill|delete-then-download', async () => {
+			const targetServices = [
+				await createService({
+					serviceName: 'one',
+					image: 'image-one',
+					labels: {
+						'io.balena.update.strategy': 'kill-then-download',
+					},
+				}),
+				await createService({
+					serviceName: 'two',
+					image: 'image-two',
+					labels: {
+						'io.balena.update.strategy': 'delete-then-download',
+					},
+				}),
+			];
+			const targetApps = createApps(
+				{
+					services: targetServices,
+				},
+				true,
+			);
+			const { currentApps, availableImages, containerIdsByAppId } =
+				createCurrentState({
+					services: [],
+					networks: [DEFAULT_NETWORK],
+				});
+
+			/**
+			 * Noop while both images are still downloading
+			 */
+			const steps = await applicationManager.inferNextSteps(
+				currentApps,
+				targetApps,
+				{
+					downloading: ['image-one', 'image-two'],
+					availableImages,
+					containerIdsByAppId,
+				},
+			);
+			expectSteps('noop', steps, 2);
+			// No other steps
+			expect(steps).to.have.length(2);
+
+			/**
+			 * Noop while one image is still downloading
+			 */
+			const steps2 = await applicationManager.inferNextSteps(
+				currentApps,
+				targetApps,
+				{
+					downloading: ['image-two'],
+					availableImages: [
+						createImage({
+							name: 'image-one',
+							serviceName: 'one',
+						}),
+					],
+					containerIdsByAppId,
+				},
+			);
+			expectSteps('noop', steps2, 1);
+			// No other steps
+			expect(steps2).to.have.length(1);
+
+			/**
+			 * Only start target services after both images downloaded
+			 */
+			const steps3 = await applicationManager.inferNextSteps(
+				currentApps,
+				targetApps,
+				{
+					downloading: [],
+					availableImages: [
+						createImage({
+							name: 'image-one',
+							serviceName: 'one',
+						}),
+						createImage({
+							name: 'image-two',
+							serviceName: 'two',
+						}),
+					],
+					containerIdsByAppId,
+				},
+			);
+			expectSteps('start', steps3, 2);
+			// No other steps
+			expect(steps3).to.have.length(2);
+		});
+	});
+
 	it('does not infer to kill a service with default strategy if a dependency is not downloaded', async () => {
 		const targetApps = createApps(
 			{