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

@@ -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<ChangingPair<Network>>,
 		volumePairs: Array<ChangingPair<Volume>>,
 	): 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;
 		}

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);
 			}
 		});

test/src/compose/app.spec.ts

@@ -76,7 +76,13 @@ function createImage(
 		...extra
 	} = {} as Partial<Image>,
 ) {
-	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);
 		});
 	});