diff --git a/src/compose/app.ts b/src/compose/app.ts index caa5bc28..d62fae3f 100644 --- a/src/compose/app.ts +++ b/src/compose/app.ts @@ -813,24 +813,28 @@ class AppImpl implements App { serviceName: target.serviceName, }), ]; - } else if ( - target != null && - this.dependenciesMetForServiceStart( - target, - targetApp, - availableImages, - networkPairs, - volumePairs, - servicePairs, - ) - ) { - if (!servicesLocked) { - this.services - .concat(targetApp.services) - .forEach((svc) => appsToLock[target.appId].add(svc.serviceName)); - return []; + } else if (target != null) { + if ( + this.dependenciesMetForServiceStart( + target, + targetApp, + availableImages, + networkPairs, + volumePairs, + servicePairs, + ) + ) { + if (!servicesLocked) { + this.services + .concat(targetApp.services) + .forEach((svc) => appsToLock[target.appId].add(svc.serviceName)); + return []; + } + return [generateStep('start', { target })]; + } else { + // Wait for dependencies to be started + return [generateStep('noop', {})]; } - return [generateStep('start', { target })]; } else { return []; } @@ -881,7 +885,7 @@ class AppImpl implements 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(targetApp.services, (svc) => { + const depInstallingButNotRunning = _.some(this.services, (svc) => { if (target.dependsOn?.includes(svc.serviceName)) { if (!svc.config.running) { return true; diff --git a/test/integration/compose/application-manager.spec.ts b/test/integration/compose/application-manager.spec.ts index 07d66f8b..f06a8632 100644 --- a/test/integration/compose/application-manager.spec.ts +++ b/test/integration/compose/application-manager.spec.ts @@ -821,9 +821,9 @@ describe('compose/application-manager', () => { containerIdsByAppId, }, ); - expectSteps('noop', steps2, 1); + // No other steps - expect(steps2).to.have.length(1); + expect(steps2.every((s) => s.action === 'noop')); /** * Only start target services after both images downloaded @@ -932,7 +932,7 @@ describe('compose/application-manager', () => { ); // Only noop steps should be seen at this point - expect(steps.filter((s) => s.action !== 'noop')).to.have.lengthOf(0); + expect(steps.every((s) => s.action === 'noop')); }); it('infers to kill several services as long as there is no unmet dependency', async () => { @@ -1099,7 +1099,7 @@ describe('compose/application-manager', () => { .that.deep.includes({ serviceName: 'dep' }); // No more steps until the first container has been started - expect(nextSteps).to.have.lengthOf(0); + expect(nextSteps.every((s) => s.action === 'noop')); }); it('infers to start a service once its dependency has been met', async () => { diff --git a/test/unit/compose/app.spec.ts b/test/unit/compose/app.spec.ts index fb0de210..e3718170 100644 --- a/test/unit/compose/app.spec.ts +++ b/test/unit/compose/app.spec.ts @@ -348,7 +348,6 @@ describe('compose/app', () => { target, ); - expect(recreateVolumeSteps).to.have.length(1); expectSteps('createVolume', recreateVolumeSteps); // Step 5: takeLock @@ -1294,22 +1293,23 @@ describe('compose/app', () => { .to.deep.include({ serviceName: 'main' }); }); - it('should not try to start a container which has exited and has restart policy of no', async () => { + it('should not try to start a container which has exited', async () => { // Container is a "run once" type of service so it has exitted. const current = createApp({ services: [ await createService( - { composition: { restart: 'no' }, running: false }, + { composition: { restart: 'yes' }, running: false }, { state: { containerId: 'run_once' } }, ), ], + networks: [DEFAULT_NETWORK], }); // Now test that another start step is not added on this service const target = createApp({ services: [ await createService( - { composition: { restart: 'no' }, running: false }, + { composition: { restart: 'always' }, running: false }, { state: { containerId: 'run_once' } }, ), ], @@ -1317,6 +1317,7 @@ describe('compose/app', () => { }); const steps = current.nextStepsForAppUpdate(defaultContext, target); + expect(steps.length).to.equal(0); expectNoStep('start', steps); }); @@ -1472,6 +1473,83 @@ describe('compose/app', () => { .that.deep.includes({ serviceName: 'main' }); }); + it('should not start a container when it depends on a service that is not running', async () => { + const current = createApp({ + services: [ + await createService( + { + running: false, + appId: 1, + serviceName: 'dep', + }, + { + state: { + containerId: 'dep-id', + }, + }, + ), + ], + networks: [DEFAULT_NETWORK], + }); + const target = createApp({ + services: [ + await createService({ + appId: 1, + serviceName: 'main', + composition: { + depends_on: ['dep'], + }, + }), + await createService({ + appId: 1, + serviceName: 'dep', + }), + ], + networks: [DEFAULT_NETWORK], + 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 }, + lock: mockLock, + }; + + // Only one start step and it should be that of the 'dep' service + const stepsToIntermediate = current.nextStepsForAppUpdate( + contextWithImages, + target, + ); + expectNoStep('start', stepsToIntermediate); + expectSteps('noop', stepsToIntermediate); + + // we now make our current state have the 'dep' service as started... + const intermediate = createApp({ + services: [ + await createService( + { appId: 1, serviceName: 'dep' }, + { state: { containerId: 'dep-id' } }, + ), + ], + networks: [DEFAULT_NETWORK], + }); + + // we should now see a start for the 'main' service... + const stepsToTarget = intermediate.nextStepsForAppUpdate( + { ...contextWithImages, ...{ containerIds: { dep: 'dep-id' } } }, + target, + ); + const [startMainStep] = expectSteps('start', stepsToTarget); + expect(startMainStep) + .to.have.property('target') + .that.deep.includes({ serviceName: 'main' }); + }); + it('should not create a start step when all that changes is a running state', async () => { const contextWithImages = { ...defaultContext, @@ -1993,7 +2071,7 @@ describe('compose/app', () => { target, ); expectNoStep('start', steps); - expectSteps('noop', steps, 1); + expectSteps('noop', steps, 1, Infinity); // Take lock before starting once downloads complete const steps2 = current.nextStepsForAppUpdate(