Take lock before updating service metadata

Change-type: minor
Signed-off-by: Christina Ying Wang <christina@balena.io>

src/compose/app.ts

@@ -558,6 +558,8 @@ export class App {
 		} else {
 			// This service is in both current & target so requires an update,
 			// or it's a service that's not in target so requires removal
+
+			// Skip updateMetadata for services with networks or volumes
 			const needsSpecialKill = this.serviceHasNetworkOrVolume(
 				current,
 				context.networkPairs,
@@ -568,8 +570,8 @@ export class App {
 				target != null &&
 				current.isEqualConfig(target, context.containerIds)
 			) {
-				// we're only starting/stopping a service
-				return this.generateContainerStep(current, target);
+				// Update service metadata or start/stop a service
+				return this.generateContainerStep(current, target, context.locksTaken);
 			}
 
 			let strategy: string;
@@ -658,10 +660,26 @@ export class App {
 		);
 	}
 
-	private generateContainerStep(current: Service, target: Service) {
-		// if the services release doesn't match, then rename the container...
+	private generateContainerStep(
+		current: Service,
+		target: Service,
+		locksTaken: LocksTakenMap,
+	) {
+		// Update container metadata if service release has changed
 		if (current.commit !== target.commit) {
-			return generateStep('updateMetadata', { current, target });
+			// QUESTION: Should updateMetadata only be allowed when
+			// *all* services have locks taken by the Supervisor? Currently
+			// it proceeds when the service it's updating has locks taken,
+			// meaning the service could be on new release while another service
+			// with a user-taken lock is still on old release.
+			if (locksTaken.isLocked(target.appId, target.serviceName)) {
+				return generateStep('updateMetadata', { current, target });
+			}
+			// Otherwise, take lock for service first
+			return generateStep('takeLock', {
+				appId: target.appId,
+				services: [target.serviceName],
+			});
 		} else if (target.config.running !== current.config.running) {
 			if (target.config.running) {
 				return generateStep('start', { target });
@@ -687,7 +705,7 @@ export class App {
 			// We know the service name exists as it always does for targets
 			return generateStep('fetch', {
 				image: imageManager.imageFromService(target),
-				serviceName: target.serviceName!,
+				serviceName: target.serviceName,
 			});
 		} else if (
 			this.dependenciesMetForServiceStart(
@@ -749,7 +767,7 @@ export class App {
 		// are services which are changing). We could have a dependency which is
 		// starting up, but is not yet running.
 		const depInstallingButNotRunning = _.some(targetApp.services, (svc) => {
-			if (target.dependsOn?.includes(svc.serviceName!)) {
+			if (target.dependsOn?.includes(svc.serviceName)) {
 				if (!svc.config.running) {
 					return true;
 				}

src/compose/composition-steps.ts

@@ -10,7 +10,6 @@ import * as networkManager from './network-manager';
 import * as volumeManager from './volume-manager';
 import type Volume from './volume';
 import * as commitStore from './commit';
-import { checkTruthy } from '../lib/validation';
 import * as updateLock from '../lib/update-lock';
 import type { DeviceLegacyReport } from '../types/state';
 
@@ -44,10 +43,7 @@ interface CompositionStepArgs {
 	updateMetadata: {
 		current: Service;
 		target: Service;
-		options?: {
-			skipLock?: boolean;
-		};
-	} & BaseCompositionStepArgs;
+	};
 	restart: {
 		current: Service;
 		target: Service;
@@ -179,20 +175,10 @@ export function getExecutors(app: {
 			// take locks
 			await serviceManager.remove(step.current);
 		},
-		updateMetadata: (step) => {
-			const skipLock =
-				step.skipLock ||
-				checkTruthy(step.current.config.labels['io.balena.legacy-container']);
-			return app.lockFn(
-				step.current.appId,
-				{
-					force: step.force,
-					skipLock: skipLock || _.get(step, ['options', 'skipLock']),
-				},
-				async () => {
-					await serviceManager.updateMetadata(step.current, step.target);
-				},
-			);
+		updateMetadata: async (step) => {
+			// Should always be preceded by a takeLock step,
+			// so the call is executed assuming that the lock is taken.
+			await serviceManager.updateMetadata(step.current, step.target);
 		},
 		restart: (step) => {
 			return app.lockFn(

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

@@ -8,6 +8,7 @@ import Network from '~/src/compose/network';
 import * as networkManager from '~/src/compose/network-manager';
 import Volume from '~/src/compose/volume';
 import * as config from '~/src/config';
+import { LocksTakenMap } from '~/lib/update-lock';
 import { createDockerImage } from '~/test-lib/docker-helper';
 import {
 	createService,
@@ -493,6 +494,9 @@ describe('compose/application-manager', () => {
 					// to avoid removeImage steps
 					availableImages: [],
 					containerIdsByAppId: c1.containerIdsByAppId,
+					// Mock locks for service to be updated via updateMetadata
+					// to avoid takeLock step
+					locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]),
 				},
 			);
 			// Service `new` should be fetched
@@ -605,6 +609,9 @@ describe('compose/application-manager', () => {
 					// to avoid removeImage steps
 					availableImages: [],
 					containerIdsByAppId: c1.containerIdsByAppId,
+					// Mock locks for service to be updated via updateMetadata
+					// to avoid takeLock step
+					locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]),
 				},
 			);
 			// Service `new` should be fetched

test/unit/compose/app.spec.ts

@@ -951,26 +951,49 @@ describe('compose/app', () => {
 			expectNoStep('fetch', steps);
 		});
 
-		it('should emit an updateMetadata step when a service has not changed but the release has', async () => {
+		it('should emit a takeLock followed by an updateMetadata step when a service has not changed but the release has', async () => {
 			const current = createApp({
 				services: [
-					await createService({ serviceName: 'main', commit: 'old-release' }),
+					await createService({
+						serviceName: 'main',
+						appId: 1,
+						commit: 'old-release',
+					}),
 				],
+				networks: [DEFAULT_NETWORK],
 			});
 			const target = createApp({
 				services: [
-					await createService({ serviceName: 'main', commit: 'new-release' }),
+					await createService({
+						serviceName: 'main',
+						appId: 1,
+						commit: 'new-release',
+					}),
 				],
+				networks: [DEFAULT_NETWORK],
 				isTarget: true,
 			});
 
+			// Take lock before updating metadata
 			const steps = current.nextStepsForAppUpdate(defaultContext, target);
-			const [updateMetadataStep] = expectSteps('updateMetadata', steps);
+			const [takeLockStep] = expectSteps('takeLock', steps);
+			expect(takeLockStep).to.have.property('appId').that.equals(1);
+			expect(takeLockStep)
+				.to.have.property('services')
+				.that.deep.equals(['main']);
 
+			// Infer updateMetadata after locks are taken
+			const steps2 = current.nextStepsForAppUpdate(
+				{
+					...defaultContext,
+					locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]),
+				},
+				target,
+			);
+			const [updateMetadataStep] = expectSteps('updateMetadata', steps2);
 			expect(updateMetadataStep)
 				.to.have.property('current')
 				.to.deep.include({ serviceName: 'main', commit: 'old-release' });
-
 			expect(updateMetadataStep)
 				.to.have.property('target')
 				.to.deep.include({ serviceName: 'main', commit: 'new-release' });