Merge pull request #1824 from balena-os/synchronize-kill

Wait for images to be ready before moving between releases

src/compose/app.ts

@@ -488,7 +488,9 @@ export class App {
 			// Either this is a new service, or the current one has already been killed
 			return this.generateFetchOrStartStep(
 				target!,
+				context.targetApp,
 				needsDownload,
+				context.availableImages,
 				context.networkPairs,
 				context.volumePairs,
 				context.servicePairs,
@@ -520,12 +522,13 @@ export class App {
 
 			const dependenciesMetForStart = this.dependenciesMetForServiceStart(
 				target,
+				context.targetApp,
+				context.availableImages,
 				context.networkPairs,
 				context.volumePairs,
 				context.servicePairs,
 			);
 			const dependenciesMetForKill = this.dependenciesMetForServiceKill(
-				target,
 				context.targetApp,
 				context.availableImages,
 				context.localMode,
@@ -591,7 +594,9 @@ export class App {
 
 	private generateFetchOrStartStep(
 		target: Service,
+		targetApp: App,
 		needsDownload: boolean,
+		availableImages: Image[],
 		networkPairs: Array<ChangingPair<Network>>,
 		volumePairs: Array<ChangingPair<Volume>>,
 		servicePairs: Array<ChangingPair<Service>>,
@@ -605,6 +610,8 @@ export class App {
 		} else if (
 			this.dependenciesMetForServiceStart(
 				target,
+				targetApp,
+				availableImages,
 				networkPairs,
 				volumePairs,
 				servicePairs,
@@ -618,6 +625,8 @@ export class App {
 	// TODO: support networks instead of only network mode
 	private dependenciesMetForServiceStart(
 		target: Service,
+		targetApp: App,
+		availableImages: Image[],
 		networkPairs: Array<ChangingPair<Network>>,
 		volumePairs: Array<ChangingPair<Volume>>,
 		servicePairs: Array<ChangingPair<Service>>,
@@ -626,7 +635,7 @@ export class App {
 		// different to a dependency which is in the servicePairs below, as these
 		// are services which are changing). We could have a dependency which is
 		// starting up, but is not yet running.
-		const depInstallingButNotRunning = _.some(this.services, (svc) => {
+		const depInstallingButNotRunning = _.some(targetApp.services, (svc) => {
 			if (target.dependsOn?.includes(svc.serviceName!)) {
 				if (!svc.config.running) {
 					return true;
@@ -664,16 +673,15 @@ export class App {
 			return false;
 		}
 
-		// everything is ready for the service to start...
+		// do not start until all images have been downloaded
-		return true;
+		return this.targetImagesReady(targetApp, availableImages);
 	}
 
 	// Unless the update strategy requires an early kill (i.e kill-then-download,
 	// delete-then-download), we only want to kill a service once the images for the
 	// services it depends on have been downloaded, so as to minimize downtime (but not
-	// block the killing too much, potentially causing a daedlock)
+	// block the killing too much, potentially causing a deadlock)
 	private dependenciesMetForServiceKill(
-		target: Service,
 		targetApp: App,
 		availableImages: Image[],
 		localMode: boolean,
@@ -685,26 +693,18 @@ export class App {
 			return true;
 		}
 
-		if (target.dependsOn != null) {
+		// Don't kill any services before all images have been downloaded
-			for (const dependency of target.dependsOn) {
+		return this.targetImagesReady(targetApp, availableImages);
-				const dependencyService = _.find(targetApp.services, {
+	}
-					serviceName: dependency,
+
-				});
+	private targetImagesReady(targetApp: App, availableImages: Image[]) {
-				if (
+		return targetApp.services.every((service) =>
-					!_.some(
+			availableImages.some(
-						availableImages,
+				(image) =>
-						(image) =>
+					image.dockerImageId === service.config.image ||
-							image.dockerImageId === dependencyService?.config.image ||
+					imageManager.isSameImage(image, { name: service.imageName! }),
-							imageManager.isSameImage(image, {
+			),
-								name: dependencyService?.imageName!,
+		);
-							}),
-					)
-				) {
-					return false;
-				}
-			}
-		}
-		return true;
 	}
 
 	public static async fromTargetState(

test/src/compose/app.spec.ts

@@ -1147,7 +1147,7 @@ describe('compose/app', () => {
 				.that.deep.includes({ serviceName: 'test' });
 		});
 
-		it('should not create a service when a network it depends on is not ready', async () => {
+		it('should not start a service when a network it depends on is not ready', async () => {
 			const current = createApp({ networks: [defaultNetwork] });
 			const target = createApp({
 				services: [await createService({ networks: ['test'], appId: 1 })],
@@ -1199,6 +1199,151 @@ describe('compose/app', () => {
 			const steps = current.nextStepsForAppUpdate(defaultContext, target);
 			expectSteps('kill', steps, 2);
 		});
+
+		it('should not infer a kill step with the default strategy before all target images have been downloaded', async () => {
+			const contextWithImages = {
+				...defaultContext,
+				...{
+					downloading: ['other-image-2'], // One of the images is being downloaded
+					availableImages: [
+						createImage({ appId: 1, name: 'main-image', serviceName: 'main' }),
+						createImage({
+							appId: 1,
+							name: 'other-image',
+							serviceName: 'other',
+						}),
+						createImage({
+							appId: 1,
+							name: 'main-image-2',
+							serviceName: 'main',
+						}),
+					],
+				},
+			};
+
+			const current = createApp({
+				services: [
+					await createService({
+						image: 'main-image',
+						appId: 1,
+						serviceName: 'main',
+						commit: 'old-release',
+					}),
+					await createService({
+						image: 'other-image',
+						appId: 1,
+						serviceName: 'other',
+						commit: 'old-release',
+					}),
+				],
+				networks: [defaultNetwork],
+			});
+			const target = createApp({
+				services: [
+					await createService({
+						image: 'main-image-2',
+						appId: 1,
+						serviceName: 'main',
+						commit: 'new-release',
+					}),
+					await createService({
+						image: 'other-image-2',
+						appId: 1,
+						serviceName: 'other',
+						commit: 'new-release',
+					}),
+				],
+				networks: [defaultNetwork],
+				isTarget: true,
+			});
+
+			// No kill steps should be generated
+			const steps = current.nextStepsForAppUpdate(contextWithImages, target);
+			expectNoStep('kill', steps);
+		});
+
+		it('should not infer a start step before all target images have been downloaded', async () => {
+			const contextWithImages = {
+				...defaultContext,
+				...{
+					downloading: ['other-image'], // One of the images is being downloaded
+					availableImages: [
+						createImage({ appId: 1, name: 'main-image', serviceName: 'main' }),
+					],
+				},
+			};
+
+			const current = createApp({
+				services: [],
+				networks: [defaultNetwork],
+			});
+			const target = createApp({
+				services: [
+					await createService({
+						image: 'main-image',
+						appId: 1,
+						serviceName: 'main',
+						commit: 'new-release',
+					}),
+					await createService({
+						image: 'other-image',
+						appId: 1,
+						serviceName: 'other',
+						commit: 'new-release',
+					}),
+				],
+				networks: [defaultNetwork],
+				isTarget: true,
+			});
+
+			// No kill steps should be generated
+			const steps = current.nextStepsForAppUpdate(contextWithImages, target);
+			expectNoStep('start', steps);
+		});
+
+		it('should infer a start step only when target images have been downloaded', async () => {
+			const contextWithImages = {
+				...defaultContext,
+				...{
+					downloading: [], // One of the images is being downloaded
+					availableImages: [
+						createImage({ appId: 1, name: 'main-image', serviceName: 'main' }),
+						createImage({
+							appId: 1,
+							name: 'other-image',
+							serviceName: 'other',
+						}),
+					],
+				},
+			};
+
+			const current = createApp({
+				services: [],
+				networks: [defaultNetwork],
+			});
+			const target = createApp({
+				services: [
+					await createService({
+						image: 'main-image',
+						appId: 1,
+						serviceName: 'main',
+						commit: 'new-release',
+					}),
+					await createService({
+						image: 'other-image',
+						appId: 1,
+						serviceName: 'other',
+						commit: 'new-release',
+					}),
+				],
+				networks: [defaultNetwork],
+				isTarget: true,
+			});
+
+			// No kill steps should be generated
+			const steps = current.nextStepsForAppUpdate(contextWithImages, target);
+			expectSteps('start', steps, 2);
+		});
 	});
 
 	describe('image state behavior', () => {