From 3c6e9dd209a259211ed3fd2e7bbe00f3235f87aa Mon Sep 17 00:00:00 2001 From: Felipe Lalanne Date: Tue, 19 Nov 2024 19:01:25 -0300 Subject: [PATCH] Refactor update-locks implementation The refactor simplifies the implementation and ensures that locks per app can only be held by one supervisor task at the time. Change-type: patch --- entry.sh | 3 - src/compose/app.ts | 25 +- src/compose/application-manager.ts | 46 +- src/compose/composition-steps.ts | 13 +- src/compose/types/app.ts | 4 +- src/compose/types/composition-step.ts | 6 +- src/device-api/actions.ts | 42 +- src/device-state/index.ts | 36 +- src/host-config/index.ts | 40 +- src/lib/lockfile.ts | 17 +- src/lib/update-lock.ts | 542 +++++----- .../compose/application-manager.spec.ts | 294 ++++-- test/integration/device-api/actions.spec.ts | 33 +- test/integration/lib/update-lock.spec.ts | 929 ++++++------------ test/unit/compose/app.spec.ts | 79 +- test/unit/lib/update-lock.spec.ts | 58 -- 16 files changed, 965 insertions(+), 1202 deletions(-) delete mode 100644 test/unit/lib/update-lock.spec.ts diff --git a/entry.sh b/entry.sh index 1f4fa9b4..c2b13872 100755 --- a/entry.sh +++ b/entry.sh @@ -77,9 +77,6 @@ fi # not a problem. modprobe ip6_tables || true -export BASE_LOCK_DIR="/tmp/balena-supervisor/services" -export LOCKFILE_UID=65534 - if [ "${LIVEPUSH}" = "1" ]; then exec npx nodemon --watch src --watch typings --ignore tests -e js,ts,json \ --exec node -r ts-node/register/transpile-only src/app.ts diff --git a/src/compose/app.ts b/src/compose/app.ts index 03bde90e..ba9502be 100644 --- a/src/compose/app.ts +++ b/src/compose/app.ts @@ -170,7 +170,7 @@ class AppImpl implements App { if (services.size > 0) { steps.push( generateStep('takeLock', { - appId: parseInt(appId, 10), + appId, services: Array.from(services), force: state.force, }), @@ -200,19 +200,14 @@ class AppImpl implements App { }), ); } - // Current & target should be the same appId, but one of either current - // or target may not have any services, so we need to check both - const allServices = this.services.concat(target.services); - if ( - allServices.length > 0 && - allServices.some((s) => - state.locksTaken.isLocked(s.appId, s.serviceName), - ) - ) { + + // If the app still has a lock, release it + if (state.lock != null) { // Release locks for all services before settling state steps.push( generateStep('releaseLock', { appId: target.appId, + lock: state.lock, }), ); } @@ -225,11 +220,7 @@ class AppImpl implements App { ): CompositionStep[] { if (Object.keys(this.services).length > 0) { // Take all locks before killing - if ( - this.services.some( - (svc) => !state.locksTaken.isLocked(svc.appId, svc.serviceName), - ) - ) { + if (state.lock == null) { return [ generateStep('takeLock', { appId: this.appId, @@ -628,9 +619,7 @@ class AppImpl implements App { appsToLock: AppsToLockMap; } & UpdateState, ): CompositionStep[] { - const servicesLocked = this.services - .concat(context.targetApp.services) - .every((svc) => context.locksTaken.isLocked(svc.appId, svc.serviceName)); + const servicesLocked = context.lock != null; if (current?.status === 'Stopping') { // There's a kill step happening already, emit a noop to ensure // we stay alive while this happens diff --git a/src/compose/application-manager.ts b/src/compose/application-manager.ts index 0867c35e..e5188bb2 100644 --- a/src/compose/application-manager.ts +++ b/src/compose/application-manager.ts @@ -12,7 +12,7 @@ import * as contracts from '../lib/contracts'; import * as constants from '../lib/constants'; import log from '../lib/supervisor-console'; import { InternalInconsistencyError } from '../lib/errors'; -import { getServicesLockedByAppId, LocksTakenMap } from '../lib/update-lock'; +import type { Lock } from '../lib/update-lock'; import { checkTruthy } from '../lib/validation'; import { App } from './app'; @@ -61,6 +61,13 @@ export function resetTimeSpentFetching(value: number = 0) { timeSpentFetching = value; } +interface LockRegistry { + [appId: string | number]: Lock; +} + +// In memory registry of all app locks +const lockRegistry: LockRegistry = {}; + const actionExecutors = getExecutors({ callbacks: { fetchStart: () => { @@ -76,6 +83,12 @@ const actionExecutors = getExecutors({ reportCurrentState(state); }, bestDeltaSource, + registerLock: (appId: string | number, lock: Lock) => { + lockRegistry[appId.toString()] = lock; + }, + unregisterLock: (appId: string | number) => { + delete lockRegistry[appId.toString()]; + }, }, }); @@ -134,10 +147,21 @@ export async function getRequiredSteps( downloading, availableImages, containerIdsByAppId, - locksTaken: await getServicesLockedByAppId(), + appLocks: lockRegistry, }); } +interface InferNextOpts { + keepImages: boolean; + keepVolumes: boolean; + delta: boolean; + force: boolean; + downloading: UpdateState['downloading']; + availableImages: UpdateState['availableImages']; + containerIdsByAppId: { [appId: number]: UpdateState['containerIds'] }; + appLocks: LockRegistry; +} + // Calculate the required steps from the current to the target state export async function inferNextSteps( currentApps: InstancedAppState, @@ -147,13 +171,11 @@ export async function inferNextSteps( keepVolumes = false, delta = true, force = false, - downloading = [] as UpdateState['downloading'], - availableImages = [] as UpdateState['availableImages'], - containerIdsByAppId = {} as { - [appId: number]: UpdateState['containerIds']; - }, - locksTaken = new LocksTakenMap(), - } = {}, + downloading = [], + availableImages = [], + containerIdsByAppId = {}, + appLocks = {}, + }: Partial, ) { const currentAppIds = Object.keys(currentApps).map((i) => parseInt(i, 10)); const targetAppIds = Object.keys(targetApps).map((i) => parseInt(i, 10)); @@ -215,8 +237,8 @@ export async function inferNextSteps( availableImages, containerIds: containerIdsByAppId[id], downloading, - locksTaken, force, + lock: appLocks[id], }, targetApps[id], ), @@ -230,8 +252,8 @@ export async function inferNextSteps( keepVolumes, downloading, containerIds: containerIdsByAppId[id], - locksTaken, force, + lock: appLocks[id], }), ); } @@ -255,8 +277,8 @@ export async function inferNextSteps( availableImages, containerIds: containerIdsByAppId[id] ?? {}, downloading, - locksTaken, force, + lock: appLocks[id], }, targetApps[id], ), diff --git a/src/compose/composition-steps.ts b/src/compose/composition-steps.ts index c2d4fb08..a75ea39e 100644 --- a/src/compose/composition-steps.ts +++ b/src/compose/composition-steps.ts @@ -5,9 +5,10 @@ import * as serviceManager from './service-manager'; import * as networkManager from './network-manager'; import * as volumeManager from './volume-manager'; import * as commitStore from './commit'; -import * as updateLock from '../lib/update-lock'; +import { Lockable } from '../lib/update-lock'; import type { DeviceLegacyReport } from '../types/state'; import type { CompositionStepAction, CompositionStepT } from './types'; +import type { Lock } from '../lib/update-lock'; export type { CompositionStep, @@ -27,6 +28,8 @@ interface CompositionCallbacks { fetchTime: (time: number) => void; stateReport: (state: DeviceLegacyReport) => void; bestDeltaSource: (image: Image, available: Image[]) => string | null; + registerLock: (appId: string | number, lock: Lock) => void; + unregisterLock: (appId: string | number) => void; } export function generateStep( @@ -141,10 +144,14 @@ export function getExecutors(app: { callbacks: CompositionCallbacks }) { /* async noop */ }, takeLock: async (step) => { - await updateLock.takeLock(step.appId, step.services, step.force); + const lockable = Lockable.from(step.appId, step.services); + const lockOverride = await config.get('lockOverride'); + const lock = await lockable.lock({ force: step.force || lockOverride }); + app.callbacks.registerLock(step.appId, lock); }, releaseLock: async (step) => { - await updateLock.releaseLock(step.appId); + app.callbacks.unregisterLock(step.appId); + await step.lock.unlock(); }, }; diff --git a/src/compose/types/app.ts b/src/compose/types/app.ts index 3363d276..09c4802f 100644 --- a/src/compose/types/app.ts +++ b/src/compose/types/app.ts @@ -1,7 +1,7 @@ import type { Network } from './network'; import type { Volume } from './volume'; import type { Service } from './service'; -import type { LocksTakenMap } from '../../lib/update-lock'; +import type { Lock } from '../../lib/update-lock'; import type { Image } from './image'; import type { CompositionStep } from './composition-step'; @@ -9,7 +9,7 @@ export interface UpdateState { availableImages: Image[]; containerIds: Dictionary; downloading: string[]; - locksTaken: LocksTakenMap; + lock: Lock | null; force: boolean; } diff --git a/src/compose/types/composition-step.ts b/src/compose/types/composition-step.ts index ddbe0460..9cbbbf12 100644 --- a/src/compose/types/composition-step.ts +++ b/src/compose/types/composition-step.ts @@ -2,6 +2,7 @@ import type { Image } from './image'; import type { Service } from './service'; import type { Network } from './network'; import type { Volume } from './volume'; +import type { Lock } from '../../lib/update-lock'; export interface CompositionStepArgs { stop: { @@ -67,12 +68,13 @@ export interface CompositionStepArgs { ensureSupervisorNetwork: object; noop: object; takeLock: { - appId: number; + appId: string | number; services: string[]; force: boolean; }; releaseLock: { - appId: number; + appId: string | number; + lock: Lock; }; } diff --git a/src/device-api/actions.ts b/src/device-api/actions.ts index a64c1e90..02da7b71 100644 --- a/src/device-api/actions.ts +++ b/src/device-api/actions.ts @@ -26,6 +26,7 @@ import { NotFoundError, BadRequestError, } from '../lib/errors'; +import { withLock } from '../lib/update-lock'; /** * Run an array of healthchecks, outputting whether all passed or not @@ -233,39 +234,24 @@ const executeDeviceActionWithLock = async ({ targetService?: Service; force: boolean; }) => { - try { - if (currentService) { - const lockOverride = await config.get('lockOverride'); - // Take lock for current service to be modified / stopped + const lockOverride = await config.get('lockOverride'); + await withLock( + appId, + async () => { + // Execute action on service await executeDeviceAction( - generateStep('takeLock', { - appId, - services: [currentService.serviceName], - force: force || lockOverride, + generateStep(action, { + current: currentService, + target: targetService, + wait: true, }), // FIXME: deviceState.executeStepAction only accepts force as a separate arg // instead of reading force from the step object, so we have to pass it twice - force || lockOverride, + force, ); - } - - // Execute action on service - await executeDeviceAction( - generateStep(action, { - current: currentService, - target: targetService, - wait: true, - }), - force, - ); - } finally { - // Release lock regardless of action success to prevent leftover lockfile - await executeDeviceAction( - generateStep('releaseLock', { - appId, - }), - ); - } + }, + { force: force || lockOverride }, + ); }; /** diff --git a/src/device-state/index.ts b/src/device-state/index.ts index de38ed0d..149b3b9b 100644 --- a/src/device-state/index.ts +++ b/src/device-state/index.ts @@ -456,22 +456,26 @@ export async function shutdown({ const apps = await applicationManager.getCurrentApps(); const appIds = Object.keys(apps).map((strId) => parseInt(strId, 10)); // Try to create a lock for all the services before shutting down - return updateLock.lock(appIds, { force }, async () => { - let dbusAction; - switch (reboot) { - case true: - logger.logSystemMessage('Rebooting', {}, 'Reboot'); - dbusAction = await dbus.reboot(); - break; - case false: - logger.logSystemMessage('Shutting down', {}, 'Shutdown'); - dbusAction = await dbus.shutdown(); - break; - } - shuttingDown = true; - emitAsync('shutdown', undefined); - return dbusAction; - }); + return updateLock.withLock( + appIds, + async () => { + let dbusAction; + switch (reboot) { + case true: + logger.logSystemMessage('Rebooting', {}, 'Reboot'); + dbusAction = await dbus.reboot(); + break; + case false: + logger.logSystemMessage('Shutting down', {}, 'Shutdown'); + dbusAction = await dbus.shutdown(); + break; + } + shuttingDown = true; + emitAsync('shutdown', undefined); + return dbusAction; + }, + { force }, + ); } // FIXME: this method should not be exported, all target state changes diff --git a/src/host-config/index.ts b/src/host-config/index.ts index 084a5ffc..23a39200 100644 --- a/src/host-config/index.ts +++ b/src/host-config/index.ts @@ -103,25 +103,29 @@ export async function patch( const { noProxy, ...targetConf } = conf.network.proxy; // It's possible for appIds to be an empty array, but patch shouldn't fail // as it's not dependent on there being any running user applications. - return updateLock.lock(appIds, { force }, async () => { - const proxyConf = await readProxy(); - let currentConf: ProxyConfig | undefined = undefined; - if (proxyConf) { - delete proxyConf.noProxy; - currentConf = proxyConf; - } + return updateLock.withLock( + appIds, + async () => { + const proxyConf = await readProxy(); + let currentConf: ProxyConfig | undefined = undefined; + if (proxyConf) { + delete proxyConf.noProxy; + currentConf = proxyConf; + } - // Merge current & target redsocks.conf - const patchedConf = patchProxy( - { - redsocks: currentConf, - }, - { - redsocks: targetConf, - }, - ); - await setProxy(patchedConf, noProxy); - }); + // Merge current & target redsocks.conf + const patchedConf = patchProxy( + { + redsocks: currentConf, + }, + { + redsocks: targetConf, + }, + ); + await setProxy(patchedConf, noProxy); + }, + { force }, + ); } } diff --git a/src/lib/lockfile.ts b/src/lib/lockfile.ts index e1d0dd55..f3623776 100644 --- a/src/lib/lockfile.ts +++ b/src/lib/lockfile.ts @@ -26,9 +26,12 @@ interface FindAllArgs { recursive: boolean; } -// Returns all current locks taken under a directory (default: /tmp) -// Optionally accepts filter function for only getting locks that match a condition. -// A file is counted as a lock by default if it ends with `.lock`. +/** + * Find all existing lockfiles under a given root directory (defaults to /mp) + * + * Optionally accepts filter function for only getting locks that match a condition. + * It will recursively look for all locks unless `recursive` is set to `false` + */ export async function findAll({ root = '/tmp', filter = (l) => l.path.endsWith('.lock'), @@ -96,6 +99,11 @@ export class LockfileExistsError implements ChildProcessError { } } +/** + * Lock the file provided as path + * + * Optionally accepts a user id to lock the path as + */ export async function lock(path: string, uid: number = os.userInfo().uid) { /** * Set parent directory permissions to `drwxrwxrwt` (octal 1777), which are needed @@ -145,6 +153,9 @@ export async function lock(path: string, uid: number = os.userInfo().uid) { } } +/** + * Removes the lock indicated by the path + */ export async function unlock(path: string): Promise { // Removing the lockfile releases the lock await fs.unlink(path).catch((e) => { diff --git a/src/lib/update-lock.ts b/src/lib/update-lock.ts index 7ea585e2..7520ecd0 100644 --- a/src/lib/update-lock.ts +++ b/src/lib/update-lock.ts @@ -1,32 +1,27 @@ import { promises as fs } from 'fs'; import path from 'path'; -import { isRight } from 'fp-ts/lib/Either'; +import { setTimeout } from 'timers/promises'; +import type ReadWriteLock from 'rwlock'; -import { - isENOENT, - UpdatesLockedError, - InternalInconsistencyError, -} from './errors'; +import { isENOENT, UpdatesLockedError } from './errors'; import { pathOnRoot, pathExistsOnState } from './host-utils'; import { mkdirp } from './fs-utils'; -import * as config from '../config'; import * as lockfile from './lockfile'; -import { NumericIdentifier, StringIdentifier, DockerName } from '../types'; import { takeGlobalLockRW } from './process-lock'; -import * as logger from '../logger'; import * as logTypes from './log-types'; +import * as logger from '../logger'; -const decodedUid = NumericIdentifier.decode(process.env.LOCKFILE_UID); -export const LOCKFILE_UID = isRight(decodedUid) ? decodedUid.right : 65534; +export const LOCKFILE_UID = 65534; +export const BASE_LOCK_DIR = '/tmp/balena-supervisor/services'; -export const BASE_LOCK_DIR = - process.env.BASE_LOCK_DIR || '/tmp/balena-supervisor/services'; - -export function lockPath(appId: number, serviceName?: string): string { +export function lockPath(appId: string | number, serviceName?: string): string { return path.join(BASE_LOCK_DIR, appId.toString(), serviceName ?? ''); } -function lockFilesOnHost(appId: number, serviceName: string): string[] { +function lockFilesOnHost( + appId: string | number, + serviceName: string, +): string[] { return pathOnRoot( ...['updates.lock', 'resin-updates.lock'].map((filename) => path.join(lockPath(appId), serviceName, filename), @@ -40,308 +35,297 @@ function lockFilesOnHost(appId: number, serviceName: string): string[] { * prevent reboot. If the Supervisor reboots while those services are still running, * the device may become stuck in an invalid state during HUP. */ -export function abortIfHUPInProgress({ +export async function abortIfHUPInProgress({ force = false, }: { - force: boolean | undefined; + force?: boolean; }): Promise { - return Promise.all( + const breadcrumbs = await Promise.all( ['rollback-health-breadcrumb', 'rollback-altboot-breadcrumb'].map( (filename) => pathExistsOnState(filename), ), - ).then((existsArray) => { - const anyExists = existsArray.some((e) => e); - if (anyExists && !force) { - throw new UpdatesLockedError('Waiting for Host OS update to finish'); - } - return anyExists; - }); + ); + + const hasHUPBreadcrumb = breadcrumbs.some((e) => e); + if (hasHUPBreadcrumb && !force) { + throw new UpdatesLockedError('Waiting for Host OS update to finish'); + } + + return hasHUPBreadcrumb; } -/** - * Unlock all lockfiles of an appId | appUuid, then release resources. - * Meant for use in update-lock module only as as it assumes that a - * write lock has been acquired. - */ -async function dispose( - appIdentifier: string | number, - release: () => void, -): Promise { - try { - const locks = await getLocksTaken( - pathOnRoot(`${BASE_LOCK_DIR}/${appIdentifier}`), - ); - // Try to unlock all locks taken - await Promise.all(locks.map((l) => lockfile.unlock(l))); - } finally { - // Release final resource - release(); - } -} - -/** - * Composition step used by Supervisor compose module. - * Take all locks for an appId | appUuid, creating directories if they don't exist. - */ -export async function takeLock( - appId: number, - services: string[], - force: boolean = false, -) { - logger.logSystemEvent(logTypes.takeLock, { - appId, - services, - force, - }); - - const release = await takeGlobalLockRW(appId); - - let lockOverride: boolean; - try { - lockOverride = await config.get('lockOverride'); - } catch (err: any) { - throw new InternalInconsistencyError( - `Error getting lockOverride config value: ${err?.message ?? err}`, - ); - } - - try { - const actuallyLocked: string[] = []; - const locksTaken = await getServicesLockedByAppId(); - // Filter out services that already have Supervisor-taken locks. - // This needs to be done after taking the appId write lock to avoid - // race conditions with locking. - const servicesWithoutLock = services.filter( - (svc) => !locksTaken.isLocked(appId, svc), - ); - for (const service of servicesWithoutLock) { - await mkdirp(pathOnRoot(lockPath(appId, service))); - await lockService(appId, service, force || lockOverride); - actuallyLocked.push(service); - } - return actuallyLocked; - } catch (err) { - // If something errors while taking the lock, we should remove any - // lockfiles that may have been created so that all services return - // to unlocked status. - await dispose(appId, release); - // Re-throw error to be handled in caller - throw err; - } finally { - // If not already released from catch, released the RW process lock. - // If already released, this will not error. - release(); - } -} - -/** - * Composition step used by Supervisor compose module. - * Release all locks for an appId | appUuid. - */ -export async function releaseLock(appId: number) { - logger.logSystemEvent(logTypes.releaseLock, { appId }); - - const release = await takeGlobalLockRW(appId); - await dispose(appId, release); -} - -/** - * Given a lockfile path `p`, return an array [appId, serviceName, filename] of that path. - * Paths are assumed to end in the format /:appId/:serviceName/(resin-)updates.lock. - */ -function getIdentifiersFromPath(p: string) { - const parts = p.split('/'); - const filename = parts.pop(); - if (filename?.match(/updates\.lock/) === null) { - return []; - } - const serviceName = parts.pop(); - const appId = parts.pop(); - return [appId, serviceName, filename]; -} - -type LockedEntity = { appId: number; services: string[] }; - -/** - * A map of locked services by appId. - * Exported for tests only; getServicesLockedByAppId is the public generator interface. - */ -export class LocksTakenMap extends Map> { - constructor(lockedEntities: LockedEntity[] = []) { - // Construct a Map> from user-friendly input args - super( - lockedEntities.map(({ appId, services }) => [appId, new Set(services)]), - ); - } - - // Add one or more locked services to an appId - public add(appId: number, services: string | string[]): void { - if (typeof services === 'string') { - services = [services]; - } - if (this.has(appId)) { - const lockedSvcs = this.get(appId)!; - services.forEach((s) => lockedSvcs.add(s)); - } else { - this.set(appId, new Set(services)); - } - } +interface LockingOpts { + /** + * Delete existing user locks if any + */ + force: boolean; /** - * @private Use this.getServices instead as there is no need to return - * a mutable reference to the internal Set data structure. + * If the locks are being held by another operation + * on the supervisor, this is the max time that the call + * will wait before throwing */ - public get(appId: number): Set | undefined { - return super.get(appId); - } - - // Return an array copy of locked services under an appId - public getServices(appId: number): string[] { - return this.has(appId) ? Array.from(this.get(appId)!) : []; - } - - // Return whether a service is locked under an appId - public isLocked(appId: number, service: string): boolean { - return this.has(appId) && this.get(appId)!.has(service); - } + maxWaitMs: number; } -// A wrapper function for lockfile.getLocksTaken that filters for Supervisor-taken locks. -// Exported for tests only; getServicesLockedByAppId is the intended public interface. -export async function getLocksTaken( - rootDir: string = pathOnRoot(BASE_LOCK_DIR), -): Promise { - return await lockfile.findAll({ - root: rootDir, - filter: (l) => l.path.endsWith('updates.lock') && l.owner === LOCKFILE_UID, +async function takeGlobalLockOrFail( + appId: string, + maxWaitMs = 0, // 0 === wait forever +): Promise { + let abort = false; + const lockingPromise = takeGlobalLockRW(appId).then((disposer) => { + // Even if the timer resolves first, takeGlobalLockRW + // will eventually resolve and in that case we need to call the disposer + // to avoid a deadlock + if (abort) { + disposer(); + return () => { + /* noop */ + }; + } else { + return disposer; + } }); -} -/** - * Return a list of services that are locked by the Supervisor under each appId. - * Both `resin-updates.lock` and `updates.lock` should be present per - * [appId, serviceName] pair for a service to be considered locked. - */ -export async function getServicesLockedByAppId(): Promise { - const locksTaken = await getLocksTaken(); - // Group locksTaken paths by appId & serviceName. - // filesTakenByAppId is of type Map>> - // and represents files taken under every [appId, serviceName] pair. - const filesTakenByAppId = new Map>>(); - for (const lockTakenPath of locksTaken) { - const [appId, serviceName, filename] = - getIdentifiersFromPath(lockTakenPath); - if ( - !StringIdentifier.is(appId) || - !DockerName.is(serviceName) || - !filename?.match(/updates\.lock/) - ) { - continue; - } - const numAppId = +appId; - if (!filesTakenByAppId.has(numAppId)) { - filesTakenByAppId.set(numAppId, new Map()); - } - const servicesTaken = filesTakenByAppId.get(numAppId)!; - if (!servicesTaken.has(serviceName)) { - servicesTaken.set(serviceName, new Set()); - } - servicesTaken.get(serviceName)!.add(filename); + const promises: Array> = [ + lockingPromise, + ]; + + const ac = new AbortController(); + const signal = ac.signal; + if (maxWaitMs > 0) { + promises.push(setTimeout(maxWaitMs, 'abort', { signal })); } - // Construct a LocksTakenMap from filesTakenByAppId, which represents - // services locked by the Supervisor. - const servicesByAppId = new LocksTakenMap(); - for (const [appId, servicesTaken] of filesTakenByAppId) { - for (const [serviceName, filenames] of servicesTaken) { - if ( - filenames.has('resin-updates.lock') && - filenames.has('updates.lock') - ) { - servicesByAppId.add(appId, serviceName); + try { + const res = await Promise.race(promises); + if (res === 'abort') { + abort = true; + throw new UpdatesLockedError( + `Locks for app ${appId} are being held by another supervisor operation`, + ); + } + return lockingPromise; + } finally { + // Clear the timeout + ac.abort(); + } +} + +export interface Lock { + unlock(): Promise; +} + +export interface Lockable { + lock(opts?: Partial): Promise; +} + +function newLockable(appId: string, services: string[]): Lockable { + async function unlockApp(locks: string[], release: () => void) { + try { + logger.logSystemEvent(logTypes.releaseLock, { appId }); + await Promise.all(locks.map((l) => lockfile.unlock(l))); + } finally { + release(); + } + } + + async function lockApp({ + force = false, + maxWaitMs = 0, + }: Partial = {}) { + // Log before taking the global lock to detect + // possible deadlocks + logger.logSystemEvent(logTypes.takeLock, { + appId, + services, + force, + }); + + // Try to take the global lock for the given appId + // this ensures the user app locks are only taken in a single + // place on the supervisor + const release: ReadWriteLock.Release = await takeGlobalLockOrFail( + appId, + maxWaitMs, + ); + + // This keeps a list of all locks currently being + // held by the lockable + let currentLocks: string[] = []; + + try { + // Find all the locks already taken for the appId + // if this is not empty it probably means these locks are from + // a previous run of the supervisor + currentLocks = await lockfile.findAll({ + root: pathOnRoot(lockPath(appId)), + filter: (l) => + l.path.endsWith('updates.lock') && l.owner === LOCKFILE_UID, + }); + + // Group locks by service + const locksByService = services.map((service) => { + const existing = currentLocks.filter((lockFile) => + lockFilesOnHost(appId, service).includes(lockFile), + ); + return { service, existing }; + }, {}); + + // Filter out services that already have Supervisor-taken locks. + // This needs to be done after taking the appId write lock to avoid + // race conditions with locking. + const servicesWithMissingLocks = locksByService.filter( + ({ existing }) => existing.length < 2, + ); + + // For every service that has yet to be fully locked we need to + // take the locks + for (const { service, existing } of servicesWithMissingLocks) { + // Create the directory if it doesn't exist + await mkdirp(pathOnRoot(lockPath(appId, service))); + + // We will only take those locks that have not yet been taken by + // the supervisor + const missingLocks = lockFilesOnHost(appId, service).filter( + (l) => !existing.includes(l), + ); + + for await (const file of missingLocks) { + try { + if (force) { + // If force: true we remove the lock first + await lockfile.unlock(file); + } + await lockfile.lock(file, LOCKFILE_UID); + + // If locking was successful, we update the current locks + // list + currentLocks.push(file); + } catch (e) { + if (lockfile.LockfileExistsError.is(e)) { + // Throw more descriptive error + throw new UpdatesLockedError( + `Lockfile exists for { appId: ${appId}, service: ${service} }`, + ); + } + // Otherwise just throw the error + throw e; + } + } } + + return { + unlock: async () => { + await unlockApp(currentLocks, release); + }, + }; + } catch (err) { + // If any error happens while taking locks, we need to unlock + // to avoid some locks being held by user apps and some by + // the supervisor + await unlockApp(currentLocks, release); + + // Re-throw error to be handled in caller + throw err; } } - return servicesByAppId; + + return { lock: lockApp }; } +export const Lockable = { + from(appId: string | number, services: string[]) { + // Convert appId to string so we always + // use the same value when taking the process lock + return newLockable(appId.toString(), services); + }, +}; + /** - * Try to take the locks for an application. If force is set, it will remove - * all existing lockfiles before performing the operation + * Call the given function after locks for the given apps + * have been taken. * - * TODO: convert to native Promises and async/await. May require native implementation of Bluebird's dispose / using + * This is compatible with Lockable.lock() in the sense that only one locking + * operation is allowed at the time on the supervisor + * + * By default the call will wait 10 seconds for shared process locks before + * giving up */ -export async function lock( - appId: number | number[], - { force = false }: { force: boolean }, +export async function withLock( + appIds: number | number[], fn: () => Resolvable, + { force = false, maxWaitMs = 10 * 1000 }: Partial = {}, ): Promise { - const appIdsToLock = Array.isArray(appId) ? appId : [appId]; - if (!appId || !appIdsToLock.length) { + appIds = Array.isArray(appIds) ? appIds : [appIds]; + if (appIds.length === 0) { return fn(); } - // Sort appIds so they are always locked in the same sequence - const sortedIds = appIdsToLock.sort(); + // Always lock in the same order + appIds = appIds.sort(); - let lockOverride: boolean; + const locks: Lock[] = []; try { - lockOverride = await config.get('lockOverride'); - } catch (err: any) { - throw new InternalInconsistencyError( - `Error getting lockOverride config value: ${err?.message ?? err}`, - ); - } + const lockables: Lockable[] = []; + for (const appId of appIds) { + const appLockDir = pathOnRoot(lockPath(appId)); + const services: string[] = []; + try { + // Read the contents of the app lockdir + const dirs = await fs.readdir(appLockDir); + const statResults = await Promise.allSettled( + dirs.map(async (service) => ({ + service, + stat: await fs.lstat(path.join(appLockDir, service)), + })), + ); - const releases = new Map void>(); - try { - for (const id of sortedIds) { - const lockDir = pathOnRoot(lockPath(id)); - // Acquire write lock for appId - releases.set(id, await takeGlobalLockRW(id)); - // Get list of service folders in lock directory - const serviceFolders = await fs.readdir(lockDir).catch((e) => { + for (const res of statResults) { + // Ignore rejected results + if ( + res.status === 'fulfilled' && + res.value.stat.isDirectory() && + !res.value.stat.isSymbolicLink() + ) { + services.push(res.value.service); + } + } + } catch (e) { + // If the directory does not exist, continue if (isENOENT(e)) { - return []; + continue; } throw e; - }); - // Attempt to create a lock for each service - for (const service of serviceFolders) { - await lockService(id, service, force || lockOverride); + } + + lockables.push(Lockable.from(appId, services)); + } + + // Lock all apps at once to avoid adding up the wait times + const lockingResults = await Promise.allSettled( + lockables.map((l) => l.lock({ force, maxWaitMs })), + ); + + let err = null; + for (const res of lockingResults) { + if (res.status === 'fulfilled') { + locks.push(res.value); + } else { + err = res.reason; } } - // Resolve the function passed + + // If there are any errors, then throw before calling the function + if (err != null) { + throw err; + } + + // If we got here, then all locks were successfully acquired + // call the function now return await fn(); } finally { - for (const [id, release] of releases.entries()) { - // Try to dispose all the locks - await dispose(id, release); - } - } -} - -async function lockService( - appId: number, - service: string, - force: boolean = false, -): Promise { - const serviceLockFiles = lockFilesOnHost(appId, service); - for await (const file of serviceLockFiles) { - try { - if (force) { - await lockfile.unlock(file); - } - await lockfile.lock(file, LOCKFILE_UID); - } catch (e) { - if (lockfile.LockfileExistsError.is(e)) { - // Throw more descriptive error - throw new UpdatesLockedError( - `Lockfile exists for { appId: ${appId}, service: ${service} }`, - ); - } - // Otherwise just throw the error - throw e; - } + // Unlock all taken locks + await Promise.all(locks.map((l) => l.unlock())); } } diff --git a/test/integration/compose/application-manager.spec.ts b/test/integration/compose/application-manager.spec.ts index c7288b45..07d66f8b 100644 --- a/test/integration/compose/application-manager.spec.ts +++ b/test/integration/compose/application-manager.spec.ts @@ -8,7 +8,6 @@ import { Network } from '~/src/compose/network'; import * as networkManager from '~/src/compose/network-manager'; import { Volume } from '~/src/compose/volume'; import * as config from '~/src/config'; -import { LocksTakenMap } from '~/lib/update-lock'; import { createDockerImage } from '~/test-lib/docker-helper'; import { createService, @@ -113,7 +112,13 @@ describe('compose/application-manager', () => { availableImages, containerIdsByAppId, // Mock lock taken to avoid takeLock step - locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), + appLocks: { + '1': { + async unlock() { + /* noop */ + }, + }, + }, }, ); @@ -225,7 +230,13 @@ describe('compose/application-manager', () => { availableImages, containerIdsByAppId, // Mock lock taken to avoid takeLock step - locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), + appLocks: { + '1': { + async unlock() { + /* noop */ + }, + }, + }, }, ); @@ -277,7 +288,13 @@ describe('compose/application-manager', () => { availableImages, containerIdsByAppId, // Mock lock taken to avoid takeLock step - locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), + appLocks: { + '1': { + async unlock() { + /* noop */ + }, + }, + }, }, ); @@ -410,7 +427,13 @@ describe('compose/application-manager', () => { availableImages: c1.availableImages, containerIdsByAppId: c1.containerIdsByAppId, // Mock lock taken for `main` service which just needs metadata updated - locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), + appLocks: { + '1': { + async unlock() { + /* noop */ + }, + }, + }, }, ); // There should be two noop steps, one for target service which is still downloading, @@ -457,10 +480,13 @@ describe('compose/application-manager', () => { downloading, availableImages, containerIdsByAppId, - // Mock locks taken for all services in either current or target state - locksTaken: new LocksTakenMap([ - { appId: 1, services: ['old', 'main', 'new'] }, - ]), + appLocks: { + '1': { + async unlock() { + /* noop */ + }, + }, + }, }, ); // Service `old` is safe to kill after download for `new` has completed @@ -506,11 +532,14 @@ describe('compose/application-manager', () => { // to avoid removeImage steps availableImages: [], containerIdsByAppId: c1.containerIdsByAppId, - // Mock locks for service to be updated via updateMetadata - // or kill to avoid takeLock step - locksTaken: new LocksTakenMap([ - { appId: 1, services: ['old', 'main', 'new'] }, - ]), + // Mock locks to avoid takeLock step + appLocks: { + '1': { + async unlock() { + /* noop */ + }, + }, + }, }, ); // Service `new` should be fetched @@ -583,10 +612,13 @@ describe('compose/application-manager', () => { }), ], containerIdsByAppId: c1.containerIdsByAppId, - // Mock lock taken for all services in target state - locksTaken: new LocksTakenMap([ - { appId: 1, services: ['old', 'main', 'new'] }, - ]), + appLocks: { + '1': { + async unlock() { + /* noop */ + }, + }, + }, }, ); // Service `new` should be started @@ -629,9 +661,13 @@ describe('compose/application-manager', () => { containerIdsByAppId: c1.containerIdsByAppId, // Mock locks for service to be updated via updateMetadata // or kill to avoid takeLock step - locksTaken: new LocksTakenMap([ - { appId: 1, services: ['old', 'main', 'new'] }, - ]), + appLocks: { + '1': { + async unlock() { + /* noop */ + }, + }, + }, }, ); // Service `new` should be fetched @@ -705,9 +741,13 @@ describe('compose/application-manager', () => { ], containerIdsByAppId: c1.containerIdsByAppId, // Mock lock taken for all services in target state - locksTaken: new LocksTakenMap([ - { appId: 1, services: ['main', 'new'] }, - ]), + appLocks: { + '1': { + async unlock() { + /* noop */ + }, + }, + }, }, ); // Service `new` should be started @@ -805,9 +845,13 @@ describe('compose/application-manager', () => { ], containerIdsByAppId, // Mock locks taken for all services in target state - locksTaken: new LocksTakenMap([ - { appId: 1, services: ['one', 'two'] }, - ]), + appLocks: { + '1': { + async unlock() { + /* noop */ + }, + }, + }, }, ); expectSteps('start', steps3, 2); @@ -877,7 +921,13 @@ describe('compose/application-manager', () => { availableImages, containerIdsByAppId, // Mock lock taken to avoid takeLock step - locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), + appLocks: { + '1': { + async unlock() { + /* noop */ + }, + }, + }, }, ); @@ -959,9 +1009,13 @@ describe('compose/application-manager', () => { availableImages, containerIdsByAppId, // Mock locks taken to avoid takeLock step - locksTaken: new LocksTakenMap([ - { appId: 1, services: ['main', 'dep'] }, - ]), + appLocks: { + '1': { + async unlock() { + /* noop */ + }, + }, + }, }, ); @@ -1028,9 +1082,13 @@ describe('compose/application-manager', () => { availableImages, containerIdsByAppId, // Mock locks taken to avoid takeLock step - locksTaken: new LocksTakenMap([ - { appId: 1, services: ['main', 'dep'] }, - ]), + appLocks: { + '1': { + async unlock() { + /* noop */ + }, + }, + }, }, ); @@ -1100,9 +1158,13 @@ describe('compose/application-manager', () => { availableImages, containerIdsByAppId, // Mock locks taken to avoid takeLock step - locksTaken: new LocksTakenMap([ - { appId: 1, services: ['main', 'dep'] }, - ]), + appLocks: { + '1': { + async unlock() { + /* noop */ + }, + }, + }, }, ); @@ -1146,10 +1208,18 @@ describe('compose/application-manager', () => { availableImages, containerIdsByAppId, // Mock lock already taken for the new and leftover services - locksTaken: new LocksTakenMap([ - { appId: 5, services: ['old-service'] }, - { appId: 1, services: ['main'] }, - ]), + appLocks: { + '1': { + async unlock() { + /* noop */ + }, + }, + '5': { + async unlock() { + /* noop */ + }, + }, + }, }, ); @@ -1665,7 +1735,13 @@ describe('compose/application-manager', () => { availableImages, containerIdsByAppId, // Mock locks taken to avoid takeLock step - locksTaken: new LocksTakenMap([{ appId: 2, services: ['main'] }]), + appLocks: { + '2': { + async unlock() { + /* noop */ + }, + }, + }, }, ); @@ -1747,10 +1823,18 @@ describe('compose/application-manager', () => { availableImages, containerIdsByAppId, // Mock locks taken to avoid takeLock step - locksTaken: new LocksTakenMap([ - { appId: 1, services: ['main'] }, - { appId: 2, services: ['main'] }, - ]), + appLocks: { + '1': { + async unlock() { + /* noop */ + }, + }, + '2': { + async unlock() { + /* noop */ + }, + }, + }, }, ); @@ -1816,9 +1900,13 @@ describe('compose/application-manager', () => { availableImages, containerIdsByAppId, // Mock locks taken - locksTaken: new LocksTakenMap([ - { appId: 1, services: ['one', 'two'] }, - ]), + appLocks: { + '1': { + async unlock() { + /* noop */ + }, + }, + }, }, ); expectSteps('kill', steps2, 2); @@ -1875,9 +1963,13 @@ describe('compose/application-manager', () => { availableImages, containerIdsByAppId, // Mock locks taken - locksTaken: new LocksTakenMap([ - { appId: 1, services: ['one', 'two'] }, - ]), + appLocks: { + '1': { + async unlock() { + /* noop */ + }, + }, + }, }, ); expectSteps('stop', steps2, 2); @@ -1934,9 +2026,13 @@ describe('compose/application-manager', () => { availableImages, containerIdsByAppId, // Mock locks taken - locksTaken: new LocksTakenMap([ - { appId: 1, services: ['one', 'two'] }, - ]), + appLocks: { + '1': { + async unlock() { + /* noop */ + }, + }, + }, }, ); expectSteps('start', steps2, 2); @@ -2100,9 +2196,13 @@ describe('compose/application-manager', () => { availableImages, containerIdsByAppId, // Mock locks taken - locksTaken: new LocksTakenMap([ - { appId: 1, services: ['one', 'two'] }, - ]), + appLocks: { + '1': { + async unlock() { + /* noop */ + }, + }, + }, }, ); expectSteps('kill', steps4, 2); @@ -2172,9 +2272,13 @@ describe('compose/application-manager', () => { availableImages, containerIdsByAppId, // Mock locks taken - locksTaken: new LocksTakenMap([ - { appId: 1, services: ['one', 'two'] }, - ]), + appLocks: { + '1': { + async unlock() { + /* noop */ + }, + }, + }, }, ); expectSteps('kill', steps2, 2); @@ -2231,7 +2335,13 @@ describe('compose/application-manager', () => { availableImages, containerIdsByAppId, // Mock locks taken - locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), + appLocks: { + '1': { + async unlock() { + /* noop */ + }, + }, + }, }, ); expectSteps('kill', steps2); @@ -2254,7 +2364,13 @@ describe('compose/application-manager', () => { availableImages: intermediateCurrent.availableImages, containerIdsByAppId: intermediateCurrent.containerIdsByAppId, // Mock locks taken - locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), + appLocks: { + '1': { + async unlock() { + /* noop */ + }, + }, + }, }, ); expectSteps('removeNetwork', steps3); @@ -2320,7 +2436,13 @@ describe('compose/application-manager', () => { availableImages, containerIdsByAppId, // Mock locks taken - locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), + appLocks: { + '1': { + async unlock() { + /* noop */ + }, + }, + }, }, ); expectSteps('kill', steps2, 1); @@ -2343,7 +2465,13 @@ describe('compose/application-manager', () => { availableImages: intermediateCurrent.availableImages, containerIdsByAppId: intermediateCurrent.containerIdsByAppId, // Mock locks taken - locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), + appLocks: { + '1': { + async unlock() { + /* noop */ + }, + }, + }, }, ); expectSteps('removeNetwork', steps3); @@ -2405,7 +2533,13 @@ describe('compose/application-manager', () => { availableImages, containerIdsByAppId, // Mock locks taken - locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), + appLocks: { + '1': { + async unlock() { + /* noop */ + }, + }, + }, }, ); expectSteps('kill', steps2, 1); @@ -2424,7 +2558,13 @@ describe('compose/application-manager', () => { availableImages: intermediateCurrent.availableImages, containerIdsByAppId: intermediateCurrent.containerIdsByAppId, // Mock locks taken - locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), + appLocks: { + '1': { + async unlock() { + /* noop */ + }, + }, + }, }, ); expectSteps('removeVolume', steps3); @@ -2456,9 +2596,13 @@ describe('compose/application-manager', () => { downloading, availableImages, containerIdsByAppId, - locksTaken: new LocksTakenMap([ - { appId: 1, services: ['one', 'two'] }, - ]), + appLocks: { + '1': { + async unlock() { + /* noop */ + }, + }, + }, }, ); const [releaseLockStep] = expectSteps('releaseLock', steps, 1, 1); @@ -2950,9 +3094,13 @@ describe('compose/application-manager', () => { availableImages, containerIdsByAppId, // Mock locks taken for all services in target state - locksTaken: new LocksTakenMap([ - { appId: 1, services: ['one', 'two', 'three', 'four'] }, - ]), + appLocks: { + '1': { + async unlock() { + /* noop */ + }, + }, + }, }); [startStep1, startStep2, startStep3, startStep4].forEach((step) => { diff --git a/test/integration/device-api/actions.spec.ts b/test/integration/device-api/actions.spec.ts index 11504491..833f3b12 100644 --- a/test/integration/device-api/actions.spec.ts +++ b/test/integration/device-api/actions.spec.ts @@ -5,6 +5,7 @@ import Docker from 'dockerode'; import request from 'supertest'; import { setTimeout } from 'timers/promises'; import { testfs } from 'mocha-pod'; +import { promises as fs } from 'fs'; import * as deviceState from '~/src/device-state'; import * as config from '~/src/config'; @@ -378,7 +379,9 @@ describe('manages application lifecycle', () => { await setTimeout(1000); // User lock should be overridden - expect(await updateLock.getLocksTaken()).to.deep.equal([]); + await expect( + fs.readdir(pathOnRoot(updateLock.lockPath(1, 'server'))), + ).to.eventually.deep.equal([]); }); it('should restart an application when user locks are present if lockOverride is specified', async () => { @@ -423,7 +426,9 @@ describe('manages application lifecycle', () => { await setTimeout(1000); // User lock should be overridden - expect(await updateLock.getLocksTaken()).to.deep.equal([]); + await expect( + fs.readdir(pathOnRoot(updateLock.lockPath(1, 'server'))), + ).to.eventually.deep.equal([]); }); it('should restart service by removing and recreating corresponding container', async () => { @@ -519,7 +524,9 @@ describe('manages application lifecycle', () => { await setTimeout(1000); // User lock should be overridden - expect(await updateLock.getLocksTaken()).to.deep.equal([]); + await expect( + fs.readdir(pathOnRoot(updateLock.lockPath(1, 'server'))), + ).to.eventually.deep.equal([]); }); it('should restart service when user locks are present if lockOverride is specified', async () => { @@ -566,7 +573,9 @@ describe('manages application lifecycle', () => { await setTimeout(1000); // User lock should be overridden - expect(await updateLock.getLocksTaken()).to.deep.equal([]); + await expect( + fs.readdir(pathOnRoot(updateLock.lockPath(1, 'server'))), + ).to.eventually.deep.equal([]); }); it('should stop a running service', async () => { @@ -873,7 +882,9 @@ describe('manages application lifecycle', () => { await setTimeout(500); // User lock should be overridden - expect(await updateLock.getLocksTaken()).to.deep.equal([]); + await expect( + fs.readdir(pathOnRoot(updateLock.lockPath(1, 'server'))), + ).to.eventually.deep.equal([]); }); it('should restart an application when user locks are present if lockOverride is specified', async () => { @@ -918,7 +929,9 @@ describe('manages application lifecycle', () => { await setTimeout(500); // User lock should be overridden - expect(await updateLock.getLocksTaken()).to.deep.equal([]); + await expect( + fs.readdir(pathOnRoot(updateLock.lockPath(1, 'server'))), + ).to.eventually.deep.equal([]); }); it('should restart service by removing and recreating corresponding container', async () => { @@ -1026,7 +1039,9 @@ describe('manages application lifecycle', () => { await setTimeout(500); // User lock should be overridden - expect(await updateLock.getLocksTaken()).to.deep.equal([]); + await expect( + fs.readdir(pathOnRoot(updateLock.lockPath(1, 'server'))), + ).to.eventually.deep.equal([]); }); it('should restart service when user locks are present if lockOverride is specified', async () => { @@ -1073,7 +1088,9 @@ describe('manages application lifecycle', () => { await setTimeout(500); // User lock should be overridden - expect(await updateLock.getLocksTaken()).to.deep.equal([]); + await expect( + fs.readdir(pathOnRoot(updateLock.lockPath(1, 'server'))), + ).to.eventually.deep.equal([]); }); it('should stop a running service', async () => { diff --git a/test/integration/lib/update-lock.spec.ts b/test/integration/lib/update-lock.spec.ts index 2481f56b..87a61841 100644 --- a/test/integration/lib/update-lock.spec.ts +++ b/test/integration/lib/update-lock.spec.ts @@ -1,28 +1,15 @@ import { expect } from 'chai'; import * as path from 'path'; import { promises as fs } from 'fs'; -import { testfs } from 'mocha-pod'; -import type { TestFs } from 'mocha-pod'; import { setTimeout } from 'timers/promises'; -import { watch } from 'chokidar'; +import { testfs } from 'mocha-pod'; import * as updateLock from '~/lib/update-lock'; -import { UpdatesLockedError } from '~/lib/errors'; -import * as config from '~/src/config'; -import * as lockfile from '~/lib/lockfile'; +import { Lockable } from '~/lib/update-lock'; +import { isENOENT, UpdatesLockedError } from '~/lib/errors'; import { pathOnRoot, pathOnState } from '~/lib/host-utils'; -import { mkdirp } from '~/lib/fs-utils'; -import { takeGlobalLockRW } from '~/lib/process-lock'; describe('lib/update-lock', () => { - before(async () => { - await config.initialized(); - }); - - beforeEach(async () => { - await config.set({ lockOverride: false }); - }); - describe('abortIfHUPInProgress', () => { const breadcrumbFiles = [ 'rollback-health-breadcrumb', @@ -70,126 +57,250 @@ describe('lib/update-lock', () => { }); }); - describe('Lock/dispose functionality', () => { - const testAppId = 1234567; - const testServiceName = 'test'; + const lockdir = (appId: number | string, serviceName: string): string => + pathOnRoot(updateLock.lockPath(appId, serviceName)); - const supportedLockfiles = ['resin-updates.lock', 'updates.lock']; + async function expectLocks( + appId: string | number, + services: string[], + exists = true, + msg = `expect lock to ${exists ? 'exist' : 'not exist'}`, + ) { + for (const svcName of services) { + const svcMsg = `${svcName}: ${msg}`; + try { + const contents = await fs.readdir(lockdir(appId, svcName)); + if (exists) { + expect(contents, svcMsg).to.have.members([ + 'resin-updates.lock', + 'updates.lock', + ]); + } else { + expect(contents, svcMsg).to.deep.equal([]); + } + } catch (e) { + if (!isENOENT(e)) { + throw e; + } - const takeLocks = () => - Promise.all( - supportedLockfiles.map((lf) => - lockfile.lock( - path.join(lockdir(testAppId, testServiceName), lf), - updateLock.LOCKFILE_UID, - ), - ), - ); + if (exists) { + expect.fail(svcMsg); + } + } + } + } - const releaseLocks = async () => { - await Promise.all( - (await updateLock.getLocksTaken()).map((lock) => lockfile.unlock(lock)), - ); - - // Remove any other lockfiles created for the testAppId - await Promise.all( - supportedLockfiles.map((lf) => - lockfile.unlock(path.join(lockdir(testAppId, testServiceName), lf)), - ), - ); - }; - - const lockdir = (appId: number, serviceName: string): string => - pathOnRoot(updateLock.lockPath(appId, serviceName)); - - const expectLocks = async ( - exists: boolean, - msg?: string, - appId = testAppId, - serviceName = testServiceName, - ) => - expect( - fs.readdir(lockdir(appId, serviceName)), - msg, - ).to.eventually.deep.equal(exists ? supportedLockfiles : []); - - before(async () => { - // Ensure the directory is available for all tests - await fs.mkdir(lockdir(testAppId, testServiceName), { + describe('Lockable', () => { + afterEach(async () => { + await fs.rm(pathOnRoot('/tmp/balena-supervisor/services/123'), { recursive: true, + force: true, }); }); + it('allows to lock a specific app by id and the given services', async () => { + // No locks before locking takes place + await expectLocks(123, ['one', 'two'], false); + + const lockable = Lockable.from(123, ['one', 'two']); + + const lock = await lockable.lock(); + // Locks should exist now + await expectLocks(123, ['one', 'two']); + + await lock.unlock(); + + // Locks should have been removed + await expectLocks(123, ['one', 'two'], false); + }); + + it('allows only one app lock to be taken at a time', async () => { + // No locks before locking takes place + await expectLocks(123, ['one', 'two'], false); + + const lockable = Lockable.from(123, ['one', 'two']); + + const lock = await lockable.lock(); + // Locks should exist now + await expectLocks(123, ['one', 'two']); + + // Try to take the lock again. Set a timeout of 10ms + await expect(lockable.lock({ maxWaitMs: 10 })).to.be.rejectedWith( + 'Locks for app 123 are being held by another supervisor operation', + ); + + await lock.unlock(); + + // Locks should have been removed + await expectLocks(123, ['one', 'two'], false); + }); + + it('creates only missing locks', async () => { + const serviceLock = path.join(lockdir(123, 'two'), 'resin-updates.lock'); + const tmp = await testfs({ + [serviceLock]: testfs.file({ uid: updateLock.LOCKFILE_UID }), + }).enable(); + + const lockable = Lockable.from(123, ['one', 'two']); + + // Locking should succeed + const lock = await lockable.lock(); + + // Locks should exist now + await expectLocks(123, ['one', 'two']); + + await lock.unlock(); + + // Locks should have been removed + await expectLocks(123, ['one', 'two'], false); + await tmp.restore(); + }); + + it('throws UpdatesLockedError if user held lockfiles exist', async () => { + const serviceLock = path.join(lockdir(123, 'two'), 'resin-updates.lock'); + const tmp = await testfs({ + [serviceLock]: testfs.file({ uid: 0 }), + }).enable(); + + const lockable = Lockable.from(123, ['one', 'two']); + + // Locking should fail + await expect(lockable.lock()).to.be.rejectedWith( + 'Lockfile exists for { appId: 123, service: two }', + ); + + // Supervisor locks should not exist + await expect(fs.readdir(lockdir(123, 'one'))).to.eventually.deep.equal( + [], + ); + await expect(fs.readdir(lockdir(123, 'two'))).to.eventually.deep.equal([ + 'resin-updates.lock', + ]); + await tmp.restore(); + }); + + it('takes locks if `force` is used', async () => { + const serviceLock = path.join(lockdir(123, 'two'), 'resin-updates.lock'); + const tmp = await testfs({ + [serviceLock]: testfs.file({ uid: 0 }), + }).enable(); + + const lockable = Lockable.from(123, ['one', 'two']); + + // Locking should succeed + const lock = await lockable.lock({ force: true }); + + // Locks should exist now + await expectLocks(123, ['one', 'two']); + + await lock.unlock(); + + // All should have been removed now + await expectLocks(123, ['one', 'two'], false); + await tmp.restore(); + }); + + it('disposes supervisor locks if there are user held locks', async () => { + const svLock = path.join(lockdir(123, 'two'), 'resin-updates.lock'); + const userLock = path.join(lockdir(123, 'two'), 'updates.lock'); + const tmp = await testfs({ + [svLock]: testfs.file({ uid: updateLock.LOCKFILE_UID }), + [userLock]: testfs.file({ uid: 0 }), + }).enable(); + + const lockable = Lockable.from(123, ['one', 'two']); + + // Locking should fail + await expect(lockable.lock()).to.be.rejected; + + // Supervisor locks should not exist + await expect(fs.readdir(lockdir(123, 'one'))).to.eventually.deep.equal( + [], + ); + + // Only the user lock remains + await expect(fs.readdir(lockdir(123, 'two'))).to.eventually.deep.equal([ + 'updates.lock', + ]); + await tmp.restore(); + }); + }); + + describe('withLock', () => { afterEach(async () => { - // Cleanup all locks between tests - await releaseLocks(); + await fs.rm(pathOnRoot('/tmp/balena-supervisor/services/123'), { + recursive: true, + force: true, + }); }); it('should take the lock, run the function, then dispose of locks', async () => { - await expectLocks( - false, - 'locks should not exist before the lock is taken', - ); + // Create some empty directories to simulate two services + await fs.mkdir(lockdir(123, 'one'), { recursive: true }); + await fs.mkdir(lockdir(123, 'two'), { recursive: true }); + + // No locks before locking takes place + await expectLocks(123, ['one', 'two'], false); await expect( - updateLock.lock(testAppId, { force: false }, () => + updateLock.withLock(123, () => // At this point the locks should be taken and not removed // until this function has been resolved - expectLocks(true, 'lockfiles should exist while the lock is active'), + expectLocks(123, ['one', 'two']), ), ).to.be.fulfilled; - await expectLocks( - false, - 'locks should not exist after the lock is released', - ); + // Locks should be removed after + await expectLocks(123, ['one', 'two'], false); }); it('should throw UpdatesLockedError if lockfiles exists', async () => { // Take the locks before testing - await takeLocks(); + // TODO: enable mocha-pod to work with empty directories + await fs.mkdir(lockdir(123, 'one'), { recursive: true }); + const serviceLock = path.join(lockdir(123, 'two'), 'resin-updates.lock'); + const tmp = await testfs({ + [serviceLock]: testfs.file({ uid: 0 }), + }).enable(); - await expectLocks(true, 'locks should exist before the lock is taken'); + await expect( + updateLock.withLock(123, () => expect.fail('This is the wrong error')), + ).to.be.rejectedWith('Lockfile exists for { appId: 123, service: two }'); - await updateLock - .lock(testAppId, { force: false }, () => - Promise.reject( - 'the lock function should not invoke the callback if locks are taken', - ), - ) - .catch((err) => expect(err).to.be.instanceOf(UpdatesLockedError)); - - // Since the lock-taking with `nobody` uid failed, there should be no locks to dispose of - expect(await updateLock.getLocksTaken()).to.have.length(0); + // Supervisor locks should not exist + await expect(fs.readdir(lockdir(123, 'one'))).to.eventually.deep.equal( + [], + ); + await expect(fs.readdir(lockdir(123, 'two'))).to.eventually.deep.equal([ + 'resin-updates.lock', + ]); // Restore the locks that were taken at the beginning of the test - await releaseLocks(); + await tmp.restore(); }); it('should dispose of taken locks on any other errors', async () => { - await expectLocks(false, 'locks should not exist before lock is called'); - await expect( - updateLock.lock( - testAppId, - { force: false }, - // At this point 2 lockfiles have been written, so this is testing - // that even if the function rejects, lockfiles will be disposed of - () => - expectLocks( - true, - 'locks should be owned by the calling function', - ).then(() => Promise.reject('Test error')), - ), - ).to.be.rejectedWith('Test error'); + // Create some empty directories to simulate two services + await fs.mkdir(lockdir(123, 'one'), { recursive: true }); + await fs.mkdir(lockdir(123, 'two'), { recursive: true }); - await expectLocks( - false, - 'locks should be removed if an error happens within the lock callback', - ); + // No locks before locking takes place + await expectLocks(123, ['one', 'two'], false); + + await expect( + updateLock.withLock(123, async () => { + await expectLocks(123, ['one', 'two']); + throw new Error('This is an error'); + }), + ).to.be.rejectedWith('This is an error'); + + // Locks should not exist + await expectLocks(123, ['one', 'two'], false); }); it('locks all applications before resolving input function', async () => { const appIds = [111, 222, 333]; + const testServiceName = 'main'; // Set up necessary lock directories await Promise.all( @@ -197,571 +308,121 @@ describe('lib/update-lock', () => { fs.mkdir(lockdir(id, testServiceName), { recursive: true }), ), ); - await expect( - updateLock.lock(appIds, { force: false }, () => - // At this point the locks should be taken and not removed + updateLock.withLock(appIds, async () => + /// At this point the locks should be taken and not removed // until this function has been resolved // Both `updates.lock` and `resin-updates.lock` should have been taken Promise.all( - appIds.map((appId) => - expectLocks( - true, - `locks for app(${appId}) should exist`, - appId, - testServiceName, - ), - ), + appIds.map((appId) => expectLocks(appId, [testServiceName])), ), ), - ).to.eventually.be.fulfilled; + ).to.be.fulfilled; + + // No locks should exist at this point + await Promise.all( + appIds.map((appId) => expectLocks(appId, [testServiceName], false)), + ); - // Everything that was locked should have been unlocked after function resolves await Promise.all( appIds.map((appId) => - expectLocks( - false, - `locks for app(${appId}) should have been released`, - appId, - testServiceName, - ), - ), - ).finally(() => - // In case the above fails, we need to make sure to cleanup the lockdir - Promise.all( - appIds - .map((appId) => - supportedLockfiles.map((lf) => - lockfile.unlock(path.join(lockdir(appId, testServiceName), lf)), - ), - ) - .flat(), + fs.rm(pathOnRoot(`/tmp/balena-supervisor/services/${appId}`), { + recursive: true, + force: true, + }), ), ); }); - it('resolves input function without locking when appId is null', async () => { - await takeLocks(); + it('throws UpdatesLockedError if a lock in any app exist', async () => { + const appIds = [111, 222, 333]; + const testServiceName = 'main'; - await expect( - updateLock.lock(null as any, { force: false }, () => Promise.resolve()), - ).to.be.fulfilled; - - await expectLocks( - true, - 'locks should not be touched by an unrelated lock() call', - ); - - await releaseLocks(); - }); - - it('unlocks lockfile to resolve function if force option specified', async () => { - await takeLocks(); - - await expect( - updateLock.lock(testAppId, { force: true }, () => - expectLocks( - true, - 'locks should be deleted and taken again by the lock() call', - ), - ), - ).to.be.fulfilled; - - await expectLocks( - false, - 'using force gave lock ownership to the callback, so they should now be deleted', - ); - }); - - it('unlocks lockfile to resolve function if lockOverride option specified', async () => { - await takeLocks(); - - // Change the configuration - await config.set({ lockOverride: true }); - - await expect( - updateLock.lock(testAppId, { force: false }, () => - expectLocks( - true, - 'locks should be deleted and taken again by the lock() call because of the override', - ), - ), - ).to.be.fulfilled; - - await expectLocks( - false, - 'using lockOverride gave lock ownership to the callback, so they should now be deleted', - ); - }); - }); - - describe('getLocksTaken', () => { - const lockdir = pathOnRoot(updateLock.BASE_LOCK_DIR); - before(async () => { - await testfs({ - [lockdir]: {}, - }).enable(); - // TODO: enable mocha-pod to work with empty directories - await fs.mkdir(`${lockdir}/123/main`, { recursive: true }); - await fs.mkdir(`${lockdir}/123/aux`, { recursive: true }); - await fs.mkdir(`${lockdir}/123/invalid`, { recursive: true }); - }); - after(async () => { - await fs.rm(`${lockdir}/123`, { recursive: true }); - await testfs.restore(); - }); - - it('resolves with all locks taken with the Supervisor lockfile UID', async () => { - // Set up valid lockfiles including some directories + // Set up necessary lock directories await Promise.all( - ['resin-updates.lock', 'updates.lock'].map((lf) => { - const p = `${lockdir}/123/main/${lf}`; - return fs - .mkdir(p) - .then(() => - fs.chown(p, updateLock.LOCKFILE_UID, updateLock.LOCKFILE_UID), - ); + appIds.map((id) => + fs.mkdir(lockdir(id, testServiceName), { recursive: true }), + ), + ); + const serviceLock = path.join(lockdir(222, 'main'), 'resin-updates.lock'); + const tmp = await testfs({ + [serviceLock]: testfs.file({ uid: 0 }), + }).enable(); + + await expect( + updateLock.withLock(appIds, async () => { + throw new Error('This is the wrong error'); }), + ).to.be.rejectedWith('Lockfile exists for { appId: 222, service: main }'); + + // Only the original lock should exist at this point + await Promise.all( + [111, 333].map((appId) => expectLocks(appId, [testServiceName], false)), ); - await Promise.all([ - lockfile.lock( - `${lockdir}/123/aux/updates.lock`, - updateLock.LOCKFILE_UID, - ), - lockfile.lock( - `${lockdir}/123/aux/resin-updates.lock`, - updateLock.LOCKFILE_UID, - ), + await expect(fs.readdir(lockdir(222, 'main'))).to.eventually.deep.equal([ + 'resin-updates.lock', ]); - // Set up invalid lockfiles with root UID + // Cleanup + await tmp.restore(); await Promise.all( - ['resin-updates.lock', 'updates.lock'].map((lf) => - lockfile.lock(`${lockdir}/123/invalid/${lf}`), + appIds.map((appId) => + fs.rm(pathOnRoot(`/tmp/balena-supervisor/services/${appId}`), { + recursive: true, + force: true, + }), ), ); - - const locksTaken = await updateLock.getLocksTaken(); - expect(locksTaken).to.have.length(4); - expect(locksTaken).to.deep.include.members([ - `${lockdir}/123/aux/resin-updates.lock`, - `${lockdir}/123/aux/updates.lock`, - `${lockdir}/123/main/resin-updates.lock`, - `${lockdir}/123/main/updates.lock`, - ]); - expect(locksTaken).to.not.deep.include.members([ - `${lockdir}/123/invalid/resin-updates.lock`, - `${lockdir}/123/invalid/updates.lock`, - ]); - }); - }); - - describe('getServicesLockedByAppId', () => { - const lockdir = pathOnRoot(updateLock.BASE_LOCK_DIR); - const validDirs = [ - `${lockdir}/123/one`, - `${lockdir}/123/two`, - `${lockdir}/123/three`, - `${lockdir}/456/server`, - `${lockdir}/456/client`, - `${lockdir}/789/main`, - ]; - const validPaths = ['resin-updates.lock', 'updates.lock'] - .map((lf) => validDirs.map((d) => path.join(d, lf))) - .flat(); - const invalidPaths = [ - // No appId - `${lockdir}/456/updates.lock`, - // No service - `${lockdir}/server/updates.lock`, - // No appId or service - `${lockdir}/test/updates.lock`, - // One of (resin-)updates.lock is missing - `${lockdir}/123/one/resin-updates.lock`, - `${lockdir}/123/two/updates.lock`, - ]; - let tFs: TestFs.Enabled; - beforeEach(async () => { - tFs = await testfs({ - [lockdir]: {}, - }).enable(); - // TODO: mocha-pod should support empty directories - await Promise.all( - validPaths - .concat(invalidPaths) - .map((p) => fs.mkdir(path.dirname(p), { recursive: true })), - ); - }); - afterEach(async () => { - await Promise.all( - validPaths - .concat(invalidPaths) - .map((p) => fs.rm(path.dirname(p), { recursive: true })), - ); - await tFs.restore(); }); - it('should return locks taken by appId', async () => { - // Set up lockfiles - await Promise.all( - validPaths.map((p) => lockfile.lock(p, updateLock.LOCKFILE_UID)), + it('allows only one app lock to be taken at a time', async () => { + // No locks before locking takes place + await expectLocks(123, ['one', 'two'], false); + + const lockable = Lockable.from(123, ['one', 'two']); + + const lock = await lockable.lock(); + // Locks should exist now + await expectLocks(123, ['one', 'two']); + + // Try to lock with the function + await expect( + updateLock.withLock(123, () => expect.fail('This is the wrong error'), { + maxWaitMs: 10, + }), + ).to.be.rejectedWith( + 'Locks for app 123 are being held by another supervisor operation', ); - const locksTakenMap = await updateLock.getServicesLockedByAppId(); - expect([...locksTakenMap.keys()]).to.deep.include.members([ - 123, 456, 789, - ]); - // Should register as locked if only `updates.lock` is present - expect(locksTakenMap.getServices(123)).to.deep.include.members([ - 'one', - 'two', - 'three', - ]); - expect(locksTakenMap.getServices(456)).to.deep.include.members([ - 'server', - 'client', - ]); - // Should register as locked if only `resin-updates.lock` is present - expect(locksTakenMap.getServices(789)).to.deep.include.members(['main']); + await lock.unlock(); - // Cleanup lockfiles - await Promise.all(validPaths.map((p) => lockfile.unlock(p))); + // Locks should have been removed + await expectLocks(123, ['one', 'two'], false); }); - it('should ignore invalid lockfile locations', async () => { - // Set up lockfiles - await Promise.all( - invalidPaths.map((p) => lockfile.lock(p, updateLock.LOCKFILE_UID)), - ); - // Take another lock with an invalid UID but with everything else - // (appId, service, both lockfiles present) correct - await Promise.all( - ['resin-updates.lock', 'updates.lock'].map((lf) => - lockfile.lock(path.join(`${lockdir}/789/main`, lf)), - ), - ); - expect((await updateLock.getServicesLockedByAppId()).size).to.equal(0); + it('it only allows one withLock call to run at the time', async () => { + // Create some empty directories to simulate two services + await fs.mkdir(lockdir(123, 'one'), { recursive: true }); + await fs.mkdir(lockdir(123, 'two'), { recursive: true }); - // Cleanup lockfiles - await Promise.all(invalidPaths.map((p) => lockfile.unlock(p))); - }); - }); + // No locks before locking takes place + await expectLocks(123, ['one', 'two'], false); - describe('composition step actions', () => { - const lockdir = pathOnRoot(updateLock.BASE_LOCK_DIR); - const serviceLockPaths = { - 1: [ - `${lockdir}/1/server/updates.lock`, - `${lockdir}/1/server/resin-updates.lock`, - `${lockdir}/1/client/updates.lock`, - `${lockdir}/1/client/resin-updates.lock`, - ], - 2: [ - `${lockdir}/2/main/updates.lock`, - `${lockdir}/2/main/resin-updates.lock`, - ], - }; + // Try to call withLock in parallel for the same app + const res = await Promise.allSettled([ + updateLock.withLock(123, () => setTimeout(10, 'one'), { + maxWaitMs: 5, + }), + updateLock.withLock(123, () => setTimeout(10, 'two'), { + maxWaitMs: 5, + }), + ]); - describe('takeLock', () => { - let testFs: TestFs.Enabled; + expect(res.filter((r) => r.status === 'rejected')).to.have.lengthOf(1); + expect(res.filter((r) => r.status === 'fulfilled')).to.have.lengthOf(1); - beforeEach(async () => { - testFs = await testfs( - {}, - { cleanup: [path.join(lockdir, '*', '*', '**.lock')] }, - ).enable(); - // TODO: Update mocha-pod to work with creating empty directories - await mkdirp(path.join(lockdir, '1', 'server')); - await mkdirp(path.join(lockdir, '1', 'client')); - await mkdirp(path.join(lockdir, '2', 'main')); - }); - - afterEach(async () => { - await testFs.restore(); - await fs.rm(path.join(lockdir, '1'), { recursive: true }); - await fs.rm(path.join(lockdir, '2'), { recursive: true }); - }); - - it('takes locks for a list of services for an appId', async () => { - // Take locks for appId 1 - await updateLock.takeLock(1, ['server', 'client']); - // Locks should have been taken - expect(await updateLock.getLocksTaken()).to.deep.include.members( - serviceLockPaths[1], - ); - expect(await updateLock.getLocksTaken()).to.have.length(4); - expect( - await fs.readdir(path.join(lockdir, '1', 'server')), - ).to.include.members(['updates.lock', 'resin-updates.lock']); - expect( - await fs.readdir(path.join(lockdir, '1', 'client')), - ).to.include.members(['updates.lock', 'resin-updates.lock']); - // Take locks for appId 2 - await updateLock.takeLock(2, ['main']); - // Locks should have been taken for appid 1 & 2 - expect(await updateLock.getLocksTaken()).to.deep.include.members([ - ...serviceLockPaths[1], - ...serviceLockPaths[2], - ]); - expect(await updateLock.getLocksTaken()).to.have.length(6); - expect( - await fs.readdir(path.join(lockdir, '2', 'main')), - ).to.have.length(2); - // Clean up the lockfiles - for (const lockPath of serviceLockPaths[1].concat( - serviceLockPaths[2], - )) { - await lockfile.unlock(lockPath); - } - }); - - it('creates lock directory recursively if it does not exist', async () => { - // Take locks for app with nonexistent service directories - await updateLock.takeLock(3, ['api']); - // Locks should have been taken - expect(await updateLock.getLocksTaken()).to.deep.include( - path.join(lockdir, '3', 'api', 'updates.lock'), - path.join(lockdir, '3', 'api', 'resin-updates.lock'), - ); - // Directories should have been created - expect(await fs.readdir(path.join(lockdir))).to.deep.include.members([ - '3', - ]); - expect( - await fs.readdir(path.join(lockdir, '3')), - ).to.deep.include.members(['api']); - // Clean up the lockfiles & created directories - await lockfile.unlock(path.join(lockdir, '3', 'api', 'updates.lock')); - await lockfile.unlock( - path.join(lockdir, '3', 'api', 'resin-updates.lock'), - ); - await fs.rm(path.join(lockdir, '3'), { recursive: true }); - }); - - it('should not take lock for services where Supervisor-taken lock already exists', async () => { - // Take locks for one service of appId 1 - await lockfile.lock(serviceLockPaths[1][0], updateLock.LOCKFILE_UID); - await lockfile.lock(serviceLockPaths[1][1], updateLock.LOCKFILE_UID); - // Sanity check that locks are taken & tracked by Supervisor - expect(await updateLock.getLocksTaken()).to.deep.include( - serviceLockPaths[1][0], - serviceLockPaths[1][1], - ); - expect(await updateLock.getLocksTaken()).to.have.length(2); - // Take locks using takeLock, should only lock service which doesn't - // already have locks - await expect( - updateLock.takeLock(1, ['server', 'client']), - ).to.eventually.deep.include.members(['client']); - // Check that locks are taken - expect(await updateLock.getLocksTaken()).to.deep.include.members( - serviceLockPaths[1], - ); - // Clean up lockfiles - for (const lockPath of serviceLockPaths[1]) { - await lockfile.unlock(lockPath); - } - }); - - it('should error if service has a non-Supervisor-taken lock', async () => { - // Simulate a user service taking the lock for services with appId 1 - for (const lockPath of serviceLockPaths[1]) { - await fs.writeFile(lockPath, ''); - } - // Take locks using takeLock, should error - await expect( - updateLock.takeLock(1, ['server', 'client']), - ).to.eventually.be.rejectedWith(UpdatesLockedError); - // No Supervisor locks should have been taken - expect(await updateLock.getLocksTaken()).to.have.length(0); - // Clean up user-created lockfiles - for (const lockPath of serviceLockPaths[1]) { - await fs.rm(lockPath); - } - // Take locks using takeLock, should not error - await expect( - updateLock.takeLock(1, ['server', 'client']), - ).to.eventually.not.be.rejectedWith(UpdatesLockedError); - // Check that locks are taken - expect(await updateLock.getLocksTaken()).to.deep.include.members( - serviceLockPaths[1], - ); - expect(await updateLock.getLocksTaken()).to.have.length(4); - // Clean up lockfiles - for (const lockPath of serviceLockPaths[1]) { - await lockfile.unlock(lockPath); - } - }); - - it('should take locks if service has non-Supervisor-taken lock and force is true', async () => { - // Simulate a user service taking the lock for services with appId 1 - for (const lockPath of serviceLockPaths[1]) { - await fs.writeFile(lockPath, ''); - } - // Take locks using takeLock & force, should not error - await updateLock.takeLock(1, ['server', 'client'], true); - // Check that locks are taken - expect(await updateLock.getLocksTaken()).to.deep.include( - serviceLockPaths[1][0], - serviceLockPaths[1][1], - ); - // Clean up lockfiles - for (const lockPath of serviceLockPaths[1]) { - await lockfile.unlock(lockPath); - } - }); - - it('waits to take locks until resource write lock is taken', async () => { - // Take the write lock for appId 1 - const release = await takeGlobalLockRW(1); - // Queue takeLock, won't resolve until the write lock is released - const takeLockPromise = updateLock.takeLock(1, ['server', 'client']); - // Locks should have not been taken even after waiting - await setTimeout(500); - expect(await updateLock.getLocksTaken()).to.have.length(0); - // Release the write lock - release(); - // Locks should be taken - await takeLockPromise; - // Locks should have been taken - expect(await updateLock.getLocksTaken()).to.deep.include.members( - serviceLockPaths[1], - ); - }); - - it('should release locks when takeLock step errors to return services to unlocked state', async () => { - const svcs = ['server', 'client']; - - // Take lock for second service of two services - await lockfile.lock(`${lockdir}/1/${svcs[1]}/updates.lock`); - expect( - await lockfile.findAll({ root: lockdir }), - ).to.deep.include.members([`${lockdir}/1/${svcs[1]}/updates.lock`]); - - // Watch for added files, as Supervisor-taken locks should be added - // then removed within updateLock.takeLock - const addedFiles: string[] = []; - const watcher = watch(lockdir).on('add', (p) => addedFiles.push(p)); - - // updateLock.takeLock should error - await expect(updateLock.takeLock(1, svcs, false)).to.be.rejectedWith( - UpdatesLockedError, - ); - - // Service without user lock should have been locked by Supervisor.. - expect(addedFiles).to.deep.include.members([ - `${lockdir}/1/${svcs[0]}/updates.lock`, - `${lockdir}/1/${svcs[0]}/resin-updates.lock`, - ]); - - // ..but upon error, Supervisor-taken locks should have been cleaned up - expect( - await lockfile.findAll({ root: lockdir }), - ).to.not.deep.include.members([ - `${lockdir}/1/${svcs[0]}/updates.lock`, - `${lockdir}/1/${svcs[0]}/resin-updates.lock`, - ]); - - // User lock should be left behind - expect( - await lockfile.findAll({ root: lockdir }), - ).to.deep.include.members([`${lockdir}/1/${svcs[1]}/updates.lock`]); - - // Clean up watcher - await watcher.close(); - }); - }); - - describe('releaseLock', () => { - let testFs: TestFs.Enabled; - - beforeEach(async () => { - testFs = await testfs( - {}, - { cleanup: [path.join(lockdir, '*', '*', '**.lock')] }, - ).enable(); - // TODO: Update mocha-pod to work with creating empty directories - await mkdirp(`${lockdir}/1/server`); - await mkdirp(`${lockdir}/1/client`); - await mkdirp(`${lockdir}/2/main`); - }); - - afterEach(async () => { - await testFs.restore(); - await fs.rm(`${lockdir}/1`, { recursive: true }); - await fs.rm(`${lockdir}/2`, { recursive: true }); - }); - - it('releases locks for an appId', async () => { - // Lock services for appId 1 - for (const lockPath of serviceLockPaths[1]) { - await lockfile.lock(lockPath, updateLock.LOCKFILE_UID); - } - // Sanity check that locks are taken & tracked by Supervisor - expect(await updateLock.getLocksTaken()).to.deep.include.members( - serviceLockPaths[1], - ); - // Release locks for appId 1 - await updateLock.releaseLock(1); - // Locks should have been released - expect(await updateLock.getLocksTaken()).to.have.length(0); - // Double check that the lockfiles are removed - expect(await fs.readdir(`${lockdir}/1/server`)).to.have.length(0); - expect(await fs.readdir(`${lockdir}/1/client`)).to.have.length(0); - }); - - it('does not error if there are no locks to release', async () => { - expect(await updateLock.getLocksTaken()).to.have.length(0); - // Should not error - await updateLock.releaseLock(1); - expect(await updateLock.getLocksTaken()).to.have.length(0); - }); - - it('ignores locks outside of appId scope', async () => { - const lockPath = `${lockdir}/2/main/updates.lock`; - // Lock services outside of appId scope - await lockfile.lock(lockPath, updateLock.LOCKFILE_UID); - // Sanity check that locks are taken & tracked by Supervisor - expect(await updateLock.getLocksTaken()).to.deep.include.members([ - lockPath, - ]); - // Release locks for appId 1 - await updateLock.releaseLock(1); - // Locks for appId 2 should not have been released - expect(await updateLock.getLocksTaken()).to.deep.include.members([ - lockPath, - ]); - // Double check that the lockfile is still there - expect(await fs.readdir(`${lockdir}/2/main`)).to.have.length(1); - // Clean up the lockfile - await lockfile.unlock(lockPath); - }); - - it('waits to release locks until resource write lock is taken', async () => { - // Lock services for appId 1 - for (const lockPath of serviceLockPaths[1]) { - await lockfile.lock(lockPath, updateLock.LOCKFILE_UID); - } - // Sanity check that locks are taken & tracked by Supervisor - expect(await updateLock.getLocksTaken()).to.deep.include.members( - serviceLockPaths[1], - ); - // Take the write lock for appId 1 - const release = await takeGlobalLockRW(1); - // Queue releaseLock, won't resolve until the write lock is released - const releaseLockPromise = updateLock.releaseLock(1); - // Locks should have not been released even after waiting - await setTimeout(500); - expect(await updateLock.getLocksTaken()).to.deep.include.members( - serviceLockPaths[1], - ); - // Release the write lock - release(); - // Release locks for appId 1 should resolve - await releaseLockPromise; - // Locks should have been released - expect(await updateLock.getLocksTaken()).to.have.length(0); - }); + // Locks should have been removed + await expectLocks(123, ['one', 'two'], false); }); }); }); diff --git a/test/unit/compose/app.spec.ts b/test/unit/compose/app.spec.ts index 38397249..8726e689 100644 --- a/test/unit/compose/app.spec.ts +++ b/test/unit/compose/app.spec.ts @@ -2,7 +2,7 @@ import { expect } from 'chai'; import type { Image } from '~/src/compose/images'; import { Network } from '~/src/compose/network'; import { Volume } from '~/src/compose/volume'; -import { LocksTakenMap } from '~/lib/update-lock'; +import type { Lock } from '~/lib/update-lock'; import { createService, @@ -19,7 +19,13 @@ const defaultContext = { availableImages: [] as Image[], containerIds: {}, downloading: [] as string[], - locksTaken: new LocksTakenMap(), + lock: null, +}; + +const mockLock: Lock = { + async unlock() { + /* noop */ + }, }; describe('compose/app', () => { @@ -190,7 +196,7 @@ describe('compose/app', () => { ...defaultContext, availableImages, // Mock lock already taken - locksTaken: new LocksTakenMap([{ appId: 1, services: ['test'] }]), + lock: mockLock, }, target, ); @@ -287,7 +293,7 @@ describe('compose/app', () => { { ...contextWithImages, // Mock locks already taken - locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), + lock: mockLock, }, intermediateTarget, ); @@ -299,7 +305,7 @@ describe('compose/app', () => { { ...contextWithImages, // Mock locks already taken - locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), + lock: mockLock, }, intermediateTarget, ); @@ -362,7 +368,7 @@ describe('compose/app', () => { { ...contextWithImages, // Mock locks already taken - locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), + lock: mockLock, }, target, ); @@ -525,7 +531,7 @@ describe('compose/app', () => { { ...defaultContext, availableImages, - locksTaken: new LocksTakenMap([{ appId: 1, services: ['test'] }]), + lock: mockLock, }, target, ); @@ -553,7 +559,7 @@ describe('compose/app', () => { { ...defaultContext, availableImages, - locksTaken: new LocksTakenMap([{ appId: 1, services: ['test'] }]), + lock: mockLock, }, target, ); @@ -678,7 +684,7 @@ describe('compose/app', () => { { ...defaultContext, availableImages, - locksTaken: new LocksTakenMap([{ appId: 1, services: ['test'] }]), + lock: mockLock, }, target, ); @@ -806,7 +812,7 @@ describe('compose/app', () => { { ...defaultContext, availableImages, - locksTaken: new LocksTakenMap([{ appId: 1, services: ['test'] }]), + lock: mockLock, }, target, ); @@ -879,9 +885,7 @@ describe('compose/app', () => { { ...defaultContext, availableImages, - locksTaken: new LocksTakenMap([ - { appId: 1, services: ['one', 'two'] }, - ]), + lock: mockLock, }, target, ); @@ -958,9 +962,7 @@ describe('compose/app', () => { { ...defaultContext, availableImages, - locksTaken: new LocksTakenMap([ - { appId: 1, services: ['one', 'two'] }, - ]), + lock: mockLock, }, target, ); @@ -1108,9 +1110,7 @@ describe('compose/app', () => { ...defaultContext, availableImages: [createImage({ serviceName: 'main' })], // Mock locks already taken - locksTaken: new LocksTakenMap([ - { appId: 1, services: ['main', 'aux'] }, - ]), + lock: mockLock, }, target, ); @@ -1247,7 +1247,7 @@ describe('compose/app', () => { const steps2 = current.nextStepsForAppUpdate( { ...defaultContext, - locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), + lock: mockLock, }, target, ); @@ -1282,7 +1282,7 @@ describe('compose/app', () => { const steps2 = current.nextStepsForAppUpdate( { ...defaultContext, - locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), + lock: mockLock, }, target, ); @@ -1356,7 +1356,7 @@ describe('compose/app', () => { const stepsToIntermediate = current.nextStepsForAppUpdate( { ...contextWithImages, - locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), + lock: mockLock, }, target, ); @@ -1376,7 +1376,7 @@ describe('compose/app', () => { const stepsToTarget = intermediate.nextStepsForAppUpdate( { ...contextWithImages, - locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), + lock: mockLock, }, target, ); @@ -1435,9 +1435,7 @@ describe('compose/app', () => { const contextWithImages = { ...defaultContext, ...{ availableImages }, - locksTaken: new LocksTakenMap([ - { appId: 1, services: ['main', 'dep'] }, - ]), + lock: mockLock, }; // Only one start step and it should be that of the 'dep' service @@ -1555,7 +1553,7 @@ describe('compose/app', () => { { ...contextWithImages, // Mock locks taken before kill - locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), + lock: mockLock, }, target, ); @@ -1575,7 +1573,7 @@ describe('compose/app', () => { ...contextWithImages, // Mock locks still taken after kill (releaseLock not // yet inferred as state is not yet settled) - locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), + lock: mockLock, }, target, ); @@ -1703,7 +1701,7 @@ describe('compose/app', () => { { ...contextWithImages, // Mock locks taken from previous step - locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), + lock: mockLock, }, target, ); @@ -1719,7 +1717,7 @@ describe('compose/app', () => { { ...contextWithImages, // Mock locks taken from previous step - locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), + lock: mockLock, }, target, ); @@ -1762,7 +1760,7 @@ describe('compose/app', () => { const steps2 = current.nextStepsForAppUpdate( { ...defaultContext, - locksTaken: new LocksTakenMap([{ appId: 1, services: ['test'] }]), + lock: mockLock, }, target, ); @@ -1849,9 +1847,7 @@ describe('compose/app', () => { { ...contextWithImages, // Mock locks already taken - locksTaken: new LocksTakenMap([ - { appId: 1, services: ['one', 'two', 'three'] }, - ]), + lock: mockLock, }, target, ); @@ -2029,9 +2025,7 @@ describe('compose/app', () => { serviceName: 'other', }), ], - locksTaken: new LocksTakenMap([ - { appId: 1, services: ['main', 'other'] }, - ]), + lock: mockLock, }, target, ); @@ -2115,9 +2109,7 @@ describe('compose/app', () => { const steps = current.nextStepsForAppUpdate( { ...defaultContext, - locksTaken: new LocksTakenMap([ - { appId: 1, services: ['server', 'client'] }, - ]), + lock: mockLock, }, target, ); @@ -2128,7 +2120,7 @@ describe('compose/app', () => { const steps2 = current.nextStepsForAppUpdate( { ...defaultContext, - locksTaken: new LocksTakenMap([{ appId: 1, services: ['server'] }]), + lock: mockLock, }, target, ); @@ -2173,10 +2165,7 @@ describe('compose/app', () => { const steps = current.nextStepsForAppUpdate( { ...defaultContext, - locksTaken: new LocksTakenMap([ - { appId: 1, services: ['server', 'client'] }, - { appId: 2, services: ['main'] }, - ]), + lock: mockLock, }, target, ); diff --git a/test/unit/lib/update-lock.spec.ts b/test/unit/lib/update-lock.spec.ts deleted file mode 100644 index e00ae249..00000000 --- a/test/unit/lib/update-lock.spec.ts +++ /dev/null @@ -1,58 +0,0 @@ -import { expect } from 'chai'; -import * as path from 'path'; - -import * as updateLock from '~/lib/update-lock'; - -describe('lib/update-lock: unit tests', () => { - describe('lockPath', () => { - it('should return path prefix of service lockfiles on host', () => { - expect(updateLock.lockPath(123)).to.equal( - path.join(updateLock.BASE_LOCK_DIR, '123'), - ); - expect(updateLock.lockPath(123, 'main')).to.equal( - path.join(updateLock.BASE_LOCK_DIR, '123', 'main'), - ); - }); - }); - - describe('LocksTakenMap', () => { - it('should be an instance of Map>', () => { - const map = new updateLock.LocksTakenMap(); - expect(map).to.be.an.instanceof(Map); - }); - - it('should add services while ignoring duplicates', () => { - const map = new updateLock.LocksTakenMap(); - map.add(123, 'main'); - expect(map.getServices(123)).to.deep.include.members(['main']); - - map.add(123, 'main'); - expect(map.getServices(123)).to.deep.include.members(['main']); - - map.add(123, ['main', 'aux']); - expect(map.getServices(123)).to.deep.include.members(['main', 'aux']); - }); - - it('should track any number of appIds', () => { - const map = new updateLock.LocksTakenMap(); - map.add(123, 'main'); - map.add(456, ['aux', 'dep']); - expect(map.getServices(123)).to.deep.include.members(['main']); - expect(map.getServices(456)).to.deep.include.members(['aux', 'dep']); - expect(map.size).to.equal(2); - }); - - it('should return empty array for non-existent appIds', () => { - const map = new updateLock.LocksTakenMap(); - expect(map.getServices(123)).to.deep.equal([]); - }); - - it('should return whether a service is locked under an appId', () => { - const map = new updateLock.LocksTakenMap(); - map.add(123, 'main'); - expect(map.isLocked(123, 'main')).to.be.true; - expect(map.isLocked(123, 'aux')).to.be.false; - expect(map.isLocked(456, 'main')).to.be.false; - }); - }); -});