Allow storing commits against their appIds

This paves the way for running multiple applications and storing
information related to the application against the application itself. A
couple of hacks have been added to v1 and v2 endpoints to maintain
compatability but these should eventually be removed with the addition
of a v3 api.

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

src/compose/app.ts

@@ -168,7 +168,12 @@ export class App {
 			this.commit !== target.commit
 		) {
 			// TODO: The next PR should change this to support multiapp commit values
-			steps.push(generateStep('updateCommit', { target: target.commit }));
+			steps.push(
+				generateStep('updateCommit', {
+					target: target.commit,
+					appId: this.appId,
+				}),
+			);
 		}
 
 		return steps;

src/compose/application-manager.ts

@@ -24,6 +24,7 @@ import * as serviceManager from './service-manager';
 import * as imageManager from './images';
 import type { Image } from './images';
 import { getExecutors, CompositionStepT } from './composition-steps';
+import * as commitStore from './commit';
 
 import Service from './service';
 
@@ -367,12 +368,10 @@ export async function getCurrentApps(): Promise<InstancedAppState> {
 		Object.keys(services),
 	).map((i) => parseInt(i, 10));
 
-	// TODO: This will break with multiple apps
-	const commit = (await config.get('currentCommit')) ?? undefined;
-
-	return _.keyBy(
-		allAppIds.map((appId) => {
-			return new App(
+	const apps: InstancedAppState = {};
+	for (const appId of allAppIds) {
+		const commit = await commitStore.getCommitForApp(appId);
+		apps[appId] = new App(
 			{
 				appId,
 				services: services[appId] ?? [],
@@ -382,9 +381,9 @@ export async function getCurrentApps(): Promise<InstancedAppState> {
 			},
 			false,
 		);
-		}),
-		'appId',
-	);
+	}
+
+	return apps;
 }
 
 function killServicesUsingApi(current: InstancedAppState): CompositionStep[] {
@@ -758,10 +757,9 @@ function reportOptionalContainers(serviceNames: string[]) {
 // FIXME: This would be better to implement using the App class, and have each one
 // generate its status. For now we use the original from application-manager.coffee.
 export async function getStatus() {
-	const [services, images, currentCommit] = await Promise.all([
+	const [services, images] = await Promise.all([
 		serviceManager.getStatus(),
 		imageManager.getStatus(),
-		config.get('currentCommit'),
 	]);
 
 	const apps: Dictionary<any> = {};
@@ -842,5 +840,5 @@ export async function getStatus() {
 		}
 	}
 
-	return { local: apps, dependent, commit: currentCommit };
+	return { local: apps, dependent };
 }

src/compose/commit.ts

@@ -0,0 +1,32 @@
+import * as db from '../db';
+
+const cache: { [appId: number]: string } = {};
+
+export async function getCommitForApp(
+	appId: number,
+): Promise<string | undefined> {
+	if (cache[appId] != null) {
+		return cache[appId];
+	}
+
+	const commit = await db
+		.models('currentCommit')
+		.where({ appId })
+		.select('commit');
+
+	if (commit?.[0] != null) {
+		cache[appId] = commit[0].commit;
+		return commit[0].commit;
+	}
+
+	return;
+}
+
+export function upsertCommitForApp(
+	appId: number,
+	commit: string,
+	trx?: db.Transaction,
+): Promise<void> {
+	cache[appId] = commit;
+	return db.upsertModel('currentCommit', { commit, appId }, { appId }, trx);
+}

src/compose/composition-steps.ts

@@ -14,6 +14,7 @@ import { checkTruthy } from '../lib/validation';
 import * as networkManager from './network-manager';
 import * as volumeManager from './volume-manager';
 import { DeviceReportFields } from '../types/state';
+import * as commitStore from './commit';
 
 interface BaseCompositionStepArgs {
 	force?: boolean;
@@ -62,6 +63,7 @@ interface CompositionStepArgs {
 	} & BaseCompositionStepArgs;
 	updateCommit: {
 		target: string;
+		appId: number;
 	};
 	handover: {
 		current: Service;
@@ -218,7 +220,7 @@ export function getExecutors(app: {
 			app.callbacks.containerStarted(container.id);
 		},
 		updateCommit: async (step) => {
-			await config.set({ currentCommit: step.target });
+			await commitStore.upsertCommitForApp(step.appId, step.target);
 		},
 		handover: (step) => {
 			return app.lockFn(

src/config/schema-type.ts

@@ -150,10 +150,6 @@ export const schemaTypes = {
 		),
 		default: NullOrUndefined,
 	},
-	currentCommit: {
-		type: t.string,
-		default: NullOrUndefined,
-	},
 	targetStateSet: {
 		type: PermissiveBoolean,
 		default: false,

src/config/schema.ts

@@ -155,11 +155,6 @@ export const schema = {
 		mutable: true,
 		removeIfNull: false,
 	},
-	currentCommit: {
-		source: 'db',
-		mutable: true,
-		removeIfNull: false,
-	},
 	targetStateSet: {
 		source: 'db',
 		mutable: true,

src/device-api/v1.ts

@@ -1,4 +1,3 @@
-import * as Promise from 'bluebird';
 import * as express from 'express';
 import * as _ from 'lodash';
 
@@ -9,6 +8,7 @@ import { doRestart, doPurge } from './common';
 
 import * as applicationManager from '../compose/application-manager';
 import { generateStep } from '../compose/composition-steps';
+import * as commitStore from '../compose/commit';
 import { AuthorizedRequest } from '../lib/api-keys';
 
 export function createV1Api(router: express.Router) {
@@ -106,30 +106,21 @@ export function createV1Api(router: express.Router) {
 	router.post('/v1/apps/:appId/stop', createV1StopOrStartHandler('stop'));
 	router.post('/v1/apps/:appId/start', createV1StopOrStartHandler('start'));
 
-	router.get('/v1/apps/:appId', (req: AuthorizedRequest, res, next) => {
+	router.get('/v1/apps/:appId', async (req: AuthorizedRequest, res, next) => {
 		const appId = checkInt(req.params.appId);
 		eventTracker.track('GET app (v1)', { appId });
 		if (appId == null) {
 			return res.status(400).send('Missing app id');
 		}
-		return Promise.join(
-			applicationManager.getCurrentApps(),
-			applicationManager.getStatus(),
-			function (apps, status) {
+
+		try {
+			const apps = await applicationManager.getCurrentApps();
 			const app = apps[appId];
 			const service = app?.services?.[0];
 			if (service == null) {
 				return res.status(400).send('App not found');
 			}
 
-				if (app.services.length > 1) {
-					return res
-						.status(400)
-						.send(
-							'Some v1 endpoints are only allowed on single-container apps',
-						);
-				}
-
 			// handle the case where the appId is out of scope
 			if (!req.auth.isScoped({ apps: [app.appId] })) {
 				res.status(401).json({
@@ -139,10 +130,20 @@ export function createV1Api(router: express.Router) {
 				return;
 			}
 
+			if (app.services.length > 1) {
+				return res
+					.status(400)
+					.send('Some v1 endpoints are only allowed on single-container apps');
+			}
+
+			// Because we only have a single app, we can fetch the commit for that
+			// app, and maintain backwards compatability
+			const commit = await commitStore.getCommitForApp(appId);
+
 			// Don't return data that will be of no use to the user
 			const appToSend = {
 				appId,
-					commit: status.commit!,
+				commit,
 				containerId: service.containerId,
 				env: _.omit(service.config.environment, constants.privateAppEnvVars),
 				imageId: service.config.image,
@@ -150,8 +151,9 @@ export function createV1Api(router: express.Router) {
 			};
 
 			return res.json(appToSend);
-			},
-		).catch(next);
+		} catch (e) {
+			next(e);
+		}
 	});
 
 	router.post('/v1/purge', (req: AuthorizedRequest, res, next) => {

src/device-api/v2.ts

@@ -12,6 +12,7 @@ import {
 import { getApp } from '../device-state/db-format';
 import { Service } from '../compose/service';
 import Volume from '../compose/volume';
+import * as commitStore from '../compose/commit';
 import * as config from '../config';
 import * as db from '../db';
 import * as deviceConfig from '../device-config';
@@ -327,8 +328,11 @@ export function createV2Api(router: Router) {
 					delete apps.local[app];
 				}
 			}
+
+			const commit = await commitStore.getCommitForApp(appId);
+
 			// Return filtered applications
-			return res.status(200).json(apps);
+			return res.status(200).json({ commit, ...apps });
 		},
 	);
 
@@ -461,13 +465,13 @@ export function createV2Api(router: Router) {
 	});
 
 	router.get('/v2/state/status', async (req: AuthorizedRequest, res) => {
-		const currentRelease = await config.get('currentCommit');
-
+		const appIds: number[] = [];
 		const pending = deviceState.isApplyInProgress();
 		const containerStates = (await serviceManager.getAll())
 			.filter((service) => req.auth.isScoped({ apps: [service.appId] }))
-			.map((svc) =>
-				_.pick(
+			.map((svc) => {
+				appIds.push(svc.appId);
+				return _.pick(
 					svc,
 					'status',
 					'serviceName',
@@ -476,14 +480,15 @@ export function createV2Api(router: Router) {
 					'serviceId',
 					'containerId',
 					'createdAt',
-				),
 				);
+			});
 
 		let downloadProgressTotal = 0;
 		let downloads = 0;
 		const imagesStates = (await images.getStatus())
 			.filter((img) => req.auth.isScoped({ apps: [img.appId] }))
 			.map((img) => {
+				appIds.push(img.appId);
 				if (img.downloadProgress != null) {
 					downloadProgressTotal += img.downloadProgress;
 					downloads += 1;
@@ -505,13 +510,26 @@ export function createV2Api(router: Router) {
 			overallDownloadProgress = downloadProgressTotal / downloads;
 		}
 
+		if (_.uniq(appIds).length > 1) {
+			// We can't accurately return the commit value each app without changing
+			// the shape of the data, and instead we'd like users to use the new v3
+			// endpoints, which will come with multiapp
+			// If we're going to return information about more than one app, error out
+			return res.status(405).json({
+				status: 'failed',
+				message: `Cannot use /v2/ endpoints with a key that is scoped to multiple applications`,
+			});
+		}
+
+		const commit = await commitStore.getCommitForApp(appIds[0]);
+
 		return res.status(200).send({
 			status: 'success',
 			appState: pending ? 'applying' : 'applied',
 			overallDownloadProgress,
 			containers: containerStates,
 			images: imagesStates,
-			release: currentRelease,
+			release: commit,
 		});
 	});
 

src/device-state.ts

@@ -27,6 +27,7 @@ import * as validation from './lib/validation';
 import * as network from './network';
 
 import * as applicationManager from './compose/application-manager';
+import * as commitStore from './compose/commit';
 import * as deviceConfig from './device-config';
 import { ConfigStep } from './device-config';
 import { log } from './lib/supervisor-console';
@@ -514,11 +515,30 @@ export async function getStatus(): Promise<DeviceStatus> {
 		local: {},
 		dependent: {},
 	};
-	theState.local = { ...theState.local, ...currentVolatile };
+	theState.local = {
+		...theState.local,
+		...currentVolatile,
+	};
 	theState.local!.apps = appsStatus.local;
 	theState.dependent!.apps = appsStatus.dependent;
-	if (appsStatus.commit && !applyInProgress) {
-		theState.local!.is_on__commit = appsStatus.commit;
+
+	// Multi-app warning!
+	// If we have more than one app, simply return the first commit.
+	// Fortunately this won't become a problem until we have system apps, and then
+	// at that point we can filter non-system apps leaving a single user app.
+	// After this, for true multi-app, we will need to report our status back in a
+	// different way, meaning this function will no longer be needed
+	const appIds = Object.keys(theState.local!.apps).map((strId) =>
+		parseInt(strId, 10),
+	);
+
+	const appId: number | undefined = appIds[0];
+	if (appId != null) {
+		const commit = await commitStore.getCommitForApp(appId);
+
+		if (commit != null && !applyInProgress) {
+			theState.local!.is_on__commit = commit;
+		}
 	}
 
 	return theState as DeviceStatus;

src/migrations/M00006.js

@@ -0,0 +1,28 @@
+export async function up(knex) {
+	await knex.schema.createTable('currentCommit', (table) => {
+		table.integer('id').primary();
+		table.integer('appId').notNullable();
+		table.string('commit').notNullable();
+		table.unique(['appId']);
+	});
+
+	const currentCommit = await knex('config')
+		.where({ key: 'currentCommit' })
+		.select('value');
+	if (currentCommit[0] != null) {
+		const apps = await knex('app').select(['appId']);
+
+		for (const app of apps) {
+			await knex('currentCommit').insert({
+				appId: app.appId,
+				commit: currentCommit[0].value,
+			});
+		}
+
+		await knex('config').where({ key: 'currentCommit' }).delete();
+	}
+}
+
+export async function down() {
+	throw new Error('Not implemented');
+}

test/21-supervisor-api.spec.ts

@@ -237,7 +237,7 @@ describe('SupervisorAPI', () => {
 
 		// TODO: add tests for V1 endpoints
 		describe('GET /v1/apps/:appId', () => {
-			it('returns information about a SPECIFIC application', async () => {
+			it('returns information about a specific application', async () => {
 				await request
 					.get('/v1/apps/2')
 					.set('Accept', 'application/json')

test/39-compose-commit.spec.ts

@@ -0,0 +1,36 @@
+import { expect } from 'chai';
+import * as commitStore from '../src/compose/commit';
+import * as db from '../src/db';
+
+describe('compose/commit', () => {
+	before(async () => await db.initialized);
+
+	describe('Fetching commits', () => {
+		beforeEach(async () => {
+			// Clear the commit values in the db
+			await db.models('currentCommit').del();
+		});
+
+		it('should fetch a commit for an appId', async () => {
+			const commit = 'abcdef';
+			await commitStore.upsertCommitForApp(1, commit);
+
+			expect(await commitStore.getCommitForApp(1)).to.equal(commit);
+		});
+
+		it('should fetch the correct commit value when there is multiple commits', async () => {
+			const commit = 'abcdef';
+			await commitStore.upsertCommitForApp(1, '123456');
+			await commitStore.upsertCommitForApp(2, commit);
+
+			expect(await commitStore.getCommitForApp(2)).to.equal(commit);
+		});
+	});
+
+	it('should correctly insert a commit when a commit for the same app already exists', async () => {
+		const commit = 'abcdef';
+		await commitStore.upsertCommitForApp(1, '123456');
+		await commitStore.upsertCommitForApp(1, commit);
+		expect(await commitStore.getCommitForApp(1)).to.equal(commit);
+	});
+});

test/data/device-api-responses.json

@@ -16,7 +16,7 @@
         "body": {
           "appId": 2,
           "containerId": "abc123",
-          "commit": "7fc9c5bea8e361acd49886fe6cc1e1cd",
+          "commit": "4e380136c2cf56cd64197d51a1ab263a",
           "env": {},
           "releaseId": 77777
         }

test/lib/mocked-device-api.ts

@@ -6,6 +6,7 @@ import * as applicationManager from '../../src/compose/application-manager';
 import * as networkManager from '../../src/compose/network-manager';
 import * as serviceManager from '../../src/compose/service-manager';
 import * as volumeManager from '../../src/compose/volume-manager';
+import * as commitStore from '../../src/compose/commit';
 import * as config from '../../src/config';
 import * as db from '../../src/db';
 import { createV1Api } from '../../src/device-api/v1';
@@ -17,8 +18,9 @@ import SupervisorAPI from '../../src/supervisor-api';
 const DB_PATH = './test/data/supervisor-api.sqlite';
 // Holds all values used for stubbing
 const STUBBED_VALUES = {
-	config: {
-		currentCommit: '7fc9c5bea8e361acd49886fe6cc1e1cd',
+	commits: {
+		1: '7fc9c5bea8e361acd49886fe6cc1e1cd',
+		2: '4e380136c2cf56cd64197d51a1ab263a',
 	},
 	services: [
 		{
@@ -110,9 +112,9 @@ async function initConfig(): Promise<void> {
 	await config.initialized;
 
 	// Set a currentCommit
-	await config.set({
-		currentCommit: STUBBED_VALUES.config.currentCommit,
-	});
+	for (const [id, commit] of Object.entries(STUBBED_VALUES.commits)) {
+		await commitStore.upsertCommitForApp(parseInt(id, 10), commit);
+	}
 }
 
 function buildRoutes(): Router {