Merge pull request #2161 from balena-os/network-plus-service-bug

Fix device state not applied when a network change happens during the update

src/compose/app.ts

@@ -150,7 +150,7 @@ export class App {
 				.map((pair) =>
 					this.generateStepsForService(pair, {
 						...state,
-						servicePairs: installPairs.concat(updatePairs),
+						servicePairs,
 						targetApp: target,
 						networkPairs: networkChanges,
 						volumePairs: volumeChanges,
@@ -528,10 +528,7 @@ export class App {
 			return generateStep('noop', {});
 		}
 
-		if (target && current?.isEqualConfig(target, context.containerIds)) {
-			// we're only starting/stopping a service
-			return this.generateContainerStep(current, target);
-		} else if (current == null) {
+		if (current == null) {
 			// Either this is a new service, or the current one has already been killed
 			return this.generateFetchOrStartStep(
 				target!,
@@ -548,12 +545,21 @@ export class App {
 					'An empty changing pair passed to generateStepsForService',
 				);
 			}
+
 			const needsSpecialKill = this.serviceHasNetworkOrVolume(
 				current,
 				context.networkPairs,
 				context.volumePairs,
 			);
 
+			if (
+				!needsSpecialKill &&
+				current.isEqualConfig(target, context.containerIds)
+			) {
+				// we're only starting/stopping a service
+				return this.generateContainerStep(current, target);
+			}
+
 			let strategy =
 				checkString(target.config.labels['io.balena.update.strategy']) || '';
 			const validStrategies = [
@@ -601,7 +607,7 @@ export class App {
 			service.status !== 'Stopping' &&
 			!_.some(
 				changingServices,
-				({ current }) => current?.serviceName !== service.serviceName,
+				({ current }) => current?.serviceName === service.serviceName,
 			)
 		) {
 			return [generateStep('kill', { current: service })];

src/lib/docker-utils.ts

@@ -247,13 +247,18 @@ export async function fetchImageWithProgress(
 ): Promise<string> {
 	const { registry } = await dockerToolbelt.getRegistryAndName(image);
 
-	const dockerOpts = {
-		authconfig: {
-			username: `d_${uuid}`,
-			password: currentApiKey,
-			serverAddress: registry,
-		},
-	};
+	const dockerOpts =
+		// If no registry is specified, we assume the image is a public
+		// image on the default engine registry, and we don't need to pass any auth
+		registry != null
+			? {
+					authconfig: {
+						username: `d_${uuid}`,
+						password: currentApiKey,
+						serverAddress: registry,
+					},
+			  }
+			: {};
 
 	await dockerProgress.pull(image, onProgress, dockerOpts);
 	return (await docker.getImage(image).inspect()).Id;

src/lib/legacy.ts

@@ -234,7 +234,10 @@ export type TargetAppsV2 = {
 	};
 };
 
-type TargetStateV2 = {
+/**
+ * @deprecated exported only for testing
+ */
+export type TargetStateV2 = {
 	local: {
 		name: string;
 		config: { [name: string]: string };

test/integration/state-engine.spec.ts

@@ -0,0 +1,370 @@
+import { expect } from 'chai';
+import * as Docker from 'dockerode';
+import { TargetStateV2 } from '~/lib/legacy';
+import * as request from 'supertest';
+import { setTimeout as delay } from 'timers/promises';
+
+const BALENA_SUPERVISOR_ADDRESS =
+	process.env.BALENA_SUPERVISOR_ADDRESS || 'http://balena-supervisor:48484';
+
+const getCurrentState = async () =>
+	await request(BALENA_SUPERVISOR_ADDRESS)
+		.get('/v2/local/target-state')
+		.expect(200)
+		.then(({ body }) => body.state.local);
+
+const setTargetState = async (
+	target: Omit<TargetStateV2['local'], 'name'>,
+	timeout = 0,
+) => {
+	const { name, config } = await getCurrentState();
+	const targetState = {
+		local: {
+			name,
+			config,
+			apps: target.apps,
+		},
+	};
+
+	await request(BALENA_SUPERVISOR_ADDRESS)
+		.post('/v2/local/target-state')
+		.set('Content-Type', 'application/json')
+		.send(JSON.stringify(targetState))
+		.expect(200);
+
+	return new Promise(async (resolve, reject) => {
+		const timer =
+			timeout > 0
+				? setTimeout(
+						() =>
+							reject(
+								new Error(
+									`Timeout while waiting for the target state to be applied`,
+								),
+							),
+						timeout,
+				  )
+				: undefined;
+
+		while (true) {
+			const status = await getStatus();
+			if (status.appState === 'applied') {
+				clearTimeout(timer);
+				resolve(true);
+				break;
+			}
+			await delay(1000);
+		}
+	});
+};
+
+const getStatus = async () =>
+	await request(BALENA_SUPERVISOR_ADDRESS)
+		.get('/v2/state/status')
+		.then(({ body }) => body);
+
+const docker = new Docker();
+
+describe('state engine', () => {
+	beforeEach(async () => {
+		await setTargetState({
+			config: {},
+			apps: {},
+		});
+	});
+
+	after(async () => {
+		await setTargetState({
+			config: {},
+			apps: {},
+		});
+	});
+
+	it('installs an app with two services', async () => {
+		await setTargetState({
+			config: {},
+			apps: {
+				'123': {
+					name: 'test-app',
+					commit: 'deadbeef',
+					releaseId: 1,
+					services: {
+						'1': {
+							image: 'alpine',
+							imageId: 11,
+							serviceName: 'one',
+							restart: 'unless-stopped',
+							running: true,
+							command: 'sleep infinity',
+							stop_signal: 'SIGKILL',
+							networks: ['default'],
+							labels: {},
+							environment: {},
+						},
+						'2': {
+							image: 'alpine',
+							imageId: 12,
+							serviceName: 'two',
+							restart: 'unless-stopped',
+							running: true,
+							command: 'sleep infinity',
+							stop_signal: 'SIGKILL',
+							networks: ['default'],
+							labels: {},
+							environment: {},
+						},
+					},
+					networks: {},
+					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' },
+		]);
+	});
+
+	// This test recovery from issue #1576, where a device running a service from the target release
+	// would not stop the service even if there were still network and container changes to be applied
+	it('always stops running services depending on a network being changed', async () => {
+		// Install part of the target release
+		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',
+							labels: {},
+							environment: {},
+						},
+					},
+					networks: {},
+					volumes: {},
+				},
+			},
+		});
+
+		const state = await getCurrentState();
+		expect(
+			state.apps['123'].services.map((s: any) => s.serviceName),
+		).to.deep.equal(['one']);
+
+		const containers = await docker.listContainers();
+		expect(
+			containers.map(({ Names, State }) => ({ Name: Names[0], State })),
+		).to.have.deep.members([{ Name: '/one_21_2_deadca1f', State: 'running' }]);
+		const containerIds = containers.map(({ Id }) => Id);
+
+		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: 'alpine:latest',
+							imageId: 22,
+							serviceName: 'two',
+							restart: 'unless-stopped',
+							running: true,
+							command: 'sh -c "echo two && sleep infinity"',
+							stop_signal: 'SIGKILL',
+							networks: ['default'],
+							labels: {},
+							environment: {},
+						},
+					},
+					networks: {
+						default: {
+							driver: 'bridge',
+							ipam: {
+								config: [
+									{ gateway: '192.168.91.1', subnet: '192.168.91.0/24' },
+								],
+								driver: 'default',
+							},
+						},
+					},
+					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,
+		);
+
+		expect(await docker.getNetwork('123_default').inspect())
+			.to.have.property('IPAM')
+			.to.deep.equal({
+				Config: [{ Gateway: '192.168.91.1', Subnet: '192.168.91.0/24' }],
+				Driver: 'default',
+				Options: {},
+			});
+	});
+
+	it('updates an app with two services with a network change', async () => {
+		await setTargetState({
+			config: {},
+			apps: {
+				'123': {
+					name: 'test-app',
+					commit: 'deadbeef',
+					releaseId: 1,
+					services: {
+						'1': {
+							image: 'alpine:latest',
+							imageId: 11,
+							serviceName: 'one',
+							restart: 'unless-stopped',
+							running: true,
+							command: 'sleep infinity',
+							stop_signal: 'SIGKILL',
+							labels: {},
+							environment: {},
+						},
+						'2': {
+							image: 'alpine:latest',
+							imageId: 12,
+							serviceName: 'two',
+							restart: 'unless-stopped',
+							running: true,
+							command: 'sleep infinity',
+							labels: {},
+							environment: {},
+						},
+					},
+					networks: {},
+					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 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: 'alpine:latest',
+							imageId: 22,
+							serviceName: 'two',
+							restart: 'unless-stopped',
+							running: true,
+							command: 'sh -c "echo two && sleep infinity"',
+							stop_signal: 'SIGKILL',
+							networks: ['default'],
+							labels: {},
+							environment: {},
+						},
+					},
+					networks: {
+						default: {
+							driver: 'bridge',
+							ipam: {
+								config: [
+									{ gateway: '192.168.91.1', subnet: '192.168.91.0/24' },
+								],
+								driver: 'default',
+							},
+						},
+					},
+					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,
+		);
+
+		expect(await docker.getNetwork('123_default').inspect())
+			.to.have.property('IPAM')
+			.to.deep.equal({
+				Config: [{ Gateway: '192.168.91.1', Subnet: '192.168.91.0/24' }],
+				Driver: 'default',
+				Options: {},
+			});
+	});
+});

test/unit/compose/app.spec.ts

@@ -639,8 +639,12 @@ describe('compose/app', () => {
 				isTarget: true,
 			});
 
-			const steps = current.nextStepsForAppUpdate(defaultContext, target);
+			const availableImages = [createImage({ appUuid: 'deadbeef' })];
 
+			const steps = current.nextStepsForAppUpdate(
+				{ ...defaultContext, availableImages },
+				target,
+			);
 			const [killStep] = expectSteps('kill', steps);
 			expect(killStep)
 				.to.have.property('current')
@@ -681,6 +685,129 @@ describe('compose/app', () => {
 			expectNoStep('removeNetwork', steps);
 		});
 
+		it('should always kill dependencies of networks before removing', async () => {
+			const current = createApp({
+				services: [
+					// The device for some reason is already running some services
+					// of the new release, but we need to kill it anyways
+					await createService({
+						image: 'alpine',
+						serviceName: 'one',
+						commit: 'deadca1f',
+						composition: { command: 'sleep infinity', networks: ['default'] },
+					}),
+				],
+				networks: [Network.fromComposeObject('default', 1, 'appuuid', {})],
+			});
+			const target = createApp({
+				services: [
+					await createService({
+						image: 'alpine',
+						serviceName: 'one',
+						commit: 'deadca1f',
+						composition: { command: 'sleep infinity', networks: ['default'] },
+					}),
+					await createService({
+						image: 'alpine',
+						serviceName: 'two',
+						commit: 'deadca1f',
+						composition: {
+							command: 'sh -c "echo two && sleep infinity"',
+							networks: ['default'],
+						},
+					}),
+				],
+				networks: [
+					Network.fromComposeObject('default', 1, 'appuuid', {
+						labels: { test: 'test' },
+					}),
+				],
+				isTarget: true,
+			});
+
+			const availableImages = [
+				createImage({ appId: 1, serviceName: 'one', name: 'alpine' }),
+				createImage({ appId: 1, serviceName: 'two', name: 'alpine' }),
+			];
+
+			const steps = current.nextStepsForAppUpdate(
+				{ ...defaultContext, availableImages },
+				target,
+			);
+			const [killStep] = expectSteps('kill', steps);
+
+			expect(killStep)
+				.to.have.property('current')
+				.that.deep.includes({ serviceName: 'one' });
+
+			// We shouldn't try to remove the network until we have gotten rid of the dependencies
+			expectNoStep('removeNetwork', steps);
+		});
+
+		it('should kill dependencies of networks before updating between releases', async () => {
+			const current = createApp({
+				services: [
+					await createService({
+						image: 'alpine',
+						serviceName: 'one',
+						commit: 'deadbeef',
+						composition: { command: 'sleep infinity', networks: ['default'] },
+					}),
+					await createService({
+						image: 'alpine',
+						serviceName: 'two',
+						commit: 'deadbeef',
+						composition: { command: 'sleep infinity', networks: ['default'] },
+					}),
+				],
+				networks: [Network.fromComposeObject('default', 1, 'appuuid', {})],
+			});
+			const target = createApp({
+				services: [
+					await createService({
+						image: 'alpine',
+						serviceName: 'one',
+						commit: 'deadca1f',
+						composition: { command: 'sleep infinity', networks: ['default'] },
+					}),
+					await createService({
+						image: 'alpine',
+						serviceName: 'two',
+						commit: 'deadca1f',
+						composition: {
+							command: 'sh -c "echo two && sleep infinity"',
+							networks: ['default'],
+						},
+					}),
+				],
+				networks: [
+					Network.fromComposeObject('default', 1, 'appuuid', {
+						labels: { test: 'test' },
+					}),
+				],
+				isTarget: true,
+			});
+
+			const availableImages = [
+				createImage({ appId: 1, serviceName: 'one', name: 'alpine' }),
+				createImage({ appId: 1, serviceName: 'two', name: 'alpine' }),
+			];
+
+			const steps = current.nextStepsForAppUpdate(
+				{ ...defaultContext, availableImages },
+				target,
+			);
+			expectSteps('kill', steps, 2);
+
+			expect(steps.map((s) => (s as any).current.serviceName)).to.have.members([
+				'one',
+				'two',
+			]);
+
+			// We shouldn't try to remove the network until we have gotten rid of the dependencies
+			expectNoStep('removeNetwork', steps);
+		});
+
 		it('should create the default network if it does not exist', () => {
 			const current = createApp({ networks: [] });
 			const target = createApp({ networks: [], isTarget: true });