diff --git a/docker-compose.test.yml b/docker-compose.test.yml index 291e528e..c435b8d4 100644 --- a/docker-compose.test.yml +++ b/docker-compose.test.yml @@ -20,6 +20,7 @@ services: - mock-systemd volumes: - dbus:/shared/dbus + - tmp:/mnt/root/tmp/balena-supervisor/services - ./test/data/root:/mnt/root - ./test/data/root/mnt/boot:/mnt/boot - ./test/lib/wait-for-it.sh:/wait-for-it.sh @@ -71,6 +72,7 @@ services: stop_grace_period: 3s volumes: - dbus:/shared/dbus + - tmp:/mnt/root/tmp/balena-supervisor/services # Set required supervisor configuration variables here environment: DOCKER_HOST: tcp://docker:2375 @@ -97,8 +99,12 @@ services: - /mnt/data volumes: + # Use tmpfs to avoid files remaining between runs dbus: driver_opts: - # Use tmpfs to avoid files remaining between runs + type: tmpfs + device: tmpfs + tmp: + driver_opts: type: tmpfs device: tmpfs 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/app.ts b/src/compose/app.ts index 2530d7e7..e3e1dac5 100644 --- a/src/compose/app.ts +++ b/src/compose/app.ts @@ -25,6 +25,7 @@ import { checkTruthy } from '../lib/validation'; import type { ServiceComposeConfig, DeviceMetadata } from './types/service'; import { pathExistsOnRoot } from '../lib/host-utils'; import { isSupervisor } from '../lib/supervisor-metadata'; +import type { LocksTakenMap } from '../lib/update-lock'; export interface AppConstructOpts { appId: number; @@ -43,6 +44,8 @@ export interface UpdateState { availableImages: Image[]; containerIds: Dictionary; downloading: string[]; + locksTaken: LocksTakenMap; + force: boolean; } interface ChangingPair { @@ -50,6 +53,10 @@ interface ChangingPair { target?: T; } +export interface AppsToLockMap { + [appId: number]: Set; +} + export class App { public appId: number; public appUuid?: string; @@ -109,8 +116,15 @@ export class App { // Check to see if we need to polyfill in some "new" data for legacy services this.migrateLegacy(target); - // Check for changes in the volumes. We don't remove any volumes until we remove an - // entire app + let steps: CompositionStep[] = []; + + // Any services which have died get a remove step + for (const service of this.services) { + if (service.status === 'Dead') { + steps.push(generateStep('remove', { current: service })); + } + } + const volumeChanges = this.compareComponents( this.volumes, target.volumes, @@ -122,61 +136,88 @@ export class App { true, ); - let steps: CompositionStep[] = []; - - // Any services which have died get a remove step - for (const service of this.services) { - if (service.status === 'Dead') { - steps.push(generateStep('remove', { current: service })); - } - } - - const { removePairs, installPairs, updatePairs } = this.compareServices( - this.services, - target.services, - state.containerIds, - ); + const { removePairs, installPairs, updatePairs, dependentServices } = + this.compareServices( + this.services, + target.services, + state.containerIds, + networkChanges, + volumeChanges, + ); // For every service which needs to be updated, update via update strategy. const servicePairs = removePairs.concat(updatePairs, installPairs); - steps = steps.concat( - servicePairs - .map((pair) => - this.generateStepsForService(pair, { - ...state, - servicePairs, - targetApp: target, - networkPairs: networkChanges, - volumePairs: volumeChanges, + // generateStepsForService will populate appsToLock with services that + // need to be locked, including services that need to be removed due to + // network or volume changes. + const appsToLock: AppsToLockMap = { + // this.appId should always equal target.appId. + [target.appId]: new Set(), + }; + const serviceSteps = servicePairs + .flatMap((pair) => + this.generateStepsForService(pair, { + ...state, + servicePairs, + targetApp: target, + networkPairs: networkChanges, + volumePairs: volumeChanges, + appsToLock, + }), + ) + .filter((step) => step != null); + + // Generate lock steps from appsToLock + for (const [appId, services] of Object.entries(appsToLock)) { + if (services.size > 0) { + steps.push( + generateStep('takeLock', { + appId: parseInt(appId, 10), + services: Array.from(services), + force: state.force, }), - ) - .filter((step) => step != null) as CompositionStep[], - ); + ); + } + } + + // Attach service steps + steps = steps.concat(serviceSteps); // Generate volume steps steps = steps.concat( - this.generateStepsForComponent(volumeChanges, servicePairs, (v, svc) => - svc.hasVolume(v.name), - ), + this.generateStepsForComponent(volumeChanges, dependentServices), ); // Generate network steps steps = steps.concat( - this.generateStepsForComponent(networkChanges, servicePairs, (n, svc) => - svc.hasNetwork(n.name), - ), + this.generateStepsForComponent(networkChanges, dependentServices), ); - if ( - steps.length === 0 && - target.commit != null && - this.commit !== target.commit - ) { - steps.push( - generateStep('updateCommit', { - target: target.commit, - appId: this.appId, - }), - ); + if (steps.length === 0) { + // Update commit in db if different + if (target.commit != null && this.commit !== target.commit) { + steps.push( + generateStep('updateCommit', { + target: target.commit, + appId: this.appId, + }), + ); + } + // 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), + ) + ) { + // Release locks for all services before settling state + steps.push( + generateStep('releaseLock', { + appId: target.appId, + }), + ); + } } return steps; } @@ -185,6 +226,21 @@ export class App { state: Omit & { keepVolumes: boolean }, ): 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), + ) + ) { + return [ + generateStep('takeLock', { + appId: this.appId, + services: this.services.map((svc) => svc.serviceName), + force: state.force, + }), + ]; + } + return Object.values(this.services).map((service) => generateStep('kill', { current: service }), ); @@ -289,14 +345,24 @@ export class App { return outputs; } + private getDependentServices( + component: T, + dependencyFn: (component: T, service: Service) => boolean, + ) { + return this.services.filter((s) => dependencyFn(component, s)); + } + private compareServices( current: Service[], target: Service[], - containerIds: Dictionary, + containerIds: UpdateState['containerIds'], + networkChanges: Array>, + volumeChanges: Array>, ): { installPairs: Array>; removePairs: Array>; updatePairs: Array>; + dependentServices: Service[]; } { const currentByServiceName = _.keyBy(current, 'serviceName'); const targetByServiceName = _.keyBy(target, 'serviceName'); @@ -304,8 +370,26 @@ export class App { const currentServiceNames = Object.keys(currentByServiceName); const targetServiceNames = Object.keys(targetByServiceName); + // For volume|network removal or config changes, we require dependent + // services be killed first. + const dependentServices: Service[] = []; + for (const { current: c } of networkChanges) { + if (c != null) { + dependentServices.push( + ...this.getDependentServices(c, (n, svc) => svc.hasNetwork(n.name)), + ); + } + } + for (const { current: c } of volumeChanges) { + if (c != null) { + dependentServices.push( + ...this.getDependentServices(c, (v, svc) => svc.hasVolume(v.name)), + ); + } + } const toBeRemoved = _(currentServiceNames) .difference(targetServiceNames) + .union(dependentServices.map((s) => s.serviceName)) .map((id) => ({ current: currentByServiceName[id] })) .value(); @@ -413,6 +497,15 @@ export class App { ); }; + /** + * Checks if a service is destined for removal due to a network or volume change + */ + const shouldBeRemoved = (serviceCurrent: Service) => { + return toBeRemoved.some( + (pair) => pair.current.serviceName === serviceCurrent.serviceName, + ); + }; + /** * Filter all the services which should be updated due to run state change, or config mismatch. */ @@ -423,16 +516,18 @@ export class App { })) .filter( ({ current: c, target: t }) => - !isEqualExceptForRunningState(c, t) || - shouldBeStarted(c, t) || - shouldBeStopped(c, t) || - shouldWaitForStop(c), + !shouldBeRemoved(c) && + (!isEqualExceptForRunningState(c, t) || + shouldBeStarted(c, t) || + shouldBeStopped(c, t) || + shouldWaitForStop(c)), ); return { installPairs: toBeInstalled, removePairs: toBeRemoved, updatePairs: toBeUpdated, + dependentServices, }; } @@ -444,8 +539,7 @@ export class App { // it should be changed. private generateStepsForComponent( components: Array>, - changingServices: Array>, - dependencyFn: (component: T, service: Service) => boolean, + dependentServices: Service[], ): CompositionStep[] { if (components.length === 0) { return []; @@ -453,36 +547,42 @@ export class App { let steps: CompositionStep[] = []; + const componentIsVolume = + (components[0].current ?? components[0].target) instanceof Volume; + const actions: { create: CompositionStepAction; remove: CompositionStepAction; - } = - (components[0].current ?? components[0].target) instanceof Volume - ? { create: 'createVolume', remove: 'removeVolume' } - : { create: 'createNetwork', remove: 'removeNetwork' }; + } = componentIsVolume + ? { create: 'createVolume', remove: 'removeVolume' } + : { create: 'createNetwork', remove: 'removeNetwork' }; for (const { current, target } of components) { // If a current exists, we're either removing it or updating the configuration. In - // both cases, we must remove the component first, so we output those steps first. + // both cases, we must remove the component before creating it to avoid + // Engine conflicts. So we always emit a remove step first. // If we do remove the component, we first need to remove any services which depend - // on the component + // on the component. The service removal steps are generated in this.generateStepsForService + // after their removal is calculated in this.compareServices. if (current != null) { - // Find any services which are currently running which need to be killed when we - // recreate this component - const dependencies = _.filter(this.services, (s) => - dependencyFn(current, s), - ); - if (dependencies.length > 0) { - // We emit kill steps for these services, and wait to destroy the component in - // the next state application loop - // FIXME: We should add to the changingServices array, as we could emit several - // kill steps for a service - steps = steps.concat( - dependencies.flatMap((svc) => - this.generateKillStep(svc, changingServices), - ), - ); - } else { + // If there are any dependent services which have the volume or network, + // we cannot proceed to component removal. + const dependentServicesOfComponent = dependentServices.filter((s) => { + if (componentIsVolume) { + return this.serviceHasNetworkOrVolume( + s, + [], + [{ current: current as Volume, target: target as Volume }], + ); + } else { + return this.serviceHasNetworkOrVolume( + s, + [{ current: current as Network, target: target as Network }], + [], + ); + } + }); + if (dependentServicesOfComponent.length === 0) { steps = steps.concat([generateStep(actions.remove, { current })]); } } else if (target != null) { @@ -496,24 +596,25 @@ export class App { private generateStepsForService( { current, target }: ChangingPair, context: { - availableImages: Image[]; - downloading: string[]; targetApp: App; - containerIds: Dictionary; networkPairs: Array>; volumePairs: Array>; servicePairs: Array>; - }, - ): Nullable { + appsToLock: AppsToLockMap; + } & UpdateState, + ): CompositionStep[] { + const servicesLocked = this.services + .concat(context.targetApp.services) + .every((svc) => context.locksTaken.isLocked(svc.appId, svc.serviceName)); if (current?.status === 'Stopping') { // There's a kill step happening already, emit a noop to ensure // we stay alive while this happens - return generateStep('noop', {}); + return [generateStep('noop', {})]; } if (current?.status === 'Dead') { // A remove step will already have been generated, so we let the state // application loop revisit this service, once the state has settled - return; + return []; } const needsDownload = @@ -530,7 +631,7 @@ export class App { ) { // The image needs to be downloaded, and it's currently downloading. // We simply keep the application loop alive - return generateStep('noop', {}); + return [generateStep('noop', {})]; } if (current == null) { @@ -539,6 +640,8 @@ export class App { target!, context.targetApp, needsDownload, + servicesLocked, + context.appsToLock, context.availableImages, context.networkPairs, context.volumePairs, @@ -547,6 +650,8 @@ export class App { } else { // This service is in both current & target so requires an update, // or it's a service that's not in target so requires removal + + // Skip updateMetadata for services with networks or volumes const needsSpecialKill = this.serviceHasNetworkOrVolume( current, context.networkPairs, @@ -557,8 +662,14 @@ export class App { target != null && current.isEqualConfig(target, context.containerIds) ) { - // we're only starting/stopping a service - return this.generateContainerStep(current, target); + // Update service metadata or start/stop a service + return this.generateContainerStep( + current, + target, + context.appsToLock, + context.targetApp.services, + servicesLocked, + ); } let strategy: string; @@ -590,29 +701,13 @@ export class App { dependenciesMetForStart, dependenciesMetForKill, needsSpecialKill, + servicesLocked, + services: this.services.concat(context.targetApp.services), + appsToLock: context.appsToLock, }); } } - // We return an array from this function so the caller can just concatenate the arrays - // without worrying if the step is skipped or not - private generateKillStep( - service: Service, - changingServices: Array>, - ): CompositionStep[] { - if ( - service.status !== 'Stopping' && - !_.some( - changingServices, - ({ current }) => current?.serviceName === service.serviceName, - ) - ) { - return [generateStep('kill', { current: service })]; - } else { - return []; - } - } - private serviceHasNetworkOrVolume( svc: Service, networkPairs: Array>, @@ -647,16 +742,39 @@ export class App { ); } - private generateContainerStep(current: Service, target: Service) { - // if the services release doesn't match, then rename the container... + private generateContainerStep( + current: Service, + target: Service, + appsToLock: AppsToLockMap, + targetServices: Service[], + servicesLocked: boolean, + ): CompositionStep[] { + // Update container metadata if service release has changed if (current.commit !== target.commit) { - return generateStep('updateMetadata', { current, target }); - } else if (target.config.running !== current.config.running) { - if (target.config.running) { - return generateStep('start', { target }); + if (servicesLocked) { + return [generateStep('updateMetadata', { current, target })]; } else { - return generateStep('stop', { current }); + // Otherwise, take lock for all services first + this.services.concat(targetServices).forEach((s) => { + appsToLock[target.appId].add(s.serviceName); + }); + return []; } + } else if (target.config.running !== current.config.running) { + // Take lock for all services before starting/stopping container + if (!servicesLocked) { + this.services.concat(targetServices).forEach((s) => { + appsToLock[target.appId].add(s.serviceName); + }); + return []; + } + if (target.config.running) { + return [generateStep('start', { target })]; + } else { + return [generateStep('stop', { current })]; + } + } else { + return []; } } @@ -664,21 +782,26 @@ export class App { target: Service, targetApp: App, needsDownload: boolean, - availableImages: Image[], + servicesLocked: boolean, + appsToLock: AppsToLockMap, + availableImages: UpdateState['availableImages'], networkPairs: Array>, volumePairs: Array>, servicePairs: Array>, - ): CompositionStep | undefined { + ): CompositionStep[] { if ( needsDownload && this.dependenciesMetForServiceFetch(target, servicePairs) ) { // We know the service name exists as it always does for targets - return generateStep('fetch', { - image: imageManager.imageFromService(target), - serviceName: target.serviceName!, - }); + return [ + generateStep('fetch', { + image: imageManager.imageFromService(target), + serviceName: target.serviceName, + }), + ]; } else if ( + target != null && this.dependenciesMetForServiceStart( target, targetApp, @@ -688,7 +811,15 @@ export class App { servicePairs, ) ) { - return generateStep('start', { target }); + if (!servicesLocked) { + this.services + .concat(targetApp.services) + .forEach((svc) => appsToLock[target.appId].add(svc.serviceName)); + return []; + } + return [generateStep('start', { target })]; + } else { + return []; } } @@ -728,7 +859,7 @@ export class App { private dependenciesMetForServiceStart( target: Service, targetApp: App, - availableImages: Image[], + availableImages: UpdateState['availableImages'], networkPairs: Array>, volumePairs: Array>, servicePairs: Array>, @@ -738,7 +869,7 @@ export class App { // are services which are changing). We could have a dependency which is // starting up, but is not yet running. const depInstallingButNotRunning = _.some(targetApp.services, (svc) => { - if (target.dependsOn?.includes(svc.serviceName!)) { + if (target.dependsOn?.includes(svc.serviceName)) { if (!svc.config.running) { return true; } @@ -785,14 +916,14 @@ export class App { // block the killing too much, potentially causing a deadlock) private dependenciesMetForServiceKill( targetApp: App, - availableImages: Image[], + availableImages: UpdateState['availableImages'], ) { return this.targetImagesReady(targetApp.services, availableImages); } private targetImagesReady( targetServices: Service[], - availableImages: Image[], + availableImages: UpdateState['availableImages'], ) { return targetServices.every((service) => availableImages.some( diff --git a/src/compose/application-manager.ts b/src/compose/application-manager.ts index 2eb19fdd..816d00b5 100644 --- a/src/compose/application-manager.ts +++ b/src/compose/application-manager.ts @@ -17,10 +17,11 @@ import { ContractViolationError, InternalInconsistencyError, } from '../lib/errors'; -import { lock } from '../lib/update-lock'; +import { getServicesLockedByAppId, LocksTakenMap } from '../lib/update-lock'; import { checkTruthy } from '../lib/validation'; import App from './app'; +import type { UpdateState } from './app'; import * as volumeManager from './volume-manager'; import * as networkManager from './network-manager'; import * as serviceManager from './service-manager'; @@ -63,7 +64,6 @@ export function resetTimeSpentFetching(value: number = 0) { } const actionExecutors = getExecutors({ - lockFn: lock, callbacks: { fetchStart: () => { fetchesInProgress += 1; @@ -119,6 +119,7 @@ export async function getRequiredSteps( targetApps: InstancedAppState, keepImages?: boolean, keepVolumes?: boolean, + force: boolean = false, ): Promise { // get some required data const [downloading, availableImages, { localMode, delta }] = @@ -145,9 +146,11 @@ export async function getRequiredSteps( // Volumes are not removed when stopping an app when going to local mode keepVolumes, delta, + force, downloading, availableImages, containerIdsByAppId, + locksTaken: await getServicesLockedByAppId(), }); } @@ -159,9 +162,13 @@ export async function inferNextSteps( keepImages = false, keepVolumes = false, delta = true, - downloading = [] as string[], - availableImages = [] as Image[], - containerIdsByAppId = {} as { [appId: number]: Dictionary }, + force = false, + downloading = [] as UpdateState['downloading'], + availableImages = [] as UpdateState['availableImages'], + containerIdsByAppId = {} as { + [appId: number]: UpdateState['containerIds']; + }, + locksTaken = new LocksTakenMap(), } = {}, ) { const currentAppIds = Object.keys(currentApps).map((i) => parseInt(i, 10)); @@ -213,6 +220,8 @@ export async function inferNextSteps( availableImages, containerIds: containerIdsByAppId[id], downloading, + locksTaken, + force, }, targetApps[id], ), @@ -226,6 +235,8 @@ export async function inferNextSteps( keepVolumes, downloading, containerIds: containerIdsByAppId[id], + locksTaken, + force, }), ); } @@ -249,6 +260,8 @@ export async function inferNextSteps( availableImages, containerIds: containerIdsByAppId[id] ?? {}, downloading, + locksTaken, + force, }, targetApps[id], ), @@ -293,17 +306,6 @@ export async function inferNextSteps( return steps; } -export async function stopAll({ force = false, skipLock = false } = {}) { - const services = await serviceManager.getAll(); - await Promise.all( - services.map(async (s) => { - return lock(s.appId, { force, skipLock }, async () => { - await serviceManager.kill(s, { removeContainer: false, wait: true }); - }); - }), - ); -} - // The following two function may look pretty odd, but after the move to uuids, // there's a chance that the current running apps don't have a uuid set. We // still need to be able to work on these and perform various state changes. To @@ -493,7 +495,7 @@ function killServicesUsingApi(current: InstancedAppState): CompositionStep[] { // intermediate targets to perform changes export async function executeStep( step: CompositionStep, - { force = false, skipLock = false } = {}, + { force = false } = {}, ): Promise { if (!validActions.includes(step.action)) { return Promise.reject( @@ -507,7 +509,6 @@ export async function executeStep( await actionExecutors[step.action]({ ...step, force, - skipLock, } as any); } @@ -651,7 +652,7 @@ export function bestDeltaSource( function saveAndRemoveImages( current: InstancedAppState, target: InstancedAppState, - availableImages: imageManager.Image[], + availableImages: UpdateState['availableImages'], skipRemoval: boolean, ): CompositionStep[] { type ImageWithoutID = Omit; @@ -670,9 +671,8 @@ function saveAndRemoveImages( .filter((img) => img[1] != null) .value(); - const availableWithoutIds: ImageWithoutID[] = _.map( - availableImages, - (image) => _.omit(image, ['dockerImageId', 'id']), + const availableWithoutIds: ImageWithoutID[] = availableImages.map((image) => + _.omit(image, ['dockerImageId', 'id']), ); const currentImages = _.flatMap(current, (app) => @@ -692,18 +692,16 @@ function saveAndRemoveImages( const targetServices = Object.values(target).flatMap((app) => app.services); const targetImages = targetServices.map(imageManager.imageFromService); - const availableAndUnused = _.filter( - availableWithoutIds, + const availableAndUnused = availableWithoutIds.filter( (image) => - !_.some(currentImages.concat(targetImages), (imageInUse) => { + !currentImages.concat(targetImages).some((imageInUse) => { return _.isEqual(image, _.omit(imageInUse, ['dockerImageId', 'id'])); }), ); - const imagesToDownload = _.filter( - targetImages, + const imagesToDownload = targetImages.filter( (targetImage) => - !_.some(availableImages, (available) => + !availableImages.some((available) => imageManager.isSameImage(available, targetImage), ), ); @@ -713,10 +711,9 @@ function saveAndRemoveImages( ); // Images that are available but we don't have them in the DB with the exact metadata: - const imagesToSave: imageManager.Image[] = _.filter( - targetImages, + const imagesToSave: imageManager.Image[] = targetImages.filter( (targetImage) => { - const isActuallyAvailable = _.some(availableImages, (availableImage) => { + const isActuallyAvailable = availableImages.some((availableImage) => { // There is an image with same image name or digest // on the database if (imageManager.isSameImage(availableImage, targetImage)) { @@ -734,7 +731,7 @@ function saveAndRemoveImages( }); // There is no image in the database with the same metadata - const isNotSaved = !_.some(availableWithoutIds, (img) => + const isNotSaved = !availableWithoutIds.some((img) => _.isEqual(img, targetImage), ); diff --git a/src/compose/composition-steps.ts b/src/compose/composition-steps.ts index 787e3ec6..ded3b966 100644 --- a/src/compose/composition-steps.ts +++ b/src/compose/composition-steps.ts @@ -1,64 +1,43 @@ -import _ from 'lodash'; - import * as config from '../config'; - import type { Image } from './images'; import * as images from './images'; import type Network from './network'; import type Service from './service'; import * as serviceManager from './service-manager'; -import type Volume from './volume'; - -import { checkTruthy } from '../lib/validation'; import * as networkManager from './network-manager'; import * as volumeManager from './volume-manager'; -import type { DeviceLegacyReport } from '../types/state'; +import type Volume from './volume'; import * as commitStore from './commit'; +import * as updateLock from '../lib/update-lock'; +import type { DeviceLegacyReport } from '../types/state'; -interface BaseCompositionStepArgs { - force?: boolean; - skipLock?: boolean; -} - -// FIXME: Most of the steps take the -// BaseCompositionStepArgs, but some also take an options -// structure which includes some of the same fields. It -// would be nice to remove the need for this interface CompositionStepArgs { stop: { current: Service; options?: { - skipLock?: boolean; wait?: boolean; }; - } & BaseCompositionStepArgs; + }; kill: { current: Service; options?: { - skipLock?: boolean; wait?: boolean; }; - } & BaseCompositionStepArgs; + }; remove: { current: Service; - } & BaseCompositionStepArgs; + }; updateMetadata: { current: Service; target: Service; - options?: { - skipLock?: boolean; - }; - } & BaseCompositionStepArgs; + }; restart: { current: Service; target: Service; - options?: { - skipLock?: boolean; - }; - } & BaseCompositionStepArgs; + }; start: { target: Service; - } & BaseCompositionStepArgs; + }; updateCommit: { target: string; appId: number; @@ -67,10 +46,9 @@ interface CompositionStepArgs { current: Service; target: Service; options?: { - skipLock?: boolean; timeout?: number; }; - } & BaseCompositionStepArgs; + }; fetch: { image: Image; serviceName: string; @@ -96,6 +74,14 @@ interface CompositionStepArgs { }; ensureSupervisorNetwork: object; noop: object; + takeLock: { + appId: number; + services: string[]; + force: boolean; + }; + releaseLock: { + appId: number; + }; } export type CompositionStepAction = keyof CompositionStepArgs; @@ -117,13 +103,6 @@ export function generateStep( type Executors = { [key in T]: (step: CompositionStepT) => Promise; }; -type LockingFn = ( - // TODO: Once the entire codebase is typescript, change - // this to number - app: number | number[] | null, - args: BaseCompositionStepArgs, - fn: () => Promise, -) => Promise; interface CompositionCallbacks { // TODO: Once the entire codebase is typescript, change @@ -135,71 +114,36 @@ interface CompositionCallbacks { bestDeltaSource: (image: Image, available: Image[]) => string | null; } -export function getExecutors(app: { - lockFn: LockingFn; - callbacks: CompositionCallbacks; -}) { +export function getExecutors(app: { callbacks: CompositionCallbacks }) { const executors: Executors = { - stop: (step) => { - return app.lockFn( - step.current.appId, - { - force: step.force, - skipLock: step.skipLock || _.get(step, ['options', 'skipLock']), - }, - async () => { - const wait = _.get(step, ['options', 'wait'], false); - await serviceManager.kill(step.current, { - removeContainer: false, - wait, - }); - }, - ); + stop: async (step) => { + // Should always be preceded by a takeLock step, + // so the call is executed assuming that the lock is taken. + await serviceManager.kill(step.current, { + removeContainer: false, + wait: step.options?.wait || false, + }); }, - kill: (step) => { - return app.lockFn( - step.current.appId, - { - force: step.force, - skipLock: step.skipLock || _.get(step, ['options', 'skipLock']), - }, - async () => { - await serviceManager.kill(step.current); - }, - ); + kill: async (step) => { + // Should always be preceded by a takeLock step, + // so the call is executed assuming that the lock is taken. + await serviceManager.kill(step.current); }, remove: async (step) => { // Only called for dead containers, so no need to // take locks await serviceManager.remove(step.current); }, - updateMetadata: (step) => { - const skipLock = - step.skipLock || - checkTruthy(step.current.config.labels['io.balena.legacy-container']); - return app.lockFn( - step.current.appId, - { - force: step.force, - skipLock: skipLock || _.get(step, ['options', 'skipLock']), - }, - async () => { - await serviceManager.updateMetadata(step.current, step.target); - }, - ); + updateMetadata: async (step) => { + // Should always be preceded by a takeLock step, + // so the call is executed assuming that the lock is taken. + await serviceManager.updateMetadata(step.current, step.target); }, - restart: (step) => { - return app.lockFn( - step.current.appId, - { - force: step.force, - skipLock: step.skipLock || _.get(step, ['options', 'skipLock']), - }, - async () => { - await serviceManager.kill(step.current, { wait: true }); - await serviceManager.start(step.target); - }, - ); + restart: async (step) => { + // Should always be preceded by a takeLock step, + // so the call is executed assuming that the lock is taken. + await serviceManager.kill(step.current, { wait: true }); + await serviceManager.start(step.target); }, start: async (step) => { await serviceManager.start(step.target); @@ -207,17 +151,10 @@ export function getExecutors(app: { updateCommit: async (step) => { await commitStore.upsertCommitForApp(step.appId, step.target); }, - handover: (step) => { - return app.lockFn( - step.current.appId, - { - force: step.force, - skipLock: step.skipLock || _.get(step, ['options', 'skipLock']), - }, - async () => { - await serviceManager.handover(step.current, step.target); - }, - ); + handover: async (step) => { + // Should always be preceded by a takeLock step, + // so the call is executed assuming that the lock is taken. + await serviceManager.handover(step.current, step.target); }, fetch: async (step) => { const startTime = process.hrtime(); @@ -278,6 +215,12 @@ export function getExecutors(app: { noop: async () => { /* async noop */ }, + takeLock: async (step) => { + await updateLock.takeLock(step.appId, step.services, step.force); + }, + releaseLock: async (step) => { + await updateLock.releaseLock(step.appId); + }, }; return executors; diff --git a/src/compose/update-strategies.ts b/src/compose/update-strategies.ts index 7de4fa69..8d508572 100644 --- a/src/compose/update-strategies.ts +++ b/src/compose/update-strategies.ts @@ -2,6 +2,7 @@ import * as imageManager from './images'; import type Service from './service'; import type { CompositionStep } from './composition-steps'; import { generateStep } from './composition-steps'; +import type { AppsToLockMap } from './app'; import { InternalInconsistencyError } from '../lib/errors'; import { checkString } from '../lib/validation'; @@ -12,43 +13,82 @@ export interface StrategyContext { dependenciesMetForStart: boolean; dependenciesMetForKill: boolean; needsSpecialKill: boolean; + services: Service[]; + servicesLocked: boolean; + appsToLock: AppsToLockMap; +} + +function generateLockThenKillStep( + current: Service, + currentServices: Service[], + servicesLocked: boolean, + appsToLock: AppsToLockMap, +): CompositionStep[] { + if (!servicesLocked) { + currentServices.forEach((svc) => + appsToLock[svc.appId].add(svc.serviceName), + ); + return []; + } + return [generateStep('kill', { current })]; } export function getStepsFromStrategy( strategy: string, context: StrategyContext, -): CompositionStep { +): CompositionStep[] { switch (strategy) { case 'download-then-kill': if (context.needsDownload && context.target) { - return generateStep('fetch', { - image: imageManager.imageFromService(context.target), - serviceName: context.target.serviceName, - }); + return [ + generateStep('fetch', { + image: imageManager.imageFromService(context.target), + serviceName: context.target.serviceName, + }), + ]; } else if (context.dependenciesMetForKill) { // We only kill when dependencies are already met, so that we minimize downtime - return generateStep('kill', { current: context.current }); + return generateLockThenKillStep( + context.current, + context.services, + context.servicesLocked, + context.appsToLock, + ); } else { - return generateStep('noop', {}); + return [generateStep('noop', {})]; } case 'kill-then-download': case 'delete-then-download': - return generateStep('kill', { current: context.current }); + return generateLockThenKillStep( + context.current, + context.services, + context.servicesLocked, + context.appsToLock, + ); case 'hand-over': if (context.needsDownload && context.target) { - return generateStep('fetch', { - image: imageManager.imageFromService(context.target), - serviceName: context.target.serviceName, - }); + return [ + generateStep('fetch', { + image: imageManager.imageFromService(context.target), + serviceName: context.target.serviceName, + }), + ]; } else if (context.needsSpecialKill && context.dependenciesMetForKill) { - return generateStep('kill', { current: context.current }); + return generateLockThenKillStep( + context.current, + context.services, + context.servicesLocked, + context.appsToLock, + ); } else if (context.dependenciesMetForStart && context.target) { - return generateStep('handover', { - current: context.current, - target: context.target, - }); + return [ + generateStep('handover', { + current: context.current, + target: context.target, + }), + ]; } else { - return generateStep('noop', {}); + return [generateStep('noop', {})]; } default: throw new InternalInconsistencyError( diff --git a/src/config/configJson.ts b/src/config/configJson.ts index a327640e..44fd87e1 100644 --- a/src/config/configJson.ts +++ b/src/config/configJson.ts @@ -4,7 +4,7 @@ import _ from 'lodash'; import * as constants from '../lib/constants'; import * as hostUtils from '../lib/host-utils'; import * as osRelease from '../lib/os-release'; -import { readLock, writeLock } from '../lib/update-lock'; +import { takeGlobalLockRO, takeGlobalLockRW } from '../lib/process-lock'; import type * as Schema from './schema'; export default class ConfigJsonConfigBackend { @@ -25,9 +25,9 @@ export default class ConfigJsonConfigBackend { this.schema = schema; this.writeLockConfigJson = () => - writeLock('config.json').disposer((release) => release()); + takeGlobalLockRW('config.json').disposer((release) => release()); this.readLockConfigJson = () => - readLock('config.json').disposer((release) => release()); + takeGlobalLockRO('config.json').disposer((release) => release()); } public async set(keyVals: { 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-api/actions.ts b/src/device-api/actions.ts index 8718dbb8..30218f20 100644 --- a/src/device-api/actions.ts +++ b/src/device-api/actions.ts @@ -11,11 +11,11 @@ import * as applicationManager from '../compose/application-manager'; import type { CompositionStepAction } from '../compose/composition-steps'; import { generateStep } from '../compose/composition-steps'; import * as commitStore from '../compose/commit'; +import type Service from '../compose/service'; import { getApp } from '../device-state/db-format'; import * as TargetState from '../device-state/target-state'; import log from '../lib/supervisor-console'; import blink = require('../lib/blink'); -import { lock } from '../lib/update-lock'; import * as constants from '../lib/constants'; import { InternalInconsistencyError, @@ -88,32 +88,29 @@ export const regenerateKey = async (oldKey: string) => { export const doRestart = async (appId: number, force: boolean = false) => { await deviceState.initialized(); - return await lock(appId, { force }, async () => { - const currentState = await deviceState.getCurrentState(); - if (currentState.local.apps?.[appId] == null) { - throw new InternalInconsistencyError( - `Application with ID ${appId} is not in the current state`, - ); - } - const app = currentState.local.apps[appId]; - const services = app.services; - app.services = []; + const currentState = await deviceState.getCurrentState(); + if (currentState.local.apps?.[appId] == null) { + throw new InternalInconsistencyError( + `Application with ID ${appId} is not in the current state`, + ); + } - return deviceState - .applyIntermediateTarget(currentState, { - skipLock: true, - }) - .then(() => { - app.services = services; - return deviceState.applyIntermediateTarget(currentState, { - skipLock: true, - keepVolumes: false, - }); - }) - .finally(() => { - deviceState.triggerApplyTarget(); - }); - }); + const app = currentState.local.apps[appId]; + const services = app.services; + + try { + // Set target so that services get deleted + app.services = []; + await deviceState.applyIntermediateTarget(currentState, { force }); + // Restore services + app.services = services; + return deviceState.applyIntermediateTarget(currentState, { + keepVolumes: false, + force, + }); + } finally { + deviceState.triggerApplyTarget(); + } }; /** @@ -131,47 +128,37 @@ export const doPurge = async (appId: number, force: boolean = false) => { 'Purge data', ); - return await lock(appId, { force }, async () => { - const currentState = await deviceState.getCurrentState(); - if (currentState.local.apps?.[appId] == null) { - throw new InternalInconsistencyError( - `Application with ID ${appId} is not in the current state`, - ); - } + const currentState = await deviceState.getCurrentState(); + if (currentState.local.apps?.[appId] == null) { + throw new InternalInconsistencyError( + `Application with ID ${appId} is not in the current state`, + ); + } + // Save & delete the app from the current state + const app = currentState.local.apps[appId]; + delete currentState.local.apps[appId]; - const app = currentState.local.apps[appId]; - - // Delete the app from the current state - delete currentState.local.apps[appId]; - - return deviceState - .applyIntermediateTarget(currentState, { - skipLock: true, - // Purposely tell the apply function to delete volumes so they can get - // deleted even in local mode - keepVolumes: false, - }) - .then(() => { - currentState.local.apps[appId] = app; - return deviceState.applyIntermediateTarget(currentState, { - skipLock: true, - }); - }) - .finally(() => { - deviceState.triggerApplyTarget(); - }); - }) - .then(() => - logger.logSystemMessage('Purged data', { appId }, 'Purge data success'), - ) - .catch((err) => { - logger.logSystemMessage( - `Error purging data: ${err}`, - { appId, error: err }, - 'Purge data error', - ); - throw err; + try { + // Purposely tell the apply function to delete volumes so + // they can get deleted even in local mode + await deviceState.applyIntermediateTarget(currentState, { + keepVolumes: false, + force, }); + // Restore user app after purge + currentState.local.apps[appId] = app; + await deviceState.applyIntermediateTarget(currentState); + logger.logSystemMessage('Purged data', { appId }, 'Purge data success'); + } catch (err: any) { + logger.logSystemMessage( + `Error purging data: ${err}`, + { appId, error: err?.message ?? err }, + 'Purge data error', + ); + throw err; + } finally { + deviceState.triggerApplyTarget(); + } }; type ClientError = BadRequestError | NotFoundError; @@ -224,6 +211,57 @@ export const executeDeviceAction = async ( }); }; +/** + * Used internally by executeServiceAction to handle locks + * around execution of a service action. + */ +const executeDeviceActionWithLock = async ({ + action, + appId, + currentService, + targetService, + force = false, +}: { + action: CompositionStepAction; + appId: number; + currentService?: Service; + targetService?: Service; + force: boolean; +}) => { + try { + if (currentService) { + // Take lock for current service to be modified / stopped + await executeDeviceAction( + generateStep('takeLock', { + appId, + services: [currentService.serviceName], + force, + }), + // 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, + ); + } + + // 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, + }), + ); + } +}; + /** * Executes a composition step action on a service. * isLegacy indicates that the action is being called from a legacy (v1) endpoint, @@ -279,15 +317,23 @@ export const executeServiceAction = async ({ throw new NotFoundError(messages.targetServiceNotFound); } - // Execute action on service - return await executeDeviceAction( - generateStep(action, { - current: currentService, - target: targetService, - wait: true, - }), - force, - ); + // A single service start action doesn't require locks + if (action === 'start') { + // Execute action on service + await executeDeviceAction( + generateStep(action, { + target: targetService, + }), + ); + } else { + await executeDeviceActionWithLock({ + action, + appId, + currentService, + targetService, + force, + }); + } }; /** diff --git a/src/device-state.ts b/src/device-state.ts index ff4f7813..00e1d9a5 100644 --- a/src/device-state.ts +++ b/src/device-state.ts @@ -23,6 +23,7 @@ import { UpdatesLockedError, } from './lib/errors'; import * as updateLock from './lib/update-lock'; +import { takeGlobalLockRO, takeGlobalLockRW } from './lib/process-lock'; import * as dbFormat from './device-state/db-format'; import { getGlobalApiKey } from './device-api'; import * as sysInfo from './lib/system-info'; @@ -101,8 +102,6 @@ type DeviceStateStep = | deviceConfig.ConfigStep; let currentVolatile: DeviceReport = {}; -const writeLock = updateLock.writeLock; -const readLock = updateLock.readLock; let maxPollTime: number; let intermediateTarget: InstancedDeviceState | null = null; let applyBlocker: Nullable>; @@ -295,11 +294,11 @@ function emitAsync( } const readLockTarget = () => - readLock('target').disposer((release) => release()); + takeGlobalLockRO('target').disposer((release) => release()); const writeLockTarget = () => - writeLock('target').disposer((release) => release()); + takeGlobalLockRW('target').disposer((release) => release()); const inferStepsLock = () => - writeLock('inferSteps').disposer((release) => release()); + takeGlobalLockRW('inferSteps').disposer((release) => release()); function usingReadLockTarget any, U extends ReturnType>( fn: T, ): Bluebird> { @@ -575,11 +574,7 @@ export async function shutdown({ // should happen via intermediate targets export async function executeStepAction( step: DeviceStateStep, - { - force, - initial, - skipLock, - }: { force?: boolean; initial?: boolean; skipLock?: boolean }, + { force, initial }: { force?: boolean; initial?: boolean }, ) { if (deviceConfig.isValidAction(step.action)) { await deviceConfig.executeStepAction(step as deviceConfig.ConfigStep, { @@ -588,7 +583,6 @@ export async function executeStepAction( } else if (applicationManager.validActions.includes(step.action)) { return applicationManager.executeStep(step as any, { force, - skipLock, }); } else { switch (step.action) { @@ -614,11 +608,9 @@ export async function applyStep( { force, initial, - skipLock, }: { force?: boolean; initial?: boolean; - skipLock?: boolean; }, ) { if (shuttingDown) { @@ -628,7 +620,6 @@ export async function applyStep( await executeStepAction(step, { force, initial, - skipLock, }); emitAsync('step-completed', null, step); } catch (e: any) { @@ -686,7 +677,6 @@ export const applyTarget = async ({ force = false, initial = false, intermediate = false, - skipLock = false, nextDelay = 200, retryCount = 0, keepVolumes = undefined as boolean | undefined, @@ -725,6 +715,7 @@ export const applyTarget = async ({ // the value intermediate || undefined, keepVolumes, + force, ); if (_.isEmpty(appSteps)) { @@ -770,16 +761,13 @@ export const applyTarget = async ({ } try { - await Promise.all( - steps.map((s) => applyStep(s, { force, initial, skipLock })), - ); + await Promise.all(steps.map((s) => applyStep(s, { force, initial }))); await setTimeout(nextDelay); await applyTarget({ force, initial, intermediate, - skipLock, nextDelay, retryCount, keepVolumes, @@ -803,7 +791,7 @@ export const applyTarget = async ({ function pausingApply(fn: () => any) { const lock = () => { - return writeLock('pause').disposer((release) => release()); + return takeGlobalLockRW('pause').disposer((release) => release()); }; // TODO: This function is a bit of a mess const pause = () => { @@ -885,11 +873,7 @@ export function triggerApplyTarget({ export async function applyIntermediateTarget( intermediate: InstancedDeviceState, - { - force = false, - skipLock = false, - keepVolumes = undefined as boolean | undefined, - } = {}, + { force = false, keepVolumes = undefined as boolean | undefined } = {}, ) { return pausingApply(async () => { // TODO: Make sure we don't accidentally overwrite this @@ -898,7 +882,6 @@ export async function applyIntermediateTarget( return applyTarget({ intermediate: true, force, - skipLock, keepVolumes, }).then(() => { intermediateTarget = null; 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/device-state/target-state.ts b/src/device-state/target-state.ts index 86179676..6f0aa356 100644 --- a/src/device-state/target-state.ts +++ b/src/device-state/target-state.ts @@ -8,7 +8,7 @@ import type { TargetState } from '../types/state'; import { InternalInconsistencyError } from '../lib/errors'; import { getGotInstance } from '../lib/request'; import * as config from '../config'; -import { writeLock } from '../lib/update-lock'; +import { takeGlobalLockRW } from '../lib/process-lock'; import * as constants from '../lib/constants'; import log from '../lib/supervisor-console'; @@ -26,7 +26,7 @@ export const emitter: StrictEventEmitter = new EventEmitter(); const lockGetTarget = () => - writeLock('getTarget').disposer((release) => release()); + takeGlobalLockRW('getTarget').disposer((release) => release()); type CachedResponse = { etag?: string | string[]; diff --git a/src/event-tracker.ts b/src/event-tracker.ts index 427f9ffe..fe88540a 100644 --- a/src/event-tracker.ts +++ b/src/event-tracker.ts @@ -5,6 +5,8 @@ export type EventTrackProperties = Dictionary; const mixpanelMask = [ 'appId', + 'force', + 'services', 'delay', 'error', 'interval', 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/log-types.ts b/src/lib/log-types.ts index 328d0a0d..8ecf0a9c 100644 --- a/src/lib/log-types.ts +++ b/src/lib/log-types.ts @@ -168,3 +168,13 @@ export const removeNetworkError: LogType = { eventName: 'Network removal error', humanName: 'Error removing network', }; + +export const takeLock: LogType = { + eventName: 'Take update locks', + humanName: 'Taking update locks', +}; + +export const releaseLock: LogType = { + eventName: 'Release update locks', + humanName: 'Releasing update locks', +}; diff --git a/src/lib/process-lock.ts b/src/lib/process-lock.ts new file mode 100644 index 00000000..a653cf7a --- /dev/null +++ b/src/lib/process-lock.ts @@ -0,0 +1,35 @@ +/** + * This module contains the functionality for locking & unlocking resources + * within the Supervisor Node process, useful for methods that need to acquire + * exclusive access to a resource across multiple ticks in the event loop, async + * functions for example. + * + * It is different from lockfile and update-lock modules, which handle + * inter-container communication via lockfiles. + * + * TODO: + * - Use a maintained solution such as async-lock + * - Move to native Promises + */ + +import Bluebird from 'bluebird'; +import Lock from 'rwlock'; +import type { Release } from 'rwlock'; + +type LockFn = (key: string | number) => Bluebird; + +const locker = new Lock(); + +export const takeGlobalLockRW: LockFn = Bluebird.promisify( + locker.async.writeLock, + { + context: locker, + }, +); + +export const takeGlobalLockRO: LockFn = Bluebird.promisify( + locker.async.readLock, + { + context: locker, + }, +); diff --git a/src/lib/update-lock.ts b/src/lib/update-lock.ts index fff60608..9e72dbf8 100644 --- a/src/lib/update-lock.ts +++ b/src/lib/update-lock.ts @@ -1,18 +1,21 @@ -import Bluebird from 'bluebird'; import { promises as fs } from 'fs'; import path from 'path'; -import Lock from 'rwlock'; +import type { Stats } from 'fs'; import { isRight } from 'fp-ts/lib/Either'; import { - ENOENT, + isENOENT, UpdatesLockedError, InternalInconsistencyError, } 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 } from '../types'; +import { NumericIdentifier, StringIdentifier, DockerName } from '../types'; +import { takeGlobalLockRW } from './process-lock'; +import * as logger from '../logger'; +import * as logTypes from './log-types'; const decodedUid = NumericIdentifier.decode(process.env.LOCKFILE_UID); export const LOCKFILE_UID = isRight(decodedUid) ? decodedUid.right : 65534; @@ -56,24 +59,19 @@ export function abortIfHUPInProgress({ }); } -type LockFn = (key: string | number) => Bluebird<() => void>; -const locker = new Lock(); -export const writeLock: LockFn = Bluebird.promisify(locker.async.writeLock, { - context: locker, -}); -export const readLock: LockFn = Bluebird.promisify(locker.async.readLock, { - context: locker, -}); - -// Unlock all lockfiles, optionally of an appId | appUuid, then release resources. +/** + * 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 { - 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 { @@ -82,22 +80,196 @@ async function dispose( } } +/** + * 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); + 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); + 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)); + } + } + + /** + * @private Use this.getServices instead as there is no need to return + * a mutable reference to the internal Set data structure. + */ + 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); + } +} + +// 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 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); + } + + // 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; +} + /** * 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 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 async function lock( appId: number | number[], - { force = false, skipLock = false }: { force: boolean; skipLock?: boolean }, + { force = false }: { force: boolean }, fn: () => Resolvable, ): Promise { const appIdsToLock = Array.isArray(appId) ? appId : [appId]; - if (skipLock || !appId || !appIdsToLock.length) { + if (!appId || !appIdsToLock.length) { return fn(); } @@ -118,10 +290,10 @@ export async function lock( for (const id of sortedIds) { const lockDir = pathOnRoot(lockPath(id)); // Acquire write lock for appId - releases.set(id, await writeLock(id)); + 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/logger.ts b/src/logger.ts index 5290901e..9a9b25f1 100644 --- a/src/logger.ts +++ b/src/logger.ts @@ -5,7 +5,7 @@ import * as config from './config'; import * as db from './db'; import * as eventTracker from './event-tracker'; import type { LogType } from './lib/log-types'; -import { writeLock } from './lib/update-lock'; +import { takeGlobalLockRW } from './lib/process-lock'; import type { LogBackend, LogMessage } from './logging'; import { BalenaLogBackend, LocalLogBackend } from './logging'; import type { MonitorHook } from './logging/monitor'; @@ -129,7 +129,7 @@ export function logSystemMessage( } export function lock(containerId: string): Bluebird.Disposer<() => void> { - return writeLock(containerId).disposer((release) => { + return takeGlobalLockRW(containerId).disposer((release) => { release(); }); } 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/compose/application-manager.spec.ts b/test/integration/compose/application-manager.spec.ts index a1da6488..f5f204be 100644 --- a/test/integration/compose/application-manager.spec.ts +++ b/test/integration/compose/application-manager.spec.ts @@ -8,6 +8,7 @@ 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, @@ -111,6 +112,8 @@ describe('compose/application-manager', () => { downloading, availableImages, containerIdsByAppId, + // Mock lock taken to avoid takeLock step + locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), }, ); @@ -221,6 +224,8 @@ describe('compose/application-manager', () => { downloading, availableImages, containerIdsByAppId, + // Mock lock taken to avoid takeLock step + locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), }, ); @@ -271,6 +276,8 @@ describe('compose/application-manager', () => { downloading, availableImages, containerIdsByAppId, + // Mock lock taken to avoid takeLock step + locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), }, ); @@ -402,6 +409,8 @@ describe('compose/application-manager', () => { downloading: c1.downloading, availableImages: c1.availableImages, containerIdsByAppId: c1.containerIdsByAppId, + // Mock lock taken for `main` service which just needs metadata updated + locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), }, ); // There should be two noop steps, one for target service which is still downloading, @@ -448,6 +457,10 @@ 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'] }, + ]), }, ); // Service `old` is safe to kill after download for `new` has completed @@ -493,6 +506,11 @@ 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'] }, + ]), }, ); // Service `new` should be fetched @@ -565,6 +583,10 @@ 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'] }, + ]), }, ); // Service `new` should be started @@ -605,6 +627,11 @@ 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'] }, + ]), }, ); // Service `new` should be fetched @@ -677,6 +704,10 @@ describe('compose/application-manager', () => { }), ], containerIdsByAppId: c1.containerIdsByAppId, + // Mock lock taken for all services in target state + locksTaken: new LocksTakenMap([ + { appId: 1, services: ['main', 'new'] }, + ]), }, ); // Service `new` should be started @@ -773,6 +804,10 @@ describe('compose/application-manager', () => { }), ], containerIdsByAppId, + // Mock locks taken for all services in target state + locksTaken: new LocksTakenMap([ + { appId: 1, services: ['one', 'two'] }, + ]), }, ); expectSteps('start', steps3, 2); @@ -841,6 +876,8 @@ describe('compose/application-manager', () => { downloading, availableImages, containerIdsByAppId, + // Mock lock taken to avoid takeLock step + locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), }, ); @@ -921,6 +958,10 @@ describe('compose/application-manager', () => { downloading, availableImages, containerIdsByAppId, + // Mock locks taken to avoid takeLock step + locksTaken: new LocksTakenMap([ + { appId: 1, services: ['main', 'dep'] }, + ]), }, ); @@ -986,10 +1027,14 @@ describe('compose/application-manager', () => { downloading, availableImages, containerIdsByAppId, + // Mock locks taken to avoid takeLock step + locksTaken: new LocksTakenMap([ + { appId: 1, services: ['main', 'dep'] }, + ]), }, ); - // A start step should happen for the depended service first + // A start step should happen for the dependant service first expect(startStep).to.have.property('action').that.equals('start'); expect(startStep) .to.have.property('target') @@ -1054,6 +1099,10 @@ describe('compose/application-manager', () => { downloading, availableImages, containerIdsByAppId, + // Mock locks taken to avoid takeLock step + locksTaken: new LocksTakenMap([ + { appId: 1, services: ['main', 'dep'] }, + ]), }, ); @@ -1096,6 +1145,11 @@ describe('compose/application-manager', () => { downloading, availableImages, containerIdsByAppId, + // Mock lock already taken for the new and leftover services + locksTaken: new LocksTakenMap([ + { appId: 5, services: ['old-service'] }, + { appId: 1, services: ['main'] }, + ]), }, ); @@ -1601,6 +1655,11 @@ describe('compose/application-manager', () => { downloading, availableImages, containerIdsByAppId, + // Mock locks taken to avoid takeLock step + locksTaken: new LocksTakenMap([ + { appId: 1, services: ['main'] }, + { appId: 2, services: ['main'] }, + ]), }, ); @@ -1623,6 +1682,699 @@ describe('compose/application-manager', () => { ).to.have.lengthOf(1); }); + describe('taking and releasing locks', () => { + it('should take locks for all services in current state when they should be killed', async () => { + const targetApps = createApps( + { + services: [], + networks: [DEFAULT_NETWORK], + }, + true, + ); + const { currentApps, availableImages, downloading, containerIdsByAppId } = + createCurrentState({ + services: [ + await createService({ serviceName: 'one' }), + await createService({ serviceName: 'two' }), + ], + networks: [DEFAULT_NETWORK], + images: [], + }); + + // takeLock + const steps = await applicationManager.inferNextSteps( + currentApps, + targetApps, + { + downloading, + availableImages, + containerIdsByAppId, + }, + ); + const [takeLockStep] = expectSteps('takeLock', steps, 1, 1); + expect(takeLockStep) + .to.have.property('services') + .that.deep.includes.members(['one', 'two']); + + // kill + const steps2 = await applicationManager.inferNextSteps( + currentApps, + targetApps, + { + downloading, + availableImages, + containerIdsByAppId, + // Mock locks taken + locksTaken: new LocksTakenMap([ + { appId: 1, services: ['one', 'two'] }, + ]), + }, + ); + expectSteps('kill', steps2, 2); + }); + + it('should take locks for all services in current state when they should be stopped', async () => { + const targetApps = createApps( + { + services: [ + await createService({ + serviceName: 'one', + running: false, + }), + await createService({ + serviceName: 'two', + running: false, + }), + ], + networks: [DEFAULT_NETWORK], + }, + true, + ); + const { currentApps, availableImages, downloading, containerIdsByAppId } = + createCurrentState({ + services: [ + await createService({ serviceName: 'one' }), + await createService({ serviceName: 'two' }), + ], + networks: [DEFAULT_NETWORK], + images: [], + }); + + // takeLock + const steps = await applicationManager.inferNextSteps( + currentApps, + targetApps, + { + downloading, + availableImages, + containerIdsByAppId, + }, + ); + const [takeLockStep] = expectSteps('takeLock', steps, 1, 1); + expect(takeLockStep) + .to.have.property('services') + .that.deep.includes.members(['one', 'two']); + + // stop + const steps2 = await applicationManager.inferNextSteps( + currentApps, + targetApps, + { + downloading, + availableImages, + containerIdsByAppId, + // Mock locks taken + locksTaken: new LocksTakenMap([ + { appId: 1, services: ['one', 'two'] }, + ]), + }, + ); + expectSteps('stop', steps2, 2); + }); + + it('should take locks for all services in target state before they should be started', async () => { + const targetApps = createApps( + { + services: [ + await createService({ serviceName: 'one', image: 'one-image' }), + await createService({ serviceName: 'two', image: 'two-image' }), + ], + networks: [DEFAULT_NETWORK], + }, + true, + ); + const { currentApps, availableImages, downloading, containerIdsByAppId } = + createCurrentState({ + services: [], + networks: [DEFAULT_NETWORK], + images: [ + createImage({ + serviceName: 'one', + name: 'one-image', + }), + createImage({ + serviceName: 'two', + name: 'two-image', + }), + ], + }); + + // takeLock + const steps = await applicationManager.inferNextSteps( + currentApps, + targetApps, + { + downloading, + availableImages, + containerIdsByAppId, + }, + ); + const [takeLockStep] = expectSteps('takeLock', steps, 1, 1); + expect(takeLockStep) + .to.have.property('services') + .that.deep.includes.members(['one', 'two']); + + // start + const steps2 = await applicationManager.inferNextSteps( + currentApps, + targetApps, + { + downloading, + availableImages, + containerIdsByAppId, + // Mock locks taken + locksTaken: new LocksTakenMap([ + { appId: 1, services: ['one', 'two'] }, + ]), + }, + ); + expectSteps('start', steps2, 2); + }); + + it('should take locks for all services in current and target states when services should be stopped, started, or killed', async () => { + const targetApps = createApps( + { + services: [ + await createService({ + serviceName: 'to-stop', + image: 'image-to-stop', + running: false, + }), + await createService({ + serviceName: 'to-start', + image: 'image-to-start', + }), + await createService({ + serviceName: 'unchanged', + image: 'image-unchanged', + }), + ], + networks: [DEFAULT_NETWORK], + }, + true, + ); + const { currentApps, availableImages, downloading, containerIdsByAppId } = + createCurrentState({ + services: [ + await createService({ + serviceName: 'to-stop', + image: 'image-to-stop', + }), + await createService({ + serviceName: 'to-kill', + image: 'image-to-kill', + }), + await createService({ + serviceName: 'unchanged', + image: 'image-unchanged', + }), + ], + networks: [DEFAULT_NETWORK], + images: [ + createImage({ serviceName: 'to-start', name: 'image-to-start' }), + createImage({ serviceName: 'to-stop', name: 'image-to-stop' }), + createImage({ serviceName: 'unchanged', name: 'image-unchanged' }), + ], + }); + + const steps = await applicationManager.inferNextSteps( + currentApps, + targetApps, + { + downloading, + availableImages, + containerIdsByAppId, + }, + ); + // No matter the number of services, we should see a single takeLock for all services + // regardless if they're only in current or only in target + const [takeLockStep] = expectSteps('takeLock', steps, 1, 1); + expect(takeLockStep) + .to.have.property('services') + .that.deep.includes.members([ + 'to-stop', + 'to-kill', + 'to-start', + 'unchanged', + ]); + }); + + it('should download images before taking locks & killing when update strategy is download-then-kill or handover', async () => { + const targetApps = createApps( + { + services: [ + await createService({ + serviceName: 'one', + image: 'one', + labels: { + 'io.balena.update.strategy': 'download-then-kill', + 'io.updated': 'true', + }, + }), + await createService({ + serviceName: 'two', + image: 'two', + labels: { + 'io.balena.update.strategy': 'handover', + 'io.updated': 'true', + }, + }), + ], + networks: [DEFAULT_NETWORK], + }, + true, + ); + const { currentApps, availableImages, downloading, containerIdsByAppId } = + createCurrentState({ + services: [ + await createService({ + serviceName: 'one', + image: 'one', + labels: { 'io.balena.update.strategy': 'download-then-kill' }, + }), + await createService({ + serviceName: 'two', + image: 'two', + labels: { 'io.balena.update.strategy': 'handover' }, + }), + ], + networks: [DEFAULT_NETWORK], + }); + + // fetch + const steps = await applicationManager.inferNextSteps( + currentApps, + targetApps, + { + downloading: [], + availableImages: [], + containerIdsByAppId, + }, + ); + expectSteps('fetch', steps, 2); + + // noop while downloading + const steps2 = await applicationManager.inferNextSteps( + currentApps, + targetApps, + { + downloading: ['one', 'two'], + availableImages: [], + containerIdsByAppId, + }, + ); + expectSteps('noop', steps2, 2); + + // takeLock after download complete + const steps3 = await applicationManager.inferNextSteps( + currentApps, + targetApps, + { + downloading, + availableImages, + containerIdsByAppId, + }, + ); + const [takeLockStep] = expectSteps('takeLock', steps3, 1); + expect(takeLockStep) + .to.have.property('services') + .that.deep.includes.members(['one', 'two']); + + // kill + const steps4 = await applicationManager.inferNextSteps( + currentApps, + targetApps, + { + downloading, + availableImages, + containerIdsByAppId, + // Mock locks taken + locksTaken: new LocksTakenMap([ + { appId: 1, services: ['one', 'two'] }, + ]), + }, + ); + expectSteps('kill', steps4, 2); + }); + + it('should take locks & kill before downloading images when update strategy is kill|delete-then-download', async () => { + const targetApps = createApps( + { + services: [ + await createService({ + serviceName: 'one', + labels: { + 'io.balena.update.strategy': 'kill-then-download', + 'io.updated': 'true', + }, + }), + await createService({ + serviceName: 'two', + labels: { + 'io.balena.update.strategy': 'delete-then-download', + 'io.updated': 'true', + }, + }), + ], + networks: [DEFAULT_NETWORK], + }, + true, + ); + const { currentApps, availableImages, containerIdsByAppId } = + createCurrentState({ + services: [ + await createService({ + serviceName: 'one', + labels: { 'io.balena.update.strategy': 'kill-then-download' }, + }), + await createService({ + serviceName: 'two', + labels: { 'io.balena.update.strategy': 'delete-then-download' }, + }), + ], + networks: [DEFAULT_NETWORK], + }); + + // takeLock + const steps = await applicationManager.inferNextSteps( + currentApps, + targetApps, + { + // Images haven't finished downloading, + // but kill steps should still be inferred + downloading: ['one', 'two'], + availableImages, + containerIdsByAppId, + }, + ); + const [takeLockStep] = expectSteps('takeLock', steps, 1, 1); + expect(takeLockStep) + .to.have.property('services') + .that.deep.includes.members(['one', 'two']); + + // kill + const steps2 = await applicationManager.inferNextSteps( + currentApps, + targetApps, + { + downloading: ['one', 'two'], + availableImages, + containerIdsByAppId, + // Mock locks taken + locksTaken: new LocksTakenMap([ + { appId: 1, services: ['one', 'two'] }, + ]), + }, + ); + expectSteps('kill', steps2, 2); + }); + + it('should infer takeLock & kill steps for dependent services before their network should be removed', async () => { + const targetApps = createApps( + { + services: [ + await createService({ serviceName: 'main', appUuid: 'deadbeef' }), + ], + networks: [DEFAULT_NETWORK], + }, + true, + ); + const { currentApps, availableImages, downloading, containerIdsByAppId } = + createCurrentState({ + services: [ + await createService({ + serviceName: 'main', + appUuid: 'deadbeef', + composition: { networks: { test: {} } }, + }), + ], + networks: [ + DEFAULT_NETWORK, + Network.fromComposeObject('test', 1, 'deadbeef', { + driver: 'bridge', + }), + ], + }); + + // takeLock + const steps = await applicationManager.inferNextSteps( + currentApps, + targetApps, + { + downloading, + availableImages, + containerIdsByAppId, + }, + ); + const [takeLockStep] = expectSteps('takeLock', steps, 1, 1); + expect(takeLockStep) + .to.have.property('services') + .that.deep.includes.members(['main']); + + // kill + const steps2 = await applicationManager.inferNextSteps( + currentApps, + targetApps, + { + downloading, + availableImages, + containerIdsByAppId, + // Mock locks taken + locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), + }, + ); + expectSteps('kill', steps2); + + // removeNetwork + const intermediateCurrent = createCurrentState({ + services: [], + networks: [ + DEFAULT_NETWORK, + Network.fromComposeObject('test', 1, 'deadbeef', { + driver: 'bridge', + }), + ], + }); + const steps3 = await applicationManager.inferNextSteps( + intermediateCurrent.currentApps, + targetApps, + { + downloading: intermediateCurrent.downloading, + availableImages: intermediateCurrent.availableImages, + containerIdsByAppId: intermediateCurrent.containerIdsByAppId, + // Mock locks taken + locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), + }, + ); + expectSteps('removeNetwork', steps3); + }); + + it('should infer takeLock & kill steps for dependent services before their network should have config changed', async () => { + const targetApps = createApps( + { + services: [ + await createService({ + serviceName: 'main', + appUuid: 'deadbeef', + composition: { networks: { test: {} } }, + }), + ], + networks: [ + DEFAULT_NETWORK, + Network.fromComposeObject('test', 1, 'deadbeef', { + driver: 'local', + }), + ], + }, + true, + ); + const { currentApps, availableImages, downloading, containerIdsByAppId } = + createCurrentState({ + services: [ + await createService({ + serviceName: 'main', + appUuid: 'deadbeef', + composition: { networks: { test: {} } }, + }), + ], + networks: [ + DEFAULT_NETWORK, + Network.fromComposeObject('test', 1, 'deadbeef', { + driver: 'bridge', + }), + ], + }); + + // takeLock + const steps = await applicationManager.inferNextSteps( + currentApps, + targetApps, + { + downloading, + availableImages, + containerIdsByAppId, + }, + ); + const [takeLockStep] = expectSteps('takeLock', steps, 1, 1); + expect(takeLockStep) + .to.have.property('services') + .that.deep.includes.members(['main']); + + // kill + const steps2 = await applicationManager.inferNextSteps( + currentApps, + targetApps, + { + downloading, + availableImages, + containerIdsByAppId, + // Mock locks taken + locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), + }, + ); + expectSteps('kill', steps2, 1); + + // removeNetwork + const intermediateCurrent = createCurrentState({ + services: [], + networks: [ + DEFAULT_NETWORK, + Network.fromComposeObject('test', 1, 'deadbeef', { + driver: 'bridge', + }), + ], + }); + const steps3 = await applicationManager.inferNextSteps( + intermediateCurrent.currentApps, + targetApps, + { + downloading: intermediateCurrent.downloading, + availableImages: intermediateCurrent.availableImages, + containerIdsByAppId: intermediateCurrent.containerIdsByAppId, + // Mock locks taken + locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), + }, + ); + expectSteps('removeNetwork', steps3); + }); + + it('should infer takeLock & kill steps for dependent services before their volume should have config changed', async () => { + const targetApps = createApps( + { + services: [ + await createService({ + serviceName: 'main', + appUuid: 'deadbeef', + composition: { volumes: ['test:/test'] }, + }), + ], + networks: [DEFAULT_NETWORK], + volumes: [ + Volume.fromComposeObject('test', 1, 'deadbeef', { + labels: { 'io.updated': 'true' }, + }), + ], + }, + true, + ); + const { currentApps, availableImages, downloading, containerIdsByAppId } = + createCurrentState({ + services: [ + await createService({ + serviceName: 'main', + appUuid: 'deadbeef', + composition: { volumes: ['test:/test'] }, + }), + ], + networks: [DEFAULT_NETWORK], + volumes: [Volume.fromComposeObject('test', 1, 'deadbeef')], + }); + + // takeLock + const steps = await applicationManager.inferNextSteps( + currentApps, + targetApps, + { + downloading, + availableImages, + containerIdsByAppId, + }, + ); + const [takeLockStep] = expectSteps('takeLock', steps, 1, 1); + expect(takeLockStep) + .to.have.property('services') + .that.deep.includes.members(['main']); + + // kill + const steps2 = await applicationManager.inferNextSteps( + currentApps, + targetApps, + { + downloading, + availableImages, + containerIdsByAppId, + // Mock locks taken + locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), + }, + ); + expectSteps('kill', steps2, 1); + + // removeVolume + const intermediateCurrent = createCurrentState({ + services: [], + networks: [DEFAULT_NETWORK], + volumes: [Volume.fromComposeObject('test', 1, 'deadbeef')], + }); + const steps3 = await applicationManager.inferNextSteps( + intermediateCurrent.currentApps, + targetApps, + { + downloading: intermediateCurrent.downloading, + availableImages: intermediateCurrent.availableImages, + containerIdsByAppId: intermediateCurrent.containerIdsByAppId, + // Mock locks taken + locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), + }, + ); + expectSteps('removeVolume', steps3); + }); + + it('should release locks before settling state', async () => { + const targetApps = createApps( + { + services: [ + await createService({ serviceName: 'one' }), + await createService({ serviceName: 'two' }), + ], + networks: [DEFAULT_NETWORK], + }, + true, + ); + const { currentApps, availableImages, downloading, containerIdsByAppId } = + createCurrentState({ + services: [ + await createService({ serviceName: 'one' }), + await createService({ serviceName: 'two' }), + ], + networks: [DEFAULT_NETWORK], + }); + const steps = await applicationManager.inferNextSteps( + currentApps, + targetApps, + { + downloading, + availableImages, + containerIdsByAppId, + locksTaken: new LocksTakenMap([ + { appId: 1, services: ['one', 'two'] }, + ]), + }, + ); + const [releaseLockStep] = expectSteps('releaseLock', steps, 1, 1); + expect(releaseLockStep).to.have.property('appId').that.equals(1); + }); + }); + describe("getting application's current state", () => { let getImagesState: sinon.SinonStub; let getServicesState: sinon.SinonStub; @@ -2025,6 +2777,10 @@ describe('compose/application-manager', () => { downloading, availableImages, containerIdsByAppId, + // Mock locks taken for all services in target state + locksTaken: new LocksTakenMap([ + { appId: 1, services: ['one', 'two', 'three', 'four'] }, + ]), }); [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 e987b054..fda4e9e1 100644 --- a/test/integration/device-api/actions.spec.ts +++ b/test/integration/device-api/actions.spec.ts @@ -4,6 +4,7 @@ import { stub } from 'sinon'; import Docker from 'dockerode'; import request from 'supertest'; import { setTimeout } from 'timers/promises'; +import { testfs } from 'mocha-pod'; import * as deviceState from '~/src/device-state'; import * as config from '~/src/config'; @@ -11,10 +12,12 @@ import * as hostConfig from '~/src/host-config'; import * as deviceApi from '~/src/device-api'; import * as actions from '~/src/device-api/actions'; import * as TargetState from '~/src/device-state/target-state'; +import * as updateLock from '~/lib/update-lock'; +import { pathOnRoot } from '~/lib/host-utils'; +import { exec } from '~/lib/fs-utils'; +import * as lockfile from '~/lib/lockfile'; import { cleanupDocker } from '~/test-lib/docker-helper'; -import { exec } from '~/src/lib/fs-utils'; - export async function dbusSend( dest: string, path: string, @@ -79,6 +82,7 @@ describe('manages application lifecycle', () => { const BALENA_SUPERVISOR_ADDRESS = process.env.BALENA_SUPERVISOR_ADDRESS || 'http://balena-supervisor:48484'; const APP_ID = 1; + const lockdir = pathOnRoot(updateLock.BASE_LOCK_DIR); const docker = new Docker(); const getSupervisorTarget = async () => @@ -218,6 +222,11 @@ describe('manages application lifecycle', () => { ctns.every(({ State }) => !startedAt.includes(State.StartedAt)); }; + const mockFs = testfs( + { [`${lockdir}/${APP_ID}`]: {} }, + { cleanup: [`${lockdir}/${APP_ID}/**/*.lock`] }, + ); + before(async () => { // Images are ignored in local mode so we need to pull the base image await docker.pull(BASE_IMAGE); @@ -251,10 +260,16 @@ describe('manages application lifecycle', () => { }); beforeEach(async () => { + await mockFs.enable(); + // Create a single-container application in local mode await setSupervisorTarget(targetState); }); + afterEach(async () => { + await mockFs.restore(); + }); + // Make sure the app is running and correct before testing more assertions it('should setup a single container app (sanity check)', async () => { containers = await waitForSetup(targetState); @@ -292,6 +307,69 @@ describe('manages application lifecycle', () => { ); }); + it('should not restart an application when user locks are present', async () => { + containers = await waitForSetup(targetState); + + // Create a lock + await lockfile.lock( + `${lockdir}/${APP_ID}/${serviceNames[0]}/updates.lock`, + ); + + await request(BALENA_SUPERVISOR_ADDRESS) + .post(`/v1/restart`) + .set('Content-Type', 'application/json') + .send(JSON.stringify({ appId: APP_ID })) + .expect(423); + + // Containers should not have been restarted + const containersAfterRestart = await waitForSetup(targetState); + expect( + containersAfterRestart.map((ctn) => ctn.State.StartedAt), + ).to.deep.include.members(containers.map((ctn) => ctn.State.StartedAt)); + + // Remove the lock + await lockfile.unlock( + `${lockdir}/${APP_ID}/${serviceNames[0]}/updates.lock`, + ); + }); + + it('should restart an application when user locks are present if force is specified', async () => { + containers = await waitForSetup(targetState); + const isRestartSuccessful = startTimesChanged( + containers.map((ctn) => ctn.State.StartedAt), + ); + + // Create a lock + await lockfile.lock( + `${lockdir}/${APP_ID}/${serviceNames[0]}/updates.lock`, + ); + + await request(BALENA_SUPERVISOR_ADDRESS) + .post(`/v1/restart`) + .set('Content-Type', 'application/json') + .send(JSON.stringify({ appId: APP_ID, force: true })); + + const restartedContainers = await waitForSetup( + targetState, + isRestartSuccessful, + ); + + // Technically the wait function above should already verify that the two + // containers have been restarted, but verify explcitly with an assertion + expect(isRestartSuccessful(restartedContainers)).to.be.true; + + // Containers should have different Ids since they're recreated + expect(restartedContainers.map(({ Id }) => Id)).to.not.have.members( + containers.map((ctn) => ctn.Id), + ); + + // Wait briefly for state to settle which includes releasing locks + await setTimeout(1000); + + // User lock should be overridden + expect(await updateLock.getLocksTaken()).to.deep.equal([]); + }); + it('should restart service by removing and recreating corresponding container', async () => { containers = await waitForSetup(targetState); const isRestartSuccessful = startTimesChanged( @@ -321,6 +399,73 @@ describe('manages application lifecycle', () => { ); }); + // Since restart-service follows the same code paths as start|stop-service, + // these lock test cases should be sufficient to cover all three service actions. + it('should not restart service when user locks are present', async () => { + containers = await waitForSetup(targetState); + + // Create a lock + await lockfile.lock( + `${lockdir}/${APP_ID}/${serviceNames[0]}/updates.lock`, + ); + + await request(BALENA_SUPERVISOR_ADDRESS) + .post('/v2/applications/1/restart-service') + .set('Content-Type', 'application/json') + .send(JSON.stringify({ serviceName: serviceNames[0] })) + .expect(423); + + // Containers should not have been restarted + const containersAfterRestart = await waitForSetup(targetState); + expect( + containersAfterRestart.map((ctn) => ctn.State.StartedAt), + ).to.deep.include.members(containers.map((ctn) => ctn.State.StartedAt)); + + // Remove the lock + await lockfile.unlock( + `${lockdir}/${APP_ID}/${serviceNames[0]}/updates.lock`, + ); + }); + + it('should restart service when user locks are present if force is specified', async () => { + containers = await waitForSetup(targetState); + const isRestartSuccessful = startTimesChanged( + containers + .filter((ctn) => ctn.Name.includes(serviceNames[0])) + .map((ctn) => ctn.State.StartedAt), + ); + + // Create a lock + await lockfile.lock( + `${lockdir}/${APP_ID}/${serviceNames[0]}/updates.lock`, + ); + + await request(BALENA_SUPERVISOR_ADDRESS) + .post('/v2/applications/1/restart-service') + .set('Content-Type', 'application/json') + .send(JSON.stringify({ serviceName: serviceNames[0], force: true })); + + const restartedContainers = await waitForSetup( + targetState, + isRestartSuccessful, + ); + + // Technically the wait function above should already verify that the two + // containers have been restarted, but verify explcitly with an assertion + expect(isRestartSuccessful(restartedContainers)).to.be.true; + + // Containers should have different Ids since they're recreated + expect(restartedContainers.map(({ Id }) => Id)).to.not.have.members( + containers.map((ctn) => ctn.Id), + ); + + // Wait briefly for state to settle which includes releasing locks + await setTimeout(1000); + + // User lock should be overridden + expect(await updateLock.getLocksTaken()).to.deep.equal([]); + }); + it('should stop a running service', async () => { containers = await waitForSetup(targetState); @@ -520,10 +665,15 @@ describe('manages application lifecycle', () => { }); beforeEach(async () => { + await mockFs.enable(); // Create a multi-container application in local mode await setSupervisorTarget(targetState); }); + afterEach(async () => { + await mockFs.restore(); + }); + // Make sure the app is running and correct before testing more assertions it('should setup a multi-container app (sanity check)', async () => { containers = await waitForSetup(targetState); @@ -560,6 +710,69 @@ describe('manages application lifecycle', () => { ); }); + it('should not restart an application when user locks are present', async () => { + containers = await waitForSetup(targetState); + + // Create a lock + await lockfile.lock( + `${lockdir}/${APP_ID}/${serviceNames[0]}/updates.lock`, + ); + + await request(BALENA_SUPERVISOR_ADDRESS) + .post(`/v1/restart`) + .set('Content-Type', 'application/json') + .send(JSON.stringify({ appId: APP_ID })) + .expect(423); + + // Containers should not have been restarted + const containersAfterRestart = await waitForSetup(targetState); + expect( + containersAfterRestart.map((ctn) => ctn.State.StartedAt), + ).to.deep.include.members(containers.map((ctn) => ctn.State.StartedAt)); + + // Remove the lock + await lockfile.unlock( + `${lockdir}/${APP_ID}/${serviceNames[0]}/updates.lock`, + ); + }); + + it('should restart an application when user locks are present if force is specified', async () => { + containers = await waitForSetup(targetState); + const isRestartSuccessful = startTimesChanged( + containers.map((ctn) => ctn.State.StartedAt), + ); + + // Create a lock + await lockfile.lock( + `${lockdir}/${APP_ID}/${serviceNames[0]}/updates.lock`, + ); + + await request(BALENA_SUPERVISOR_ADDRESS) + .post(`/v1/restart`) + .set('Content-Type', 'application/json') + .send(JSON.stringify({ appId: APP_ID, force: true })); + + const restartedContainers = await waitForSetup( + targetState, + isRestartSuccessful, + ); + + // Technically the wait function above should already verify that the two + // containers have been restarted, but verify explcitly with an assertion + expect(isRestartSuccessful(restartedContainers)).to.be.true; + + // Containers should have different Ids since they're recreated + expect(restartedContainers.map(({ Id }) => Id)).to.not.have.members( + containers.map((ctn) => ctn.Id), + ); + + // Wait briefly for state to settle which includes releasing locks + await setTimeout(500); + + // User lock should be overridden + expect(await updateLock.getLocksTaken()).to.deep.equal([]); + }); + it('should restart service by removing and recreating corresponding container', async () => { containers = await waitForSetup(targetState); const serviceName = serviceNames[0]; @@ -601,6 +814,73 @@ describe('manages application lifecycle', () => { expect(sharedIds.length).to.equal(1); }); + // Since restart-service follows the same code paths as start|stop-service, + // these lock test cases should be sufficient to cover all three service actions. + it('should not restart service when user locks are present', async () => { + containers = await waitForSetup(targetState); + + // Create a lock + await lockfile.lock( + `${lockdir}/${APP_ID}/${serviceNames[0]}/updates.lock`, + ); + + await request(BALENA_SUPERVISOR_ADDRESS) + .post('/v2/applications/1/restart-service') + .set('Content-Type', 'application/json') + .send(JSON.stringify({ serviceName: serviceNames[0] })) + .expect(423); + + // Containers should not have been restarted + const containersAfterRestart = await waitForSetup(targetState); + expect( + containersAfterRestart.map((ctn) => ctn.State.StartedAt), + ).to.deep.include.members(containers.map((ctn) => ctn.State.StartedAt)); + + // Remove the lock + await lockfile.unlock( + `${lockdir}/${APP_ID}/${serviceNames[0]}/updates.lock`, + ); + }); + + it('should restart service when user locks are present if force is specified', async () => { + containers = await waitForSetup(targetState); + const isRestartSuccessful = startTimesChanged( + containers + .filter((ctn) => ctn.Name.includes(serviceNames[0])) + .map((ctn) => ctn.State.StartedAt), + ); + + // Create a lock + await lockfile.lock( + `${lockdir}/${APP_ID}/${serviceNames[0]}/updates.lock`, + ); + + await request(BALENA_SUPERVISOR_ADDRESS) + .post('/v2/applications/1/restart-service') + .set('Content-Type', 'application/json') + .send(JSON.stringify({ serviceName: serviceNames[0], force: true })); + + const restartedContainers = await waitForSetup( + targetState, + isRestartSuccessful, + ); + + // Technically the wait function above should already verify that the two + // containers have been restarted, but verify explcitly with an assertion + expect(isRestartSuccessful(restartedContainers)).to.be.true; + + // Containers should have different Ids since they're recreated + expect(restartedContainers.map(({ Id }) => Id)).to.not.have.members( + containers.map((ctn) => ctn.Id), + ); + + // Wait briefly for state to settle which includes releasing locks + await setTimeout(500); + + // User lock should be overridden + expect(await updateLock.getLocksTaken()).to.deep.equal([]); + }); + it('should stop a running service', async () => { containers = await waitForSetup(targetState); 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 c750f0d4..67a92584 100644 --- a/test/integration/lib/update-lock.spec.ts +++ b/test/integration/lib/update-lock.spec.ts @@ -2,12 +2,17 @@ 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 * as updateLock from '~/lib/update-lock'; import { UpdatesLockedError } from '~/lib/errors'; import * as config from '~/src/config'; import * as lockfile from '~/lib/lockfile'; import { pathOnRoot, pathOnState } from '~/lib/host-utils'; +import { mkdirp } from '~/lib/fs-utils'; +import { takeGlobalLockRW } from '~/lib/process-lock'; describe('lib/update-lock', () => { describe('abortIfHUPInProgress', () => { @@ -66,13 +71,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 @@ -146,8 +154,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(); @@ -281,4 +289,456 @@ 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 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)), + ); + + 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']); + + // Cleanup lockfiles + await Promise.all(validPaths.map((p) => lockfile.unlock(p))); + }); + + 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); + + // Cleanup lockfiles + await Promise.all(invalidPaths.map((p) => lockfile.unlock(p))); + }); + }); + + 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`, + ], + }; + + describe('takeLock', () => { + 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(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('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.getLocksTaken(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.getLocksTaken(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.getLocksTaken(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); + }); + }); + }); }); 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]; } } diff --git a/test/unit/compose/app.spec.ts b/test/unit/compose/app.spec.ts index 4bfd7638..e1a9671e 100644 --- a/test/unit/compose/app.spec.ts +++ b/test/unit/compose/app.spec.ts @@ -2,6 +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 { createService, @@ -14,9 +15,11 @@ import { const defaultContext = { keepVolumes: false, + force: false, availableImages: [] as Image[], containerIds: {}, downloading: [] as string[], + locksTaken: new LocksTakenMap(), }; describe('compose/app', () => { @@ -149,6 +152,7 @@ describe('compose/app', () => { }), ], volumes: [Volume.fromComposeObject('test-volume', 1, 'deadbeef')], + networks: [DEFAULT_NETWORK], }); const target = createApp({ services: [ @@ -162,16 +166,42 @@ describe('compose/app', () => { labels: { test: 'test' }, }), ], + networks: [DEFAULT_NETWORK], isTarget: true, }); - // Calculate steps - const steps = current.nextStepsForAppUpdate(defaultContext, target); + const availableImages = [createImage({ serviceName: 'test' })]; + // Take lock first + const steps = current.nextStepsForAppUpdate( + { + ...defaultContext, + availableImages, + }, + target, + ); + const [lockStep] = expectSteps('takeLock', steps); + expect(lockStep) + .to.have.property('services') + .that.deep.includes.members(['test']); - const [killStep] = expectSteps('kill', steps); + // Then kill + const steps2 = current.nextStepsForAppUpdate( + { + ...defaultContext, + availableImages, + // Mock lock already taken + locksTaken: new LocksTakenMap([{ appId: 1, services: ['test'] }]), + }, + target, + ); + const [killStep] = expectSteps('kill', steps2); expect(killStep) .to.have.property('current') .that.deep.includes({ serviceName: 'test' }); + + // No remove volume steps until dependencies are killed + expectNoStep('removeVolume', steps); + expectNoStep('removeVolume', steps2); }); it('should correctly infer to remove an app volumes when the app is being removed', () => { @@ -245,17 +275,32 @@ describe('compose/app', () => { volumes: [volume], }); - // Step 1: kill - const steps = current.nextStepsForAppUpdate( + // Step 1: takeLock + const lockStep = current.nextStepsForAppUpdate( contextWithImages, intermediateTarget, ); - expectSteps('kill', steps); + expectSteps('takeLock', lockStep, 1, 1); - // Step 2: noop (service is stopping) + // Step 2: kill + const killSteps = current.nextStepsForAppUpdate( + { + ...contextWithImages, + // Mock locks already taken + locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), + }, + intermediateTarget, + ); + expectSteps('kill', killSteps); + + // Step 3: noop (service is stopping) service.status = 'Stopping'; const secondStageSteps = current.nextStepsForAppUpdate( - contextWithImages, + { + ...contextWithImages, + // Mock locks already taken + locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), + }, intermediateTarget, ); expectSteps('noop', secondStageSteps); @@ -281,7 +326,7 @@ describe('compose/app', () => { volumes: [], }); - // Step 3: createVolume + // Step 4: createVolume service.status = 'Running'; const target = createApp({ services: [service], @@ -298,18 +343,29 @@ describe('compose/app', () => { expect(recreateVolumeSteps).to.have.length(1); expectSteps('createVolume', recreateVolumeSteps); - // Final step: start service + // Step 5: takeLock const currentWithVolumeRecreated = createApp({ services: [], networks: [DEFAULT_NETWORK], volumes: [volume], }); - - const createServiceSteps = + const lockStepAfterRecreate = currentWithVolumeRecreated.nextStepsForAppUpdate( contextWithImages, target, ); + expectSteps('takeLock', lockStepAfterRecreate, 1, 1); + + // Final step: start service + const createServiceSteps = + currentWithVolumeRecreated.nextStepsForAppUpdate( + { + ...contextWithImages, + // Mock locks already taken + locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), + }, + target, + ); expectSteps('start', createServiceSteps); }); }); @@ -436,6 +492,7 @@ describe('compose/app', () => { }); const target = createApp({ networks: [ + DEFAULT_NETWORK, Network.fromComposeObject('test-network', 1, 'deadbeef', {}), ], services: [ @@ -449,13 +506,61 @@ describe('compose/app', () => { isTarget: true, }); - const steps = current.nextStepsForAppUpdate(defaultContext, target); + const availableImages = [createImage({ appUuid: 'deadbeef' })]; + // Take lock first + const steps = current.nextStepsForAppUpdate( + { + ...defaultContext, + availableImages, + }, + target, + ); + const [lockStep] = expectSteps('takeLock', steps); + expect(lockStep) + .to.have.property('services') + .that.deep.includes.members(['test']); - const [removeNetworkStep] = expectSteps('kill', steps); - - expect(removeNetworkStep).to.have.property('current').that.deep.includes({ + // Then kill + const steps2 = current.nextStepsForAppUpdate( + { + ...defaultContext, + availableImages, + locksTaken: new LocksTakenMap([{ appId: 1, services: ['test'] }]), + }, + target, + ); + const [killStep] = expectSteps('kill', steps2); + expect(killStep).to.have.property('current').that.deep.includes({ serviceName: 'test', }); + + // removeNetwork should not be generated until after the kill + expectNoStep('removeNetwork', steps); + expectNoStep('removeNetwork', steps2); + + // Then remove duplicate networks + const current2 = createApp({ + appUuid: 'deadbeef', + networks: [ + DEFAULT_NETWORK, + Network.fromComposeObject('test-network', 1, 'deadbeef', {}), + Network.fromComposeObject('test-network', 1, 'deadbeef', {}), + ], + services: [], + }); + + const steps3 = current2.nextStepsForAppUpdate( + { + ...defaultContext, + availableImages, + locksTaken: new LocksTakenMap([{ appId: 1, services: ['test'] }]), + }, + target, + ); + const [removeNetworkStep] = expectSteps('removeNetwork', steps3); + expect(removeNetworkStep).to.have.property('current').that.deep.includes({ + name: 'test-network', + }); }); it('should correctly infer more than one network removal step', () => { @@ -558,15 +663,33 @@ describe('compose/app', () => { }); const availableImages = [createImage({ appUuid: 'deadbeef' })]; - + // Take lock first const steps = current.nextStepsForAppUpdate( { ...defaultContext, availableImages }, target, ); - const [killStep] = expectSteps('kill', steps); + const [lockStep] = expectSteps('takeLock', steps); + expect(lockStep) + .to.have.property('services') + .that.deep.includes.members(['test']); + + // Then kill + const steps2 = current.nextStepsForAppUpdate( + { + ...defaultContext, + availableImages, + locksTaken: new LocksTakenMap([{ appId: 1, services: ['test'] }]), + }, + target, + ); + const [killStep] = expectSteps('kill', steps2); expect(killStep) .to.have.property('current') .that.deep.includes({ serviceName: 'test' }); + + // Network should not be removed until after dependency kills + expectNoStep('removeNetwork', steps); + expectNoStep('removeNetwork', steps2); }); it('should kill dependencies of networks before changing config', async () => { @@ -574,7 +697,7 @@ describe('compose/app', () => { services: [ await createService({ serviceName: 'test', - composition: { networks: ['test-network'] }, + composition: { networks: { 'test-network': {} } }, }), ], networks: [Network.fromComposeObject('test-network', 1, 'appuuid', {})], @@ -593,16 +716,37 @@ describe('compose/app', () => { ], isTarget: true, }); + const availableImages = [createImage({ appId: 1, serviceName: 'test' })]; + // Take lock first + const steps = current.nextStepsForAppUpdate( + { + ...defaultContext, + availableImages, + }, + target, + ); + const [lockStep] = expectSteps('takeLock', steps); + expect(lockStep) + .to.have.property('services') + .that.deep.includes.members(['test']); - const steps = current.nextStepsForAppUpdate(defaultContext, target); - const [killStep] = expectSteps('kill', steps); - + // Then kill + const steps2 = current.nextStepsForAppUpdate( + { + ...defaultContext, + availableImages, + locksTaken: new LocksTakenMap([{ appId: 1, services: ['test'] }]), + }, + target, + ); + const [killStep] = expectSteps('kill', steps2); expect(killStep) .to.have.property('current') .that.deep.includes({ serviceName: 'test' }); - // We shouldn't try to remove the network until we have gotten rid of the dependencies + // Network should not be removed until after dependency kills expectNoStep('removeNetwork', steps); + expectNoStep('removeNetwork', steps2); }); it('should always kill dependencies of networks before removing', async () => { @@ -649,19 +793,35 @@ describe('compose/app', () => { createImage({ appId: 1, serviceName: 'one', name: 'alpine' }), createImage({ appId: 1, serviceName: 'two', name: 'alpine' }), ]; - + // Take lock first const steps = current.nextStepsForAppUpdate( { ...defaultContext, availableImages }, target, ); - const [killStep] = expectSteps('kill', steps); + const [lockStep] = expectSteps('takeLock', steps); + expect(lockStep) + .to.have.property('services') + .that.deep.includes.members(['one']); + // Then kill + const steps2 = current.nextStepsForAppUpdate( + { + ...defaultContext, + availableImages, + locksTaken: new LocksTakenMap([ + { appId: 1, services: ['one', 'two'] }, + ]), + }, + target, + ); + const [killStep] = expectSteps('kill', steps2); expect(killStep) .to.have.property('current') .that.deep.includes({ serviceName: 'one' }); // We shouldn't try to remove the network until we have gotten rid of the dependencies expectNoStep('removeNetwork', steps); + expectNoStep('removeNetwork', steps2); }); it('should kill dependencies of networks before updating between releases', async () => { @@ -712,20 +872,35 @@ describe('compose/app', () => { createImage({ appId: 1, serviceName: 'one', name: 'alpine' }), createImage({ appId: 1, serviceName: 'two', name: 'alpine' }), ]; - + // Take lock first const steps = current.nextStepsForAppUpdate( { ...defaultContext, availableImages }, target, ); - expectSteps('kill', steps, 2); + const [lockStep] = expectSteps('takeLock', steps); + expect(lockStep) + .to.have.property('services') + .that.deep.includes.members(['one', 'two']); - expect(steps.map((s) => (s as any).current.serviceName)).to.have.members([ - 'one', - 'two', - ]); + // Then kill + const steps2 = current.nextStepsForAppUpdate( + { + ...defaultContext, + availableImages, + locksTaken: new LocksTakenMap([ + { appId: 1, services: ['one', 'two'] }, + ]), + }, + target, + ); + expectSteps('kill', steps2, 2); + expect(steps2.map((s) => (s as any).current.serviceName)).to.have.members( + ['one', 'two'], + ); // We shouldn't try to remove the network until we have gotten rid of the dependencies expectNoStep('removeNetwork', steps); + expectNoStep('removeNetwork', steps2); }); it('should create the default network if it does not exist', () => { @@ -841,6 +1016,7 @@ describe('compose/app', () => { isTarget: true, }); + // Take lock first const steps = current.nextStepsForAppUpdate( { ...defaultContext, @@ -850,7 +1026,24 @@ describe('compose/app', () => { }, target, ); - const [killStep] = expectSteps('kill', steps); + const [lockStep] = expectSteps('takeLock', steps); + expect(lockStep) + .to.have.property('services') + .that.deep.includes.members(['main', 'aux']); + + // Then kill + const steps2 = current.nextStepsForAppUpdate( + { + ...defaultContext, + availableImages: [createImage({ serviceName: 'main' })], + // Mock locks already taken + locksTaken: new LocksTakenMap([ + { appId: 1, services: ['main', 'aux'] }, + ]), + }, + target, + ); + const [killStep] = expectSteps('kill', steps2); expect(killStep) .to.have.property('current') .to.deep.include({ serviceName: 'aux' }); @@ -949,26 +1142,48 @@ describe('compose/app', () => { expectNoStep('fetch', steps); }); - it('should emit an updateMetadata step when a service has not changed but the release has', async () => { + it('should emit a takeLock followed by an updateMetadata step when a service has not changed but the release has', async () => { const current = createApp({ services: [ - await createService({ serviceName: 'main', commit: 'old-release' }), + await createService({ + serviceName: 'main', + appId: 1, + commit: 'old-release', + }), ], + networks: [DEFAULT_NETWORK], }); const target = createApp({ services: [ - await createService({ serviceName: 'main', commit: 'new-release' }), + await createService({ + serviceName: 'main', + appId: 1, + commit: 'new-release', + }), ], + networks: [DEFAULT_NETWORK], isTarget: true, }); + // Take lock before updating metadata const steps = current.nextStepsForAppUpdate(defaultContext, target); - const [updateMetadataStep] = expectSteps('updateMetadata', steps); + const [takeLockStep] = expectSteps('takeLock', steps); + expect(takeLockStep) + .to.have.property('services') + .that.deep.equals(['main']); + // Infer updateMetadata after locks are taken + const steps2 = current.nextStepsForAppUpdate( + { + ...defaultContext, + locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), + }, + target, + ); + const [updateMetadataStep] = expectSteps('updateMetadata', steps2); expect(updateMetadataStep) .to.have.property('current') .to.deep.include({ serviceName: 'main', commit: 'old-release' }); - expect(updateMetadataStep) .to.have.property('target') .to.deep.include({ serviceName: 'main', commit: 'new-release' }); @@ -985,8 +1200,22 @@ describe('compose/app', () => { isTarget: true, }); + // Take lock first const steps = current.nextStepsForAppUpdate(defaultContext, target); - const [stopStep] = expectSteps('stop', steps); + const [lockStep] = expectSteps('takeLock', steps); + expect(lockStep) + .to.have.property('services') + .that.deep.includes.members(['main']); + + // Then stop + const steps2 = current.nextStepsForAppUpdate( + { + ...defaultContext, + locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), + }, + target, + ); + const [stopStep] = expectSteps('stop', steps2); expect(stopStep) .to.have.property('current') .to.deep.include({ serviceName: 'main' }); @@ -1045,9 +1274,19 @@ describe('compose/app', () => { isTarget: true, }); - // should see a 'stop' + // Take lock first + const steps = current.nextStepsForAppUpdate(contextWithImages, target); + const [lockStep] = expectSteps('takeLock', steps); + expect(lockStep) + .to.have.property('services') + .that.deep.includes.members(['main']); + + // Then kill const stepsToIntermediate = current.nextStepsForAppUpdate( - contextWithImages, + { + ...contextWithImages, + locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), + }, target, ); const [killStep] = expectSteps('kill', stepsToIntermediate); @@ -1055,19 +1294,21 @@ describe('compose/app', () => { .to.have.property('current') .that.deep.includes({ serviceName: 'main' }); - // assume the intermediate step has already removed the app + // Assume the intermediate step has already removed the app const intermediate = createApp({ services: [], // Default network was already created networks: [DEFAULT_NETWORK], }); - // now should see a 'start' + // Then start const stepsToTarget = intermediate.nextStepsForAppUpdate( - contextWithImages, + { + ...contextWithImages, + locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), + }, target, ); - const [startStep] = expectSteps('start', stepsToTarget); expect(startStep) .to.have.property('target') @@ -1079,12 +1320,6 @@ describe('compose/app', () => { }); it('should not start a container when it depends on a service which is being installed', async () => { - const availableImages = [ - createImage({ appId: 1, serviceName: 'main', name: 'main-image' }), - createImage({ appId: 1, serviceName: 'dep', name: 'dep-image' }), - ]; - const contextWithImages = { ...defaultContext, ...{ availableImages } }; - const current = createApp({ services: [ await createService( @@ -1121,12 +1356,24 @@ describe('compose/app', () => { isTarget: true, }); + const availableImages = [ + createImage({ appId: 1, serviceName: 'main', name: 'main-image' }), + createImage({ appId: 1, serviceName: 'dep', name: 'dep-image' }), + ]; + // As service is already being installed, lock for target should have been taken + const contextWithImages = { + ...defaultContext, + ...{ availableImages }, + locksTaken: new LocksTakenMap([ + { appId: 1, services: ['main', 'dep'] }, + ]), + }; + + // Only one start step and it should be that of the 'dep' service const stepsToIntermediate = current.nextStepsForAppUpdate( contextWithImages, target, ); - - // Only one start step and it should be that of the 'dep' service const [startStep] = expectSteps('start', stepsToIntermediate); expect(startStep) .to.have.property('target') @@ -1148,7 +1395,6 @@ describe('compose/app', () => { { ...contextWithImages, ...{ containerIds: { dep: 'dep-id' } } }, target, ); - const [startMainStep] = expectSteps('start', stepsToTarget); expect(startMainStep) .to.have.property('target') @@ -1220,11 +1466,28 @@ describe('compose/app', () => { isTarget: true, }); - const stepsToIntermediate = current.nextStepsForAppUpdate( + // Take lock first + const stepsToIntermediateBeforeLock = current.nextStepsForAppUpdate( contextWithImages, target, ); + const [takeLockStep] = expectSteps( + 'takeLock', + stepsToIntermediateBeforeLock, + ); + expect(takeLockStep) + .to.have.property('services') + .that.deep.includes.members(['main']); + // Then kill + const stepsToIntermediate = current.nextStepsForAppUpdate( + { + ...contextWithImages, + // Mock locks taken before kill + locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), + }, + target, + ); const [killStep] = expectSteps('kill', stepsToIntermediate); expect(killStep) .to.have.property('current') @@ -1237,7 +1500,12 @@ describe('compose/app', () => { }); const stepsToTarget = intermediate.nextStepsForAppUpdate( - contextWithImages, + { + ...contextWithImages, + // Mock locks still taken after kill (releaseLock not + // yet inferred as state is not yet settled) + locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), + }, target, ); @@ -1349,23 +1617,42 @@ describe('compose/app', () => { isTarget: true, }); - const stepsFirstTry = current.nextStepsForAppUpdate( + // Take lock first + const stepsBeforeLock = current.nextStepsForAppUpdate( contextWithImages, target, ); + const [takeLockStep] = expectSteps('takeLock', stepsBeforeLock); + expect(takeLockStep) + .to.have.property('services') + .that.deep.includes.members(['main']); + + // Then kill + const stepsFirstTry = current.nextStepsForAppUpdate( + { + ...contextWithImages, + // Mock locks taken from previous step + locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), + }, + target, + ); const [killStep] = expectSteps('kill', stepsFirstTry); expect(killStep) .to.have.property('current') .that.deep.includes({ serviceName: 'main' }); - // if at first you don't succeed + // As long as a kill step has not succeeded (current state hasn't + // changed), a kill step should be generated. const stepsSecondTry = current.nextStepsForAppUpdate( - contextWithImages, + { + ...contextWithImages, + // Mock locks taken from previous step + locksTaken: new LocksTakenMap([{ appId: 1, services: ['main'] }]), + }, target, ); - // Since current state has not changed, another kill step needs to be generated const [newKillStep] = expectSteps('kill', stepsSecondTry); expect(newKillStep) .to.have.property('current') @@ -1393,8 +1680,22 @@ describe('compose/app', () => { isTarget: true, }); + // Take lock first const steps = current.nextStepsForAppUpdate(defaultContext, target); - const [killStep] = expectSteps('kill', steps); + const [lockStep] = expectSteps('takeLock', steps); + expect(lockStep) + .to.have.property('services') + .that.deep.includes.members(['test']); + + // Then kill + const steps2 = current.nextStepsForAppUpdate( + { + ...defaultContext, + locksTaken: new LocksTakenMap([{ appId: 1, services: ['test'] }]), + }, + target, + ); + const [killStep] = expectSteps('kill', steps2); expect(killStep) .to.have.property('current') .that.deep.includes({ serviceName: 'test' }); @@ -1445,6 +1746,7 @@ describe('compose/app', () => { commit: 'old-release', }), ], + networks: [DEFAULT_NETWORK], }); const target = createApp({ services: [ @@ -1454,19 +1756,35 @@ describe('compose/app', () => { commit: 'new-release', }), ], + networks: [DEFAULT_NETWORK], isTarget: true, }); - const steps = current.nextStepsForAppUpdate( + const contextWithImages = { + ...defaultContext, + // With default download-then-kill strategy, target images + // should all be available before a kill step is inferred + availableImages: [createImage({ serviceName: 'three' })], + }; + // Take lock first + const steps = current.nextStepsForAppUpdate(contextWithImages, target); + const [lockStep] = expectSteps('takeLock', steps); + expect(lockStep) + .to.have.property('services') + .that.deep.includes.members(['one', 'two', 'three']); + + // Then kill + const steps2 = current.nextStepsForAppUpdate( { - ...defaultContext, - // With default download-then-kill strategy, target images - // should all be available before a kill step is inferred - availableImages: [createImage({ serviceName: 'three' })], + ...contextWithImages, + // Mock locks already taken + locksTaken: new LocksTakenMap([ + { appId: 1, services: ['one', 'two', 'three'] }, + ]), }, target, ); - expectSteps('kill', steps, 2); + expectSteps('kill', steps2, 2); }); it('should not infer a kill step with the default strategy before all target images have been downloaded', async () => { @@ -1571,21 +1889,6 @@ describe('compose/app', () => { }); it('should infer a start step only when target images have been downloaded', async () => { - const contextWithImages = { - ...defaultContext, - ...{ - downloading: [], // One of the images is being downloaded - availableImages: [ - createImage({ appId: 1, name: 'main-image', serviceName: 'main' }), - createImage({ - appId: 1, - name: 'other-image', - serviceName: 'other', - }), - ], - }, - }; - const current = createApp({ services: [], networks: [DEFAULT_NETWORK], @@ -1609,9 +1912,59 @@ describe('compose/app', () => { isTarget: true, }); - // No kill steps should be generated - const steps = current.nextStepsForAppUpdate(contextWithImages, target); - expectSteps('start', steps, 2); + // No start steps should be generated as long as any target image is downloading + const steps = current.nextStepsForAppUpdate( + { + ...defaultContext, + downloading: ['other-image'], + availableImages: [ + createImage({ appId: 1, name: 'main-image', serviceName: 'main' }), + ], + }, + target, + ); + expectNoStep('start', steps); + expectSteps('noop', steps, 1); + + // Take lock before starting once downloads complete + const steps2 = current.nextStepsForAppUpdate( + { + ...defaultContext, + availableImages: [ + createImage({ appId: 1, name: 'main-image', serviceName: 'main' }), + createImage({ + appId: 1, + name: 'other-image', + serviceName: 'other', + }), + ], + }, + target, + ); + const [lockStep] = expectSteps('takeLock', steps2); + expect(lockStep) + .to.have.property('services') + .that.deep.includes.members(['main', 'other']); + + // Then start + const steps3 = current.nextStepsForAppUpdate( + { + ...defaultContext, + availableImages: [ + createImage({ appId: 1, name: 'main-image', serviceName: 'main' }), + createImage({ + appId: 1, + name: 'other-image', + serviceName: 'other', + }), + ], + locksTaken: new LocksTakenMap([ + { appId: 1, services: ['main', 'other'] }, + ]), + }, + target, + ); + expectSteps('start', steps3, 2); }); }); @@ -1671,4 +2024,93 @@ describe('compose/app', () => { expectNoStep('kill', steps); }); }); + + describe('update lock state behavior', () => { + it('should infer a releaseLock step if there are locks to be released before settling target state', async () => { + const services = [ + await createService({ serviceName: 'server' }), + await createService({ serviceName: 'client' }), + ]; + const current = createApp({ + services, + networks: [DEFAULT_NETWORK], + }); + const target = createApp({ + services, + networks: [DEFAULT_NETWORK], + isTarget: true, + }); + + const steps = current.nextStepsForAppUpdate( + { + ...defaultContext, + locksTaken: new LocksTakenMap([ + { appId: 1, services: ['server', 'client'] }, + ]), + }, + target, + ); + const [releaseLockStep] = expectSteps('releaseLock', steps, 1); + expect(releaseLockStep).to.have.property('appId').that.equals(1); + + // Even if not all the locks are taken, releaseLock should be inferred + const steps2 = current.nextStepsForAppUpdate( + { + ...defaultContext, + locksTaken: new LocksTakenMap([{ appId: 1, services: ['server'] }]), + }, + target, + ); + const [releaseLockStep2] = expectSteps('releaseLock', steps2, 1); + expect(releaseLockStep2).to.have.property('appId').that.equals(1); + }); + + it('should not infer a releaseLock step if there are no locks to be released', async () => { + const services = [ + await createService({ serviceName: 'server' }), + await createService({ serviceName: 'client' }), + ]; + const current = createApp({ + services, + networks: [DEFAULT_NETWORK], + }); + const target = createApp({ + services, + networks: [DEFAULT_NETWORK], + isTarget: true, + }); + + const steps = current.nextStepsForAppUpdate(defaultContext, target); + expect(steps).to.have.length(0); + }); + + it('should infer a releaseLock step for the current appId only', async () => { + const services = [ + await createService({ serviceName: 'server' }), + await createService({ serviceName: 'client' }), + ]; + const current = createApp({ + services, + networks: [DEFAULT_NETWORK], + }); + const target = createApp({ + services, + networks: [DEFAULT_NETWORK], + isTarget: true, + }); + + const steps = current.nextStepsForAppUpdate( + { + ...defaultContext, + locksTaken: new LocksTakenMap([ + { appId: 1, services: ['server', 'client'] }, + { appId: 2, services: ['main'] }, + ]), + }, + target, + ); + const [releaseLockStep] = expectSteps('releaseLock', steps, 1); + expect(releaseLockStep).to.have.property('appId').that.equals(1); + }); + }); }); diff --git a/test/unit/lib/update-lock.spec.ts b/test/unit/lib/update-lock.spec.ts index 1f8d8bde..e00ae249 100644 --- a/test/unit/lib/update-lock.spec.ts +++ b/test/unit/lib/update-lock.spec.ts @@ -14,4 +14,45 @@ describe('lib/update-lock: unit tests', () => { ); }); }); + + 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; + }); + }); });