Keep the network and volume models consistent across usage

Change-type: patch
Signed-off-by: Cameron Diver <cameron@balena.io>

src/application-manager.coffee

@@ -153,16 +153,14 @@ module.exports = class ApplicationManager extends EventEmitter
 						@images.cleanup()
 			createNetworkOrVolume: (step) =>
 				if step.model is 'network'
-					# TODO: The network model allows the network
-					# itself to check for duplicates on creation, and
-					# this differs from the volume module, which has
-					# better encapsulation, so we should change the
-					# following to use the NetworkManager class instead
-					step.target.create()
+					@networks.create(step.target)
 				else
 					@volumes.create(step.target)
-			removeNetworkOrVolume: (step) ->
-				step.current.remove()
+			removeNetworkOrVolume: (step) =>
+				if step.model is 'network'
+					@networks.remove(step.current)
+				else
+					@volumes.remove(step.current)
 			ensureSupervisorNetwork: =>
 				@networks.ensureSupervisorNetwork()
 		}
@@ -390,22 +388,23 @@ module.exports = class ApplicationManager extends EventEmitter
 		outputPairs = []
 		currentNames = _.keys(current)
 		targetNames = _.keys(target)
+
 		toBeRemoved = _.difference(currentNames, targetNames)
 		for name in toBeRemoved
 			outputPairs.push({ current: current[name], target: null })
+
 		toBeInstalled = _.difference(targetNames, currentNames)
 		for name in toBeInstalled
 			outputPairs.push({ current: null, target: target[name] })
+
 		toBeUpdated = _.filter _.intersection(targetNames, currentNames), (name) ->
-			# While we're in this in-between state of a network-manager, but not
-			# a volume-manager, we'll have to inspect the object to detect a
-			# network-manager
 			return !current[name].isEqualConfig(target[name])
 		for name in toBeUpdated
 			outputPairs.push({
 				current: current[name],
 				target: target[name]
 			})
+
 		return outputPairs
 
 	compareNetworksForUpdate: ({ current, target }, appId) =>
@@ -672,7 +671,7 @@ module.exports = class ApplicationManager extends EventEmitter
 			return Service.fromComposeObject(service, serviceOpts)
 
 	createTargetVolume: (name, appId, volume) ->
-		return Volume.fromComposeVolume(
+		return Volume.fromComposeObject(
 			name,
 			appId,
 			volume,
@@ -680,12 +679,11 @@ module.exports = class ApplicationManager extends EventEmitter
 		)
 
 	createTargetNetwork: (name, appId, network) ->
-		# FIXME: Re-order the construct options to match volume
 		return Network.fromComposeObject(
-			{ @docker, @logger },
 			name,
 			appId,
 			network
+			{ @docker, @logger },
 		)
 
 	normaliseAndExtendAppFromDB: (app) =>
@@ -714,7 +712,7 @@ module.exports = class ApplicationManager extends EventEmitter
 
 				networks = JSON.parse(app.networks)
 				networks = _.mapValues networks, (networkConfig, networkName) =>
-					networkConfig ? {}
+					networkConfig ?= {}
 					@createTargetNetwork(networkName, app.appId, networkConfig)
 
 				Promise.map(JSON.parse(app.services), (service) => @createTargetService(service, configOpts))

src/compose/network-manager.ts

@@ -5,10 +5,12 @@ import { fs } from 'mz';
 import * as constants from '../lib/constants';
 import Docker from '../lib/docker-utils';
 import { ENOENT, NotFoundError } from '../lib/errors';
+import logTypes = require('../lib/log-types');
 import { Logger } from '../logger';
 import { Network, NetworkOptions } from './network';
 
 import log from '../lib/supervisor-console';
+import { ResourceRecreationAttemptError } from './errors';
 
 export class NetworkManager {
 	private docker: Docker;
@@ -40,17 +42,51 @@ export class NetworkManager {
 		return this.getAll().filter((network: Network) => network.appId === appId);
 	}
 
-	public get(network: { name: string; appId: number }): Bluebird<Network> {
-		return Network.fromNameAndAppId(
-			{
-				logger: this.logger,
-				docker: this.docker,
-			},
-			network.name,
-			network.appId,
+	public async get(network: {
+		name: string;
+		appId: number;
+	}): Bluebird<Network> {
+		const dockerNet = await this.docker
+			.getNetwork(Network.generateDockerName(network.appId, network.name))
+			.inspect();
+		return Network.fromDockerNetwork(
+			{ docker: this.docker, logger: this.logger },
+			dockerNet,
 		);
 	}
 
+	public async create(network: Network) {
+		try {
+			const existing = await this.get({
+				name: network.name,
+				appId: network.appId,
+			});
+			if (!network.isEqualConfig(existing)) {
+				throw new ResourceRecreationAttemptError('network', network.name);
+			}
+
+			// We have a network with the same config and name
+			// already created, we can skip this
+		} catch (e) {
+			if (!NotFoundError(e)) {
+				this.logger.logSystemEvent(logTypes.createNetworkError, {
+					network: { name: network.name, appId: network.appId },
+					error: e,
+				});
+				throw e;
+			}
+
+			// If we got a not found error, create the network
+			await network.create();
+		}
+	}
+
+	public async remove(network: Network) {
+		// We simply forward this to the network object, but we
+		// add this method to provide a consistent interface
+		await network.remove();
+	}
+
 	public supervisorNetworkReady(): Bluebird<boolean> {
 		return Bluebird.resolve(
 			fs.stat(`/sys/class/net/${constants.supervisorNetworkInterface}`),

src/compose/network.ts

@@ -2,7 +2,7 @@ import * as Bluebird from 'bluebird';
 import * as _ from 'lodash';
 
 import Docker from '../lib/docker-utils';
-import { InvalidAppIdError, NotFoundError } from '../lib/errors';
+import { InvalidAppIdError } from '../lib/errors';
 import logTypes = require('../lib/log-types');
 import { checkInt } from '../lib/validation';
 import { Logger } from '../logger';
@@ -18,7 +18,6 @@ import {
 import {
 	InvalidNetworkConfigurationError,
 	InvalidNetworkNameError,
-	ResourceRecreationAttemptError,
 } from './errors';
 
 export interface NetworkOptions {
@@ -33,12 +32,10 @@ export class Network {
 
 	private docker: Docker;
 	private logger: Logger;
-	private networkOpts: NetworkOptions;
 
 	private constructor(opts: NetworkOptions) {
 		this.docker = opts.docker;
 		this.logger = opts.logger;
-		this.networkOpts = opts;
 	}
 
 	public static fromDockerNetwork(
@@ -93,20 +90,11 @@ export class Network {
 		return ret;
 	}
 
-	public static async fromNameAndAppId(
-		opts: NetworkOptions,
-		name: string,
-		appId: number,
-	): Bluebird<Network> {
-		const network = await opts.docker.getNetwork(`${appId}_${name}`).inspect();
-		return Network.fromDockerNetwork(opts, network);
-	}
-
 	public static fromComposeObject(
-		opts: NetworkOptions,
 		name: string,
 		appId: number,
 		network: NetworkConfig,
+		opts: NetworkOptions,
 	): Network {
 		const net = new Network(opts);
 		net.name = name;
@@ -133,34 +121,17 @@ export class Network {
 		return net;
 	}
 
-	public create(): Bluebird<void> {
+	public async create(): Promise<void> {
 		this.logger.logSystemEvent(logTypes.createNetwork, {
 			network: { name: this.name },
 		});
 
-		return Network.fromNameAndAppId(this.networkOpts, this.name, this.appId)
-			.then(current => {
-				if (!this.isEqualConfig(current)) {
-					throw new ResourceRecreationAttemptError('network', this.name);
-				}
-
-				// We have a network with the same config and name already created -
-				// we can skip this.
-			})
-			.catch(NotFoundError, () => {
-				return this.docker.createNetwork(this.toDockerConfig());
-			})
-			.tapCatch(err => {
-				this.logger.logSystemEvent(logTypes.createNetworkError, {
-					network: { name: this.name, appId: this.appId },
-					error: err,
-				});
-			});
+		return await this.docker.createNetwork(this.toDockerConfig());
 	}
 
 	public toDockerConfig(): DockerNetworkConfig {
 		return {
-			Name: this.getDockerName(),
+			Name: Network.generateDockerName(this.appId, this.name),
 			Driver: this.config.driver,
 			CheckDuplicate: true,
 			IPAM: {
@@ -201,7 +172,9 @@ export class Network {
 		});
 
 		return Bluebird.resolve(
-			this.docker.getNetwork(this.getDockerName()).remove(),
+			this.docker
+				.getNetwork(Network.generateDockerName(this.appId, this.name))
+				.remove(),
 		).tapCatch(error => {
 			this.logger.logSystemEvent(logTypes.createNetworkError, {
 				network: { name: this.name, appId: this.appId },
@@ -224,10 +197,6 @@ export class Network {
 		return _.isEqual(configToCompare, network.config);
 	}
 
-	public getDockerName(): string {
-		return `${this.appId}_${this.name}`;
-	}
-
 	private static validateComposeConfig(config: NetworkConfig): void {
 		// Check if every ipam config entry has both a subnet and a gateway
 		_.each(_.get(config, 'config.ipam.config', []), ({ subnet, gateway }) => {
@@ -238,4 +207,8 @@ export class Network {
 			}
 		});
 	}
+
+	public static generateDockerName(appId: number, name: string) {
+		return `${appId}_${name}`;
+	}
 }

src/compose/volume-manager.ts

@@ -5,11 +5,12 @@ import unionBy = require('lodash/unionBy');
 import * as Path from 'path';
 
 import constants = require('../lib/constants');
-import { InternalInconsistencyError, NotFoundError } from '../lib/errors';
+import { NotFoundError } from '../lib/errors';
 import { safeRename } from '../lib/fs-utils';
 import * as LogTypes from '../lib/log-types';
 import { defaultLegacyVolume } from '../lib/migration';
 import Logger from '../logger';
+import { ResourceRecreationAttemptError } from './errors';
 import Volume, { VolumeConfig } from './volume';
 
 export interface VolumeMangerConstructOpts {
@@ -65,11 +66,7 @@ export class VolumeManager {
 			});
 
 			if (!volume.isEqualConfig(existing)) {
-				throw new InternalInconsistencyError(
-					`Trying to create volume '${
-						volume.name
-					}', but a volume with the same name and different configuration exists`,
-				);
+				throw new ResourceRecreationAttemptError('volume', volume.name);
 			}
 		} catch (e) {
 			if (!NotFoundError(e)) {
@@ -84,6 +81,12 @@ export class VolumeManager {
 		}
 	}
 
+	// We simply forward this to the volume object, but we
+	// add this method to provide a consistent interface
+	public async remove(volume: Volume) {
+		await volume.remove();
+	}
+
 	public async createFromLegacy(appId: number): Promise<Volume | void> {
 		const name = defaultLegacyVolume();
 		const legacyPath = Path.join(
@@ -108,7 +111,7 @@ export class VolumeManager {
 		config: Partial<VolumeConfig>,
 		oldPath: string,
 	): Promise<Volume> {
-		const volume = Volume.fromComposeVolume(name, appId, config, {
+		const volume = Volume.fromComposeObject(name, appId, config, {
 			logger: this.logger,
 			docker: this.docker,
 		});

src/compose/volume.ts

@@ -57,7 +57,7 @@ export class Volume {
 		return new Volume(name, appId, config, opts);
 	}
 
-	public static fromComposeVolume(
+	public static fromComposeObject(
 		name: string,
 		appId: number,
 		config: Partial<VolumeConfig>,
@@ -86,8 +86,6 @@ export class Volume {
 		this.logger.logSystemEvent(LogTypes.createVolume, {
 			volume: { name: this.name },
 		});
-		// Check that we're not trying to recreate a volume that
-		// already exists
 		await this.docker.createVolume({
 			Name: Volume.generateDockerName(this.appId, this.name),
 			Labels: this.config.labels,

src/device-state.coffee

@@ -447,7 +447,8 @@ module.exports = class DeviceState extends EventEmitter
 							# If the volume exists (from a previous incomplete run of this restoreBackup), we delete it first
 							@applications.volumes.get({ appId, name: volumeName })
 							.then =>
-								@applications.volumes.remove({ appId, name: volumeName })
+								@applications.volumes.get({ appId, name: volumeName }).then (volume) ->
+									volume.remove()
 							.catch(NotFoundError, _.noop)
 							.then =>
 								@applications.volumes.createFromPath({ appId, name: volumeName, config: volumes[volumeName] }, path.join(backupPath, volumeName))

test/14-application-manager.spec.coffee

@@ -152,10 +152,10 @@ describe 'ApplicationManager', ->
 					appCloned.services = normalisedServices
 					appCloned.networks = _.mapValues appCloned.networks, (config, name) =>
 						Network.fromComposeObject(
-							{ docker: @applications.docker, @logger }
 							name,
 							app.appId,
 							config
+							{ docker: @applications.docker, @logger }
 						)
 					return appCloned
 			.then (normalisedApps) ->

test/18-compose-network.coffee

@@ -8,7 +8,7 @@ describe 'compose/network', ->
 
 		it 'should convert a compose configuration to an internal representation', ->
 
-			network = Network.fromComposeObject({ logger: null, docker: null }, 'test', 123, {
+			network = Network.fromComposeObject('test', 123, {
 				'driver': 'bridge',
 				'ipam': {
 					'driver': 'default',
@@ -19,7 +19,7 @@ describe 'compose/network', ->
 						}
 					]
 				}
-			})
+			}, { logger: null, docker: null })
 
 			expect(network.config).to.deep.equal({
 				driver: 'bridge'
@@ -41,7 +41,7 @@ describe 'compose/network', ->
 
 		it 'should convert an internal representation to a docker representation', ->
 
-			network = Network.fromComposeObject({ logger: null, docker: null }, 'test', 123, {
+			network = Network.fromComposeObject('test', 123, {
 				'driver': 'bridge',
 				'ipam': {
 					'driver': 'default',
@@ -52,7 +52,7 @@ describe 'compose/network', ->
 						}
 					]
 				}
-			})
+			}, { logger: null, docker: null })
 
 			expect(network.toDockerConfig()).to.deep.equal({
 				Name: '123_test',