Merge pull request #1802 from balena-os/service-config-helpers

Check config for networks and volumes inside Service

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