From 9afb7ef22d65b51846f2bef8282e4c78bdd8ecaa Mon Sep 17 00:00:00 2001 From: Christina Ying Wang Date: Wed, 29 Jan 2025 11:42:12 +0000 Subject: [PATCH] Add Docker network label if custom ipam config In a target release where the only change is the addition or removal of a custom ipam config, the Supervisor does not recreate the network due to ignoring ipam config differences when comparing current and target network (in network.isEqualConfig). This commit implements the addition of a network label if the target compose object includes a network with custom ipam. With the label, the Supervisor will detect a difference between a network with a custom ipam and a network without, without needing to compare the ipam configs themselves. This is a major change, as devices running networks with custom ipam configs will have their networks recreated to add the network label. Closes: #2251 Change-type: major See: https://balena.fibery.io/Work/Project/Supervisor-maintenance-work-(Dec-2024)-948 Signed-off-by: Christina Ying Wang --- src/compose/network.ts | 9 + test/integration/compose/network.spec.ts | 2 + test/integration/state-engine.spec.ts | 232 ++++++++++++++++++----- test/unit/compose/network.spec.ts | 54 +++--- 4 files changed, 219 insertions(+), 78 deletions(-) diff --git a/src/compose/network.ts b/src/compose/network.ts index 6dca1693b..162798252 100644 --- a/src/compose/network.ts +++ b/src/compose/network.ts @@ -160,6 +160,15 @@ class NetworkImpl implements Network { configOnly: network.config_only || false, }; + // Add label if there's non-default ipam config + // e.g. explicitly defined subnet or gateway. + // When updating between a release where the ipam config + // changes, this label informs the Supervisor that + // there's an ipam diff that requires recreating the network. + if (net.config.ipam.config.length > 0) { + net.config.labels['io.balena.private.ipam.config'] = 'true'; + } + return net; } diff --git a/test/integration/compose/network.spec.ts b/test/integration/compose/network.spec.ts index 21ef7acfd..4340bc17a 100644 --- a/test/integration/compose/network.spec.ts +++ b/test/integration/compose/network.spec.ts @@ -67,6 +67,8 @@ describe('compose/network: integration tests', () => { Labels: { 'io.balena.supervised': 'true', 'io.balena.app-id': '12345', + // This label should be present as we've defined a custom ipam config + 'io.balena.private.ipam.config': 'true', }, Options: {}, ConfigOnly: false, diff --git a/test/integration/state-engine.spec.ts b/test/integration/state-engine.spec.ts index 831f39400..d596162ef 100644 --- a/test/integration/state-engine.spec.ts +++ b/test/integration/state-engine.spec.ts @@ -260,7 +260,33 @@ describe('state engine', () => { }); }); - it('updates an app with two services with a network change', async () => { + it('updates an app with two services with a network change where the only change is a custom ipam config addition', async () => { + const services = { + '1': { + image: 'alpine:latest', + imageId: 11, + serviceName: 'one', + restart: 'unless-stopped', + running: true, + command: 'sleep infinity', + stop_signal: 'SIGKILL', + networks: ['default'], + labels: {}, + environment: {}, + }, + '2': { + image: 'alpine:latest', + imageId: 12, + serviceName: 'two', + restart: 'unless-stopped', + running: true, + command: 'sleep infinity', + stop_signal: 'SIGKILL', + networks: ['default'], + labels: {}, + environment: {}, + }, + }; await setTargetState({ config: {}, apps: { @@ -268,30 +294,10 @@ describe('state engine', () => { name: 'test-app', commit: 'deadbeef', releaseId: 1, - services: { - '1': { - image: 'alpine:latest', - imageId: 11, - serviceName: 'one', - restart: 'unless-stopped', - running: true, - command: 'sleep infinity', - stop_signal: 'SIGKILL', - labels: {}, - environment: {}, - }, - '2': { - image: 'alpine:latest', - imageId: 12, - serviceName: 'two', - restart: 'unless-stopped', - running: true, - command: 'sleep infinity', - labels: {}, - environment: {}, - }, + services, + networks: { + default: {}, }, - networks: {}, volumes: {}, }, }, @@ -311,6 +317,21 @@ describe('state engine', () => { ]); const containerIds = containers.map(({ Id }) => Id); + // Network should not have custom ipam config + const defaultNet = await docker.getNetwork('123_default').inspect(); + expect(defaultNet) + .to.have.property('IPAM') + .to.not.deep.equal({ + Config: [{ Gateway: '192.168.91.1', Subnet: '192.168.91.0/24' }], + Driver: 'default', + Options: {}, + }); + + // Network should not have custom ipam label + expect(defaultNet) + .to.have.property('Labels') + .to.not.have.property('io.balena.private.ipam.config'); + await setTargetState({ config: {}, apps: { @@ -318,32 +339,87 @@ describe('state engine', () => { 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: 'alpine:latest', - imageId: 22, - serviceName: 'two', - restart: 'unless-stopped', - running: true, - command: 'sh -c "echo two && sleep infinity"', - stop_signal: 'SIGKILL', - networks: ['default'], - labels: {}, - environment: {}, + services, + networks: { + default: { + driver: 'bridge', + ipam: { + config: [ + { gateway: '192.168.91.1', subnet: '192.168.91.0/24' }, + ], + driver: 'default', + }, }, }, + volumes: {}, + }, + }, + }); + + const updatedContainers = await docker.listContainers(); + expect( + updatedContainers.map(({ Names, State }) => ({ Name: Names[0], State })), + ).to.have.deep.members([ + { Name: '/one_11_2_deadca1f', State: 'running' }, + { Name: '/two_12_2_deadca1f', State: 'running' }, + ]); + + // Container ids must have changed + expect(updatedContainers.map(({ Id }) => Id)).to.not.have.members( + containerIds, + ); + + // Network should have custom ipam config + const customNet = await docker.getNetwork('123_default').inspect(); + expect(customNet) + .to.have.property('IPAM') + .to.deep.equal({ + Config: [{ Gateway: '192.168.91.1', Subnet: '192.168.91.0/24' }], + Driver: 'default', + Options: {}, + }); + + // Network should have custom ipam label + expect(customNet) + .to.have.property('Labels') + .to.have.property('io.balena.private.ipam.config'); + }); + + it('updates an app with two services with a network change where the only change is a custom ipam config removal', async () => { + const services = { + '1': { + image: 'alpine:latest', + imageId: 11, + serviceName: 'one', + restart: 'unless-stopped', + running: true, + command: 'sleep infinity', + stop_signal: 'SIGKILL', + networks: ['default'], + labels: {}, + environment: {}, + }, + '2': { + image: 'alpine:latest', + imageId: 12, + serviceName: 'two', + restart: 'unless-stopped', + running: true, + command: 'sleep infinity', + stop_signal: 'SIGKILL', + networks: ['default'], + labels: {}, + environment: {}, + }, + }; + await setTargetState({ + config: {}, + apps: { + '123': { + name: 'test-app', + commit: 'deadbeef', + releaseId: 1, + services, networks: { default: { driver: 'bridge', @@ -360,12 +436,57 @@ describe('state engine', () => { }, }); + const state = await getCurrentState(); + expect( + state.apps['123'].services.map((s: any) => s.serviceName), + ).to.deep.equal(['one', 'two']); + + // Network should have custom ipam config + const customNet = await docker.getNetwork('123_default').inspect(); + expect(customNet) + .to.have.property('IPAM') + .to.deep.equal({ + Config: [{ Gateway: '192.168.91.1', Subnet: '192.168.91.0/24' }], + Driver: 'default', + Options: {}, + }); + + // Network should have custom ipam label + expect(customNet) + .to.have.property('Labels') + .to.have.property('io.balena.private.ipam.config'); + + 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 setTargetState({ + config: {}, + apps: { + '123': { + name: 'test-app', + commit: 'deadca1f', + releaseId: 2, + services, + networks: { + default: {}, + }, + 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' }, + { Name: '/one_11_2_deadca1f', State: 'running' }, + { Name: '/two_12_2_deadca1f', State: 'running' }, ]); // Container ids must have changed @@ -373,13 +494,20 @@ describe('state engine', () => { containerIds, ); - expect(await docker.getNetwork('123_default').inspect()) + // Network should not have custom ipam config + const defaultNet = await docker.getNetwork('123_default').inspect(); + expect(defaultNet) .to.have.property('IPAM') - .to.deep.equal({ + .to.not.deep.equal({ Config: [{ Gateway: '192.168.91.1', Subnet: '192.168.91.0/24' }], Driver: 'default', Options: {}, }); + + // Network should not have custom ipam label + expect(defaultNet) + .to.have.property('Labels') + .to.not.have.property('io.balena.private.ipam.config'); }); it('updates an app with two services with a network removal', async () => { diff --git a/test/unit/compose/network.spec.ts b/test/unit/compose/network.spec.ts index 1d945f8cb..882a3557d 100644 --- a/test/unit/compose/network.spec.ts +++ b/test/unit/compose/network.spec.ts @@ -183,6 +183,8 @@ describe('compose/network', () => { 'io.balena.supervised': 'true', 'io.balena.app-id': '12345', 'com.docker.some-label': 'yes', + // This label should be present as we've defined a custom ipam config + 'io.balena.private.ipam.config': 'true', }); expect(dockerConfig.Options).to.deep.equal({ @@ -344,12 +346,14 @@ describe('compose/network', () => { 'io.resin.features.something': '123', 'io.balena.features.dummy': 'abc', 'io.balena.supervised': 'true', + 'io.balena.private.ipam.config': 'true', } as NetworkInspectInfo['Labels'], } as NetworkInspectInfo); expect(network.config.labels).to.deep.equal({ 'io.balena.features.something': '123', 'io.balena.features.dummy': 'abc', + 'io.balena.private.ipam.config': 'true', }); }); }); @@ -425,34 +429,32 @@ describe('compose/network', () => { }); describe('comparing network configurations', () => { - it('ignores IPAM configuration', () => { - const network = Network.fromComposeObject('default', 12345, 'deadbeef', { - ipam: { - driver: 'default', - config: [ - { - subnet: '172.20.0.0/16', - ip_range: '172.20.10.0/24', - gateway: '172.20.0.1', - }, - ], - options: {}, + it('distinguishes a network with custom ipam config from a network without', () => { + const customIpam = Network.fromComposeObject( + 'default', + 12345, + 'deadbeef', + { + ipam: { + driver: 'default', + config: [ + { + subnet: '172.20.0.0/16', + gateway: '172.20.0.1', + }, + ], + options: {}, + }, }, - }); - expect( - network.isEqualConfig( - Network.fromComposeObject('default', 12345, 'deadbeef', {}), - ), - ).to.be.true; + ); + const noCustomIpam = Network.fromComposeObject( + 'default', + 12345, + 'deadbeef', + {}, + ); - // Only ignores ipam.config, not other ipam elements - expect( - network.isEqualConfig( - Network.fromComposeObject('default', 12345, 'deadbeef', { - ipam: { driver: 'aaa' }, - }), - ), - ).to.be.false; + expect(customIpam.isEqualConfig(noCustomIpam)).to.be.false; }); it('compares configurations recursively', () => {