Merge pull request #1826 from balena-os/delete-then-download

Fix `delete-then-download` strategy

src/compose/application-manager.ts

@@ -680,9 +680,9 @@ function saveAndRemoveImages(
 				}) ?? _.find(availableImages, { dockerImageId: svc.config.image }),
 		),
 	) as imageManager.Image[];
-	const targetImages = _.flatMap(target, (app) =>
-		_.map(app.services, imageForService),
-	);
+
+	const targetServices = Object.values(target).flatMap((app) => app.services);
+	const targetImages = targetServices.map(imageForService);
 
 	const availableAndUnused = _.filter(
 		availableWithoutIds,
@@ -741,32 +741,35 @@ function saveAndRemoveImages(
 		});
 	}
 
-	const deltaSources = _.map(imagesToDownload, (image) => {
-		return bestDeltaSource(image, availableImages);
-	});
+	// Find images that will be be used as delta sources. Any existing image for the
+	// same app service is considered a delta source unless the target service has set
+	// the `delete-then-download` strategy
+	const deltaSources = imagesToDownload
+		.filter(
+			(img) =>
+				// We don't need to look for delta sources for delete-then-download
+				// services
+				!targetServices.some(
+					(svc) =>
+						imageManager.isSameImage(img, imageForService(svc)) &&
+						svc.config.labels['io.balena.update.strategy'] ===
+							'delete-then-download',
+				),
+		)
+		.map((img) => bestDeltaSource(img, availableImages))
+		.filter((img) => img != null);
+
 	const proxyvisorImages = proxyvisor.imagesInUse(current, target);
 
-	const potentialDeleteThenDownload = _(current)
-		.flatMap((app) => _.values(app.services))
-		.filter(
-			(svc) =>
-				svc.config.labels['io.balena.update.strategy'] ===
-					'delete-then-download' && svc.status === 'Stopped',
-		)
-		.value();
-
-	const imagesToRemove = _.filter(
-		availableAndUnused.concat(potentialDeleteThenDownload.map(imageForService)),
-		(image) => {
-			const notUsedForDelta = !_.includes(deltaSources, image.name);
-			const notUsedByProxyvisor = !_.some(proxyvisorImages, (proxyvisorImage) =>
-				imageManager.isSameImage(image, {
-					name: proxyvisorImage,
-				}),
-			);
-			return notUsedForDelta && notUsedByProxyvisor;
-		},
-	);
+	const imagesToRemove = availableAndUnused.filter((image) => {
+		const notUsedForDelta = !deltaSources.includes(image.name);
+		const notUsedByProxyvisor = !proxyvisorImages.some((proxyvisorImage) =>
+			imageManager.isSameImage(image, {
+				name: proxyvisorImage,
+			}),
+		);
+		return notUsedForDelta && notUsedByProxyvisor;
+	});
 
 	return imagesToSave
 		.map((image) => ({ action: 'saveImage', image } as CompositionStep))

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

@@ -389,6 +389,110 @@ describe('compose/application-manager', () => {
 			.that.deep.includes({ serviceName: 'main' });
 	});
 
+	it('infers a kill step when a service has to be updated but the strategy is delete-then-download', async () => {
+		const labels = {
+			'io.balena.update.strategy': 'delete-then-download',
+		};
+		const targetApps = createApps(
+			{
+				services: [
+					await createService({
+						image: 'image-new',
+						labels,
+						appId: 1,
+						commit: 'new-release',
+					}),
+				],
+				networks: [DEFAULT_NETWORK],
+			},
+			true,
+		);
+		const {
+			currentApps,
+			availableImages,
+			downloading,
+			containerIdsByAppId,
+		} = createCurrentState({
+			services: [
+				await createService({
+					image: 'image-old',
+					labels,
+					appId: 1,
+					commit: 'old-release',
+				}),
+			],
+			networks: [DEFAULT_NETWORK],
+		});
+
+		const [killStep] = await applicationManager.inferNextSteps(
+			currentApps,
+			targetApps,
+			{
+				downloading,
+				availableImages,
+				containerIdsByAppId,
+			},
+		);
+
+		expect(killStep).to.have.property('action').that.equals('kill');
+		expect(killStep)
+			.to.have.property('current')
+			.that.deep.includes({ serviceName: 'main' });
+	});
+
+	it('infers a remove step when the current service has stopped and the strategy is delete-then-download', async () => {
+		const labels = {
+			'io.balena.update.strategy': 'delete-then-download',
+		};
+		const targetApps = createApps(
+			{
+				services: [
+					await createService({
+						image: 'image-new',
+						labels,
+						appId: 1,
+						serviceName: 'main',
+						commit: 'new-release',
+					}),
+				],
+			},
+			true,
+		);
+		const {
+			currentApps,
+			availableImages,
+			downloading,
+			containerIdsByAppId,
+		} = createCurrentState({
+			services: [],
+			images: [
+				createImage({
+					appId: 1,
+					name: 'image-old',
+					serviceName: 'main',
+					dockerImageId: 'image-old-id',
+				}),
+			],
+			networks: [DEFAULT_NETWORK],
+		});
+
+		const [removeImage] = await applicationManager.inferNextSteps(
+			currentApps,
+			targetApps,
+			{
+				downloading,
+				availableImages,
+				containerIdsByAppId,
+			},
+		);
+
+		// First we should see a kill
+		expect(removeImage).to.have.property('action').that.equals('removeImage');
+		expect(removeImage)
+			.to.have.property('image')
+			.that.deep.includes({ name: 'image-old' });
+	});
+
 	it('does not infer to kill a service with default strategy if a dependency is not downloaded', async () => {
 		const targetApps = createApps(
 			{

tsconfig.json

@@ -9,7 +9,7 @@
 		"inlineSourceMap": true,
 		"outDir": "./build/",
 		"skipLibCheck": true,
-		"lib": ["es6"],
+		"lib": ["es2019"],
 		"resolveJsonModule": true,
 		"allowJs": true
 	},