From af6359f7ae2db4a656598947120bb46d5d3730de Mon Sep 17 00:00:00 2001 From: Christina Ying Wang Date: Mon, 26 Feb 2024 16:45:56 -0800 Subject: [PATCH] Take lock before updating service metadata Change-type: minor Signed-off-by: Christina Ying Wang --- src/compose/app.ts | 32 ++++++++++++++---- src/compose/composition-steps.ts | 24 +++----------- .../compose/application-manager.spec.ts | 7 ++++ test/unit/compose/app.spec.ts | 33 ++++++++++++++++--- 4 files changed, 65 insertions(+), 31 deletions(-) diff --git a/src/compose/app.ts b/src/compose/app.ts index 68d3cfbf..c538b38a 100644 --- a/src/compose/app.ts +++ b/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; } diff --git a/src/compose/composition-steps.ts b/src/compose/composition-steps.ts index 45df3f56..93741a33 100644 --- a/src/compose/composition-steps.ts +++ b/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( diff --git a/test/integration/compose/application-manager.spec.ts b/test/integration/compose/application-manager.spec.ts index a1da6488..9430c4eb 100644 --- a/test/integration/compose/application-manager.spec.ts +++ b/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 diff --git a/test/unit/compose/app.spec.ts b/test/unit/compose/app.spec.ts index f45dc93f..82a4b3ab 100644 --- a/test/unit/compose/app.spec.ts +++ b/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' });