From c0e170c61f05a8fc01dc167276ccb041d6d2d846 Mon Sep 17 00:00:00 2001 From: Cameron Diver Date: Wed, 10 Jun 2020 11:56:30 +0100 Subject: [PATCH] Make target-state-cache a singleton Change-type: patch Signed-off-by: Cameron Diver --- src/application-manager.js | 58 +++++------- src/device-state/target-state-cache.ts | 121 +++++++++++++------------ 2 files changed, 89 insertions(+), 90 deletions(-) diff --git a/src/application-manager.js b/src/application-manager.js index 777c08ac..66395fba 100644 --- a/src/application-manager.js +++ b/src/application-manager.js @@ -24,7 +24,7 @@ import { } from './lib/errors'; import { pathExistsOnHost } from './lib/fs-utils'; -import { TargetStateAccessor } from './device-state/target-state-cache'; +import * as targetStateCache from './device-state/target-state-cache'; import { ServiceManager } from './compose/service-manager'; import { Service } from './compose/service'; @@ -185,8 +185,6 @@ export class ApplicationManager extends EventEmitter { this._targetVolatilePerImageId = {}; this._containerStarted = {}; - this.targetStateWrapper = new TargetStateAccessor(this); - this.actionExecutors = compositionSteps.getExecutors({ lockFn: this._lockingIfNecessary, services: this.services, @@ -225,34 +223,28 @@ export class ApplicationManager extends EventEmitter { return this.emit('change', data); } - init() { - return Images.initialized - .then(() => Images.cleanupDatabase()) - .then(() => { - const cleanup = () => { - return docker.listContainers({ all: true }).then((containers) => { - return logger.clearOutOfDateDBLogs(_.map(containers, 'Id')); - }); - }; - // Rather than relying on removing out of date database entries when we're no - // longer using them, set a task that runs periodically to clear out the database - // This has the advantage that if for some reason a container is removed while the - // supervisor is down, we won't have zombie entries in the db - - // Once a day - setInterval(cleanup, 1000 * 60 * 60 * 24); - // But also run it in on startup - return cleanup(); - }) - .then(() => { - return this.localModeManager.init(); - }) - .then(() => { - return this.services.attachToRunning(); - }) - .then(() => { - return this.services.listenToEvents(); + async init() { + await Images.initialized; + await Images.cleanupDatabase(); + const cleanup = () => { + return docker.listContainers({ all: true }).then((containers) => { + return logger.clearOutOfDateDBLogs(_.map(containers, 'Id')); }); + }; + // Rather than relying on removing out of date database entries when we're no + // longer using them, set a task that runs periodically to clear out the database + // This has the advantage that if for some reason a container is removed while the + // supervisor is down, we won't have zombie entries in the db + + // Once a day + setInterval(cleanup, 1000 * 60 * 60 * 24); + // But also run it in on startup + await cleanup(); + await this.localModeManager.init(); + await this.services.attachToRunning(); + this.services.listenToEvents(); + + await targetStateCache.initialized; } // Returns the status of applications and their services @@ -409,7 +401,7 @@ export class ApplicationManager extends EventEmitter { } getTargetApp(appId) { - return this.targetStateWrapper.getTargetApp(appId).then((app) => { + return targetStateCache.getTargetApp(appId).then((app) => { if (app == null) { return; } @@ -1129,7 +1121,7 @@ export class ApplicationManager extends EventEmitter { }); return Promise.map(appsArray, this.normaliseAppForDB) .then((appsForDB) => { - return this.targetStateWrapper.setTargetApps(appsForDB, trx); + return targetStateCache.setTargetApps(appsForDB, trx); }) .then(() => trx('app') @@ -1219,7 +1211,7 @@ export class ApplicationManager extends EventEmitter { getTargetApps() { return Promise.map( - this.targetStateWrapper.getTargetApps(), + targetStateCache.getTargetApps(), this.normaliseAndExtendAppFromDB, ) .map((app) => { diff --git a/src/device-state/target-state-cache.ts b/src/device-state/target-state-cache.ts index 6d83d8b1..9c2ff31c 100644 --- a/src/device-state/target-state-cache.ts +++ b/src/device-state/target-state-cache.ts @@ -4,71 +4,78 @@ import { ApplicationManager } from '../application-manager'; import * as config from '../config'; import * as db from '../db'; -// Once we have correct types for both applications and the -// incoming target state this should be changed -export type DatabaseApp = Dictionary; +// We omit the id (which does appear in the db) in this type, as we don't use it +// at all, and we can use the below type for both insertion and retrieval. +export interface DatabaseApp { + name: string; + releaseId: number; + commit: string; + appId: number; + services: string; + networks: string; + volumes: string; + source: string; +} export type DatabaseApps = DatabaseApp[]; /* - * This class is a wrapper around the database setting and - * receiving of target state. Because the target state can - * only be set from a single place, but several workflows - * rely on getting the target state at one point or another, - * we cache the values using this class. Accessing the - * database is inherently expensive, and for example the - * local log backend accesses the target state for every log - * line. This can very quickly cause serious memory problems - * and database connection timeouts. + * This module is a wrapper around the database fetching and retrieving of + * target state. Because the target state can only be set only be set from a + * single place, but several workflows rely on getting the target state at one + * point or another, we cache the values using this class. Accessing the + * database is inherently expensive, and for example the local log backend + * accesses the target state for every log line. This can very quickly cause + * serious memory problems and database connection timeouts. */ -export class TargetStateAccessor { - private targetState?: DatabaseApps; +let targetState: DatabaseApps | undefined; - public constructor(protected applications: ApplicationManager) { - // If we switch backend, the target state also needs to - // be invalidated (this includes switching to and from - // local mode) - config.on('change', (conf) => { - if (conf.apiEndpoint != null || conf.localMode != null) { - this.targetState = undefined; - } - }); - } - - public async getTargetApp(appId: number): Promise { - if (this.targetState == null) { - // TODO: Perhaps only fetch a single application from - // the DB, at the expense of repeating code - await this.getTargetApps(); +export const initialized = (async () => { + await db.initialized; + await config.initialized; + // If we switch backend, the target state also needs to + // be invalidated (this includes switching to and from + // local mode) + config.on('change', (conf) => { + if (conf.apiEndpoint != null || conf.localMode != null) { + targetState = undefined; } + }); +})(); - return _.find(this.targetState, (app) => app.appId === appId); +export async function getTargetApp( + appId: number, +): Promise { + if (targetState == null) { + // TODO: Perhaps only fetch a single application from + // the DB, at the expense of repeating code + await getTargetApps(); } - public async getTargetApps(): Promise { - if (this.targetState == null) { - const { apiEndpoint, localMode } = await config.getMany([ - 'apiEndpoint', - 'localMode', - ]); - - const source = localMode ? 'local' : apiEndpoint; - this.targetState = await db.models('app').where({ source }); - } - return this.targetState!; - } - - public async setTargetApps( - apps: DatabaseApps, - trx: db.Transaction, - ): Promise { - // We can't cache the value here, as it could be for a - // different source - this.targetState = undefined; - - await Promise.all( - apps.map((app) => db.upsertModel('app', app, { appId: app.appId }, trx)), - ); - } + return _.find(targetState, (app) => app.appId === appId); } -export default TargetStateAccessor; +export async function getTargetApps(): Promise { + if (targetState == null) { + const { apiEndpoint, localMode } = await config.getMany([ + 'apiEndpoint', + 'localMode', + ]); + + const source = localMode ? 'local' : apiEndpoint; + targetState = await db.models('app').where({ source }); + } + return targetState!; +} + +export async function setTargetApps( + apps: DatabaseApps, + trx?: db.Transaction, +): Promise { + // We can't cache the value here, as it could be for a + // different source + targetState = undefined; + + await Promise.all( + apps.map((app) => db.upsertModel('app', app, { appId: app.appId }, trx)), + ); +}