From 7aedc97ee185953a3068ce3ef9743989e32a75b2 Mon Sep 17 00:00:00 2001 From: Felipe Lalanne Date: Wed, 10 Nov 2021 20:22:40 +0000 Subject: [PATCH] Wait for images to be ready before moving between releases For download-then-kill strategy, this waits for all changing images on the target release to be available on device before killing the old services. This will prevent that multicontainer applications get to a state where some services of the new release start runnning much before others have been downloaded. When adding new services to a multicontainer app, the supervisor will now wait for other changing services to be downloaded before starting the new service. Closes: #1812 Change-type: patch --- src/compose/app.ts | 52 ++++++------- test/src/compose/app.spec.ts | 147 ++++++++++++++++++++++++++++++++++- 2 files changed, 172 insertions(+), 27 deletions(-) diff --git a/src/compose/app.ts b/src/compose/app.ts index a342af6b..7efa18cb 100644 --- a/src/compose/app.ts +++ b/src/compose/app.ts @@ -488,7 +488,9 @@ export class App { // Either this is a new service, or the current one has already been killed return this.generateFetchOrStartStep( target!, + context.targetApp, needsDownload, + context.availableImages, context.networkPairs, context.volumePairs, context.servicePairs, @@ -520,12 +522,13 @@ export class App { const dependenciesMetForStart = this.dependenciesMetForServiceStart( target, + context.targetApp, + context.availableImages, context.networkPairs, context.volumePairs, context.servicePairs, ); const dependenciesMetForKill = this.dependenciesMetForServiceKill( - target, context.targetApp, context.availableImages, context.localMode, @@ -591,7 +594,9 @@ export class App { private generateFetchOrStartStep( target: Service, + targetApp: App, needsDownload: boolean, + availableImages: Image[], networkPairs: Array>, volumePairs: Array>, servicePairs: Array>, @@ -605,6 +610,8 @@ export class App { } else if ( this.dependenciesMetForServiceStart( target, + targetApp, + availableImages, networkPairs, volumePairs, servicePairs, @@ -618,6 +625,8 @@ export class App { // TODO: support networks instead of only network mode private dependenciesMetForServiceStart( target: Service, + targetApp: App, + availableImages: Image[], networkPairs: Array>, volumePairs: Array>, servicePairs: Array>, @@ -626,7 +635,7 @@ export class App { // different to a dependency which is in the servicePairs below, as these // are services which are changing). We could have a dependency which is // starting up, but is not yet running. - const depInstallingButNotRunning = _.some(this.services, (svc) => { + const depInstallingButNotRunning = _.some(targetApp.services, (svc) => { if (target.dependsOn?.includes(svc.serviceName!)) { if (!svc.config.running) { return true; @@ -664,16 +673,15 @@ export class App { return false; } - // everything is ready for the service to start... - return true; + // do not start until all images have been downloaded + return this.targetImagesReady(targetApp, availableImages); } // Unless the update strategy requires an early kill (i.e kill-then-download, // delete-then-download), we only want to kill a service once the images for the // services it depends on have been downloaded, so as to minimize downtime (but not - // block the killing too much, potentially causing a daedlock) + // block the killing too much, potentially causing a deadlock) private dependenciesMetForServiceKill( - target: Service, targetApp: App, availableImages: Image[], localMode: boolean, @@ -685,26 +693,18 @@ export class App { return true; } - if (target.dependsOn != null) { - for (const dependency of target.dependsOn) { - const dependencyService = _.find(targetApp.services, { - serviceName: dependency, - }); - if ( - !_.some( - availableImages, - (image) => - image.dockerImageId === dependencyService?.config.image || - imageManager.isSameImage(image, { - name: dependencyService?.imageName!, - }), - ) - ) { - return false; - } - } - } - return true; + // Don't kill any services before all images have been downloaded + return this.targetImagesReady(targetApp, availableImages); + } + + private targetImagesReady(targetApp: App, availableImages: Image[]) { + return targetApp.services.every((service) => + availableImages.some( + (image) => + image.dockerImageId === service.config.image || + imageManager.isSameImage(image, { name: service.imageName! }), + ), + ); } public static async fromTargetState( diff --git a/test/src/compose/app.spec.ts b/test/src/compose/app.spec.ts index b2d484f1..d9dd6cad 100644 --- a/test/src/compose/app.spec.ts +++ b/test/src/compose/app.spec.ts @@ -1147,7 +1147,7 @@ describe('compose/app', () => { .that.deep.includes({ serviceName: 'test' }); }); - it('should not create a service when a network it depends on is not ready', async () => { + it('should not start a service when a network it depends on is not ready', async () => { const current = createApp({ networks: [defaultNetwork] }); const target = createApp({ services: [await createService({ networks: ['test'], appId: 1 })], @@ -1199,6 +1199,151 @@ describe('compose/app', () => { const steps = current.nextStepsForAppUpdate(defaultContext, target); expectSteps('kill', steps, 2); }); + + it('should not infer a kill step with the default strategy before all target images have been downloaded', async () => { + const contextWithImages = { + ...defaultContext, + ...{ + downloading: ['other-image-2'], // One of the images is being downloaded + availableImages: [ + createImage({ appId: 1, name: 'main-image', serviceName: 'main' }), + createImage({ + appId: 1, + name: 'other-image', + serviceName: 'other', + }), + createImage({ + appId: 1, + name: 'main-image-2', + serviceName: 'main', + }), + ], + }, + }; + + const current = createApp({ + services: [ + await createService({ + image: 'main-image', + appId: 1, + serviceName: 'main', + commit: 'old-release', + }), + await createService({ + image: 'other-image', + appId: 1, + serviceName: 'other', + commit: 'old-release', + }), + ], + networks: [defaultNetwork], + }); + const target = createApp({ + services: [ + await createService({ + image: 'main-image-2', + appId: 1, + serviceName: 'main', + commit: 'new-release', + }), + await createService({ + image: 'other-image-2', + appId: 1, + serviceName: 'other', + commit: 'new-release', + }), + ], + networks: [defaultNetwork], + isTarget: true, + }); + + // No kill steps should be generated + const steps = current.nextStepsForAppUpdate(contextWithImages, target); + expectNoStep('kill', steps); + }); + + it('should not infer a start step before all target images have been downloaded', async () => { + const contextWithImages = { + ...defaultContext, + ...{ + downloading: ['other-image'], // One of the images is being downloaded + availableImages: [ + createImage({ appId: 1, name: 'main-image', serviceName: 'main' }), + ], + }, + }; + + const current = createApp({ + services: [], + networks: [defaultNetwork], + }); + const target = createApp({ + services: [ + await createService({ + image: 'main-image', + appId: 1, + serviceName: 'main', + commit: 'new-release', + }), + await createService({ + image: 'other-image', + appId: 1, + serviceName: 'other', + commit: 'new-release', + }), + ], + networks: [defaultNetwork], + isTarget: true, + }); + + // No kill steps should be generated + const steps = current.nextStepsForAppUpdate(contextWithImages, target); + expectNoStep('start', steps); + }); + + 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: [defaultNetwork], + }); + const target = createApp({ + services: [ + await createService({ + image: 'main-image', + appId: 1, + serviceName: 'main', + commit: 'new-release', + }), + await createService({ + image: 'other-image', + appId: 1, + serviceName: 'other', + commit: 'new-release', + }), + ], + networks: [defaultNetwork], + isTarget: true, + }); + + // No kill steps should be generated + const steps = current.nextStepsForAppUpdate(contextWithImages, target); + expectSteps('start', steps, 2); + }); }); describe('image state behavior', () => {