From 87b195685a4354ae50bfbd852d6e3daff7bc6a58 Mon Sep 17 00:00:00 2001 From: Felipe Lalanne <1822826+pipex@users.noreply.github.com> Date: Tue, 7 Mar 2023 19:32:10 -0300 Subject: [PATCH 1/2] Use the state-helper functions in app module tests --- test/lib/state-helper.ts | 65 +++++++++-- test/unit/compose/app.spec.ts | 209 ++++++++++++---------------------- 2 files changed, 129 insertions(+), 145 deletions(-) diff --git a/test/lib/state-helper.ts b/test/lib/state-helper.ts index 69fa62887..1ea3e4853 100644 --- a/test/lib/state-helper.ts +++ b/test/lib/state-helper.ts @@ -5,6 +5,10 @@ import Network from '~/src/compose/network'; import Service from '~/src/compose/service'; import { ServiceComposeConfig } from '~/src/compose/types/service'; import Volume from '~/src/compose/volume'; +import { + CompositionStep, + CompositionStepAction, +} from '~/src/compose/composition-steps'; import { InstancedAppState } from '~/src/types/state'; export const DEFAULT_NETWORK = Network.fromComposeObject( @@ -74,6 +78,26 @@ export function createImage( } as Image; } +export function createApp({ + services = [] as Service[], + networks = [] as Network[], + volumes = [] as Volume[], + isTarget = false, + appId = 1, + appUuid = 'appuuid', +} = {}) { + return new App( + { + appId, + appUuid, + services, + networks, + volumes, + }, + isTarget, + ); +} + export function createApps( { services = [] as Service[], @@ -105,15 +129,14 @@ export function createApps( const apps: InstancedAppState = {}; for (const appId of allAppIds) { - apps[appId] = new App( - { - appId, - services: servicesByAppId[appId] ?? [], - networks: networksByAppId[appId] ?? [], - volumes: volumesByAppId[appId] ?? [], - }, - target, - ); + apps[appId] = createApp({ + services: servicesByAppId[appId] ?? [], + networks: networksByAppId[appId] ?? [], + volumes: volumesByAppId[appId] ?? [], + appId, + appUuid: servicesByAppId[appId]?.[0]?.appUuid ?? 'deadbeef', + isTarget: target, + }); } return apps; @@ -151,3 +174,27 @@ export function createCurrentState({ containerIdsByAppId, }; } + +export const expectSteps = ( + action: CompositionStepAction, + steps: CompositionStep[], + min = 1, + max = min, + message = `Expected to find ${min} step(s) with action '${action}', instead found ${JSON.stringify( + steps.map((s) => s.action), + )}`, +) => { + const filtered = steps.filter((s) => s.action === action); + + if (filtered.length < min || filtered.length > max) { + throw new Error(message); + } + return filtered; +}; + +export function expectNoStep( + action: CompositionStepAction, + steps: CompositionStep[], +) { + expectSteps(action, steps, 0, 0); +} diff --git a/test/unit/compose/app.spec.ts b/test/unit/compose/app.spec.ts index a068d4bf5..a0de2ae7b 100644 --- a/test/unit/compose/app.spec.ts +++ b/test/unit/compose/app.spec.ts @@ -1,15 +1,17 @@ import { expect } from 'chai'; -import App from '~/src/compose/app'; -import { - CompositionStep, - CompositionStepAction, -} from '~/src/compose/composition-steps'; import { Image } from '~/src/compose/images'; import Network from '~/src/compose/network'; -import Service from '~/src/compose/service'; -import { ServiceComposeConfig } from '~/src/compose/types/service'; import Volume from '~/src/compose/volume'; +import { + createService, + createImage, + createApp, + DEFAULT_NETWORK, + expectSteps, + expectNoStep, +} from '~/test-lib/state-helper'; + const defaultContext = { keepVolumes: false, availableImages: [] as Image[], @@ -17,98 +19,6 @@ const defaultContext = { downloading: [] as string[], }; -function createApp({ - services = [] as Service[], - networks = [] as Network[], - volumes = [] as Volume[], - isTarget = false, - appId = 1, - appUuid = 'appuuid', -} = {}) { - return new App( - { - appId, - appUuid, - services, - networks, - volumes, - }, - isTarget, - ); -} - -async function createService( - { - appId = 1, - appUuid = 'appuuid', - serviceName = 'test', - commit = 'test-commit', - ...conf - } = {} as Partial, - { state = {} as Partial, options = {} as any } = {}, -) { - const svc = await Service.fromComposeObject( - { - appId, - appUuid, - serviceName, - commit, - running: true, - ...conf, - }, - options, - ); - - // Add additonal configuration - for (const k of Object.keys(state)) { - (svc as any)[k] = (state as any)[k]; - } - return svc; -} - -function createImage( - { - appId = 1, - appUuid = 'appuuid', - name = 'test-image', - serviceName = 'test', - commit = 'test-commit', - ...extra - } = {} as Partial, -) { - return { - appId, - appUuid, - commit, - name, - serviceName, - ...extra, - } as Image; -} - -const expectSteps = ( - action: CompositionStepAction, - steps: CompositionStep[], - min = 1, - max = min, - message = `Expected to find ${min} step(s) with action '${action}', instead found ${JSON.stringify( - steps.map((s) => s.action), - )}`, -) => { - const filtered = steps.filter((s) => s.action === action); - - if (filtered.length < min || filtered.length > max) { - throw new Error(message); - } - return filtered; -}; - -function expectNoStep(action: CompositionStepAction, steps: CompositionStep[]) { - expectSteps(action, steps, 0, 0); -} - -const defaultNetwork = Network.fromComposeObject('default', 1, 'appuuid', {}); - describe('compose/app', () => { describe('volume state behavior', () => { it('should correctly infer a volume create step', () => { @@ -234,6 +144,7 @@ describe('compose/app', () => { const current = createApp({ services: [ await createService({ + serviceName: 'test', composition: { volumes: ['test-volume:/data'] }, }), ], @@ -242,6 +153,7 @@ describe('compose/app', () => { const target = createApp({ services: [ await createService({ + serviceName: 'test', composition: { volumes: ['test-volume:/data'] }, }), ], @@ -277,6 +189,7 @@ describe('compose/app', () => { it('should not output a kill step for a service which is already stopping when changing a volume', async () => { const service = await createService({ + serviceName: 'test', composition: { volumes: ['test-volume:/data'] }, }); service.status = 'Stopping'; @@ -321,14 +234,14 @@ describe('compose/app', () => { // Temporarily set target services & volumes to empty, as in doPurge const intermediateTarget = createApp({ services: [], - networks: [defaultNetwork], + networks: [DEFAULT_NETWORK], isTarget: true, }); // Generate initial state with one service & one volume const current = createApp({ services: [service], - networks: [defaultNetwork], + networks: [DEFAULT_NETWORK], volumes: [volume], }); @@ -351,7 +264,7 @@ describe('compose/app', () => { // No steps, simulate container removal & explicit volume removal as in doPurge const currentWithServiceRemoved = createApp({ services: [], - networks: [defaultNetwork], + networks: [DEFAULT_NETWORK], volumes: [volume], }); expect( @@ -364,7 +277,7 @@ describe('compose/app', () => { // Simulate volume removal const currentWithVolumesRemoved = createApp({ services: [], - networks: [defaultNetwork], + networks: [DEFAULT_NETWORK], volumes: [], }); @@ -372,7 +285,7 @@ describe('compose/app', () => { service.status = 'Running'; const target = createApp({ services: [service], - networks: [defaultNetwork], + networks: [DEFAULT_NETWORK], volumes: [volume], isTarget: true, }); @@ -388,7 +301,7 @@ describe('compose/app', () => { // Final step: start service const currentWithVolumeRecreated = createApp({ services: [], - networks: [defaultNetwork], + networks: [DEFAULT_NETWORK], volumes: [volume], }); @@ -436,7 +349,7 @@ describe('compose/app', () => { it('should correctly remove default duplicate networks', () => { const current = createApp({ - networks: [defaultNetwork, defaultNetwork], + networks: [DEFAULT_NETWORK, DEFAULT_NETWORK], }); const target = createApp({ networks: [], @@ -508,7 +421,7 @@ describe('compose/app', () => { const current = createApp({ appUuid: 'deadbeef', networks: [ - defaultNetwork, + DEFAULT_NETWORK, Network.fromComposeObject('test-network', 1, 'deadbeef', {}), Network.fromComposeObject('test-network', 1, 'deadbeef', {}), ], @@ -516,6 +429,7 @@ describe('compose/app', () => { await createService({ appId: 1, appUuid: 'deadbeef', + serviceName: 'test', composition: { networks: ['test-network'] }, }), ], @@ -528,6 +442,7 @@ describe('compose/app', () => { await createService({ appId: 1, appUuid: 'deadbeef', + serviceName: 'test', composition: { networks: ['test-network'] }, }), ], @@ -625,6 +540,7 @@ describe('compose/app', () => { await createService({ appId: 1, appUuid: 'deadbeef', + serviceName: 'test', composition: { networks: ['test-network'] }, }), ], @@ -634,7 +550,9 @@ describe('compose/app', () => { }); const target = createApp({ appUuid: 'deadbeef', - services: [await createService({ appUuid: 'deadbeef' })], + services: [ + await createService({ appUuid: 'deadbeef', serviceName: 'test' }), + ], networks: [], isTarget: true, }); @@ -655,6 +573,7 @@ describe('compose/app', () => { const current = createApp({ services: [ await createService({ + serviceName: 'test', composition: { networks: ['test-network'] }, }), ], @@ -663,6 +582,7 @@ describe('compose/app', () => { const target = createApp({ services: [ await createService({ + serviceName: 'test', composition: { networks: { 'test-network': {} } }, }), ], @@ -913,15 +833,23 @@ describe('compose/app', () => { await createService({ appId: 1, serviceName: 'main' }), await createService({ appId: 1, serviceName: 'aux' }), ], - networks: [defaultNetwork], + networks: [DEFAULT_NETWORK], }); const target = createApp({ services: [await createService({ appId: 1, serviceName: 'main' })], - networks: [defaultNetwork], + networks: [DEFAULT_NETWORK], isTarget: true, }); - const steps = current.nextStepsForAppUpdate(defaultContext, target); + const steps = current.nextStepsForAppUpdate( + { + ...defaultContext, + // With default download-then-kill, a kill should be inferred + // once all images in target state are available + availableImages: [createImage({ serviceName: 'main' })], + }, + target, + ); const [killStep] = expectSteps('kill', steps); expect(killStep) .to.have.property('current') @@ -1103,7 +1031,7 @@ describe('compose/app', () => { const current = createApp({ services: [await createService({ appId: 1, serviceName: 'main' })], // Default network was already created - networks: [defaultNetwork], + networks: [DEFAULT_NETWORK], }); const target = createApp({ services: [ @@ -1113,7 +1041,7 @@ describe('compose/app', () => { composition: { privileged: true }, }), ], - networks: [defaultNetwork], + networks: [DEFAULT_NETWORK], isTarget: true, }); @@ -1131,7 +1059,7 @@ describe('compose/app', () => { const intermediate = createApp({ services: [], // Default network was already created - networks: [defaultNetwork], + networks: [DEFAULT_NETWORK], }); // now should see a 'start' @@ -1173,7 +1101,7 @@ describe('compose/app', () => { }, ), ], - networks: [defaultNetwork], + networks: [DEFAULT_NETWORK], }); const target = createApp({ services: [ @@ -1189,7 +1117,7 @@ describe('compose/app', () => { serviceName: 'dep', }), ], - networks: [defaultNetwork], + networks: [DEFAULT_NETWORK], isTarget: true, }); @@ -1212,7 +1140,7 @@ describe('compose/app', () => { { state: { containerId: 'dep-id' } }, ), ], - networks: [defaultNetwork], + networks: [DEFAULT_NETWORK], }); // we should now see a start for the 'main' service... @@ -1240,11 +1168,11 @@ describe('compose/app', () => { services: [ await createService({ running: false, serviceName: 'main' }), ], - networks: [defaultNetwork], + networks: [DEFAULT_NETWORK], }); const target = createApp({ services: [await createService({ serviceName: 'main' })], - networks: [defaultNetwork], + networks: [DEFAULT_NETWORK], isTarget: true, }); @@ -1277,7 +1205,7 @@ describe('compose/app', () => { commit: 'old-release', }), ], - networks: [defaultNetwork], + networks: [DEFAULT_NETWORK], }); const target = createApp({ services: [ @@ -1288,7 +1216,7 @@ describe('compose/app', () => { commit: 'new-release', }), ], - networks: [defaultNetwork], + networks: [DEFAULT_NETWORK], isTarget: true, }); @@ -1305,7 +1233,7 @@ describe('compose/app', () => { // assume steps were applied const intermediate = createApp({ services: [], - networks: [defaultNetwork], + networks: [DEFAULT_NETWORK], }); const stepsToTarget = intermediate.nextStepsForAppUpdate( @@ -1354,7 +1282,7 @@ describe('compose/app', () => { commit: 'old-release', }), ], - networks: [defaultNetwork], + networks: [DEFAULT_NETWORK], }); const target = createApp({ services: [ @@ -1374,7 +1302,7 @@ describe('compose/app', () => { commit: 'new-release', }), ], - networks: [defaultNetwork], + networks: [DEFAULT_NETWORK], isTarget: true, }); @@ -1406,7 +1334,7 @@ describe('compose/app', () => { commit: 'old-release', }), ], - networks: [defaultNetwork], + networks: [DEFAULT_NETWORK], }); const target = createApp({ services: [ @@ -1417,7 +1345,7 @@ describe('compose/app', () => { commit: 'new-release', }), ], - networks: [defaultNetwork], + networks: [DEFAULT_NETWORK], isTarget: true, }); @@ -1450,11 +1378,12 @@ describe('compose/app', () => { }; const current = createApp({ - services: [await createService({ labels })], + services: [await createService({ serviceName: 'test', labels })], }); const target = createApp({ services: [ await createService({ + serviceName: 'test', labels, composition: { privileged: true, @@ -1472,7 +1401,7 @@ describe('compose/app', () => { }); it('should not start a service when a network it depends on is not ready', async () => { - const current = createApp({ networks: [defaultNetwork] }); + const current = createApp({ networks: [DEFAULT_NETWORK] }); const target = createApp({ services: [ await createService({ @@ -1481,7 +1410,7 @@ describe('compose/app', () => { }), ], networks: [ - defaultNetwork, + DEFAULT_NETWORK, Network.fromComposeObject('test', 1, 'appuuid', {}), ], isTarget: true, @@ -1528,7 +1457,15 @@ describe('compose/app', () => { isTarget: true, }); - const steps = current.nextStepsForAppUpdate(defaultContext, target); + const steps = 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' })], + }, + target, + ); expectSteps('kill', steps, 2); }); @@ -1568,7 +1505,7 @@ describe('compose/app', () => { commit: 'old-release', }), ], - networks: [defaultNetwork], + networks: [DEFAULT_NETWORK], }); const target = createApp({ services: [ @@ -1585,7 +1522,7 @@ describe('compose/app', () => { commit: 'new-release', }), ], - networks: [defaultNetwork], + networks: [DEFAULT_NETWORK], isTarget: true, }); @@ -1607,7 +1544,7 @@ describe('compose/app', () => { const current = createApp({ services: [], - networks: [defaultNetwork], + networks: [DEFAULT_NETWORK], }); const target = createApp({ services: [ @@ -1624,7 +1561,7 @@ describe('compose/app', () => { commit: 'new-release', }), ], - networks: [defaultNetwork], + networks: [DEFAULT_NETWORK], isTarget: true, }); @@ -1651,7 +1588,7 @@ describe('compose/app', () => { const current = createApp({ services: [], - networks: [defaultNetwork], + networks: [DEFAULT_NETWORK], }); const target = createApp({ services: [ @@ -1668,7 +1605,7 @@ describe('compose/app', () => { commit: 'new-release', }), ], - networks: [defaultNetwork], + networks: [DEFAULT_NETWORK], isTarget: true, }); From 3afcef296914840268356f7f98bc4bd8cce9427c Mon Sep 17 00:00:00 2001 From: Christina Ying Wang Date: Thu, 14 Dec 2023 19:54:07 -0800 Subject: [PATCH 2/2] Respect update strategies app-wide instead of at the service level Fixes behavior for release updates which removes a service in current state and adds a new service in target state. Change-type: patch Closes: #2095 Signed-off-by: Christina Ying Wang --- src/compose/app.ts | 126 ++--- src/compose/update-strategies.ts | 38 +- .../compose/application-manager.spec.ts | 454 ++++++++++++++++++ 3 files changed, 554 insertions(+), 64 deletions(-) diff --git a/src/compose/app.ts b/src/compose/app.ts index 43aa76718..cdf77f737 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 34256b1bb..fb5bb1e5e 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 7683a0049..4739ac739 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( {