From e8af22633b2a3e8babc6163e1fd2609f8eb736b1 Mon Sep 17 00:00:00 2001 From: Edward Hope-Morley Date: Tue, 17 Jul 2018 12:41:42 +0100 Subject: [PATCH] Fix charm upgrade broken by commit 862c362 Currently, upgrading this charm on a host that is running ovs >= 2.6 will break because the OVS_DEFAULT config file is not expected to be written by the charm. Change-Id: I33352deb3b60231347045d5f39f3508a29dda61e --- hooks/neutron_ovs_hooks.py | 34 ++++++++++++++++------------ hooks/neutron_ovs_utils.py | 3 ++- unit_tests/test_neutron_ovs_hooks.py | 25 +++++++++++++------- unit_tests/test_neutron_ovs_utils.py | 29 ++++++++++++------------ 4 files changed, 52 insertions(+), 39 deletions(-) diff --git a/hooks/neutron_ovs_hooks.py b/hooks/neutron_ovs_hooks.py index d2206730..eca523bc 100755 --- a/hooks/neutron_ovs_hooks.py +++ b/hooks/neutron_ovs_hooks.py @@ -62,21 +62,25 @@ def install(): # for the implications of modifications to the /etc/default/openvswitch-switch. @hooks.hook('upgrade-charm') def upgrade_charm(): - # In the 16.10 release of the charms, the code changed from managing the - # /etc/default/openvswitch-switch file only when dpdk was enabled to always - # managing this file. Thus, an upgrade of the charm from a release prior - # to 16.10 or higher will always cause the contents of the file to change - # and will trigger a restart of the openvswitch-switch service, which in - # turn causes a temporary network outage. To prevent this outage, determine - # if the /etc/default/openvswitch-switch file needs to be migrated and if - # so, migrate the file but do NOT restart the openvswitch-switch service. - # See bug LP #1712444 - with open(OVS_DEFAULT, 'r') as f: - # The 'Service restart triggered ...' line was added to the OVS_DEFAULT - # template in the 16.10 version of the charm to allow restarts so we - # use this as the key to see if the file needs migrating. - if 'Service restart triggered' not in f.read(): - CONFIGS.write(OVS_DEFAULT) + if OVS_DEFAULT in restart_map(): + # In the 16.10 release of the charms, the code changed from managing + # the /etc/default/openvswitch-switch file only when dpdk was enabled + # to always managing this file. Thus, an upgrade of the charm from a + # release prior to 16.10 or higher will always cause the contents of + # the file to change and will trigger a restart of the + # openvswitch-switch service, which in turn causes a temporary + # network outage. To prevent this outage, determine if the + # /etc/default/openvswitch-switch file needs to be migrated and if + # so, migrate the file but do NOT restart the openvswitch-switch + # service. + # See bug LP #1712444 + with open(OVS_DEFAULT, 'r') as f: + # The 'Service restart triggered ...' line was added to the + # OVS_DEFAULT template in the 16.10 version of the charm to allow + # restarts so we use this as the key to see if the file needs + # migrating. + if 'Service restart triggered' not in f.read(): + CONFIGS.write(OVS_DEFAULT) @hooks.hook('neutron-plugin-relation-changed') diff --git a/hooks/neutron_ovs_utils.py b/hooks/neutron_ovs_utils.py index 163ab584..ec165789 100644 --- a/hooks/neutron_ovs_utils.py +++ b/hooks/neutron_ovs_utils.py @@ -309,7 +309,8 @@ def resource_map(): ) if not use_dpdk(): drop_config.append(DPDK_INTERFACES) - if ovs_has_late_dpdk_init(): + drop_config.append(OVS_DEFAULT) + elif ovs_has_late_dpdk_init(): drop_config.append(OVS_DEFAULT) else: drop_config.extend([OVS_CONF, DPDK_INTERFACES]) diff --git a/unit_tests/test_neutron_ovs_hooks.py b/unit_tests/test_neutron_ovs_hooks.py index 5e82b173..c278c4cd 100644 --- a/unit_tests/test_neutron_ovs_hooks.py +++ b/unit_tests/test_neutron_ovs_hooks.py @@ -67,8 +67,9 @@ class NeutronOVSHooksTests(CharmTestCase): self._call_hook('install') self.install_packages.assert_called_with() + @patch.object(hooks, 'restart_map') @patch.object(hooks, 'restart_on_change') - def test_migrate_ovs_default_file(self, mock_restart): + def test_migrate_ovs_default_file(self, mock_restart, mock_restart_map): # Tests that the /etc/default/openvswitch-switch file is/isn't # migrated on the upgrade-charm hook and that no restarts are # attempted of the openvswitch-switch service. @@ -82,14 +83,22 @@ class NeutronOVSHooksTests(CharmTestCase): with open('unit_tests/%s' % sample, 'r') as f: content = f.read() - with patch('builtins.open', mock_open(read_data=content), - create=True): - self._call_hook('upgrade-charm') - if should_migrate: - self.CONFIGS.write.assert_called_with(utils.OVS_DEFAULT) + for ovs_default_exists in [False, True]: + if ovs_default_exists: + mock_restart_map.return_value = {utils.OVS_DEFAULT: {}} else: - self.CONFIGS.write.assert_not_called() - self.assertEqual(0, mock_restart.call_count) + mock_restart_map.return_value = {} + + with patch('builtins.open', mock_open(read_data=content), + create=True): + self.CONFIGS.write.reset_mock() + self._call_hook('upgrade-charm') + if should_migrate and ovs_default_exists: + self.CONFIGS.write.assert_called_with( + utils.OVS_DEFAULT) + else: + self.CONFIGS.write.assert_not_called() + self.assertEqual(0, mock_restart.call_count) def test_config_changed_dvr(self): self._call_hook('config-changed') diff --git a/unit_tests/test_neutron_ovs_utils.py b/unit_tests/test_neutron_ovs_utils.py index 35a6dde7..8319270b 100644 --- a/unit_tests/test_neutron_ovs_utils.py +++ b/unit_tests/test_neutron_ovs_utils.py @@ -256,7 +256,6 @@ class TestNeutronOVSUtils(CharmTestCase): _regconfs = nutils.register_configs() confs = ['/etc/neutron/neutron.conf', '/etc/neutron/plugins/ml2/openvswitch_agent.ini', - '/etc/default/openvswitch-switch', '/etc/init/os-charm-phy-nic-mtu.conf'] self.assertEqual(_regconfs.configs, confs) @@ -331,11 +330,11 @@ class TestNeutronOVSUtils(CharmTestCase): def test_resource_map_dhcp(self, _use_dvr, _enable_local_dhcp): _enable_local_dhcp.return_value = True _use_dvr.return_value = False - self.os_release.return_value = 'diablo' - self.lsb_release.return_value = {'DISTRIB_CODENAME': 'lucid'} + self.os_release.return_value = 'mitaka' + self.lsb_release.return_value = {'DISTRIB_CODENAME': 'xenial'} _map = nutils.resource_map() - svcs = ['neutron-plugin-openvswitch-agent', 'neutron-metadata-agent', - 'neutron-dhcp-agent'] + svcs = ['neutron-metadata-agent', 'neutron-dhcp-agent', + 'neutron-openvswitch-agent'] confs = [nutils.NEUTRON_CONF, nutils.NEUTRON_METADATA_AGENT_CONF, nutils.NEUTRON_DHCP_AGENT_CONF] [self.assertIn(q_conf, _map.keys()) for q_conf in confs] @@ -367,20 +366,20 @@ class TestNeutronOVSUtils(CharmTestCase): _map = nutils.resource_map() self.assertFalse(nutils.EXT_PORT_CONF in _map.keys()) + @patch.object(nutils, 'use_dpdk') @patch.object(nutils, 'use_dvr') - def test_restart_map(self, _use_dvr): - _use_dvr.return_value = False - self.os_release.return_value = "diablo" - self.lsb_release.return_value = {'DISTRIB_CODENAME': 'lucid'} + def test_restart_map(self, mock_use_dvr, mock_use_dpdk): + mock_use_dvr.return_value = False + mock_use_dpdk.return_value = False + self.os_release.return_value = "mitaka" + self.lsb_release.return_value = {'DISTRIB_CODENAME': 'xenial'} + ML2CONF = "/etc/neutron/plugins/ml2/openvswitch_agent.ini" _restart_map = nutils.restart_map() - ML2CONF = "/etc/neutron/plugins/ml2/ml2_conf.ini" expect = OrderedDict([ - (nutils.NEUTRON_CONF, ['neutron-plugin-openvswitch-agent']), - (ML2CONF, ['neutron-plugin-openvswitch-agent']), - (nutils.OVS_DEFAULT, ['openvswitch-switch']), - (nutils.PHY_NIC_MTU_CONF, ['os-charm-phy-nic-mtu']) + (ML2CONF, ['neutron-openvswitch-agent']), + (nutils.NEUTRON_CONF, ['neutron-openvswitch-agent']), ]) - self.assertEqual(expect, _restart_map) + self.assertEqual(expect, OrderedDict(_restart_map)) for item in _restart_map: self.assertTrue(item in _restart_map) self.assertTrue(expect[item] == _restart_map[item])