From 969f4225e54ded644d38315289e0cc84a0f3bb51 Mon Sep 17 00:00:00 2001 From: Felipe Lalanne Date: Fri, 27 Aug 2021 15:22:04 -0400 Subject: [PATCH] Check config for networks and volumes inside Service This removes the need for the app module to know about the naming conventions for networks and volumes since those exist now within the service itself. This also fixes a small bug where the volume would be removed before the service itself had been successfully stopped. Change-type: patch --- src/compose/app.ts | 60 +++++++++++------------------------- src/compose/service.ts | 54 ++++++++++++++++---------------- test/src/compose/app.spec.ts | 49 +++++++++++++++++++++++------ 3 files changed, 85 insertions(+), 78 deletions(-) diff --git a/src/compose/app.ts b/src/compose/app.ts index 5dd06d60..a342af6b 100644 --- a/src/compose/app.ts +++ b/src/compose/app.ts @@ -145,17 +145,13 @@ export class App { // Generate volume steps steps = steps.concat( this.generateStepsForComponent(volumeChanges, servicePairs, (v, svc) => - // TODO: Volumes are stored without the appId prepended, but networks are stored - // with it prepended. Sort out this inequality - svc.config.volumes.includes(v.name), + svc.hasVolume(v.name), ), ); // Generate network steps steps = steps.concat( - this.generateStepsForComponent( - networkChanges, - servicePairs, - (n, svc) => `${this.appId}_${n.name}` in svc.config.networks, + this.generateStepsForComponent(networkChanges, servicePairs, (n, svc) => + svc.hasNetwork(n.name), ), ); @@ -570,21 +566,14 @@ export class App { networkPairs: Array>, volumePairs: Array>, ): boolean { - const serviceVolumes = svc.config.volumes; - for (const { current } of volumePairs) { - if (current && serviceVolumes.includes(`${this.appId}_${current.name}`)) { - return true; - } - } - - const serviceNetworks = svc.config.networks; - for (const { current } of networkPairs) { - if (current && `${this.appId}_${current.name}` in serviceNetworks) { - return true; - } - } - - return false; + return ( + volumePairs.some( + ({ current }) => current && svc.hasVolume(current.name), + ) || + networkPairs.some( + ({ current }) => current && svc.hasNetwork(current.name), + ) + ); } private generateContainerStep(current: Service, target: Service) { @@ -657,33 +646,20 @@ export class App { return false; } + // Wait for networks to be created before starting the service if ( - _.some( - networkPairs, - (pair) => - `${this.appId}_${pair.target?.name}` === target.config.networkMode, + networkPairs.some( + (pair) => pair.target && target.hasNetworkMode(pair.target.name), ) ) { return false; } + // Wait for volumes to be created before starting the service if ( - _.some(target.config.volumes, (volumeDefinition) => { - const [sourceName, destName] = volumeDefinition.split(':'); - if (destName == null) { - // If this is not a named volume, ignore it - return false; - } - if (sourceName[0] === '/') { - // Absolute paths should also be ignored - return false; - } - - return _.some( - volumePairs, - (pair) => `${target.appId}_${pair.target?.name}` === sourceName, - ); - }) + volumePairs.some( + (pair) => pair.target && target.hasVolume(pair.target.name), + ) ) { return false; } diff --git a/src/compose/service.ts b/src/compose/service.ts index dc0b9569..a847ad86 100644 --- a/src/compose/service.ts +++ b/src/compose/service.ts @@ -877,32 +877,6 @@ export class Service { ); } - public getNamedVolumes() { - const defaults = Service.defaultBinds( - this.appId || 0, - this.serviceName || '', - ); - const validVolumes = _.map(this.config.volumes, (volume) => { - if (_.includes(defaults, volume) || !_.includes(volume, ':')) { - return null; - } - const bindSource = volume.split(':')[0]; - if (!path.isAbsolute(bindSource)) { - const match = bindSource.match(/[0-9]+_(.+)/); - if (match == null) { - log.error( - `Error: There was an error parsing a volume bind source, ignoring.\nBind source: ${bindSource}`, - ); - return null; - } - return match[1]; - } - return null; - }); - - return _.reject(validVolumes, _.isNil); - } - public handoverCompleteFullPathsOnHost(): string[] { const lockPath = updateLock.lockPath( this.appId || 0, @@ -995,6 +969,32 @@ export class Service { return env; } + public hasNetwork(networkName: string) { + // TODO; we could probably export network naming methods to another + // module to avoid duplicate code + return `${this.appId}_${networkName}` in this.config.networks; + } + + public hasNetworkMode(networkName: string) { + return `${this.appId}_${networkName}` === this.config.networkMode; + } + + public hasVolume(volumeName: string) { + return this.config.volumes.some((volumeDefinition) => { + const [sourceName, destName] = volumeDefinition.split(':'); + if (destName == null) { + // If this is not a named volume, ignore it + return false; + } + if (sourceName[0] === '/') { + // Absolute paths should also be ignored + return false; + } + + return `${this.appId}_${volumeName}` === sourceName; + }); + } + private isSameNetwork( current: ServiceConfig['networks'][0], target: ServiceConfig['networks'][0], @@ -1086,6 +1086,8 @@ export class Service { log.warn(`Ignoring invalid bind mount ${volume}`); } } else { + // Push anonymous volume. Anonymous volumes can only be + // set by using the `VOLUME` instruction inside a dockerfile volumes.push(volume); } }); diff --git a/test/src/compose/app.spec.ts b/test/src/compose/app.spec.ts index 6b109d17..b2d484f1 100644 --- a/test/src/compose/app.spec.ts +++ b/test/src/compose/app.spec.ts @@ -76,7 +76,13 @@ function createImage( ...extra } = {} as Partial, ) { - return { appId, dependent, name, serviceName, ...extra } as Image; + return { + appId, + dependent, + name, + serviceName, + ...extra, + } as Image; } const expectSteps = ( @@ -240,11 +246,19 @@ describe('compose/app', () => { it('should kill dependencies of a volume before changing config', async () => { const current = createApp({ - services: [await createService({ volumes: ['test-volume'] })], + services: [ + await createService({ + volumes: ['test-volume:/data'], + }), + ], volumes: [Volume.fromComposeObject('test-volume', 1, {})], }); const target = createApp({ - services: [await createService({ volumes: ['test-volume'] })], + services: [ + await createService({ + volumes: ['test-volume:/data'], + }), + ], volumes: [ Volume.fromComposeObject('test-volume', 1, { labels: { test: 'test' }, @@ -276,7 +290,9 @@ describe('compose/app', () => { }); it('should not output a kill step for a service which is already stopping when changing a volume', async () => { - const service = await createService({ volumes: ['test-volume'] }); + const service = await createService({ + volumes: ['test-volume:/data'], + }); service.status = 'Stopping'; const current = createApp({ services: [service], @@ -298,7 +314,7 @@ describe('compose/app', () => { it('should generate the correct step sequence for a volume purge request', async () => { const service = await createService({ - volumes: ['db-volume'], + volumes: ['db-volume:/data'], image: 'test-image', }); const volume = Volume.fromComposeObject('db-volume', service.appId, {}); @@ -364,7 +380,7 @@ describe('compose/app', () => { volumes: [], }); - // Step 3: start & createVolume + // Step 3: createVolume service.status = 'Running'; const target = createApp({ services: [service], @@ -372,13 +388,26 @@ describe('compose/app', () => { volumes: [volume], isTarget: true, }); - const finalSteps = currentWithVolumesRemoved.nextStepsForAppUpdate( + const recreateVolumeSteps = currentWithVolumesRemoved.nextStepsForAppUpdate( contextWithImages, target, ); - expect(finalSteps).to.have.length(2); - expectSteps('start', finalSteps); - expectSteps('createVolume', finalSteps); + + expect(recreateVolumeSteps).to.have.length(1); + expectSteps('createVolume', recreateVolumeSteps); + + // Final step: start service + const currentWithVolumeRecreated = createApp({ + services: [], + networks: [defaultNetwork], + volumes: [volume], + }); + + const createServiceSteps = currentWithVolumeRecreated.nextStepsForAppUpdate( + contextWithImages, + target, + ); + expectSteps('start', createServiceSteps); }); });