diff --git a/src/compose/application-manager.ts b/src/compose/application-manager.ts index 87a9b9c9..1073a20c 100644 --- a/src/compose/application-manager.ts +++ b/src/compose/application-manager.ts @@ -78,6 +78,13 @@ export let containerStarted: { [containerId: string]: boolean } = {}; export let fetchesInProgress = 0; export let timeSpentFetching = 0; +// In the case of intermediate target apply, toggle to true to avoid unintended image deletion +let isApplyingIntermediate = false; + +export function setIsApplyingIntermediate(value: boolean = false) { + isApplyingIntermediate = value; +} + export function resetTimeSpentFetching(value: number = 0) { timeSpentFetching = value; } @@ -206,12 +213,15 @@ export async function getRequiredSteps( steps.push({ action: 'ensureSupervisorNetwork' }); } } else { - if (!localMode && downloading.length === 0) { + if (!localMode && downloading.length === 0 && !isApplyingIntermediate) { + // Avoid cleaning up dangling images while purging if (cleanupNeeded) { steps.push({ action: 'cleanup' }); } - // Detect any images which must be saved/removed + // Detect any images which must be saved/removed, except when purging, + // as we only want to remove containers, remove volumes, create volumes + // anew, and start containers without images being removed. steps = steps.concat( saveAndRemoveImages( currentApps, @@ -325,6 +335,7 @@ export async function getRequiredSteps( steps, ), ); + return steps; } @@ -717,7 +728,9 @@ function saveAndRemoveImages( (image) => { const notUsedForDelta = !_.includes(deltaSources, image.name); const notUsedByProxyvisor = !_.some(proxyvisorImages, (proxyvisorImage) => - imageManager.isSameImage(image, { name: proxyvisorImage }), + imageManager.isSameImage(image, { + name: proxyvisorImage, + }), ); return notUsedForDelta && notUsedByProxyvisor; }, diff --git a/src/device-api/common.d.ts b/src/device-api/common.d.ts deleted file mode 100644 index b0d2d621..00000000 --- a/src/device-api/common.d.ts +++ /dev/null @@ -1,29 +0,0 @@ -import { Service } from '../compose/service'; -import { InstancedDeviceState } from '../types/state'; - -export interface ServiceAction { - action: string; - serviceId: number; - current: Service; - target: Service; - options: any; -} - -declare function doRestart(appId: number, force: boolean): Promise; - -declare function doPurge(appId: number, force: boolean): Promise; - -declare function serviceAction( - action: string, - serviceId: number, - current: Service, - target?: Service, - options?: any, -): ServiceAction; - -declare function safeStateClone( - targetState: InstancedDeviceState, -): // Use an any here, because it's not an InstancedDeviceState, -// and it's also not the exact same type as the API serves from -// state endpoint (more details in the function) -Dictionary; diff --git a/src/device-api/common.js b/src/device-api/common.js index 44b9367d..870c8440 100644 --- a/src/device-api/common.js +++ b/src/device-api/common.js @@ -57,13 +57,29 @@ export async function doPurge(appId, force) { throw new Error(appNotFoundMessage); } - const app = allApps[appId]; + const clonedState = safeStateClone(currentState); + /** + * With multi-container, Docker adds an invalid network alias equal to the current containerId + * to that service's network configs when starting a service. Thus when reapplying intermediateState + * after purging, use a cloned state instance which automatically filters out invalid network aliases. + * + * This will prevent error logs like the following: + * https://gist.github.com/cywang117/84f9cd4e6a9641dbed530c94e1172f1d#file-logs-sh-L58 + * + * When networks do not match because of their aliases, services are killed and recreated + * an additional time which is unnecessary. Filtering prevents this additional restart BUT + * it is a stopgap measure until we can keep containerId network aliases from being stored + * in state's service config objects (TODO) + * + * See https://github.com/balena-os/balena-supervisor/blob/master/src/device-api/common.js#L160-L180 + * for a more in-depth explanation of why aliases need to be filtered out. + */ - const currentServices = app.services; - const currentVolumes = app.volumes; + // After cloning, set services & volumes as empty to be applied as intermediateTargetState + allApps[appId].services = []; + allApps[appId].volumes = {}; - app.services = []; - app.volumes = {}; + applicationManager.setIsApplyingIntermediate(true); return deviceState .pausingApply(() => @@ -79,20 +95,23 @@ export async function doPurge(appId, force) { ); }) .then(() => { - app.services = currentServices; - app.volumes = currentVolumes; - return deviceState.applyIntermediateTarget(currentState, { + return deviceState.applyIntermediateTarget(clonedState, { skipLock: true, }); }), ) - .finally(() => deviceState.triggerApplyTarget()); + .finally(() => { + applicationManager.setIsApplyingIntermediate(false); + deviceState.triggerApplyTarget(); + }); }), ) .then(() => logger.logSystemMessage('Purged data', { appId }, 'Purge data success'), ) .catch((err) => { + applicationManager.setIsApplyingIntermediate(false); + logger.logSystemMessage( `Error purging data: ${err}`, { appId, error: err }, @@ -109,6 +128,11 @@ export function serviceAction(action, serviceId, current, target, options) { return { action, serviceId, current, target, options }; } +/** + * This doesn't truly return an InstancedDeviceState, but it's close enough to mostly work where it's used + * + * @returns { import('../types/state').InstancedDeviceState } + */ export function safeStateClone(targetState) { // We avoid using cloneDeep here, as the class // instances can cause a maximum call stack exceeded @@ -122,6 +146,7 @@ export function safeStateClone(targetState) { // thing to do would be to represent the input with // io-ts and make sure the below conforms to it + /** @type { any } */ const cloned = { local: { config: {}, diff --git a/test/36-compose-app.spec.ts b/test/36-compose-app.spec.ts index fe85fadf..a2400640 100644 --- a/test/36-compose-app.spec.ts +++ b/test/36-compose-app.spec.ts @@ -1079,6 +1079,7 @@ describe('compose/app', () => { const steps = current.nextStepsForAppUpdate(defaultContext, target); withSteps(steps).expectStep('kill').to.have.length(2); }); + it('should not create a service when a network it depends on is not ready', async () => { const current = createApp([], [defaultNetwork], [], false); const target = createApp( @@ -1092,4 +1093,93 @@ describe('compose/app', () => { withSteps(steps).expectStep('createNetwork'); withSteps(steps).rejectStep('start'); }); // no create service, is create network + + describe('Intermediate state apply support', () => { + beforeEach(() => { + appMock.mockSupervisorNetwork(true); + appMock.mockManagers([], [], []); + appMock.mockImages([], false, []); + + applicationManager.setIsApplyingIntermediate(true); + }); + afterEach(() => { + appMock.unmockAll(); + applicationManager.setIsApplyingIntermediate(false); + }); + + it('should generate the correct step sequence for a volume purge request', async () => { + const service = await createService({ + volumes: ['db-volume'], + image: 'test-image', + }); + const volume = Volume.fromComposeObject('db-volume', service.appId, {}); + const contextWithImages = { + ...defaultContext, + ...{ + availableImages: [ + { + appId: service.appId, + dependent: 0, + imageId: service.imageId, + releaseId: service.releaseId, + serviceId: service.serviceId, + name: 'test-image', + serviceName: service.serviceName, + } as Image, + ], + }, + }; + // Temporarily set target services & volumes to empty, as in doPurge + let intermediateTarget = createApp([], [defaultNetwork], [], true); + + // Generate current with one service & one volume + const current = createApp([service], [defaultNetwork], [volume], false); + + // Step 1: kill + let steps = current.nextStepsForAppUpdate( + contextWithImages, + intermediateTarget, + ); + withSteps(steps) + .expectStep('kill') + .forCurrent(service.serviceName as ServicePredicate); + expect(steps).to.have.length(1); + + // Step 2: noop (service is stopping) + service.status = 'Stopping'; + steps = current.nextStepsForAppUpdate( + contextWithImages, + intermediateTarget, + ); + expectStep('noop', steps); + expect(steps).to.have.length(1); + + // No steps, simulate container removal & explicit volume removal as in doPurge + current.services = []; + steps = current.nextStepsForAppUpdate( + contextWithImages, + intermediateTarget, + ); + expect(steps).to.have.length(0); + current.volumes = {}; + + // Step 3: start & createVolume + service.status = 'Running'; + intermediateTarget = createApp( + [service], + [defaultNetwork], + [volume], + true, + ); + steps = current.nextStepsForAppUpdate( + contextWithImages, + intermediateTarget, + ); + expect(steps).to.have.length(2); + withSteps(steps) + .expectStep('start') + .forTarget(service.serviceName as ServicePredicate); + expectStep('createVolume', steps); + }); + }); });