ExternalVendorCode/balena-supervisor
1
0
Fork 0
mirror of https://github.com/balena-os/balena-supervisor.git synced 2025-02-20 01:36:18 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

Modify update lock module to use new lockfile binary and library

Browse Source 
Also uninstall lockfile NPM package as we're no longer using it

Signed-off-by: Christina Wang <christina@balena.io>
This commit is contained in:
Christina Wang 2022-03-07 16:56:11 -08:00
parent 51e63ea22b
commit e9738b5f78
7 changed files with 241 additions and 265 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

15
package-lock.json generated
View File

@ -616,12 +616,6 @@
"integrity": "sha512-KZfv4ea6bEbdQhfwpxtDuTPO2mHAAXMQqPOZyS4MgNyCymKoLHp0FVzzYq3H2zCeIotN4h1453TahLCCm8rf2w==",
"dev": true
},
"@types/lockfile": {
"version": "1.0.1",
"resolved": "https://registry.npmjs.org/@types/lockfile/-/lockfile-1.0.1.tgz",
"integrity": "sha512-65WZedEm4AnOsBDdsapJJG42MhROu3n4aSSiu87JXF/pSdlubxZxp3S1yz3kTfkJ2KBPud4CpjoHVAptOm9Zmw==",
"dev": true
},
"@types/lodash": {
"version": "4.14.159",
"resolved": "https://registry.npmjs.org/@types/lodash/-/lodash-4.14.159.tgz",
@ -6607,15 +6601,6 @@
"p-locate": "^4.1.0"
}
},
"lockfile": {
"version": "1.0.4",
"resolved": "https://registry.npmjs.org/lockfile/-/lockfile-1.0.4.tgz",
"integrity": "sha512-cvbTwETRfsFh4nHsL1eGWapU1XFi5Ot9E85sWAwia7Y7EgB7vfqcZhTKZ+l7hCGxSPoushMv5GKhT5PdLv03WA==",
"dev": true,
"requires": {
"signal-exit": "^3.0.2"
}
},
"lodash": {
"version": "4.17.21",
"resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.21.tgz",

2
package.json
View File

@ -54,7 +54,6 @@
"@types/dockerode": "^2.5.34",
"@types/event-stream": "^3.3.34",
"@types/express": "^4.17.3",
"@types/lockfile": "^1.0.1",
"@types/lodash": "^4.14.159",
"@types/memoizee": "^0.4.4",
"@types/mocha": "^8.2.2",
@ -100,7 +99,6 @@
"knex": "^0.20.13",
"lint-staged": "^10.2.11",
"livepush": "^3.5.1",
"lockfile": "^1.0.4",
"lodash": "^4.17.21",
"memoizee": "^0.4.14",
"mixpanel": "^0.10.3",

6
src/lib/lockfile.ts
View File

@ -107,9 +107,11 @@ export async function lock(path: string, uid = LOCKFILE_UID) {
}
}
export async function unlock(path: string) {
export async function unlock(path: string): Promise<void> {
// Removing the updates.lock file releases the lock
return await unlinkAll(path);
await unlinkAll(path);
// Remove lockfile's in-memory tracking of a file
delete locksTaken[path];
}
export function unlockSync(path: string) {

110
src/lib/update-lock.ts
View File

@ -1,5 +1,4 @@
import * as Bluebird from 'bluebird';
import * as lockFileLib from 'lockfile';
import * as _ from 'lodash';
import { promises as fs } from 'fs';
import * as path from 'path';
@ -8,30 +7,15 @@ import * as Lock from 'rwlock';
import * as constants from './constants';
import {
ENOENT,
EEXIST,
UpdatesLockedError,
InternalInconsistencyError,
} from './errors';
import { getPathOnHost, pathExistsOnHost } from './fs-utils';
import * as config from '../config';
type asyncLockFile = typeof lockFileLib & {
unlockAsync(path: string): Bluebird<void>;
lockAsync(path: string): Bluebird<void>;
};
const lockFile = Bluebird.promisifyAll(lockFileLib) as asyncLockFile;
export type LockCallback = (
appId: number,
opts: { force: boolean },
fn: () => PromiseLike<void>,
) => Bluebird<void>;
import * as lockfile from './lockfile';
export function lockPath(appId: number, serviceName?: string): string {
return path.join(
'/tmp/balena-supervisor/services',
appId.toString(),
serviceName ?? '',
);
return path.join(lockfile.BASE_LOCK_DIR, appId.toString(), serviceName ?? '');
}
function lockFilesOnHost(appId: number, serviceName: string): string[] {
@ -69,19 +53,6 @@ export function abortIfHUPInProgress({
});
}
const locksTaken: { [lockName: string]: boolean } = {};
// Try to clean up any existing locks when the program exits
process.on('exit', () => {
for (const lockName of _.keys(locksTaken)) {
try {
lockFile.unlockSync(lockName);
} catch (e) {
// Ignore unlocking errors
}
}
});
type LockFn = (key: string | number) => Bluebird<() => void>;
const locker = new Lock();
export const writeLock: LockFn = Bluebird.promisify(locker.async.writeLock, {
@ -92,54 +63,32 @@ export const readLock: LockFn = Bluebird.promisify(locker.async.readLock, {
});
function dispose(release: () => void): Bluebird<void> {
return Bluebird.map(_.keys(locksTaken), (lockName) => {
delete locksTaken[lockName];
return lockFile.unlockAsync(lockName);
return Bluebird.map(lockfile.getLocksTaken(), (lockName) => {
return lockfile.unlock(lockName);
})
.finally(release)
.return();
}
const lockExistsErrHandler = (err: Error, release: () => void) => {
let errMsg = err.message;
if (EEXIST(err)) {
// Extract appId|appUuid and serviceName from lockfile path for log message
// appId: [0-9]{7}, appUuid: [0-9a-w]{32}, short appUuid: [0-9a-w]{7}
const pathMatch = err.message.match(
/\/([0-9]{7}|[0-9a-w]{32}|[0-9a-w]{7})\/(.*)\/(?:resin-)?updates.lock/,
);
if (pathMatch && pathMatch.length === 3) {
errMsg = `Lockfile exists for ${JSON.stringify({
serviceName: pathMatch[2],
[/^[0-9]{7}$/.test(pathMatch[1]) ? 'appId' : 'appUuid']: pathMatch[1],
})}`;
}
}
return dispose(release).throw(new UpdatesLockedError(errMsg));
};
/**
* Try to take the locks for an application. If force is set, it will remove
* all existing lockfiles before performing the operation
*
* TODO: convert to native Promises. May require native implementation of Bluebird's dispose / using
* TODO: convert to native Promises and async/await. May require native implementation of Bluebird's dispose / using
*
* TODO: Remove skipLock as it's not a good interface. If lock is called it should try to take the lock
* without an option to skip.
*/
export function lock<T extends unknown>(
appId: number | null,
appId: number,
{ force = false, skipLock = false }: { force: boolean; skipLock?: boolean },
fn: () => Resolvable<T>,
): Bluebird<T> {
if (skipLock) {
if (skipLock || appId == null) {
return Bluebird.resolve(fn());
}
const takeTheLock = () => {
if (appId == null) {
return;
}
return config
.get('lockOverride')
.then((lockOverride) => {
@ -152,20 +101,36 @@ export function lock<T extends unknown>(
return Bluebird.mapSeries(
lockFilesOnHost(appId, serviceName),
(tmpLockName) => {
return Bluebird.try(() => {
if (force || lockOverride) {
return lockFile.unlockAsync(tmpLockName);
}
})
.then(() => lockFile.lockAsync(tmpLockName))
.then(() => {
locksTaken[tmpLockName] = true;
return (
Bluebird.try(() => {
if (force || lockOverride) {
return lockfile.unlock(tmpLockName);
}
})
.catchReturn(ENOENT, undefined);
.then(() => {
return lockfile.lock(tmpLockName);
})
// If lockfile exists, throw a user-friendly error.
// Otherwise throw the error as-is.
// This will interrupt the call to Bluebird.using, so
// dispose needs to be called even though it's referenced
// by .disposer later.
.catch((error) => {
return dispose(release).throw(
lockfile.LockfileExistsError.is(error)
? new UpdatesLockedError(
`Lockfile exists for ${JSON.stringify({
serviceName,
appId,
})}`,
)
: (error as Error),
);
})
);
},
);
})
.catch((err) => lockExistsErrHandler(err, release));
});
})
.disposer(dispose);
})
@ -177,9 +142,6 @@ export function lock<T extends unknown>(
};
const disposer = takeTheLock();
if (disposer) {
return Bluebird.using(disposer, fn as () => PromiseLike<T>);
} else {
return Bluebird.resolve(fn());
}
return Bluebird.using(disposer, fn as () => PromiseLike<T>);
}

2
src/supervisor.ts
View File

@ -8,10 +8,8 @@ import { normaliseLegacyDatabase } from './lib/legacy';
import * as osRelease from './lib/os-release';
import * as logger from './logger';
import SupervisorAPI from './supervisor-api';
import log from './lib/supervisor-console';
import version = require('./lib/supervisor-version');
import * as avahi from './lib/avahi';
import * as firewall from './lib/firewall';
import logMonitor from './logging/monitor';

32
test/src/lib/lockfile.spec.ts
View File

@ -6,7 +6,6 @@ import mock = require('mock-fs');
import * as lockfile from '../../../src/lib/lockfile';
import * as fsUtils from '../../../src/lib/fs-utils';
import { ChildProcessError } from '../../../src/lib/errors';
describe('lib/lockfile', () => {
const lockPath = `${lockfile.BASE_LOCK_DIR}/1234567/updates.lock`;
@ -70,8 +69,14 @@ describe('lib/lockfile', () => {
});
});
afterEach(() => {
afterEach(async () => {
execStub.restore();
// Even though mock-fs is restored, this is needed to delete any in-memory storage of locks
for (const lock of lockfile.getLocksTaken()) {
await lockfile.unlock(lock);
}
mock.restore();
});
@ -109,15 +114,14 @@ describe('lib/lockfile', () => {
// no errors, but we want it to throw an error just for this unit test
execStub.restore();
const childProcessError = new Error() as ChildProcessError;
childProcessError.code = 73;
childProcessError.stderr = 'lockfile: Test error';
const childProcessError = new lockfile.LockfileExistsError(
'/tmp/test/path',
);
execStub = stub(fsUtils, 'exec').throws(childProcessError);
try {
await lockfile.lock(lockPath);
expect.fail('lockfile.lock should have thrown an error');
expect.fail('lockfile.lock should throw an error');
} catch (err) {
expect(err).to.exist;
}
@ -164,4 +168,18 @@ describe('lib/lockfile', () => {
expect.fail((err as Error)?.message ?? err);
});
});
it('should try to clean up existing locks on process exit', async () => {
// Mock directory with sticky + write permissions
mockDir(STICKY_WRITE_PERMISSIONS.unix, { createLock: false });
// Lock file, which stores lock path in memory
await lockfile.lock(lockPath);
// @ts-ignore
process.emit('exit');
// Verify lockfile removal
await checkLockDirFiles(lockPath, { shouldExist: false });
});
});

339
test/src/lib/update-lock.spec.ts
View File

@ -1,48 +1,40 @@
import { expect } from 'chai';
import { SinonSpy, SinonStub, spy, stub } from 'sinon';
import * as path from 'path';
import * as Bluebird from 'bluebird';
import rewire = require('rewire');
import { promises as fs } from 'fs';
import mockFs = require('mock-fs');
import * as updateLock from '../../../src/lib/update-lock';
import * as constants from '../../../src/lib/constants';
import { UpdatesLockedError } from '../../../src/lib/errors';
import * as config from '../../../src/config';
import * as lockfile from '../../../src/lib/lockfile';
import * as fsUtils from '../../../src/lib/fs-utils';
describe('lib/update-lock', () => {
const updateLock = rewire('../../../src/lib/update-lock');
const breadcrumbFiles = [
'rollback-health-breadcrumb',
'rollback-altboot-breadcrumb',
];
const mockBreadcrumbs = (breadcrumb?: string) => {
mockFs({
[path.join(
constants.rootMountPoint,
constants.stateMountPoint,
breadcrumb ? breadcrumb : '',
)]: '',
});
};
const appId = 1234567;
const serviceName = 'test';
const mockLockDir = ({
appId,
service,
createLockfile = true,
}: {
appId: number;
service: string;
createLockfile?: boolean;
}) => {
const lockDirFiles: any = {};
if (createLockfile) {
lockDirFiles['updates.lock'] = mockFs.file({
uid: lockfile.LOCKFILE_UID,
});
lockDirFiles['resin-updates.lock'] = mockFs.file({
uid: lockfile.LOCKFILE_UID,
});
}
mockFs({
[path.join(
constants.rootMountPoint,
updateLock.lockPath(appId),
service,
)]: {
[createLockfile ? 'updates.lock' : 'ignore-this.lock']: '',
},
serviceName,
)]: lockDirFiles,
});
};
@ -57,29 +49,33 @@ describe('lib/update-lock', () => {
constants.rootMountPoint = process.env.ROOT_MOUNTPOINT;
});
describe('Lockfile path methods', () => {
const testAppId = 1234567;
const testService = 'test';
describe('lockPath', () => {
it('should return path prefix of service lockfiles on host', () => {
expect(updateLock.lockPath(testAppId)).to.equal(
`/tmp/balena-supervisor/services/${testAppId}`,
expect(updateLock.lockPath(appId)).to.equal(
`/tmp/balena-supervisor/services/${appId}`,
);
expect(updateLock.lockPath(testAppId, testService)).to.equal(
`/tmp/balena-supervisor/services/${testAppId}/${testService}`,
expect(updateLock.lockPath(appId, serviceName)).to.equal(
`/tmp/balena-supervisor/services/${appId}/${serviceName}`,
);
});
it('should return the complete paths of (non-)legacy lockfiles on host', () => {
const lockFilesOnHost = updateLock.__get__('lockFilesOnHost');
expect(lockFilesOnHost(testAppId, testService)).to.deep.equal([
`${constants.rootMountPoint}/tmp/balena-supervisor/services/${testAppId}/${testService}/updates.lock`,
`${constants.rootMountPoint}/tmp/balena-supervisor/services/${testAppId}/${testService}/resin-updates.lock`,
]);
});
});
describe('abortIfHUPInProgress', () => {
const breadcrumbFiles = [
'rollback-health-breadcrumb',
'rollback-altboot-breadcrumb',
];
const mockBreadcrumbs = (breadcrumb?: string) => {
mockFs({
[path.join(
constants.rootMountPoint,
constants.stateMountPoint,
breadcrumb ? breadcrumb : '',
)]: '',
});
};
afterEach(() => mockFs.restore());
it('should throw if any breadcrumbs exist on host', async () => {
@ -109,152 +105,169 @@ describe('lib/update-lock', () => {
});
describe('Lock/dispose functionality', () => {
const lockFile = updateLock.__get__('lockFile');
const locksTaken = updateLock.__get__('locksTaken');
const dispose = updateLock.__get__('dispose');
const lockExistsErrHandler = updateLock.__get__('lockExistsErrHandler');
const getLockParentDir = (): string =>
`${constants.rootMountPoint}${updateLock.lockPath(appId, serviceName)}`;
const releaseFn = stub();
const testLockPaths = ['/tmp/test/1', '/tmp/test/2'];
const expectLocks = async (exists: boolean = true) => {
expect(await fs.readdir(getLockParentDir())).to.deep.equal(
exists ? ['resin-updates.lock', 'updates.lock'] : [],
);
};
let unlockSyncStub: SinonStub;
let unlockAsyncSpy: SinonSpy;
let lockAsyncSpy: SinonSpy;
let unlockSpy: SinonSpy;
let lockSpy: SinonSpy;
let execStub: SinonStub;
let configGetStub: SinonStub;
beforeEach(() => {
unlockSpy = spy(lockfile, 'unlock');
lockSpy = spy(lockfile, 'lock');
// lockfile.lock calls exec to interface with the lockfile binary,
// so mock it here as we don't have access to the binary in the test env
// @ts-ignore
unlockSyncStub = stub(lockFile, 'unlockSync').callsFake((lockPath) => {
// Throw error on process.exit for one of the two lockpaths
if (lockPath === testLockPaths[1]) {
throw new Error(
'handled unlockSync error which should not crash test process',
);
}
execStub = stub(fsUtils, 'exec').callsFake(async (command, opts) => {
// Sanity check for the command call
expect(command.trim().startsWith('lockfile')).to.be.true;
// Remove any `lockfile` command options to leave just the command and the target filepath
const [, targetPath] = command
.replace(/-v|-nnn|-r\s+\d+|-l\s+\d+|-s\s+\d+|-!|-ml|-mu/g, '')
.split(/\s+/);
// Emulate the lockfile binary exec call
await fsUtils.touch(targetPath);
await fs.chown(targetPath, opts!.uid!, 0);
});
unlockAsyncSpy = spy(lockFile, 'unlockAsync');
lockAsyncSpy = spy(lockFile, 'lockAsync');
// config.get is called in updateLock.lock to get `lockOverride` value,
// so mock it here to definitively avoid any side effects
configGetStub = stub(config, 'get').resolves(false);
});
afterEach(() => {
for (const key of Object.keys(locksTaken)) {
delete locksTaken[key];
}
unlockSyncStub.restore();
unlockAsyncSpy.restore();
lockAsyncSpy.restore();
});
afterEach(async () => {
unlockSpy.restore();
lockSpy.restore();
execStub.restore();
it('should try to clean up existing locks on process exit', () => {
testLockPaths.forEach((p) => (locksTaken[p] = true));
configGetStub.restore();
// @ts-ignore
process.emit('exit');
testLockPaths.forEach((p) => {
expect(unlockSyncStub).to.have.been.calledWith(p);
});
});
it('should dispose of locks', async () => {
for (const lock of testLockPaths) {
locksTaken[lock] = true;
// Even though mock-fs is restored, this is needed to delete any in-memory storage of locks
for (const lock of lockfile.getLocksTaken()) {
await lockfile.unlock(lock);
}
await dispose(releaseFn);
expect(locksTaken).to.deep.equal({});
expect(releaseFn).to.have.been.called;
testLockPaths.forEach((p) => {
expect(unlockAsyncSpy).to.have.been.calledWith(p);
});
mockFs.restore();
});
describe('lockExistsErrHandler', () => {
it('should handle EEXIST', async () => {
const appIdentifiers = [
{ id: '1234567', service: 'test1', type: 'appId' },
{
id: 'c89a7cb83d974518479591ffaf7c2417',
service: 'test2',
type: 'appUuid',
it('should take the lock, run the function, then dispose of locks', async () => {
// Set up fake filesystem for lockfiles
mockLockDir({ createLockfile: false });
await expect(
updateLock.lock(appId, { force: false }, async () => {
// At this point the locks should be taken and not removed
// until this function has been resolved
await expectLocks(true);
return Promise.resolve();
}),
).to.eventually.be.fulfilled;
// Both `updates.lock` and `resin-updates.lock` should have been taken
expect(lockSpy.args).to.have.length(2);
// Everything that was locked should have been unlocked
expect(lockSpy.args).to.deep.equal(unlockSpy.args);
});
it('should throw UpdatesLockedError if lockfile exists', async () => {
// Set up fake filesystem for lockfiles
mockLockDir({ createLockfile: true });
const lockPath = `${getLockParentDir()}/updates.lock`;
execStub.throws(new lockfile.LockfileExistsError(lockPath));
try {
await updateLock.lock(appId, { force: false }, async () => {
await expectLocks(false);
return Promise.resolve();
});
expect.fail('updateLock.lock should throw an UpdatesLockedError');
} catch (err) {
expect(err).to.be.instanceOf(UpdatesLockedError);
}
// Should only have attempted to take `updates.lock`
expect(lockSpy.args.flat()).to.deep.equal([lockPath]);
// Since the lock-taking failed, there should be no locks to dispose of
expect(lockfile.getLocksTaken()).to.have.length(0);
// Since nothing was locked, nothing should be unlocked
expect(unlockSpy.args).to.have.length(0);
});
it('should dispose of taken locks on any other errors', async () => {
// Set up fake filesystem for lockfiles
mockLockDir({ createLockfile: false });
try {
await updateLock.lock(
appId,
{ 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
async () => {
await expectLocks();
return Promise.reject(new Error('Test error'));
},
{ id: 'c89a7cb', service: 'test3', type: 'appUuid' },
];
for (const { id, service, type } of appIdentifiers) {
// Handle legacy & nonlegacy lockfile names
for (const lockfile of ['updates.lock', 'resin-updates.lock']) {
const error = {
code: 'EEXIST',
message: `EEXIST: open "/tmp/balena-supervisor/services/${id}/${service}/${lockfile}"`,
};
await expect(lockExistsErrHandler(error, releaseFn))
.to.eventually.be.rejectedWith(
`Lockfile exists for ${JSON.stringify({
serviceName: service,
[type]: id,
})}`,
)
.and.be.an.instanceOf(UpdatesLockedError);
}
}
});
);
} catch {
/* noop */
// This just catches the 'Test error' above
}
it('should handle any other errors', async () => {
await expect(lockExistsErrHandler(new Error('Test error'), releaseFn))
.to.eventually.be.rejectedWith('Test error')
.and.be.an.instanceOf(UpdatesLockedError);
});
// Both `updates.lock` and `resin-updates.lock` should have been taken
expect(lockSpy.args).to.have.length(2);
// Everything that was locked should have been unlocked
expect(lockSpy.args).to.deep.equal(unlockSpy.args);
});
describe('lock', () => {
let bluebirdUsing: SinonSpy;
let bluebirdResolve: SinonSpy;
const lockParamFn = stub().resolves();
it('resolves input function without locking when appId is null', async () => {
mockLockDir({ createLockfile: true });
beforeEach(() => {
bluebirdUsing = spy(Bluebird, 'using');
bluebirdResolve = spy(Bluebird, 'resolve');
});
await expect(
updateLock.lock(null as any, { force: false }, stub().resolves()),
).to.be.fulfilled;
afterEach(() => {
bluebirdUsing.restore();
bluebirdResolve.restore();
// Since appId is null, updateLock.lock should just run the function, so
// there should be no interfacing with the lockfile module
expect(unlockSpy).to.not.have.been.called;
expect(lockSpy).to.not.have.been.called;
});
mockFs.restore();
});
it('unlocks lockfile to resolve function if force option specified', async () => {
mockLockDir({ createLockfile: true });
it('resolves input function without dispose pattern when appId is null', async () => {
mockLockDir({ appId: 1234567, service: 'test', createLockfile: true });
await expect(updateLock.lock(null, { force: false }, lockParamFn)).to.be
.fulfilled;
expect(bluebirdResolve).to.have.been.called;
});
await expect(updateLock.lock(1234567, { force: true }, stub().resolves()))
.to.be.fulfilled;
it('resolves input function without dispose pattern when no lockfiles exist', async () => {
mockLockDir({ appId: 1234567, service: 'test', createLockfile: false });
await expect(updateLock.lock(1234567, { force: false }, lockParamFn)).to
.be.fulfilled;
expect(bluebirdResolve).to.have.been.called;
});
expect(unlockSpy).to.have.been.called;
expect(lockSpy).to.have.been.called;
});
it('uses dispose pattern if lockfile present and throws error', async () => {
mockLockDir({ appId: 1234567, service: 'test' });
await expect(updateLock.lock(1234567, { force: false }, lockParamFn))
.to.eventually.be.rejectedWith(
'Lockfile exists for {"serviceName":"test","appId":"1234567"}',
)
.and.be.an.instanceOf(UpdatesLockedError);
expect(lockAsyncSpy).to.have.been.called;
expect(bluebirdUsing).to.have.been.called;
});
it('unlocks lockfile to resolve function if lockOverride option specified', async () => {
configGetStub.resolves(true);
mockLockDir({ createLockfile: true });
it('unlocks lockfile to resolve function if force option specified', async () => {
mockLockDir({ appId: 1234567, service: 'test' });
await expect(updateLock.lock(1234567, { force: true }, lockParamFn)).to
.be.fulfilled;
expect(unlockAsyncSpy).to.have.been.called;
expect(lockAsyncSpy).to.have.been.called;
expect(bluebirdUsing).to.have.been.called;
});
await expect(
updateLock.lock(1234567, { force: false }, stub().resolves()),
).to.be.fulfilled;
expect(unlockSpy).to.have.been.called;
expect(lockSpy).to.have.been.called;
});
});
});