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

This pertains to a release update which removes a service in current state and adds a new service in target state, which the Supervisor previously didn't follow update strategies for starting / killing / fetching images for. Change-type: patch Signed-off-by: Christina Ying Wang <[email protected]>
balena-os · Dec 15, 2023 · 369c358 · 369c358
1 parent 4dccdff
commit 369c358
Show file tree

Hide file tree

Showing 3 changed files with 445 additions and 59 deletions.
diff --git a/src/compose/app.ts b/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, ...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,38 @@ 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);
+				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';
-			}
+			const strategy =
+				target == null
+					? getStrategyFromService(current)
+					: getStrategyFromService(target);
+
+			const dependenciesMetForStart = target
+				? this.dependenciesMetForServiceStart(
+						target,
+						context.targetApp,
+						context.availableImages,
+						context.networkPairs,
+						context.volumePairs,
+						context.servicePairs,
+				  )
+				: 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 +659,7 @@ export class App {
 		volumePairs: Array<ChangingPair<Volume>>,
 		servicePairs: Array<ChangingPair<Service>>,
 	): CompositionStep | undefined {
-		if (needsDownload) {
+		if (needsDownload && this.dependenciesMetForServiceFetch(servicePairs)) {
 			// We know the service name exists as it always does for targets
 			return generateStep('fetch', {
 				image: imageManager.imageFromService(target),
@@ -698,6 +679,27 @@ export class App {
 		}
 	}
 
+	private dependenciesMetForServiceFetch(
+		servicePairs: Array<ChangingPair<Service>>,
+	) {
+		// Current services should be killed before target
+		// service fetch depending on update strategy
+		const servicePairsWithCurrent = servicePairs.filter(
+			(pair) => pair.current != null,
+		);
+		for (const { current, target } of servicePairsWithCurrent) {
+			// Prefer target's update strategy if target service exists
+			const strategy = getStrategyFromService(target ?? current!);
+			if (
+				['kill-then-download', 'delete-then-download'].includes(strategy) &&
+				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(

diff --git a/src/compose/update-strategies.ts b/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,10 +19,10 @@ 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
@@ -34,14 +34,14 @@ export function getStepsFromStrategy(
 		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,
@@ -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;
+}