From 3d43f7e3b367636f01ac9b6388c933bab5c896e5 Mon Sep 17 00:00:00 2001
From: Felipe Lalanne <1822826+pipex@users.noreply.github.com>
Date: Fri, 7 Apr 2023 11:45:09 -0400
Subject: [PATCH] Simplify doRestart and doPurge actions

The actions now work by passing an intermediate state to the state
engine.

- doPurge first removes the user app from the target state and passes
  that to the state engine for purging. Since intermediate state doesn't
  remove images, this will have the effect of basically re-installing
  the app.

- doRestart modifies the target state by first removing only the
  services from the current state but keeping volumes and networks. This
  has the same effect as before where services were stopped one by one

Change-type: patch
---
 src/compose/application-manager.ts          |  7 +-
 src/device-api/actions.ts                   | 71 +++++++++------------
 src/device-state.ts                         | 17 +++--
 test/integration/device-api/actions.spec.ts |  9 ++-
 4 files changed, 55 insertions(+), 49 deletions(-)

diff --git a/src/compose/application-manager.ts b/src/compose/application-manager.ts
index 4d7f7942..2294a563 100644
--- a/src/compose/application-manager.ts
+++ b/src/compose/application-manager.ts
@@ -123,6 +123,7 @@ export async function getRequiredSteps(
 	currentApps: InstancedAppState,
 	targetApps: InstancedAppState,
 	keepImages?: boolean,
+	keepVolumes?: boolean,
 ): Promise<CompositionStep[]> {
 	// get some required data
 	const [downloading, availableImages, { localMode, delta }] =
@@ -139,11 +140,15 @@ export async function getRequiredSteps(
 		keepImages = localMode;
 	}
 
+	if (keepVolumes == null) {
+		keepVolumes = localMode;
+	}
+
 	return await inferNextSteps(currentApps, targetApps, {
 		// Images are not removed while in local mode to avoid removing the user app images
 		keepImages,
 		// Volumes are not removed when stopping an app when going to local mode
-		keepVolumes: localMode,
+		keepVolumes,
 		delta,
 		downloading,
 		availableImages,
diff --git a/src/device-api/actions.ts b/src/device-api/actions.ts
index 757a7e7f..3c651a1a 100644
--- a/src/device-api/actions.ts
+++ b/src/device-api/actions.ts
@@ -1,4 +1,3 @@
-import * as Bluebird from 'bluebird';
 import * as _ from 'lodash';
 
 import { getGlobalApiKey, refreshKey } from '.';
@@ -10,8 +9,6 @@ import * as config from '../config';
 import * as hostConfig from '../host-config';
 import { App } from '../compose/app';
 import * as applicationManager from '../compose/application-manager';
-import * as serviceManager from '../compose/service-manager';
-import * as volumeManager from '../compose/volume-manager';
 import {
 	CompositionStepAction,
 	generateStep,
@@ -103,17 +100,26 @@ export const doRestart = async (appId: number, force: boolean = false) => {
 				`Application with ID ${appId} is not in the current state`,
 			);
 		}
-		const { services } = currentState.local.apps?.[appId];
-		applicationManager.clearTargetVolatileForServices(
-			services.map((svc) => svc.imageId),
-		);
+		const app = currentState.local.apps[appId];
+		const services = app.services;
+		app.services = [];
 
-		return deviceState.pausingApply(async () => {
-			for (const service of services) {
-				await serviceManager.kill(service, { wait: true });
-				await serviceManager.start(service);
-			}
-		});
+		return deviceState
+			.pausingApply(() =>
+				deviceState
+					.applyIntermediateTarget(currentState, {
+						skipLock: true,
+					})
+					.then(() => {
+						app.services = services;
+						return deviceState.applyIntermediateTarget(currentState, {
+							skipLock: true,
+						});
+					}),
+			)
+			.finally(() => {
+				deviceState.triggerApplyTarget();
+			});
 	});
 };
 
@@ -218,45 +224,28 @@ export const doPurge = async (appId: number, force: boolean = false) => {
 			);
 		}
 
-		const app = currentState.local.apps?.[appId];
-		/**
-		 * 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)
-		 */
-		const clonedState = safeStateClone(currentState);
-		// Set services & volumes as empty to be applied as intermediate state
-		app.services = [];
-		app.volumes = [];
+		const app = currentState.local.apps[appId];
 
-		applicationManager.setIsApplyingIntermediate(true);
+		// Delete the app from the current state
+		delete currentState.local.apps[appId];
 
 		return deviceState
 			.pausingApply(() =>
 				deviceState
-					.applyIntermediateTarget(currentState, { skipLock: true })
-					.then(() => {
-						// Explicitly remove volumes because application-manager won't
-						// remove any volumes that are part of an active application.
-						return Bluebird.each(volumeManager.getAllByAppId(appId), (vol) =>
-							vol.remove(),
-						);
+					.applyIntermediateTarget(currentState, {
+						skipLock: true,
+						// Purposely tell the apply function to delete volumes so they can get
+						// deleted even in local mode
+						keepVolumes: false,
 					})
 					.then(() => {
-						return deviceState.applyIntermediateTarget(clonedState, {
+						currentState.local.apps[appId] = app;
+						return deviceState.applyIntermediateTarget(currentState, {
 							skipLock: true,
 						});
 					}),
 			)
 			.finally(() => {
-				applicationManager.setIsApplyingIntermediate(false);
 				deviceState.triggerApplyTarget();
 			});
 	})
@@ -264,8 +253,6 @@ export const doPurge = async (appId: number, force: boolean = false) => {
 			logger.logSystemMessage('Purged data', { appId }, 'Purge data success'),
 		)
 		.catch((err) => {
-			applicationManager.setIsApplyingIntermediate(false);
-
 			logger.logSystemMessage(
 				`Error purging data: ${err}`,
 				{ appId, error: err },
diff --git a/src/device-state.ts b/src/device-state.ts
index 29a053de..1cc6f772 100644
--- a/src/device-state.ts
+++ b/src/device-state.ts
@@ -671,6 +671,7 @@ export const applyTarget = async ({
 	skipLock = false,
 	nextDelay = 200,
 	retryCount = 0,
+	keepVolumes = undefined as boolean | undefined,
 } = {}) => {
 	if (!intermediate) {
 		await applyBlocker;
@@ -705,6 +706,7 @@ export const applyTarget = async ({
 				// if not applying intermediate, we let getRequired steps set
 				// the value
 				intermediate || undefined,
+				keepVolumes,
 			);
 
 			if (_.isEmpty(appSteps)) {
@@ -762,6 +764,7 @@ export const applyTarget = async ({
 				skipLock,
 				nextDelay,
 				retryCount,
+				keepVolumes,
 			});
 		} catch (e: any) {
 			if (e instanceof UpdatesLockedError) {
@@ -864,11 +867,17 @@ export function triggerApplyTarget({
 
 export async function applyIntermediateTarget(
 	intermediate: InstancedDeviceState,
-	{ force = false, skipLock = false } = {},
+	{
+		force = false,
+		skipLock = false,
+		keepVolumes = undefined as boolean | undefined,
+	} = {},
 ) {
 	// TODO: Make sure we don't accidentally overwrite this
 	intermediateTarget = intermediate;
-	return applyTarget({ intermediate: true, force, skipLock }).then(() => {
-		intermediateTarget = null;
-	});
+	return applyTarget({ intermediate: true, force, skipLock, keepVolumes }).then(
+		() => {
+			intermediateTarget = null;
+		},
+	);
 }
diff --git a/test/integration/device-api/actions.spec.ts b/test/integration/device-api/actions.spec.ts
index 4818ebc8..bef070e5 100644
--- a/test/integration/device-api/actions.spec.ts
+++ b/test/integration/device-api/actions.spec.ts
@@ -237,7 +237,10 @@ describe('manages application lifecycle', () => {
 				containers.map((ctn) => ctn.State.StartedAt),
 			);
 
-			await actions.doRestart(APP_ID);
+			await request(BALENA_SUPERVISOR_ADDRESS)
+				.post(`/v1/restart`)
+				.set('Content-Type', 'application/json')
+				.send(JSON.stringify({ appId: APP_ID }));
 
 			const restartedContainers = await waitForSetup(
 				targetState,
@@ -503,7 +506,9 @@ describe('manages application lifecycle', () => {
 				containers.map((ctn) => ctn.State.StartedAt),
 			);
 
-			await actions.doRestart(APP_ID);
+			await request(BALENA_SUPERVISOR_ADDRESS)
+				.post(`/v2/applications/${APP_ID}/restart`)
+				.set('Content-Type', 'application/json');
 
 			const restartedContainers = await waitForSetup(
 				targetState,