From fb1bd33ab60f2eae340da7b9bb90ef0f44b07ffc Mon Sep 17 00:00:00 2001 From: Christina Ying Wang Date: Wed, 13 Mar 2024 00:10:08 -0700 Subject: [PATCH] Refine update locking interface * Remove Supervisor lockfile cleanup SIGTERM listener * Modify lockfile.getLocksTaken to read files from the filesystem * Remove in-memory tracking of locks taken in favor of filesystem * Require both `(resin-)updates.lock` to be locked with `nobody` UID for service to count as locked by the Supervisor Signed-off-by: Christina Ying Wang --- entry.sh | 4 - src/compose/application-manager.ts | 6 +- src/config/index.ts | 6 +- src/device-state/preload.ts | 6 +- src/host-config.ts | 10 +- src/lib/errors.ts | 24 +-- src/lib/fs-utils.ts | 3 +- src/lib/lockfile.ts | 67 ++++----- src/lib/update-lock.ts | 76 ++++++++-- src/network.ts | 4 +- test/integration/lib/lockfile.spec.ts | 112 ++++++++------ test/integration/lib/update-lock.spec.ts | 180 +++++++++++++++++------ test/lib/mocked-dockerode.ts | 2 +- 13 files changed, 329 insertions(+), 171 deletions(-) diff --git a/entry.sh b/entry.sh index 8a95abd2..1f4fa9b4 100755 --- a/entry.sh +++ b/entry.sh @@ -80,10 +80,6 @@ modprobe ip6_tables || true export BASE_LOCK_DIR="/tmp/balena-supervisor/services" export LOCKFILE_UID=65534 -# Cleanup leftover Supervisor-created lockfiles from any previous processes. -# Supervisor-created lockfiles have a UID of 65534. -find "${ROOT_MOUNTPOINT}${BASE_LOCK_DIR}" -type f -user "${LOCKFILE_UID}" -name "*updates.lock" -delete || true - 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/application-manager.ts b/src/compose/application-manager.ts index 72da1b5c..816d00b5 100644 --- a/src/compose/application-manager.ts +++ b/src/compose/application-manager.ts @@ -17,7 +17,7 @@ import { ContractViolationError, InternalInconsistencyError, } from '../lib/errors'; -import { getServicesLockedByAppId } from '../lib/update-lock'; +import { getServicesLockedByAppId, LocksTakenMap } from '../lib/update-lock'; import { checkTruthy } from '../lib/validation'; import App from './app'; @@ -150,7 +150,7 @@ export async function getRequiredSteps( downloading, availableImages, containerIdsByAppId, - locksTaken: getServicesLockedByAppId(), + locksTaken: await getServicesLockedByAppId(), }); } @@ -168,7 +168,7 @@ export async function inferNextSteps( containerIdsByAppId = {} as { [appId: number]: UpdateState['containerIds']; }, - locksTaken = getServicesLockedByAppId(), + locksTaken = new LocksTakenMap(), } = {}, ) { const currentAppIds = Object.keys(currentApps).map((i) => parseInt(i, 10)); diff --git a/src/config/index.ts b/src/config/index.ts index afb51a2f..46b1fd2a 100644 --- a/src/config/index.ts +++ b/src/config/index.ts @@ -56,7 +56,7 @@ export async function get( ): Promise> { const $db = trx || db.models; - if (Object.prototype.hasOwnProperty.call(Schema.schema, key)) { + if (Object.hasOwn(Schema.schema, key)) { const schemaKey = key as Schema.SchemaKey; return getSchema(schemaKey, $db).then((value) => { @@ -82,7 +82,7 @@ export async function get( // the type system happy return checkValueDecode(decoded, key, value) && decoded.right; }); - } else if (Object.prototype.hasOwnProperty.call(FnSchema.fnSchema, key)) { + } else if (Object.hasOwn(FnSchema.fnSchema, key)) { const fnKey = key as FnSchema.FnSchemaKey; // Cast the promise as something that produces an unknown, and this means that // we can validate the output of the function as well, ensuring that the type matches @@ -269,7 +269,7 @@ function validateConfigMap( // throw if any value fails verification return _.mapValues(configMap, (value, key) => { if ( - !Object.prototype.hasOwnProperty.call(Schema.schema, key) || + !Object.hasOwn(Schema.schema, key) || !Schema.schema[key as Schema.SchemaKey].mutable ) { throw new Error( diff --git a/src/device-state/preload.ts b/src/device-state/preload.ts index 27db9ef2..36830b46 100644 --- a/src/device-state/preload.ts +++ b/src/device-state/preload.ts @@ -12,8 +12,8 @@ import * as imageManager from '../compose/images'; import { AppsJsonParseError, - EISDIR, - ENOENT, + isEISDIR, + isENOENT, InternalInconsistencyError, } from '../lib/errors'; import log from '../lib/supervisor-console'; @@ -163,7 +163,7 @@ export async function loadTargetFromFile(appsPath: string): Promise { // It can be an empty path because if the file does not exist // on host, the docker daemon creates an empty directory when // the bind mount is added - if (ENOENT(e) || EISDIR(e)) { + if (isENOENT(e) || isEISDIR(e)) { log.debug('No apps.json file present, skipping preload'); } else { log.debug(e.message); diff --git a/src/host-config.ts b/src/host-config.ts index a4527199..bf64e138 100644 --- a/src/host-config.ts +++ b/src/host-config.ts @@ -6,7 +6,7 @@ import path from 'path'; import * as config from './config'; import * as applicationManager from './compose/application-manager'; import * as dbus from './lib/dbus'; -import { ENOENT } from './lib/errors'; +import { isENOENT } from './lib/errors'; import { mkdirp, unlinkAll } from './lib/fs-utils'; import { writeToBoot, @@ -66,8 +66,8 @@ async function readProxy(): Promise { let redsocksConf: string; try { redsocksConf = await readFromBoot(redsocksConfPath, 'utf-8'); - } catch (e: any) { - if (!ENOENT(e)) { + } catch (e: unknown) { + if (!isENOENT(e)) { throw e; } return; @@ -97,8 +97,8 @@ async function readProxy(): Promise { if (noProxy.length) { conf.noProxy = noProxy; } - } catch (e: any) { - if (!ENOENT(e)) { + } catch (e: unknown) { + if (!isENOENT(e)) { throw e; } } diff --git a/src/lib/errors.ts b/src/lib/errors.ts index 86903757..f7fb9b86 100644 --- a/src/lib/errors.ts +++ b/src/lib/errors.ts @@ -40,23 +40,27 @@ export class BadRequestError extends StatusError { export const isBadRequestError = (e: unknown): e is BadRequestError => isStatusError(e) && e.statusCode === 400; +export class DeviceNotFoundError extends TypedError {} + interface CodedSysError extends Error { code?: string; } -export class DeviceNotFoundError extends TypedError {} +const isCodedSysError = (e: unknown): e is CodedSysError => + // See https://mdn.io/hasOwn + e != null && e instanceof Error && Object.hasOwn(e, 'code'); -export function ENOENT(err: CodedSysError): boolean { - return err.code === 'ENOENT'; -} +export const isENOENT = (e: unknown): e is CodedSysError => + isCodedSysError(e) && e.code === 'ENOENT'; -export function EEXIST(err: CodedSysError): boolean { - return err.code === 'EEXIST'; -} +export const isEEXIST = (e: unknown): e is CodedSysError => + isCodedSysError(e) && e.code === 'EEXIST'; -export function EISDIR(err: CodedSysError): boolean { - return err.code === 'EISDIR'; -} +export const isEISDIR = (e: unknown): e is CodedSysError => + isCodedSysError(e) && e.code === 'EISDIR'; + +export const isEPERM = (e: unknown): e is CodedSysError => + isCodedSysError(e) && e.code === 'EPERM'; export function UnitNotLoadedError(err: string[]): boolean { return endsWith(err[0], 'not loaded.'); diff --git a/src/lib/fs-utils.ts b/src/lib/fs-utils.ts index 82a0392d..484b6434 100644 --- a/src/lib/fs-utils.ts +++ b/src/lib/fs-utils.ts @@ -3,6 +3,7 @@ import path from 'path'; import { exec as execSync } from 'child_process'; import { promisify } from 'util'; import { uptime } from 'os'; +import { isENOENT } from './errors'; export const exec = promisify(execSync); @@ -76,7 +77,7 @@ export const touch = (file: string, time = new Date()) => fs.utimes(file, time, time).catch((e) => // only create the file if it doesn't exist, // if some other error happens is probably better to not touch it - e.code === 'ENOENT' + isENOENT(e) ? fs .open(file, 'w') .then((fd) => fd.close()) diff --git a/src/lib/lockfile.ts b/src/lib/lockfile.ts index 576dab2c..7a7527dd 100644 --- a/src/lib/lockfile.ts +++ b/src/lib/lockfile.ts @@ -1,34 +1,44 @@ -import { promises as fs, unlinkSync, rmdirSync } from 'fs'; +import { promises as fs } from 'fs'; +import type { Stats, Dirent } from 'fs'; import os from 'os'; import { dirname } from 'path'; import { exec } from './fs-utils'; +import { isENOENT, isEISDIR, isEPERM } from './errors'; // Equivalent to `drwxrwxrwt` const STICKY_WRITE_PERMISSIONS = 0o1777; -/** - * Internal lockfile manager to track files in memory - */ -// Track locksTaken, so that the proper locks can be cleaned up on process exit -const locksTaken: { [lockName: string]: boolean } = {}; - -// Returns all current locks taken, as they've been stored in-memory. +// Returns all current locks taken under a directory (default: /tmp) // Optionally accepts filter function for only getting locks that match a condition. -export const getLocksTaken = ( - lockFilter: (path: string) => boolean = () => true, -): string[] => Object.keys(locksTaken).filter(lockFilter); - -// Try to clean up any existing locks when the process exits -process.on('exit', () => { - for (const lockName of getLocksTaken()) { - try { - unlockSync(lockName); - } catch (e) { - // Ignore unlocking errors +// A file is counted as a lock by default if it ends with `.lock`. +export const getLocksTaken = async ( + rootDir: string = '/tmp', + lockFilter: (path: string, stat: Stats) => boolean = (p) => + p.endsWith('.lock'), +): Promise => { + const locksTaken: string[] = []; + let filesOrDirs: Dirent[] = []; + try { + filesOrDirs = await fs.readdir(rootDir, { withFileTypes: true }); + } catch (err) { + // If lockfile directory doesn't exist, no locks are taken + if (isENOENT(err)) { + return locksTaken; } } -}); + for (const fileOrDir of filesOrDirs) { + const lockPath = `${rootDir}/${fileOrDir.name}`; + // A lock is taken if it's a file or directory within rootDir that passes filter fn + if (lockFilter(lockPath, await fs.stat(lockPath))) { + locksTaken.push(lockPath); + // Otherwise, if non-lock directory, seek locks recursively within directory + } else if (fileOrDir.isDirectory()) { + locksTaken.push(...(await getLocksTaken(lockPath, lockFilter))); + } + } + return locksTaken; +}; interface ChildProcessError { code: number; @@ -77,8 +87,6 @@ export async function lock(path: string, uid: number = os.userInfo().uid) { try { // Lock the file using binary await exec(`lockfile -r 0 ${path}`, { uid }); - // Store a lock in memory as taken - locksTaken[path] = true; } catch (error) { // Code 73 refers to EX_CANTCREAT (73) in sysexits.h, or: // A (user specified) output file cannot be created. @@ -110,7 +118,7 @@ export async function unlock(path: string): Promise { // Removing the lockfile releases the lock await fs.unlink(path).catch((e) => { // if the error is EPERM|EISDIR, the file is a directory - if (e.code === 'EPERM' || e.code === 'EISDIR') { + if (isEPERM(e) || isEISDIR(e)) { return fs.rmdir(path).catch(() => { // if the directory is not empty or something else // happens, ignore @@ -119,17 +127,4 @@ export async function unlock(path: string): Promise { // If the file does not exist or some other error // happens, then ignore the error }); - // Remove lockfile's in-memory tracking of a file - delete locksTaken[path]; -} - -export function unlockSync(path: string) { - try { - return unlinkSync(path); - } catch (e: any) { - if (e.code === 'EPERM' || e.code === 'EISDIR') { - return rmdirSync(path); - } - throw e; - } } diff --git a/src/lib/update-lock.ts b/src/lib/update-lock.ts index a2d3e6db..fbdd74c7 100644 --- a/src/lib/update-lock.ts +++ b/src/lib/update-lock.ts @@ -1,9 +1,10 @@ import { promises as fs } from 'fs'; import path from 'path'; +import type { Stats } from 'fs'; import { isRight } from 'fp-ts/lib/Either'; import { - ENOENT, + isENOENT, UpdatesLockedError, InternalInconsistencyError, } from './errors'; @@ -65,10 +66,10 @@ async function dispose( appIdentifier: string | number, release: () => void, ): Promise { - const locks = lockfile.getLocksTaken((p: string) => - p.includes(`${BASE_LOCK_DIR}/${appIdentifier}`), - ); 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 { @@ -89,11 +90,12 @@ export async function takeLock( const release = await takeGlobalLockRW(appId); 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) => !getServicesLockedByAppId().isLocked(appId, svc), + (svc) => !locksTaken.isLocked(appId, svc), ); for (const service of servicesWithoutLock) { await mkdirp(pathOnRoot(lockPath(appId, service))); @@ -116,17 +118,18 @@ export async function releaseLock(appId: number) { } /** - * Given a lockfile path `p`, return a tuple [appId, serviceName] of that path. + * 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('/'); - if (parts.pop()?.match(/updates\.lock/) === null) { + const filename = parts.pop(); + if (filename?.match(/updates\.lock/) === null) { return []; } const serviceName = parts.pop(); const appId = parts.pop(); - return [appId, serviceName]; + return [appId, serviceName, filename]; } type LockedEntity = { appId: number; services: string[] }; @@ -175,19 +178,62 @@ export class LocksTakenMap extends Map> { } } +// 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.getLocksTaken( + rootDir, + (p: string, s: Stats) => + p.endsWith('updates.lock') && s.uid === LOCKFILE_UID, + ); +} + /** * 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 function getServicesLockedByAppId(): LocksTakenMap { - const locksTaken = lockfile.getLocksTaken(); - const servicesByAppId = new LocksTakenMap(); +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] = getIdentifiersFromPath(lockTakenPath); - if (!StringIdentifier.is(appId) || !DockerName.is(serviceName)) { + const [appId, serviceName, filename] = + getIdentifiersFromPath(lockTakenPath); + if ( + !StringIdentifier.is(appId) || + !DockerName.is(serviceName) || + !filename?.match(/updates\.lock/) + ) { continue; } const numAppId = +appId; - servicesByAppId.add(numAppId, serviceName); + 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); + } + + // 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); + } + } } return servicesByAppId; } @@ -228,7 +274,7 @@ export async function lock( releases.set(id, await takeGlobalLockRW(id)); // Get list of service folders in lock directory const serviceFolders = await fs.readdir(lockDir).catch((e) => { - if (ENOENT(e)) { + if (isENOENT(e)) { return []; } throw e; diff --git a/src/network.ts b/src/network.ts index 557018d1..037d7fc7 100644 --- a/src/network.ts +++ b/src/network.ts @@ -5,7 +5,7 @@ import os from 'os'; import url from 'url'; import * as constants from './lib/constants'; -import { EEXIST } from './lib/errors'; +import { isEEXIST } from './lib/errors'; import { checkFalsey } from './lib/validation'; import blink = require('./lib/blink'); @@ -71,7 +71,7 @@ export const startConnectivityCheck = _.once( try { await fs.mkdir(constants.vpnStatusPath); } catch (err: any) { - if (EEXIST(err)) { + if (isEEXIST(err)) { log.debug('VPN status path exists.'); } else { throw err; diff --git a/test/integration/lib/lockfile.spec.ts b/test/integration/lib/lockfile.spec.ts index eb2d646d..b316e752 100644 --- a/test/integration/lib/lockfile.spec.ts +++ b/test/integration/lib/lockfile.spec.ts @@ -1,5 +1,5 @@ import { expect } from 'chai'; -import { promises as fs, mkdirSync } from 'fs'; +import { promises as fs } from 'fs'; import type { TestFs } from 'mocha-pod'; import { testfs } from 'mocha-pod'; import * as os from 'os'; @@ -148,54 +148,82 @@ describe('lib/lockfile', () => { await expect(fs.access(lock)).to.be.rejected; }); - it('should synchronously unlock a lockfile', () => { - const lock = path.join(lockdir, 'other.lock'); + it('should get locks taken with default args', async () => { + // Set up lock dirs + await fs.mkdir(`${lockdir}/1/main`, { recursive: true }); + await fs.mkdir(`${lockdir}/2/aux`, { recursive: true }); - lockfile.unlockSync(lock); + // Take some locks + const locks = [ + `${lockdir}/updates.lock`, + `${lockdir}/two.lock`, + `${lockdir}/1/main/updates.lock`, + `${lockdir}/1/main/resin-updates.lock`, + `${lockdir}/2/aux/updates.lock`, + `${lockdir}/2/aux/resin-updates.lock`, + ]; + await Promise.all(locks.map((lock) => lockfile.lock(lock))); - // Verify lockfile does not exist - return expect(fs.access(lock)).to.be.rejected; + // Assert all locks are listed as taken + expect(await lockfile.getLocksTaken(lockdir)).to.have.members( + locks.concat([`${lockdir}/other.lock`]), + ); + + // Clean up locks + await fs.rm(`${lockdir}`, { recursive: true }); }); - it('should synchronously unlock a lockfile dir', () => { - const lock = path.join(lockdir, 'update.lock'); + it('should get locks taken with a custom filter', async () => { + // Set up lock dirs + await fs.mkdir(`${lockdir}/1`, { recursive: true }); + await fs.mkdir(`${lockdir}/services/main`, { recursive: true }); + await fs.mkdir(`${lockdir}/services/aux`, { recursive: true }); - mkdirSync(lock, { recursive: true }); - - lockfile.unlockSync(lock); - - // Verify lockfile does not exist - return expect(fs.access(lock)).to.be.rejected; - }); - - it('should try to clean up existing locks on process exit', async () => { - // Create lockfiles - const lockOne = path.join(lockdir, 'updates.lock'); - const lockTwo = path.join(lockdir, 'two.lock'); - await expect(lockfile.lock(lockOne)).to.not.be.rejected; - await expect(lockfile.lock(lockTwo, NOBODY_UID)).to.not.be.rejected; - - // @ts-expect-error simulate process exit event - process.emit('exit'); - - // Verify lockfile removal regardless of appId / appUuid - await expect(fs.access(lockOne)).to.be.rejected; - await expect(fs.access(lockTwo)).to.be.rejected; - }); - - it('allows to list locks taken according to a filter function', async () => { - // Create multiple lockfiles - const lockOne = path.join(lockdir, 'updates.lock'); - const lockTwo = path.join(lockdir, 'two.lock'); - await expect(lockfile.lock(lockOne)).to.not.be.rejected; - await expect(lockfile.lock(lockTwo, NOBODY_UID)).to.not.be.rejected; + // Take some locks... + // - with a specific UID + await lockfile.lock(`${lockdir}/updates.lock`, NOBODY_UID); + // - as a directory + await fs.mkdir(`${lockdir}/1/updates.lock`); + // - as a directory with a specific UID + await fs.mkdir(`${lockdir}/1/resin-updates.lock`); + await fs.chown(`${lockdir}/1/resin-updates.lock`, NOBODY_UID, NOBODY_UID); + // - under a different root dir from default + await lockfile.lock(`${lockdir}/services/main/updates.lock`); + await lockfile.lock(`${lockdir}/services/aux/resin-updates.lock`); + // Assert appropriate locks are listed as taken... + // - with a specific UID expect( - lockfile.getLocksTaken((filepath) => filepath.includes('lockdir')), - ).to.have.members([lockOne, lockTwo]); + await lockfile.getLocksTaken( + lockdir, + (p, stats) => p.endsWith('.lock') && stats.uid === NOBODY_UID, + ), + ).to.have.members([ + `${lockdir}/updates.lock`, + `${lockdir}/1/resin-updates.lock`, + `${lockdir}/other.lock`, + ]); + // - as a directory expect( - lockfile.getLocksTaken((filepath) => filepath.includes('two')), - ).to.have.members([lockTwo]); - expect(lockfile.getLocksTaken()).to.have.members([lockOne, lockTwo]); + await lockfile.getLocksTaken( + lockdir, + (p, stats) => p.endsWith('.lock') && stats.isDirectory(), + ), + ).to.have.members([ + `${lockdir}/1/updates.lock`, + `${lockdir}/1/resin-updates.lock`, + ]); + // - under a different root dir from default + expect( + await lockfile.getLocksTaken(`${lockdir}/services`, (p) => + p.endsWith('.lock'), + ), + ).to.have.members([ + `${lockdir}/services/main/updates.lock`, + `${lockdir}/services/aux/resin-updates.lock`, + ]); + + // Clean up locks + await fs.rm(`${lockdir}`, { recursive: true }); }); }); diff --git a/test/integration/lib/update-lock.spec.ts b/test/integration/lib/update-lock.spec.ts index 796f5f17..eb09850a 100644 --- a/test/integration/lib/update-lock.spec.ts +++ b/test/integration/lib/update-lock.spec.ts @@ -70,13 +70,16 @@ describe('lib/update-lock', () => { const takeLocks = () => Promise.all( supportedLockfiles.map((lf) => - lockfile.lock(path.join(lockdir(testAppId, testServiceName), lf)), + lockfile.lock( + path.join(lockdir(testAppId, testServiceName), lf), + updateLock.LOCKFILE_UID, + ), ), ); const releaseLocks = async () => { await Promise.all( - lockfile.getLocksTaken().map((lock) => lockfile.unlock(lock)), + (await updateLock.getLocksTaken()).map((lock) => lockfile.unlock(lock)), ); // Remove any other lockfiles created for the testAppId @@ -150,8 +153,8 @@ describe('lib/update-lock', () => { ) .catch((err) => expect(err).to.be.instanceOf(UpdatesLockedError)); - // Since the lock-taking failed, there should be no locks to dispose of - expect(lockfile.getLocksTaken()).to.have.length(0); + // Since the lock-taking with `nobody` uid failed, there should be no locks to dispose of + expect(await updateLock.getLocksTaken()).to.have.length(0); // Restore the locks that were taken at the beginning of the test await releaseLocks(); @@ -286,23 +289,96 @@ describe('lib/update-lock', () => { }); }); + 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 + 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), + ); + }), + ); + await Promise.all([ + lockfile.lock( + `${lockdir}/123/aux/updates.lock`, + updateLock.LOCKFILE_UID, + ), + lockfile.lock( + `${lockdir}/123/aux/resin-updates.lock`, + updateLock.LOCKFILE_UID, + ), + ]); + + // Set up invalid lockfiles with root UID + await Promise.all( + ['resin-updates.lock', 'updates.lock'].map((lf) => + lockfile.lock(`${lockdir}/123/invalid/${lf}`), + ), + ); + + 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 validPaths = [ - '/tmp/123/one/updates.lock', - '/tmp/123/two/updates.lock', - '/tmp/123/three/updates.lock', - '/tmp/balena-supervisor/services/456/server/updates.lock', - '/tmp/balena-supervisor/services/456/client/updates.lock', - '/tmp/balena-supervisor/services/789/main/resin-updates.lock', + 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 = [ - '/tmp/balena-supervisor/services/456/updates.lock', - '/tmp/balena-supervisor/services/server/updates.lock', - '/tmp/test/updates.lock', + // 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({ '/tmp': {} }).enable(); + tFs = await testfs({ + [lockdir]: {}, + }).enable(); // TODO: mocha-pod should support empty directories await Promise.all( validPaths @@ -325,7 +401,7 @@ describe('lib/update-lock', () => { validPaths.map((p) => lockfile.lock(p, updateLock.LOCKFILE_UID)), ); - const locksTakenMap = updateLock.getServicesLockedByAppId(); + const locksTakenMap = await updateLock.getServicesLockedByAppId(); expect([...locksTakenMap.keys()]).to.deep.include.members([ 123, 456, 789, ]); @@ -348,9 +424,17 @@ describe('lib/update-lock', () => { it('should ignore invalid lockfile locations', async () => { // Set up lockfiles - await Promise.all(invalidPaths.map((p) => lockfile.lock(p))); - - expect(updateLock.getServicesLockedByAppId().size).to.equal(0); + 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); // Cleanup lockfiles await Promise.all(invalidPaths.map((p) => lockfile.unlock(p))); @@ -396,10 +480,10 @@ describe('lib/update-lock', () => { // Take locks for appId 1 await updateLock.takeLock(1, ['server', 'client']); // Locks should have been taken - expect(lockfile.getLocksTaken()).to.deep.include.members( + expect(await updateLock.getLocksTaken()).to.deep.include.members( serviceLockPaths[1], ); - expect(lockfile.getLocksTaken()).to.have.length(4); + 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']); @@ -409,11 +493,11 @@ describe('lib/update-lock', () => { // Take locks for appId 2 await updateLock.takeLock(2, ['main']); // Locks should have been taken for appid 1 & 2 - expect(lockfile.getLocksTaken()).to.deep.include.members([ + expect(await updateLock.getLocksTaken()).to.deep.include.members([ ...serviceLockPaths[1], ...serviceLockPaths[2], ]); - expect(lockfile.getLocksTaken()).to.have.length(6); + expect(await updateLock.getLocksTaken()).to.have.length(6); expect( await fs.readdir(path.join(lockdir, '2', 'main')), ).to.have.length(2); @@ -429,7 +513,7 @@ describe('lib/update-lock', () => { // Take locks for app with nonexistent service directories await updateLock.takeLock(3, ['api']); // Locks should have been taken - expect(lockfile.getLocksTaken()).to.deep.include( + expect(await updateLock.getLocksTaken()).to.deep.include( path.join(lockdir, '3', 'api', 'updates.lock'), path.join(lockdir, '3', 'api', 'resin-updates.lock'), ); @@ -450,21 +534,21 @@ describe('lib/update-lock', () => { 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]); - await lockfile.lock(serviceLockPaths[1][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(lockfile.getLocksTaken()).to.deep.include( + expect(await updateLock.getLocksTaken()).to.deep.include( serviceLockPaths[1][0], serviceLockPaths[1][1], ); - expect(lockfile.getLocksTaken()).to.have.length(2); + 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(lockfile.getLocksTaken()).to.deep.include.members( + expect(await updateLock.getLocksTaken()).to.deep.include.members( serviceLockPaths[1], ); // Clean up lockfiles @@ -483,7 +567,7 @@ describe('lib/update-lock', () => { updateLock.takeLock(1, ['server', 'client']), ).to.eventually.be.rejectedWith(UpdatesLockedError); // No Supervisor locks should have been taken - expect(lockfile.getLocksTaken()).to.have.length(0); + expect(await updateLock.getLocksTaken()).to.have.length(0); // Clean up user-created lockfiles for (const lockPath of serviceLockPaths[1]) { await fs.rm(lockPath); @@ -493,10 +577,10 @@ describe('lib/update-lock', () => { updateLock.takeLock(1, ['server', 'client']), ).to.eventually.not.be.rejectedWith(UpdatesLockedError); // Check that locks are taken - expect(lockfile.getLocksTaken()).to.deep.include.members( + expect(await updateLock.getLocksTaken()).to.deep.include.members( serviceLockPaths[1], ); - expect(lockfile.getLocksTaken()).to.have.length(4); + expect(await updateLock.getLocksTaken()).to.have.length(4); // Clean up lockfiles for (const lockPath of serviceLockPaths[1]) { await lockfile.unlock(lockPath); @@ -510,13 +594,13 @@ describe('lib/update-lock', () => { const takeLockPromise = updateLock.takeLock(1, ['server', 'client']); // Locks should have not been taken even after waiting await setTimeout(500); - expect(lockfile.getLocksTaken()).to.have.length(0); + 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(lockfile.getLocksTaken()).to.deep.include.members( + expect(await updateLock.getLocksTaken()).to.deep.include.members( serviceLockPaths[1], ); }); @@ -545,38 +629,42 @@ describe('lib/update-lock', () => { it('releases locks for an appId', async () => { // Lock services for appId 1 for (const lockPath of serviceLockPaths[1]) { - await lockfile.lock(lockPath); + await lockfile.lock(lockPath, updateLock.LOCKFILE_UID); } // Sanity check that locks are taken & tracked by Supervisor - expect(lockfile.getLocksTaken()).to.deep.include.members( + 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(lockfile.getLocksTaken()).to.have.length(0); + 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(lockfile.getLocksTaken()).to.have.length(0); + expect(await updateLock.getLocksTaken()).to.have.length(0); // Should not error await updateLock.releaseLock(1); - expect(lockfile.getLocksTaken()).to.have.length(0); + 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); + await lockfile.lock(lockPath, updateLock.LOCKFILE_UID); // Sanity check that locks are taken & tracked by Supervisor - expect(lockfile.getLocksTaken()).to.deep.include.members([lockPath]); + 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(lockfile.getLocksTaken()).to.deep.include.members([lockPath]); + 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 @@ -586,10 +674,10 @@ describe('lib/update-lock', () => { 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); + await lockfile.lock(lockPath, updateLock.LOCKFILE_UID); } // Sanity check that locks are taken & tracked by Supervisor - expect(lockfile.getLocksTaken()).to.deep.include.members( + expect(await updateLock.getLocksTaken()).to.deep.include.members( serviceLockPaths[1], ); // Take the write lock for appId 1 @@ -598,7 +686,7 @@ describe('lib/update-lock', () => { const releaseLockPromise = updateLock.releaseLock(1); // Locks should have not been released even after waiting await setTimeout(500); - expect(lockfile.getLocksTaken()).to.deep.include.members( + expect(await updateLock.getLocksTaken()).to.deep.include.members( serviceLockPaths[1], ); // Release the write lock @@ -606,7 +694,7 @@ describe('lib/update-lock', () => { // Release locks for appId 1 should resolve await releaseLockPromise; // Locks should have been released - expect(lockfile.getLocksTaken()).to.have.length(0); + expect(await updateLock.getLocksTaken()).to.have.length(0); }); }); }); diff --git a/test/lib/mocked-dockerode.ts b/test/lib/mocked-dockerode.ts index c447413c..7a885a88 100644 --- a/test/lib/mocked-dockerode.ts +++ b/test/lib/mocked-dockerode.ts @@ -74,7 +74,7 @@ export function registerOverride< } export function restoreOverride(name: T) { - if (Object.prototype.hasOwnProperty.call(overrides, name)) { + if (Object.hasOwn(overrides, name)) { delete overrides[name]; } }