Merge pull request #1926 from balena-os/prevent-restart-policy-violation

Prevent restart policy violation

src/compose/app.ts

@@ -8,7 +8,6 @@ import Service from './service';
 
 import * as imageManager from './images';
 import type { Image } from './images';
-import * as applicationManager from './application-manager';
 import {
 	CompositionStep,
 	generateStep,
@@ -336,15 +335,13 @@ export class App {
 				return false;
 			}
 
-			// Check if we previously remember starting it
-			if (
-				applicationManager.containerStarted[serviceCurrent.containerId!] != null
-			) {
-				return false;
-			}
-
-			// If the config otherwise matches, then we should be running
-			return isEqualExceptForRunningState(serviceCurrent, serviceTarget);
+			// Only start a Service if we have never started it before and the service matches target!
+			// This is so the engine can handle the restart policy configured for the container.
+			return (
+				(serviceCurrent.status === 'Installing' ||
+					serviceCurrent.status === 'Installed') &&
+				isEqualExceptForRunningState(serviceCurrent, serviceTarget)
+			);
 		};
 
 		/**
@@ -360,9 +357,7 @@ export class App {
 			// check that we want to stop it, and that it isn't stopped
 			return (
 				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
+				serviceCurrent.status !== 'Stopped'
 			);
 		};
 

src/compose/application-manager.ts

@@ -74,9 +74,6 @@ export const router = (() => {
 	return $router;
 })();
 
-// We keep track of the containers we've started, to avoid triggering successive start
-// requests for a container
-export let containerStarted: { [containerId: string]: boolean } = {};
 export let fetchesInProgress = 0;
 export let timeSpentFetching = 0;
 
@@ -94,12 +91,6 @@ export function resetTimeSpentFetching(value: number = 0) {
 const actionExecutors = getExecutors({
 	lockFn: lock,
 	callbacks: {
-		containerStarted: (id: string) => {
-			containerStarted[id] = true;
-		},
-		containerKilled: (id: string) => {
-			delete containerStarted[id];
-		},
 		fetchStart: () => {
 			fetchesInProgress += 1;
 		},
@@ -347,9 +338,6 @@ export async function stopAll({ force = false, skipLock = false } = {}) {
 		services.map(async (s) => {
 			return lock(s.appId, { force, skipLock }, async () => {
 				await serviceManager.kill(s, { removeContainer: false, wait: true });
-				if (s.containerId) {
-					delete containerStarted[s.containerId];
-				}
 			});
 		}),
 	);

src/compose/composition-steps.ts

@@ -128,8 +128,6 @@ type LockingFn = (
 interface CompositionCallbacks {
 	// TODO: Once the entire codebase is typescript, change
 	// this to number
-	containerStarted: (containerId: string | null) => void;
-	containerKilled: (containerId: string | null) => void;
 	fetchStart: () => void;
 	fetchEnd: () => void;
 	fetchTime: (time: number) => void;
@@ -155,7 +153,6 @@ export function getExecutors(app: {
 						removeContainer: false,
 						wait,
 					});
-					app.callbacks.containerKilled(step.current.containerId);
 				},
 			);
 		},
@@ -168,7 +165,6 @@ export function getExecutors(app: {
 				},
 				async () => {
 					await serviceManager.kill(step.current);
-					app.callbacks.containerKilled(step.current.containerId);
 				},
 			);
 		},
@@ -201,15 +197,12 @@ export function getExecutors(app: {
 				},
 				async () => {
 					await serviceManager.kill(step.current, { wait: true });
-					app.callbacks.containerKilled(step.current.containerId);
-					const container = await serviceManager.start(step.target);
-					app.callbacks.containerStarted(container.id);
+					await serviceManager.start(step.target);
 				},
 			);
 		},
 		start: async (step) => {
-			const container = await serviceManager.start(step.target);
-			app.callbacks.containerStarted(container.id);
+			await serviceManager.start(step.target);
 		},
 		updateCommit: async (step) => {
 			await commitStore.upsertCommitForApp(step.appId, step.target);

test/src/compose/app.spec.ts

@@ -1,7 +1,6 @@
 import { expect } from 'chai';
 import * as sinon from 'sinon';
 import App from '../../../src/compose/app';
-import * as applicationManager from '../../../src/compose/application-manager';
 import {
 	CompositionStep,
 	CompositionStepAction,
@@ -127,17 +126,7 @@ describe('compose/app', () => {
 		sinon.stub(log, 'success');
 	});
 
-	beforeEach(() => {
-		// Cleanup application manager
-		// @ts-ignore
-		applicationManager.containerStarted = {};
-	});
-
 	after(() => {
-		// Cleanup application manager once more just in case
-		// @ts-ignore
-		applicationManager.containerStarted = {};
-
 		// Restore stubbed methods
 		sinon.restore();
 	});
@@ -780,11 +769,6 @@ describe('compose/app', () => {
 				],
 			});
 
-			// Mark this container as previously being started
-			// TODO: this is a circular dependency and is an implementation detail that should
-			// not be part of a test. NEEDS refactor
-			applicationManager.containerStarted['run_once'] = true;
-
 			// Now test that another start step is not added on this service
 			const target = createApp({
 				services: [
@@ -798,10 +782,6 @@ describe('compose/app', () => {
 
 			const steps = current.nextStepsForAppUpdate(defaultContext, target);
 			expectNoStep('start', steps);
-
-			// Cleanup application manager
-			// @ts-ignore
-			applicationManager.containerStarted = {};
 		});
 
 		it('should recreate a container if the target configuration changes', async () => {
@@ -929,12 +909,6 @@ describe('compose/app', () => {
 				networks: [defaultNetwork],
 			});
 
-			// We keep track of the containers that we've tried to start so that we
-			// dont spam start requests if the container hasn't started running
-			// TODO: this is a circular dependency and is an implementation detail that should
-			// not be part of a test. NEEDS refactor
-			applicationManager.containerStarted['dep-id'] = true;
-
 			// we should now see a start for the 'main' service...
 			const stepsToTarget = intermediate.nextStepsForAppUpdate(
 				{ ...contextWithImages, ...{ containerIds: { dep: 'dep-id' } } },
@@ -945,13 +919,9 @@ describe('compose/app', () => {
 			expect(startMainStep)
 				.to.have.property('target')
 				.that.deep.includes({ serviceName: 'main' });
-
-			// Reset the state of applicationManager
-			// @ts-ignore
-			applicationManager.containerStarted = {};
 		});
 
-		it('should create a start step when all that changes is a running state', async () => {
+		it('should not create a start step when all that changes is a running state', async () => {
 			const contextWithImages = {
 				...defaultContext,
 				...{
@@ -972,13 +942,10 @@ describe('compose/app', () => {
 				isTarget: true,
 			});
 
-			// now should see a 'start'
 			const steps = current.nextStepsForAppUpdate(contextWithImages, target);
 
-			const [startStep] = expectSteps('start', steps);
-			expect(startStep)
-				.to.have.property('target')
-				.that.deep.includes({ serviceName: 'main' });
+			// There should be no steps since the engine manages restart policy for stopped containers
+			expect(steps.length).to.equal(0);
 		});
 
 		it('should create a kill step when a service release has to be updated but the strategy is kill-then-download', async () => {

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

@@ -208,7 +208,7 @@ describe('compose/application-manager', () => {
 
 	// TODO: missing tests for getCurrentApps
 
-	it('infers a start step when all that changes is a running state', async () => {
+	it('should not infer a start step when all that changes is a running state', async () => {
 		const targetApps = createApps(
 			{
 				services: [await createService({ running: true, appId: 1 })],
@@ -226,7 +226,7 @@ describe('compose/application-manager', () => {
 			networks: [DEFAULT_NETWORK],
 		});
 
-		const [startStep] = await applicationManager.inferNextSteps(
+		const steps = await applicationManager.inferNextSteps(
 			currentApps,
 			targetApps,
 			{
@@ -236,10 +236,8 @@ describe('compose/application-manager', () => {
 			},
 		);
 
-		expect(startStep).to.have.property('action').that.equals('start');
-		expect(startStep)
-			.to.have.property('target')
-			.that.deep.includes({ serviceName: 'main' });
+		// There should be no steps since the engine manages restart policy for stopped containers
+		expect(steps.length).to.equal(0);
 	});
 
 	it('infers a kill step when a service has to be removed', async () => {