diff --git a/src/compose/app.ts b/src/compose/app.ts index 43aa7671..cdf77f73 100644 --- a/src/compose/app.ts +++ b/src/compose/app.ts @@ -15,12 +15,13 @@ import { import * as targetStateCache from '../device-state/target-state-cache'; import { getNetworkGateway } from '../lib/docker-utils'; import * as constants from '../lib/constants'; - -import { getStepsFromStrategy } from './update-strategies'; - -import { InternalInconsistencyError, isNotFoundError } from '../lib/errors'; +import { + getStepsFromStrategy, + getStrategyFromService, +} from './update-strategies'; +import { isNotFoundError } from '../lib/errors'; import * as config from '../config'; -import { checkTruthy, checkString } from '../lib/validation'; +import { checkTruthy } from '../lib/validation'; import { ServiceComposeConfig, DeviceMetadata } from './types/service'; import { pathExistsOnRoot } from '../lib/host-utils'; import { isSupervisor } from '../lib/supervisor-metadata'; @@ -133,17 +134,8 @@ export class App { state.containerIds, ); - for (const { current: svc } of removePairs) { - // All removes get a kill action if they're not already stopping - if (svc!.status !== 'Stopping') { - steps.push(generateStep('kill', { current: svc! })); - } else { - steps.push(generateStep('noop', {})); - } - } - // For every service which needs to be updated, update via update strategy. - const servicePairs = updatePairs.concat(installPairs); + const servicePairs = removePairs.concat(updatePairs, installPairs); steps = steps.concat( servicePairs .map((pair) => @@ -511,8 +503,8 @@ export class App { }, ): Nullable { if (current?.status === 'Stopping') { - // Theres a kill step happening already, emit a noop to ensure we stay alive while - // this happens + // There's a kill step happening already, emit a noop to ensure + // we stay alive while this happens return generateStep('noop', {}); } if (current?.status === 'Dead') { @@ -521,16 +513,14 @@ export class App { return; } - const needsDownload = !_.some( - context.availableImages, + const needsDownload = !context.availableImages.some( (image) => image.dockerImageId === target?.config.image || imageManager.isSameImage(image, { name: target?.imageName! }), ); - if (needsDownload && context.downloading.includes(target?.imageName!)) { - // The image needs to be downloaded, and it's currently downloading. We simply keep - // the application loop alive + // The image needs to be downloaded, and it's currently downloading. + // We simply keep the application loop alive return generateStep('noop', {}); } @@ -546,47 +536,39 @@ export class App { context.servicePairs, ); } else { - if (!target) { - throw new InternalInconsistencyError( - 'An empty changing pair passed to generateStepsForService', - ); - } - + // This service is in both current & target so requires an update, + // or it's a service that's not in target so requires removal const needsSpecialKill = this.serviceHasNetworkOrVolume( current, context.networkPairs, context.volumePairs, ); - if ( !needsSpecialKill && + target != null && current.isEqualConfig(target, context.containerIds) ) { // we're only starting/stopping a service return this.generateContainerStep(current, target); } - let strategy = - checkString(target.config.labels['io.balena.update.strategy']) || ''; - const validStrategies = [ - 'download-then-kill', - 'kill-then-download', - 'delete-then-download', - 'hand-over', - ]; - - if (!validStrategies.includes(strategy)) { - strategy = 'download-then-kill'; + let strategy: string; + let dependenciesMetForStart: boolean; + if (target != null) { + strategy = getStrategyFromService(target); + dependenciesMetForStart = this.dependenciesMetForServiceStart( + target, + context.targetApp, + context.availableImages, + context.networkPairs, + context.volumePairs, + context.servicePairs, + ); + } else { + strategy = getStrategyFromService(current); + dependenciesMetForStart = false; } - const dependenciesMetForStart = this.dependenciesMetForServiceStart( - target, - context.targetApp, - context.availableImages, - context.networkPairs, - context.volumePairs, - context.servicePairs, - ); const dependenciesMetForKill = this.dependenciesMetForServiceKill( context.targetApp, context.availableImages, @@ -678,7 +660,10 @@ export class App { volumePairs: Array>, servicePairs: Array>, ): CompositionStep | undefined { - if (needsDownload) { + 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), @@ -698,6 +683,37 @@ export class App { } } + private dependenciesMetForServiceFetch( + target: Service, + servicePairs: Array>, + ) { + const [servicePairsWithCurrent, servicePairsWithoutCurrent] = _.partition( + servicePairs, + (pair) => pair.current != null, + ); + + // Target services not in current can be safely fetched + for (const pair of servicePairsWithoutCurrent) { + if (target.serviceName === pair.target!.serviceName) { + return true; + } + } + + // Current services should be killed before target + // service fetch depending on update strategy + for (const pair of servicePairsWithCurrent) { + // Prefer target's update strategy if target service exists + const strategy = getStrategyFromService(pair.target ?? pair.current!); + if ( + ['kill-then-download', 'delete-then-download'].includes(strategy) && + pair.current!.config.running + ) { + return false; + } + } + return true; + } + // TODO: account for volumes-from, networks-from, links, etc // TODO: support networks instead of only network mode private dependenciesMetForServiceStart( @@ -751,7 +767,7 @@ export class App { } // do not start until all images have been downloaded - return this.targetImagesReady(targetApp, availableImages); + return this.targetImagesReady(targetApp.services, availableImages); } // Unless the update strategy requires an early kill (i.e kill-then-download, @@ -762,12 +778,14 @@ export class App { targetApp: App, availableImages: Image[], ) { - // Don't kill any services before all images have been downloaded - return this.targetImagesReady(targetApp, availableImages); + return this.targetImagesReady(targetApp.services, availableImages); } - private targetImagesReady(targetApp: App, availableImages: Image[]) { - return targetApp.services.every((service) => + private targetImagesReady( + targetServices: Service[], + availableImages: Image[], + ) { + return targetServices.every((service) => availableImages.some( (image) => image.dockerImageId === service.config.image || diff --git a/src/compose/update-strategies.ts b/src/compose/update-strategies.ts index 34256b1b..fb5bb1e5 100644 --- a/src/compose/update-strategies.ts +++ b/src/compose/update-strategies.ts @@ -1,12 +1,12 @@ import * as imageManager from './images'; import Service from './service'; - -import { InternalInconsistencyError } from '../lib/errors'; import { CompositionStep, generateStep } from './composition-steps'; +import { InternalInconsistencyError } from '../lib/errors'; +import { checkString } from '../lib/validation'; export interface StrategyContext { current: Service; - target: Service; + target?: Service; needsDownload: boolean; dependenciesMetForStart: boolean; dependenciesMetForKill: boolean; @@ -19,35 +19,35 @@ export function getStepsFromStrategy( ): CompositionStep { switch (strategy) { case 'download-then-kill': - if (context.needsDownload) { + if (context.needsDownload && context.target) { return generateStep('fetch', { image: imageManager.imageFromService(context.target), - serviceName: context.target.serviceName!, + 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 }); } else { - return { action: 'noop' }; + return generateStep('noop', {}); } case 'kill-then-download': case 'delete-then-download': return generateStep('kill', { current: context.current }); case 'hand-over': - if (context.needsDownload) { + if (context.needsDownload && context.target) { return generateStep('fetch', { image: imageManager.imageFromService(context.target), - serviceName: context.target.serviceName!, + serviceName: context.target.serviceName, }); } else if (context.needsSpecialKill && context.dependenciesMetForKill) { return generateStep('kill', { current: context.current }); - } else if (context.dependenciesMetForStart) { + } else if (context.dependenciesMetForStart && context.target) { return generateStep('handover', { current: context.current, target: context.target, }); } else { - return { action: 'noop' }; + return generateStep('noop', {}); } default: throw new InternalInconsistencyError( @@ -55,3 +55,21 @@ export function getStepsFromStrategy( ); } } + +export function getStrategyFromService(svc: Service): string { + let strategy = + checkString(svc.config.labels['io.balena.update.strategy']) || ''; + + const validStrategies = [ + 'download-then-kill', + 'kill-then-download', + 'delete-then-download', + 'hand-over', + ]; + + if (!validStrategies.includes(strategy)) { + strategy = 'download-then-kill'; + } + + return strategy; +} diff --git a/test/integration/compose/application-manager.spec.ts b/test/integration/compose/application-manager.spec.ts index 7683a004..4739ac73 100644 --- a/test/integration/compose/application-manager.spec.ts +++ b/test/integration/compose/application-manager.spec.ts @@ -15,6 +15,8 @@ import { createApps, createCurrentState, DEFAULT_NETWORK, + expectSteps, + expectNoStep, } from '~/test-lib/state-helper'; import { InstancedAppState } from '~/src/types'; @@ -327,6 +329,458 @@ describe('compose/application-manager', () => { .that.deep.includes({ name: 'image-old' }); }); + // These cases test whether the Supervisor handles multi-container updates + // on a per-app basis rather than a per-service while also respecting update strategies. + describe('update strategy compliance for multi-container apps', () => { + const createCurrentAndTargetServicesWithLabels = async (labels: { + [key: string]: string; + }) => { + const currentServices = [ + await createService({ + image: 'image-main', + labels, + appId: 1, + serviceName: 'main', + commit: 'old-release', + releaseId: 1, + }), + await createService({ + image: 'image-old', + labels, + appId: 1, + serviceName: 'old', + commit: 'old-release', + releaseId: 1, + }), + ]; + const targetServices = [ + await createService({ + image: 'image-main', + labels, + appId: 1, + serviceName: 'main', + commit: 'new-release', + releaseId: 2, + }), + await createService({ + image: 'image-new', + labels, + appId: 1, + serviceName: 'new', + commit: 'new-release', + releaseId: 2, + }), + ]; + return { currentServices, targetServices }; + }; + + it('should not infer a kill step for current service A before target service B download finishes with download-then-kill', async () => { + const labels = { + 'io.balena.update.strategy': 'download-then-kill', + }; + const { currentServices, targetServices } = + await createCurrentAndTargetServicesWithLabels(labels); + + /** + * Before target image finishes downloading, do not infer kill step + */ + let targetApps = createApps( + { + services: targetServices, + }, + true, + ); + const c1 = createCurrentState({ + services: currentServices, + networks: [DEFAULT_NETWORK], + downloading: ['image-new'], + }); + const steps = await applicationManager.inferNextSteps( + c1.currentApps, + targetApps, + { + downloading: c1.downloading, + availableImages: c1.availableImages, + containerIdsByAppId: c1.containerIdsByAppId, + }, + ); + // There should be two noop steps, one for target service which is still downloading, + // and one for current service which is waiting on target download to complete. + expectSteps('noop', steps, 2); + // No kill step yet + expectNoStep('kill', steps); + + /** + * After target image finishes downloading, infer a kill step + */ + targetApps = createApps( + { + services: targetServices, + }, + true, + ); + const { currentApps, availableImages, downloading, containerIdsByAppId } = + createCurrentState({ + services: currentServices, + networks: [DEFAULT_NETWORK], + images: [ + // Both images have been downloaded or had their metadata updated + createImage({ + name: 'image-main', + appId: 1, + serviceName: 'main', + commit: 'new-release', + releaseId: 2, + }), + createImage({ + name: 'image-new', + appId: 1, + serviceName: 'new', + commit: 'new-release', + releaseId: 2, + }), + ], + }); + const steps2 = await applicationManager.inferNextSteps( + currentApps, + targetApps, + { + downloading, + availableImages, + containerIdsByAppId, + }, + ); + // Service `old` is safe to kill after download for `new` has completed + const [killStep] = expectSteps('kill', steps2, 1); + expect(killStep) + .to.have.property('current') + .that.deep.includes({ serviceName: 'old' }); + // Service `new` in target release should be started as download has completed + const [startStep] = expectSteps('start', steps2, 1); + expect(startStep) + .to.have.property('target') + .that.deep.includes({ serviceName: 'new' }); + // No noop steps + expectNoStep('noop', steps2); + }); + + it('should infer a fetch step for target service B together with current service A kill with kill-then-download', async () => { + const labels = { + 'io.balena.update.strategy': 'kill-then-download', + }; + const { currentServices, targetServices } = + await createCurrentAndTargetServicesWithLabels(labels); + const targetApps = createApps( + { + services: targetServices, + }, + true, + ); + + /** + * Infer fetch step for new target service together with kill & updateMetadata steps for current services + */ + const c1 = createCurrentState({ + services: currentServices, + networks: [DEFAULT_NETWORK], + }); + const steps = await applicationManager.inferNextSteps( + c1.currentApps, + targetApps, + { + downloading: c1.downloading, + // Simulate old images already removed from image table + // to avoid removeImage steps + availableImages: [], + containerIdsByAppId: c1.containerIdsByAppId, + }, + ); + // Service `new` should be fetched + const [fetchStep] = expectSteps('fetch', steps, 1); + expect(fetchStep).to.have.property('serviceName').that.equals('new'); + // Service `old` should be killed + const [killStep] = expectSteps('kill', steps, 1); + expect(killStep) + .to.have.property('current') + .that.deep.includes({ serviceName: 'old' }); + // Service `main` should have its metadata updated + const [updateMetadataStep] = expectSteps('updateMetadata', steps, 1); + expect(updateMetadataStep) + .to.have.property('target') + .that.deep.includes({ serviceName: 'main' }); + + /** + * Noop while target image is downloading + */ + const c2 = createCurrentState({ + // Simulate `kill` and `updateMetadata` steps already executed + services: currentServices + .filter(({ serviceName }) => serviceName !== 'old') + .map((svc) => { + svc.releaseId = 2; + svc.commit = 'new-release'; + return svc; + }), + networks: [DEFAULT_NETWORK], + }); + const steps2 = await applicationManager.inferNextSteps( + c2.currentApps, + targetApps, + { + downloading: ['image-new'], + availableImages: c2.availableImages, + containerIdsByAppId: c2.containerIdsByAppId, + }, + ); + // Noop while service `new` is downloading + expectSteps('noop', steps2, 1); + // No start step should be generated until fetch completes + expectNoStep('start', steps2); + // No other steps + expect(steps2).to.have.length(1); + + /** + * Infer start step only after download completes + */ + const steps3 = await applicationManager.inferNextSteps( + c2.currentApps, + targetApps, + { + downloading: c1.downloading, + // Both images have been downloaded or had their metadata updated + availableImages: [ + createImage({ + name: 'image-main', + appId: 1, + serviceName: 'main', + commit: 'new-release', + releaseId: 2, + }), + createImage({ + name: 'image-new', + appId: 1, + serviceName: 'new', + commit: 'new-release', + releaseId: 2, + }), + ], + containerIdsByAppId: c1.containerIdsByAppId, + }, + ); + // Service `new` should be started + const [startStep] = expectSteps('start', steps3, 1); + expect(startStep) + .to.have.property('target') + .that.deep.includes({ serviceName: 'new' }); + // No other steps + expect(steps3).to.have.length(1); + }); + + it('should infer a fetch step for target service B together with current service A removal with delete-then-download', async () => { + const labels = { + 'io.balena.update.strategy': 'delete-then-download', + }; + const { currentServices, targetServices } = + await createCurrentAndTargetServicesWithLabels(labels); + const targetApps = createApps( + { + services: targetServices, + }, + true, + ); + + /** + * Infer fetch step for new target service together with kill & updateMetadata steps for current services + */ + const c1 = createCurrentState({ + services: currentServices, + networks: [DEFAULT_NETWORK], + }); + const steps = await applicationManager.inferNextSteps( + c1.currentApps, + targetApps, + { + downloading: c1.downloading, + // Simulate old images already removed from image table + // to avoid removeImage steps + availableImages: [], + containerIdsByAppId: c1.containerIdsByAppId, + }, + ); + // Service `new` should be fetched + const [fetchStep] = expectSteps('fetch', steps, 1); + expect(fetchStep).to.have.property('serviceName').that.equals('new'); + // Service `old` should be killed + const [killStep] = expectSteps('kill', steps, 1); + expect(killStep) + .to.have.property('current') + .that.deep.includes({ serviceName: 'old' }); + // Service `main` should have its metadata updated + const [updateMetadataStep] = expectSteps('updateMetadata', steps, 1); + expect(updateMetadataStep) + .to.have.property('target') + .that.deep.includes({ serviceName: 'main' }); + + /** + * Noop while target image is downloading + */ + const c2 = createCurrentState({ + // Simulate `kill` and `updateMetadata` steps already executed + services: currentServices + .filter(({ serviceName }) => serviceName !== 'old') + .map((svc) => { + svc.releaseId = 2; + svc.commit = 'new-release'; + return svc; + }), + networks: [DEFAULT_NETWORK], + }); + const steps2 = await applicationManager.inferNextSteps( + c2.currentApps, + targetApps, + { + downloading: ['image-new'], + availableImages: c2.availableImages, + containerIdsByAppId: c2.containerIdsByAppId, + }, + ); + // Noop while service `new` is downloading + expectSteps('noop', steps2, 1); + // No start step should be generated until fetch completes + expectNoStep('start', steps2); + // No other steps + expect(steps2).to.have.length(1); + + /** + * Infer start step only after download completes + */ + const steps3 = await applicationManager.inferNextSteps( + c2.currentApps, + targetApps, + { + downloading: c1.downloading, + // Both images have been downloaded or had their metadata updated + availableImages: [ + createImage({ + name: 'image-main', + appId: 1, + serviceName: 'main', + commit: 'new-release', + releaseId: 2, + }), + createImage({ + name: 'image-new', + appId: 1, + serviceName: 'new', + commit: 'new-release', + releaseId: 2, + }), + ], + containerIdsByAppId: c1.containerIdsByAppId, + }, + ); + // Service `new` should be started + const [startStep] = expectSteps('start', steps3, 1); + expect(startStep) + .to.have.property('target') + .that.deep.includes({ serviceName: 'new' }); + // No other steps + expect(steps3).to.have.length(1); + }); + + it('should not start target services until all downloads have completed with kill|delete-then-download', async () => { + const targetServices = [ + await createService({ + serviceName: 'one', + image: 'image-one', + labels: { + 'io.balena.update.strategy': 'kill-then-download', + }, + }), + await createService({ + serviceName: 'two', + image: 'image-two', + labels: { + 'io.balena.update.strategy': 'delete-then-download', + }, + }), + ]; + const targetApps = createApps( + { + services: targetServices, + }, + true, + ); + const { currentApps, availableImages, containerIdsByAppId } = + createCurrentState({ + services: [], + networks: [DEFAULT_NETWORK], + }); + + /** + * Noop while both images are still downloading + */ + const steps = await applicationManager.inferNextSteps( + currentApps, + targetApps, + { + downloading: ['image-one', 'image-two'], + availableImages, + containerIdsByAppId, + }, + ); + expectSteps('noop', steps, 2); + // No other steps + expect(steps).to.have.length(2); + + /** + * Noop while one image is still downloading + */ + const steps2 = await applicationManager.inferNextSteps( + currentApps, + targetApps, + { + downloading: ['image-two'], + availableImages: [ + createImage({ + name: 'image-one', + serviceName: 'one', + }), + ], + containerIdsByAppId, + }, + ); + expectSteps('noop', steps2, 1); + // No other steps + expect(steps2).to.have.length(1); + + /** + * Only start target services after both images downloaded + */ + const steps3 = await applicationManager.inferNextSteps( + currentApps, + targetApps, + { + downloading: [], + availableImages: [ + createImage({ + name: 'image-one', + serviceName: 'one', + }), + createImage({ + name: 'image-two', + serviceName: 'two', + }), + ], + containerIdsByAppId, + }, + ); + expectSteps('start', steps3, 2); + // No other steps + expect(steps3).to.have.length(2); + }); + }); + it('does not infer to kill a service with default strategy if a dependency is not downloaded', async () => { const targetApps = createApps( {