Merge pull request #1635 from balena-os/patch/purge-image-removal-fix

Prevent unintended image removal when calling purge endpoints to remove volumes

src/compose/application-manager.ts

@@ -78,6 +78,13 @@ export let containerStarted: { [containerId: string]: boolean } = {};
 export let fetchesInProgress = 0;
 export let timeSpentFetching = 0;
 
+// In the case of intermediate target apply, toggle to true to avoid unintended image deletion
+let isApplyingIntermediate = false;
+
+export function setIsApplyingIntermediate(value: boolean = false) {
+	isApplyingIntermediate = value;
+}
+
 export function resetTimeSpentFetching(value: number = 0) {
 	timeSpentFetching = value;
 }
@@ -206,12 +213,15 @@ export async function getRequiredSteps(
 			steps.push({ action: 'ensureSupervisorNetwork' });
 		}
 	} else {
-		if (!localMode && downloading.length === 0) {
+		if (!localMode && downloading.length === 0 && !isApplyingIntermediate) {
+			// Avoid cleaning up dangling images while purging
 			if (cleanupNeeded) {
 				steps.push({ action: 'cleanup' });
 			}
 
-			// Detect any images which must be saved/removed
+			// Detect any images which must be saved/removed, except when purging,
+			// as we only want to remove containers, remove volumes, create volumes
+			// anew, and start containers without images being removed.
 			steps = steps.concat(
 				saveAndRemoveImages(
 					currentApps,
@@ -325,6 +335,7 @@ export async function getRequiredSteps(
 			steps,
 		),
 	);
+
 	return steps;
 }
 
@@ -717,7 +728,9 @@ function saveAndRemoveImages(
 		(image) => {
 			const notUsedForDelta = !_.includes(deltaSources, image.name);
 			const notUsedByProxyvisor = !_.some(proxyvisorImages, (proxyvisorImage) =>
-				imageManager.isSameImage(image, { name: proxyvisorImage }),
+				imageManager.isSameImage(image, {
+					name: proxyvisorImage,
+				}),
 			);
 			return notUsedForDelta && notUsedByProxyvisor;
 		},

src/device-api/common.d.ts

@@ -1,29 +0,0 @@
-import { Service } from '../compose/service';
-import { InstancedDeviceState } from '../types/state';
-
-export interface ServiceAction {
-	action: string;
-	serviceId: number;
-	current: Service;
-	target: Service;
-	options: any;
-}
-
-declare function doRestart(appId: number, force: boolean): Promise<void>;
-
-declare function doPurge(appId: number, force: boolean): Promise<void>;
-
-declare function serviceAction(
-	action: string,
-	serviceId: number,
-	current: Service,
-	target?: Service,
-	options?: any,
-): ServiceAction;
-
-declare function safeStateClone(
-	targetState: InstancedDeviceState,
-): // Use an any here, because it's not an InstancedDeviceState,
-// and it's also not the exact same type as the API serves from
-// state endpoint (more details in the function)
-Dictionary<any>;

src/device-api/common.js

@@ -57,13 +57,29 @@ export async function doPurge(appId, force) {
 				throw new Error(appNotFoundMessage);
 			}
 
-			const app = allApps[appId];
+			const clonedState = safeStateClone(currentState);
+			/**
+			 * With multi-container, Docker adds an invalid network alias equal to the current containerId
+			 * to that service's network configs when starting a service. Thus when reapplying intermediateState
+			 * after purging, use a cloned state instance which automatically filters out invalid network aliases.
+			 *
+			 * This will prevent error logs like the following:
+			 * https://gist.github.com/cywang117/84f9cd4e6a9641dbed530c94e1172f1d#file-logs-sh-L58
+			 *
+			 * When networks do not match because of their aliases, services are killed and recreated
+			 * an additional time which is unnecessary. Filtering prevents this additional restart BUT
+			 * it is a stopgap measure until we can keep containerId network aliases from being stored
+			 * in state's service config objects (TODO)
+			 *
+			 * See https://github.com/balena-os/balena-supervisor/blob/master/src/device-api/common.js#L160-L180
+			 * for a more in-depth explanation of why aliases need to be filtered out.
+			 */
 
-			const currentServices = app.services;
+			// After cloning, set services & volumes as empty to be applied as intermediateTargetState
-			const currentVolumes = app.volumes;
+			allApps[appId].services = [];
+			allApps[appId].volumes = {};
 
-			app.services = [];
+			applicationManager.setIsApplyingIntermediate(true);
-			app.volumes = {};
 
 			return deviceState
 				.pausingApply(() =>
@@ -79,20 +95,23 @@ export async function doPurge(appId, force) {
 							);
 						})
 						.then(() => {
-							app.services = currentServices;
+							return deviceState.applyIntermediateTarget(clonedState, {
-							app.volumes = currentVolumes;
-							return deviceState.applyIntermediateTarget(currentState, {
 								skipLock: true,
 							});
 						}),
 				)
-				.finally(() => deviceState.triggerApplyTarget());
+				.finally(() => {
+					applicationManager.setIsApplyingIntermediate(false);
+					deviceState.triggerApplyTarget();
+				});
 		}),
 	)
 		.then(() =>
 			logger.logSystemMessage('Purged data', { appId }, 'Purge data success'),
 		)
 		.catch((err) => {
+			applicationManager.setIsApplyingIntermediate(false);
+
 			logger.logSystemMessage(
 				`Error purging data: ${err}`,
 				{ appId, error: err },
@@ -109,6 +128,11 @@ export function serviceAction(action, serviceId, current, target, options) {
 	return { action, serviceId, current, target, options };
 }
 
+/**
+ * This doesn't truly return an InstancedDeviceState, but it's close enough to mostly work where it's used
+ *
+ * @returns { import('../types/state').InstancedDeviceState }
+ */
 export function safeStateClone(targetState) {
 	// We avoid using cloneDeep here, as the class
 	// instances can cause a maximum call stack exceeded
@@ -122,6 +146,7 @@ export function safeStateClone(targetState) {
 	// thing to do would be to represent the input with
 	// io-ts and make sure the below conforms to it
 
+	/** @type { any } */
 	const cloned = {
 		local: {
 			config: {},

test/36-compose-app.spec.ts

@@ -1079,6 +1079,7 @@ describe('compose/app', () => {
 		const steps = current.nextStepsForAppUpdate(defaultContext, target);
 		withSteps(steps).expectStep('kill').to.have.length(2);
 	});
+
 	it('should not create a service when a network it depends on is not ready', async () => {
 		const current = createApp([], [defaultNetwork], [], false);
 		const target = createApp(
@@ -1092,4 +1093,93 @@ describe('compose/app', () => {
 		withSteps(steps).expectStep('createNetwork');
 		withSteps(steps).rejectStep('start');
 	}); // no create service, is create network
+
+	describe('Intermediate state apply support', () => {
+		beforeEach(() => {
+			appMock.mockSupervisorNetwork(true);
+			appMock.mockManagers([], [], []);
+			appMock.mockImages([], false, []);
+
+			applicationManager.setIsApplyingIntermediate(true);
+		});
+		afterEach(() => {
+			appMock.unmockAll();
+			applicationManager.setIsApplyingIntermediate(false);
+		});
+
+		it('should generate the correct step sequence for a volume purge request', async () => {
+			const service = await createService({
+				volumes: ['db-volume'],
+				image: 'test-image',
+			});
+			const volume = Volume.fromComposeObject('db-volume', service.appId, {});
+			const contextWithImages = {
+				...defaultContext,
+				...{
+					availableImages: [
+						{
+							appId: service.appId,
+							dependent: 0,
+							imageId: service.imageId,
+							releaseId: service.releaseId,
+							serviceId: service.serviceId,
+							name: 'test-image',
+							serviceName: service.serviceName,
+						} as Image,
+					],
+				},
+			};
+			// Temporarily set target services & volumes to empty, as in doPurge
+			let intermediateTarget = createApp([], [defaultNetwork], [], true);
+
+			// Generate current with one service & one volume
+			const current = createApp([service], [defaultNetwork], [volume], false);
+
+			// Step 1: kill
+			let steps = current.nextStepsForAppUpdate(
+				contextWithImages,
+				intermediateTarget,
+			);
+			withSteps(steps)
+				.expectStep('kill')
+				.forCurrent(service.serviceName as ServicePredicate);
+			expect(steps).to.have.length(1);
+
+			// Step 2: noop (service is stopping)
+			service.status = 'Stopping';
+			steps = current.nextStepsForAppUpdate(
+				contextWithImages,
+				intermediateTarget,
+			);
+			expectStep('noop', steps);
+			expect(steps).to.have.length(1);
+
+			// No steps, simulate container removal & explicit volume removal as in doPurge
+			current.services = [];
+			steps = current.nextStepsForAppUpdate(
+				contextWithImages,
+				intermediateTarget,
+			);
+			expect(steps).to.have.length(0);
+			current.volumes = {};
+
+			// Step 3: start & createVolume
+			service.status = 'Running';
+			intermediateTarget = createApp(
+				[service],
+				[defaultNetwork],
+				[volume],
+				true,
+			);
+			steps = current.nextStepsForAppUpdate(
+				contextWithImages,
+				intermediateTarget,
+			);
+			expect(steps).to.have.length(2);
+			withSteps(steps)
+				.expectStep('start')
+				.forTarget(service.serviceName as ServicePredicate);
+			expectStep('createVolume', steps);
+		});
+	});
 });