From 77333f1e110070f17f4bf95c1554075c94a35d63 Mon Sep 17 00:00:00 2001
From: Miguel Casqueira <miguel@balena.io>
Date: Wed, 21 Oct 2020 21:01:05 -0400
Subject: [PATCH] Fixed evaluating if updates are needed to reach target state

Closes: #1476
Change-type: patch
Signed-off-by: Miguel Casqueira <miguel@balena.io>
---
 src/compose/app.ts          | 94 +++++++++++++++++++++++++------------
 test/36-compose-app.spec.ts | 43 +++++++++++++++++
 2 files changed, 106 insertions(+), 31 deletions(-)

diff --git a/src/compose/app.ts b/src/compose/app.ts
index cdbaa2cb..d866f571 100644
--- a/src/compose/app.ts
+++ b/src/compose/app.ts
@@ -307,46 +307,78 @@ export class App {
 			}
 		}
 
-		const alreadyStarted = (serviceId: number) => {
-			const equalExceptForRunning = currentByServiceId[
-				serviceId
-			].isEqualExceptForRunningState(
-				targetByServiceId[serviceId],
-				containerIds,
-			);
-			if (!equalExceptForRunning) {
-				// We need to recreate the container, as the configuration has changed
+		/**
+		 * Checks that the config for the current and target services matches, ignoring their run state.
+		 * @param serviceCurrent
+		 * @param serviceTarget
+		 */
+		const isEqualExceptForRunningState = (
+			serviceCurrent: Service,
+			serviceTarget: Service,
+		) =>
+			serviceCurrent.isEqualExceptForRunningState(serviceTarget, containerIds);
+
+		/**
+		 * Checks if a service is running, if we tracked it as being started, if the config matches the desired config, and if we actually want it to ever be started.
+		 * @param serviceCurrent
+		 * @param serviceTarget
+		 */
+		const shouldBeStarted = (
+			serviceCurrent: Service,
+			serviceTarget: Service,
+		) => {
+			// If the target run state is stopped, or we are actually running, then we don't care about anything else
+			if (
+				serviceTarget.config.running === false ||
+				serviceCurrent.config.running === true
+			) {
 				return false;
 			}
 
-			if (targetByServiceId[serviceId].config.running) {
-				// If the container is already running, and we don't need to change it
-				// due to the config, we know it's already been started
-				return true;
+			// Check if we previously remember starting it
+			if (
+				applicationManager.containerStarted[serviceCurrent.containerId!] != null
+			) {
+				return false;
 			}
 
-			// We recently ran a start step for this container, it just hasn't
-			// started running yet
+			// If the config otherwise matches, then we should be running
+			return isEqualExceptForRunningState(serviceCurrent, serviceTarget);
+		};
+
+		/**
+		 * Checks if a service should be stopped and if we have tracked it as being stopped.
+		 *
+		 * @param serviceCurrent
+		 * @param serviceTarget
+		 */
+		const shouldBeStopped = (
+			serviceCurrent: Service,
+			serviceTarget: Service,
+		) => {
+			// check that we want to stop it, and that it isn't stopped
 			return (
-				applicationManager.containerStarted[
-					currentByServiceId[serviceId].containerId!
-				] != null
+				serviceTarget.config.running === false &&
+				// When we issue a stop step, we remove the containerId from this structure.
+				// We check if the container has been removed first, so that we can ensure we're not providing multiple stop steps.
+				applicationManager.containerStarted[serviceCurrent.containerId!] == null
 			);
 		};
 
-		const needUpdate = maybeUpdate.filter(
-			(serviceId) =>
-				!(
-					currentByServiceId[serviceId].isEqual(
-						targetByServiceId[serviceId],
-						containerIds,
-					) && alreadyStarted(serviceId)
-				),
-		);
-		const toBeUpdated = needUpdate.map((serviceId) => ({
-			current: currentByServiceId[serviceId],
-			target: targetByServiceId[serviceId],
-		}));
+		/**
+		 * Filter all the services which should be updated due to run state change, or config mismatch.
+		 */
+		const toBeUpdated = maybeUpdate
+			.map((serviceId) => ({
+				current: currentByServiceId[serviceId],
+				target: targetByServiceId[serviceId],
+			}))
+			.filter(
+				({ current: c, target: t }) =>
+					!isEqualExceptForRunningState(c, t) ||
+					shouldBeStarted(c, t) ||
+					shouldBeStopped(c, t),
+			);
 
 		return {
 			installPairs: toBeInstalled,
diff --git a/test/36-compose-app.spec.ts b/test/36-compose-app.spec.ts
index c58b0dff..fe85fadf 100644
--- a/test/36-compose-app.spec.ts
+++ b/test/36-compose-app.spec.ts
@@ -583,6 +583,49 @@ describe('compose/app', () => {
 		expectStep('stop', steps);
 	});
 
+	it('should not try to start a container which has exitted and has restart policy of no', async () => {
+		// Container is a "run once" type of service so it has exitted.
+		const current = createApp(
+			[
+				await createService(
+					{ restart: 'no', running: false },
+					1,
+					'main',
+					1,
+					1,
+					1,
+					{ containerId: 'run_once' },
+				),
+			],
+			[],
+			[],
+			false,
+		);
+
+		// Mark this container as previously being started
+		applicationManager.containerStarted['run_once'] = true;
+
+		// Now test that another start step is not added on this service
+		const target = createApp(
+			[
+				await createService(
+					{ restart: 'no', running: true },
+					1,
+					'main',
+					1,
+					1,
+					1,
+					{ containerId: 'run_once' },
+				),
+			],
+			[],
+			[],
+			true,
+		);
+		const steps = current.nextStepsForAppUpdate(defaultContext, target);
+		withSteps(steps).rejectStep('start');
+	});
+
 	it('should recreate a container if the target configuration changes', async () => {
 		const contextWithImages = {
 			...defaultContext,