diff --git a/src/compose/app.ts b/src/compose/app.ts index d04c21e0..bcf406f1 100644 --- a/src/compose/app.ts +++ b/src/compose/app.ts @@ -379,9 +379,9 @@ class AppImpl implements App { ); } } - const toBeRemoved = _(currentServiceNames) + + const maybeRemove = _(currentServiceNames) .difference(targetServiceNames) - .union(dependentServices.map((s) => s.serviceName)) .map((id) => ({ current: currentByServiceName[id] })) .value(); @@ -409,7 +409,7 @@ class AppImpl implements App { currentByServiceName[serviceName], ); for (const service of otherContainers) { - toBeRemoved.push({ current: service }); + maybeRemove.push({ current: service }); } } else { currentByServiceName[serviceName] = currentServiceContainers[0]; @@ -489,6 +489,33 @@ class AppImpl implements App { ); }; + /** + * Services with changing configuration will have their containers recreated + */ + const toBeRecreated = maybeUpdate + .map((serviceName) => ({ + current: currentByServiceName[serviceName], + target: targetByServiceName[serviceName], + })) + .filter( + ({ current: c, target: t }) => !isEqualExceptForRunningState(c, t), + ); + + /** + * Services that depend on network changes should be added to the removal + * list if they are not being recreated already + */ + const toBeRemoved = maybeRemove.concat( + dependentServices + .filter( + (s) => + !toBeRecreated.some( + ({ current: c }) => c.serviceName === s.serviceName, + ), + ) + .map((s) => ({ current: s })), + ); + /** * Checks if a service is destined for removal due to a network or volume change */ diff --git a/test/integration/.mocharc.js b/test/integration/.mocharc.js index 683453ed..0e8f6ffc 100644 --- a/test/integration/.mocharc.js +++ b/test/integration/.mocharc.js @@ -10,5 +10,5 @@ module.exports = { 'test/lib/chai.ts', 'test/lib/mocha-hooks.ts', ], - timeout: '30000', + timeout: '60000', }; diff --git a/test/integration/state-engine.spec.ts b/test/integration/state-engine.spec.ts index a4a795f9..3d5b5af2 100644 --- a/test/integration/state-engine.spec.ts +++ b/test/integration/state-engine.spec.ts @@ -380,4 +380,115 @@ describe('state engine', () => { Options: {}, }); }); + + it('updates an app with two services with a network removal', async () => { + await setTargetState({ + config: {}, + apps: { + '123': { + name: 'test-app', + commit: 'deadbeef', + releaseId: 1, + services: { + '1': { + image: 'alpine:3.18', + imageId: 11, + serviceName: 'one', + restart: 'unless-stopped', + running: true, + command: 'sleep infinity', + stop_signal: 'SIGKILL', + labels: {}, + environment: {}, + networks: ['balena'], + }, + '2': { + image: 'ubuntu:focal', + imageId: 12, + serviceName: 'two', + restart: 'unless-stopped', + running: true, + command: 'sleep infinity', + labels: {}, + environment: {}, + network_mode: 'host', + }, + }, + networks: { + balena: {}, + }, + volumes: {}, + }, + }, + }); + + const state = await getCurrentState(); + expect( + state.apps['123'].services.map((s: any) => s.serviceName), + ).to.deep.equal(['one', 'two']); + + const containers = await docker.listContainers(); + expect( + containers.map(({ Names, State }) => ({ Name: Names[0], State })), + ).to.have.deep.members([ + { Name: '/one_11_1_deadbeef', State: 'running' }, + { Name: '/two_12_1_deadbeef', State: 'running' }, + ]); + const containerIds = containers.map(({ Id }) => Id); + await expect(docker.getNetwork('123_balena').inspect()).to.not.be.rejected; + + await setTargetState({ + config: {}, + apps: { + '123': { + name: 'test-app', + commit: 'deadca1f', + releaseId: 2, + services: { + '1': { + image: 'alpine:latest', + imageId: 21, + serviceName: 'one', + restart: 'unless-stopped', + running: true, + command: 'sleep infinity', + stop_signal: 'SIGKILL', + networks: ['default'], + labels: {}, + environment: {}, + }, + '2': { + image: 'ubuntu:latest', + imageId: 22, + serviceName: 'two', + restart: 'unless-stopped', + running: true, + command: 'sh -c "echo two && sleep infinity"', + stop_signal: 'SIGKILL', + network_mode: 'host', + labels: {}, + environment: {}, + }, + }, + networks: {}, + volumes: {}, + }, + }, + }); + + const updatedContainers = await docker.listContainers(); + expect( + updatedContainers.map(({ Names, State }) => ({ Name: Names[0], State })), + ).to.have.deep.members([ + { Name: '/one_21_2_deadca1f', State: 'running' }, + { Name: '/two_22_2_deadca1f', State: 'running' }, + ]); + + // Container ids must have changed + expect(updatedContainers.map(({ Id }) => Id)).to.not.have.members( + containerIds, + ); + + await expect(docker.getNetwork('123_balena').inspect()).to.be.rejected; + }); }); diff --git a/test/unit/compose/app.spec.ts b/test/unit/compose/app.spec.ts index fdb5e2c6..38397249 100644 --- a/test/unit/compose/app.spec.ts +++ b/test/unit/compose/app.spec.ts @@ -692,6 +692,77 @@ describe('compose/app', () => { expectNoStep('removeNetwork', steps2); }); + it('should perform fetch steps first even for services that reference deleted networks', async () => { + const current = createApp({ + appUuid: 'deadbeef', + services: [ + await createService({ + appId: 1, + appUuid: 'deadbeef', + image: 'test-image', + serviceName: 'test', + composition: { networks: ['test-network'] }, + }), + await createService({ + appId: 1, + appUuid: 'deadbeef', + image: 'other-image', + serviceName: 'other', + composition: { network_mode: 'host' }, + }), + ], + networks: [ + Network.fromComposeObject('test-network', 1, 'deadbeef', {}), + DEFAULT_NETWORK, + ], + }); + const target = createApp({ + appUuid: 'deadbeef', + services: [ + await createService({ + appId: 1, + appUuid: 'deadbeef', + serviceName: 'test', + image: 'new-test-image', + commit: 'new-commit', + }), + await createService({ + appId: 1, + appUuid: 'deadbeef', + serviceName: 'other', + image: 'new-other-image', + commit: 'new-commit', + composition: { network_mode: 'host' }, + }), + ], + networks: [], + isTarget: true, + }); + + const availableImages = [ + createImage({ + appUuid: 'deadbeef', + name: 'test-image', + serviceName: 'test', + }), + createImage({ + appUuid: 'deadbeef', + name: 'other-image', + serviceName: 'other', + }), + ]; + // Take lock first + const steps = current.nextStepsForAppUpdate( + { ...defaultContext, availableImages }, + target, + ); + + const fetchSteps = expectSteps('fetch', steps, 2); + expect(fetchSteps.map((s) => (s as any).image.name)).to.have.deep.members( + ['new-test-image', 'new-other-image'], + ); + }); + it('should kill dependencies of networks before changing config', async () => { const current = createApp({ services: [