Merge pull request #2349 from balena-os/network-remove

Fix engine deadlock on network+service change

src/compose/app.ts

@@ -379,9 +379,9 @@ class AppImpl implements App {
 				);
 			}
 		}
-		const toBeRemoved = _(currentServiceNames)
+
+		const maybeRemove = _(currentServiceNames)
 			.difference(targetServiceNames)
-			.union(dependentServices.map((s) => s.serviceName))
 			.map((id) => ({ current: currentByServiceName[id] }))
 			.value();
 
@@ -409,7 +409,7 @@ class AppImpl implements App {
 					currentByServiceName[serviceName],
 				);
 				for (const service of otherContainers) {
-					toBeRemoved.push({ current: service });
+					maybeRemove.push({ current: service });
 				}
 			} else {
 				currentByServiceName[serviceName] = currentServiceContainers[0];
@@ -489,6 +489,33 @@ class AppImpl implements App {
 			);
 		};
 
+		/**
+		 * Services with changing configuration will have their containers recreated
+		 */
+		const toBeRecreated = maybeUpdate
+			.map((serviceName) => ({
+				current: currentByServiceName[serviceName],
+				target: targetByServiceName[serviceName],
+			}))
+			.filter(
+				({ current: c, target: t }) => !isEqualExceptForRunningState(c, t),
+			);
+
+		/**
+		 * Services that depend on network changes should be added to the removal
+		 * list if they are not being recreated already
+		 */
+		const toBeRemoved = maybeRemove.concat(
+			dependentServices
+				.filter(
+					(s) =>
+						!toBeRecreated.some(
+							({ current: c }) => c.serviceName === s.serviceName,
+						),
+				)
+				.map((s) => ({ current: s })),
+		);
+
 		/**
 		 * Checks if a service is destined for removal due to a network or volume change
 		 */

test/integration/.mocharc.js

@@ -10,5 +10,5 @@ module.exports = {
 		'test/lib/chai.ts',
 		'test/lib/mocha-hooks.ts',
 	],
-	timeout: '30000',
+	timeout: '60000',
 };

test/integration/state-engine.spec.ts

@@ -380,4 +380,115 @@ describe('state engine', () => {
 				Options: {},
 			});
 	});
+
+	it('updates an app with two services with a network removal', async () => {
+		await setTargetState({
+			config: {},
+			apps: {
+				'123': {
+					name: 'test-app',
+					commit: 'deadbeef',
+					releaseId: 1,
+					services: {
+						'1': {
+							image: 'alpine:3.18',
+							imageId: 11,
+							serviceName: 'one',
+							restart: 'unless-stopped',
+							running: true,
+							command: 'sleep infinity',
+							stop_signal: 'SIGKILL',
+							labels: {},
+							environment: {},
+							networks: ['balena'],
+						},
+						'2': {
+							image: 'ubuntu:focal',
+							imageId: 12,
+							serviceName: 'two',
+							restart: 'unless-stopped',
+							running: true,
+							command: 'sleep infinity',
+							labels: {},
+							environment: {},
+							network_mode: 'host',
+						},
+					},
+					networks: {
+						balena: {},
+					},
+					volumes: {},
+				},
+			},
+		});
+
+		const state = await getCurrentState();
+		expect(
+			state.apps['123'].services.map((s: any) => s.serviceName),
+		).to.deep.equal(['one', 'two']);
+
+		const containers = await docker.listContainers();
+		expect(
+			containers.map(({ Names, State }) => ({ Name: Names[0], State })),
+		).to.have.deep.members([
+			{ Name: '/one_11_1_deadbeef', State: 'running' },
+			{ Name: '/two_12_1_deadbeef', State: 'running' },
+		]);
+		const containerIds = containers.map(({ Id }) => Id);
+		await expect(docker.getNetwork('123_balena').inspect()).to.not.be.rejected;
+
+		await setTargetState({
+			config: {},
+			apps: {
+				'123': {
+					name: 'test-app',
+					commit: 'deadca1f',
+					releaseId: 2,
+					services: {
+						'1': {
+							image: 'alpine:latest',
+							imageId: 21,
+							serviceName: 'one',
+							restart: 'unless-stopped',
+							running: true,
+							command: 'sleep infinity',
+							stop_signal: 'SIGKILL',
+							networks: ['default'],
+							labels: {},
+							environment: {},
+						},
+						'2': {
+							image: 'ubuntu:latest',
+							imageId: 22,
+							serviceName: 'two',
+							restart: 'unless-stopped',
+							running: true,
+							command: 'sh -c "echo two && sleep infinity"',
+							stop_signal: 'SIGKILL',
+							network_mode: 'host',
+							labels: {},
+							environment: {},
+						},
+					},
+					networks: {},
+					volumes: {},
+				},
+			},
+		});
+
+		const updatedContainers = await docker.listContainers();
+		expect(
+			updatedContainers.map(({ Names, State }) => ({ Name: Names[0], State })),
+		).to.have.deep.members([
+			{ Name: '/one_21_2_deadca1f', State: 'running' },
+			{ Name: '/two_22_2_deadca1f', State: 'running' },
+		]);
+
+		// Container ids must have changed
+		expect(updatedContainers.map(({ Id }) => Id)).to.not.have.members(
+			containerIds,
+		);
+
+		await expect(docker.getNetwork('123_balena').inspect()).to.be.rejected;
+	});
 });

test/unit/compose/app.spec.ts

@@ -692,6 +692,77 @@ describe('compose/app', () => {
 			expectNoStep('removeNetwork', steps2);
 		});
 
+		it('should perform fetch steps first even for services that reference deleted networks', async () => {
+			const current = createApp({
+				appUuid: 'deadbeef',
+				services: [
+					await createService({
+						appId: 1,
+						appUuid: 'deadbeef',
+						image: 'test-image',
+						serviceName: 'test',
+						composition: { networks: ['test-network'] },
+					}),
+					await createService({
+						appId: 1,
+						appUuid: 'deadbeef',
+						image: 'other-image',
+						serviceName: 'other',
+						composition: { network_mode: 'host' },
+					}),
+				],
+				networks: [
+					Network.fromComposeObject('test-network', 1, 'deadbeef', {}),
+					DEFAULT_NETWORK,
+				],
+			});
+			const target = createApp({
+				appUuid: 'deadbeef',
+				services: [
+					await createService({
+						appId: 1,
+						appUuid: 'deadbeef',
+						serviceName: 'test',
+						image: 'new-test-image',
+						commit: 'new-commit',
+					}),
+					await createService({
+						appId: 1,
+						appUuid: 'deadbeef',
+						serviceName: 'other',
+						image: 'new-other-image',
+						commit: 'new-commit',
+						composition: { network_mode: 'host' },
+					}),
+				],
+				networks: [],
+				isTarget: true,
+			});
+
+			const availableImages = [
+				createImage({
+					appUuid: 'deadbeef',
+					name: 'test-image',
+					serviceName: 'test',
+				}),
+				createImage({
+					appUuid: 'deadbeef',
+					name: 'other-image',
+					serviceName: 'other',
+				}),
+			];
+			// Take lock first
+			const steps = current.nextStepsForAppUpdate(
+				{ ...defaultContext, availableImages },
+				target,
+			);
+
+			const fetchSteps = expectSteps('fetch', steps, 2);
+			expect(fetchSteps.map((s) => (s as any).image.name)).to.have.deep.members(
+				['new-test-image', 'new-other-image'],
+			);
+		});
+
 		it('should kill dependencies of networks before changing config', async () => {
 			const current = createApp({
 				services: [