From f08316dc57aaf15300f7810bc700345534a3368b Mon Sep 17 00:00:00 2001 From: Cameron Diver Date: Wed, 4 Nov 2020 17:11:19 +0000 Subject: [PATCH] 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 --- src/compose/app.ts | 7 ++- src/compose/application-manager.ts | 38 +++++++------- src/compose/commit.ts | 32 ++++++++++++ src/compose/composition-steps.ts | 4 +- src/config/schema-type.ts | 4 -- src/config/schema.ts | 5 -- src/device-api/v1.ts | 78 +++++++++++++++-------------- src/device-api/v2.ts | 34 ++++++++++--- src/device-state.ts | 26 ++++++++-- src/migrations/M00006.js | 28 +++++++++++ test/21-supervisor-api.spec.ts | 2 +- test/39-compose-commit.spec.ts | 36 +++++++++++++ test/data/device-api-responses.json | 4 +- test/lib/mocked-device-api.ts | 12 +++-- 14 files changed, 222 insertions(+), 88 deletions(-) create mode 100644 src/compose/commit.ts create mode 100644 src/migrations/M00006.js create mode 100644 test/39-compose-commit.spec.ts diff --git a/src/compose/app.ts b/src/compose/app.ts index d866f571..90a32df8 100644 --- a/src/compose/app.ts +++ b/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; diff --git a/src/compose/application-manager.ts b/src/compose/application-manager.ts index 5d6fc02d..5a118037 100644 --- a/src/compose/application-manager.ts +++ b/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,24 +368,22 @@ export async function getCurrentApps(): Promise { Object.keys(services), ).map((i) => parseInt(i, 10)); - // TODO: This will break with multiple apps - const commit = (await config.get('currentCommit')) ?? undefined; + const apps: InstancedAppState = {}; + for (const appId of allAppIds) { + const commit = await commitStore.getCommitForApp(appId); + apps[appId] = new App( + { + appId, + services: services[appId] ?? [], + networks: _.keyBy(networks[appId], 'name'), + volumes: _.keyBy(volumes[appId], 'name'), + commit, + }, + false, + ); + } - return _.keyBy( - allAppIds.map((appId) => { - return new App( - { - appId, - services: services[appId] ?? [], - networks: _.keyBy(networks[appId], 'name'), - volumes: _.keyBy(volumes[appId], 'name'), - commit, - }, - 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 = {}; @@ -842,5 +840,5 @@ export async function getStatus() { } } - return { local: apps, dependent, commit: currentCommit }; + return { local: apps, dependent }; } diff --git a/src/compose/commit.ts b/src/compose/commit.ts new file mode 100644 index 00000000..17705f82 --- /dev/null +++ b/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 { + 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 { + cache[appId] = commit; + return db.upsertModel('currentCommit', { commit, appId }, { appId }, trx); +} diff --git a/src/compose/composition-steps.ts b/src/compose/composition-steps.ts index 5e031088..686275b2 100644 --- a/src/compose/composition-steps.ts +++ b/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( diff --git a/src/config/schema-type.ts b/src/config/schema-type.ts index c39f2aed..de0b0fe6 100644 --- a/src/config/schema-type.ts +++ b/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, diff --git a/src/config/schema.ts b/src/config/schema.ts index 5581013e..588660cf 100644 --- a/src/config/schema.ts +++ b/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, diff --git a/src/device-api/v1.ts b/src/device-api/v1.ts index 82d79e75..96af71e7 100644 --- a/src/device-api/v1.ts +++ b/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,52 +106,54 @@ 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) { - 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', - ); - } + 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'); + } - // handle the case where the appId is out of scope - if (!req.auth.isScoped({ apps: [app.appId] })) { - res.status(401).json({ - status: 'failed', - message: 'Application is not available', - }); - return; - } + // handle the case where the appId is out of scope + if (!req.auth.isScoped({ apps: [app.appId] })) { + res.status(401).json({ + status: 'failed', + message: 'Application is not available', + }); + return; + } - // Don't return data that will be of no use to the user - const appToSend = { - appId, - commit: status.commit!, - containerId: service.containerId, - env: _.omit(service.config.environment, constants.privateAppEnvVars), - imageId: service.config.image, - releaseId: service.releaseId, - }; + if (app.services.length > 1) { + return res + .status(400) + .send('Some v1 endpoints are only allowed on single-container apps'); + } - return res.json(appToSend); - }, - ).catch(next); + // 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, + containerId: service.containerId, + env: _.omit(service.config.environment, constants.privateAppEnvVars), + imageId: service.config.image, + releaseId: service.releaseId, + }; + + return res.json(appToSend); + } catch (e) { + next(e); + } }); router.post('/v1/purge', (req: AuthorizedRequest, res, next) => { diff --git a/src/device-api/v2.ts b/src/device-api/v2.ts index da5d67fc..ccb1f2b4 100644 --- a/src/device-api/v2.ts +++ b/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, }); }); diff --git a/src/device-state.ts b/src/device-state.ts index e4efbf13..5c1f3650 100644 --- a/src/device-state.ts +++ b/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 { 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; diff --git a/src/migrations/M00006.js b/src/migrations/M00006.js new file mode 100644 index 00000000..8f915d23 --- /dev/null +++ b/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'); +} diff --git a/test/21-supervisor-api.spec.ts b/test/21-supervisor-api.spec.ts index 64be45b4..b367736e 100644 --- a/test/21-supervisor-api.spec.ts +++ b/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') diff --git a/test/39-compose-commit.spec.ts b/test/39-compose-commit.spec.ts new file mode 100644 index 00000000..b8a64424 --- /dev/null +++ b/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); + }); +}); diff --git a/test/data/device-api-responses.json b/test/data/device-api-responses.json index 1323e0fa..373b0537 100644 --- a/test/data/device-api-responses.json +++ b/test/data/device-api-responses.json @@ -16,7 +16,7 @@ "body": { "appId": 2, "containerId": "abc123", - "commit": "7fc9c5bea8e361acd49886fe6cc1e1cd", + "commit": "4e380136c2cf56cd64197d51a1ab263a", "env": {}, "releaseId": 77777 } @@ -81,4 +81,4 @@ }, "POST": {} } -} \ No newline at end of file +} diff --git a/test/lib/mocked-device-api.ts b/test/lib/mocked-device-api.ts index 587a8798..db675413 100644 --- a/test/lib/mocked-device-api.ts +++ b/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 { 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 {