From bbc621edca348c2d0a9061bffb6d65f823b236c1 Mon Sep 17 00:00:00 2001 From: Aurelien Lourot Date: Mon, 30 Mar 2020 13:19:07 +0200 Subject: [PATCH] Mark OVS bridges and ports as managed by charm-neutron-gateway This patchset updates the configure_ovs() function in hooks/neutron_utils.py such that ports and bridges in OVS are marked as being managed by this charm. This will allow us to clean up obsolete managed bridges and ports in a later patchset. (On configuration change new ports and bridges might be created and former ones might become obsolete.) This patchset also fully deprecates the 'ext-port' config option such that if both 'data-port' and 'ext-port' config options are set, the unit is blocked. The README and config.yaml are updated to reflect this change. This patchset also fixes and removes a few dead links. Relies on a charm-helpers version containing these patchsets: https://github.com/juju/charm-helpers/pull/443 https://github.com/juju/charm-helpers/pull/447 https://github.com/juju/charm-helpers/pull/449 Related documentation: * Deployment guide / Upgrades / Known issues: https://review.opendev.org/630290 * Release notes: https://review.opendev.org/742660 Change-Id: I8b459135d131e16865de40ff3eae16ea3bc7195e Partial-Bug: #1809190 --- README.md | 12 +-- config.yaml | 4 + hooks/neutron_utils.py | 143 ++++++++++++++++++++++++++++--- tests/tests.yaml | 1 + unit_tests/test_neutron_utils.py | 105 ++++++++++++++++++----- 5 files changed, 223 insertions(+), 42 deletions(-) diff --git a/README.md b/README.md index a7b4309d..eba2c678 100644 --- a/README.md +++ b/README.md @@ -10,7 +10,7 @@ instances plug into. Neutron supports a rich plugin/extension framework for propriety networking solutions and supports (in core) Nicira NVP, NEC, Cisco and others... -See the upstream [Neutron documentation](http://docs.openstack.org/trunk/openstack-network/admin/content/use_cases_single_router.html) +See the upstream [Neutron documentation](https://docs.openstack.org/neutron/latest/) for more details. Usage @@ -44,8 +44,6 @@ The gateway provides two key services; L3 network routing and DHCP services. These are both required in a fully functional Neutron OpenStack deployment. -See upstream [Neutron multi extnet](http://docs.openstack.org/trunk/config-reference/content/adv_cfg_l3_agent_multi_extnet.html) - Configuration Options --------------------- @@ -109,6 +107,11 @@ This replaces the previous system of using ext-port, which always created a brid called br-ex for external networks which was used implicitly by external router interfaces. +Note: if the 'data-port' config item is set, then the 'ext-port' option is +ignored. This is to prevent misconfiguration of the charm. A warning is +logged and the unit is marked as blocked in order to indicate that the charm is +misconfigured. + Instance MTU ============ @@ -119,7 +122,4 @@ charm's instance-mtu option can be used to reduce instance MTU via DHCP. juju set neutron-gateway instance-mtu=1400 -OpenStack upstream documentation recommends a MTU value of 1400: -[OpenStack documentation](http://docs.openstack.org/admin-guide-cloud/content/openvswitch_plugin.html) - Note that this option was added in Havana and will be ignored in older releases. diff --git a/config.yaml b/config.yaml index d49f7006..d79ec5db 100644 --- a/config.yaml +++ b/config.yaml @@ -86,6 +86,10 @@ options: traffic to the external public network. Valid values are either MAC addresses (in which case only MAC addresses for interfaces without an IP address already assigned will be used), or interfaces (eth0) + . + Note that if data-port is used then this config item is ignored, a + warning is logged, and the unit is marked as blocked in order to indicate + that the charm is misconfigured. data-port: type: string default: diff --git a/hooks/neutron_utils.py b/hooks/neutron_utils.py index 6ccd4ac0..63d2cf27 100644 --- a/hooks/neutron_utils.py +++ b/hooks/neutron_utils.py @@ -16,6 +16,7 @@ from charmhelpers.core.hookenv import ( log, DEBUG, INFO, + WARNING, ERROR, config, is_relation_made, @@ -34,25 +35,27 @@ from charmhelpers.fetch import ( from charmhelpers.contrib.network.ovs import ( add_bridge, add_bridge_port, - is_linuxbridge_interface, add_ovsbridge_linuxbridge, - full_restart, enable_ipfix, disable_ipfix, + full_restart, + get_bridges_and_ports_map, + is_linuxbridge_interface, ) from charmhelpers.contrib.hahelpers.cluster import ( get_hacluster_config, ) from charmhelpers.contrib.openstack.utils import ( + CompareOpenStackReleases, configure_installation_source, get_os_codename_install_source, make_assess_status_func, + os_application_version_set, os_release, pause_unit, reset_os_release, resume_unit, - os_application_version_set, - CompareOpenStackReleases, + workload_state_compare, ) from charmhelpers.contrib.openstack.neutron import ( @@ -841,28 +844,70 @@ def do_openstack_upgrade(configs): def configure_ovs(): + """Configure the OVS plugin. + + This function uses the config.yaml parameters ext-port, data-port and + bridge-mappings to configure the bridges and ports on the ovs on the + unit. + + Note that the ext-port is deprecated and data-port/bridge-mappings are + preferred. + + Thus, if data-port is set, then ext-port is ignored (and if set, then + it is removed from the set of bridges unless it is defined in + bridge-mappings/data-port). A warning is issued, if both data-port and + ext-port are set. + """ if config('plugin') in [OVS, OVS_ODL]: if not service_running('openvswitch-switch'): full_restart() - add_bridge(INT_BRIDGE) - add_bridge(EXT_BRIDGE) - ext_port_ctx = ExternalPortContext()() - if ext_port_ctx and ext_port_ctx['ext_port']: - add_bridge_port(EXT_BRIDGE, ext_port_ctx['ext_port']) + # Get existing set of bridges and ports + current_bridges_and_ports = get_bridges_and_ports_map() + log("configure OVS: Current bridges and ports map: {}" + .format(", ".join("{}: {}".format(b, ",".join(v)) + for b, v in current_bridges_and_ports.items()))) + add_bridge(INT_BRIDGE, brdata=_ovs_additional_data()) + add_bridge(EXT_BRIDGE, brdata=_ovs_additional_data()) + + ext_port_ctx = ExternalPortContext()() portmaps = DataPortContext()() bridgemaps = parse_bridge_mappings(config('bridge-mappings')) + + # if we have portmaps, then we ignore its value and log an + # error/warning to the unit's log. + if config('data-port') and config('ext-port'): + log("Both ext-port and data-port are set. ext-port is deprecated" + " and is not used when data-port is set", level=ERROR) + + # only use ext-port if data-port is not set + if not portmaps and ext_port_ctx and ext_port_ctx['ext_port']: + _port = ext_port_ctx['ext_port'] + add_bridge_port(EXT_BRIDGE, _port, + ifdata=_ovs_additional_data(EXT_BRIDGE), + portdata=_ovs_additional_data(EXT_BRIDGE)) + log("DEPRECATION: using ext-port to set the port {} on the " + "EXT_BRIDGE ({}) is deprecated. Please use data-port instead." + .format(_port, EXT_BRIDGE), + level=WARNING) + for br in bridgemaps.values(): - add_bridge(br) + add_bridge(br, brdata=_ovs_additional_data()) if not portmaps: continue for port, _br in portmaps.items(): if _br == br: if not is_linuxbridge_interface(port): - add_bridge_port(br, port, promisc=True) + add_bridge_port(br, port, promisc=True, + ifdata=_ovs_additional_data(br), + portdata=_ovs_additional_data(br)) else: - add_ovsbridge_linuxbridge(br, port) + # NOTE(lourot): this will raise on focal+ and/or if the + # system has no `ifup`. See lp:1877594 + add_ovsbridge_linuxbridge( + br, port, ifdata=_ovs_additional_data(br), + portdata=_ovs_additional_data(br)) target = config('ipfix-target') bridges = [INT_BRIDGE, EXT_BRIDGE] @@ -878,11 +923,42 @@ def configure_ovs(): for bridge in bridges: disable_ipfix(bridge) + new_bridges_and_ports = get_bridges_and_ports_map() + log("configure OVS: Final bridges and ports map: {}" + .format(", ".join("{}: {}".format(b, ",".join(v)) + for b, v in new_bridges_and_ports.items())), + level=DEBUG) + # Ensure this runs so that mtu is applied to data-port interfaces if # provided. service_restart('os-charm-phy-nic-mtu') +def _ovs_additional_data(external_id_value=None): + """Returns OVS additional data from the given external-id value. + + The returned value is in the input format expected by the + charmhelpers.contrib.network.ovs functions. + + We set an external-id as OVS additional data on all the bridges and ports + that we create in order to mark them as managed by us. See + http://docs.openvswitch.org/en/latest/topics/integration/ + + :param external_id_value: Value to be set as external ID. For a bridge, + typically 'managed' (which is also the default if nothing is passed). + For a port, typically the name of the corresponding bridge. + :type external_id_value: str + :rtype: Dict[str,Dict[str,str]] + """ + external_id_value = ('managed' if external_id_value is None + else external_id_value) + return { + 'external-ids': { + 'charm-neutron-gateway': external_id_value + } + } + + def copy_file(src, dst, perms=None, force=False): """Copy file to destination and optionally set permissionss. @@ -1066,6 +1142,45 @@ def check_optional_relations(configs): return validate_ovs_use_veth() +def check_ext_port_data_port_config(configs): + """Checks that if data-port is set (other than None) then if ext-port is + also set, add a warning to the status line. + + :param configs: an OSConfigRender() instance. + :type configs: OSConfigRender + :returns: (status, message) + :rtype: (str, str) + """ + if config('data-port') and config('ext-port'): + return ("blocked", "ext-port set when data-port set: see config.yaml") + # return 'unknown' as the lowest priority to not clobber an existing + # status. + return 'unknown', '' + + +def sequence_functions(*functions): + """Sequence the functions passed so that they all get a chance to run as + the check_charm_func for the assess_status. + + :param *functions: a list of functions that return (state, message) + :type *functions: List[Callable[[OSConfigRender], (str, str)]] + :returns: the Callable that takes configs and returns (state, message) + :rtype: Callable[[OSConfigRender], (str, str)] + """ + def _inner_sequenced_functions(configs): + state, message = 'unknown', '' + for f in functions: + new_state, new_message = f(configs) + state = workload_state_compare(state, new_state) + if message: + message = "{}, {}".format(message, new_message) + else: + message = new_message + return state, message + + return _inner_sequenced_functions + + def assess_status(configs): """Assess status of current unit Decides what the state of the unit should be based on the current @@ -1101,9 +1216,11 @@ def assess_status_func(configs): required_interfaces = REQUIRED_INTERFACES.copy() required_interfaces.update(get_optional_interfaces()) active_services = [s for s in services() if s not in STOPPED_SERVICES] + charm_func = sequence_functions(check_optional_relations, + check_ext_port_data_port_config) return make_assess_status_func( configs, required_interfaces, - charm_func=check_optional_relations, + charm_func=charm_func, services=active_services, ports=None) diff --git a/tests/tests.yaml b/tests/tests.yaml index 9961a725..f0cbc65a 100644 --- a/tests/tests.yaml +++ b/tests/tests.yaml @@ -20,6 +20,7 @@ tests: - zaza.openstack.charm_tests.neutron.tests.NeutronGatewayTest - zaza.openstack.charm_tests.neutron.tests.SecurityTest - zaza.openstack.charm_tests.neutron.tests.NeutronNetworkingTest + - zaza.openstack.charm_tests.neutron.tests.NeutronOvsVsctlTest configure: - zaza.openstack.charm_tests.glance.setup.add_lts_image - zaza.openstack.charm_tests.neutron.setup.basic_overcloud_network diff --git a/unit_tests/test_neutron_utils.py b/unit_tests/test_neutron_utils.py index 57b82263..92351a86 100644 --- a/unit_tests/test_neutron_utils.py +++ b/unit_tests/test_neutron_utils.py @@ -25,6 +25,7 @@ TO_PATCH = [ 'add_bridge', 'add_bridge_port', 'add_ovsbridge_linuxbridge', + 'get_bridges_and_ports_map', 'is_linuxbridge_interface', 'headers_package', 'full_restart', @@ -297,11 +298,13 @@ class TestNeutronUtils(CharmTestCase): DummyExternalPortContext(return_value={'ext_port': 'eth0'}) neutron_utils.configure_ovs() self.add_bridge.assert_has_calls([ - call('br-int'), - call('br-ex'), - call('br-data') + call('br-int', brdata=ANY), + call('br-ex', brdata=ANY), + call('br-data', brdata=ANY) ]) - self.add_bridge_port.assert_called_with('br-ex', 'eth0') + self.add_bridge_port.assert_called_with( + 'br-ex', 'eth0', ifdata=ANY, portdata=ANY + ) @patch('charmhelpers.contrib.openstack.context.list_nics', return_value=['eth0', 'eth0.100', 'eth0.200']) @@ -318,11 +321,12 @@ class TestNeutronUtils(CharmTestCase): self.test_config.set('data-port', 'eth0') neutron_utils.configure_ovs() self.add_bridge.assert_has_calls([ - call('br-int'), - call('br-ex'), - call('br-data') + call('br-int', brdata=ANY), + call('br-ex', brdata=ANY), + call('br-data', brdata=ANY) ]) - calls = [call('br-data', 'eth0', promisc=True)] + calls = [call('br-data', 'eth0', promisc=True, ifdata=ANY, + portdata=ANY)] self.add_bridge_port.assert_has_calls(calls) # Now test with bridge:port format and bogus bridge @@ -331,9 +335,9 @@ class TestNeutronUtils(CharmTestCase): self.add_bridge_port.reset_mock() neutron_utils.configure_ovs() self.add_bridge.assert_has_calls([ - call('br-int'), - call('br-ex'), - call('br-data') + call('br-int', brdata=ANY), + call('br-ex', brdata=ANY), + call('br-data', brdata=ANY) ]) # Not called since we have a bogus bridge in data-ports self.assertFalse(self.add_bridge_port.called) @@ -345,12 +349,13 @@ class TestNeutronUtils(CharmTestCase): self.add_bridge_port.reset_mock() neutron_utils.configure_ovs() self.add_bridge.assert_has_calls([ - call('br-int'), - call('br-ex'), - call('br1') + call('br-int', brdata=ANY), + call('br-ex', brdata=ANY), + call('br1', brdata=ANY) ]) - calls = [call('br1', 'eth0.100', promisc=True), - call('br1', 'eth0.200', promisc=True)] + calls = [ + call('br1', 'eth0.100', promisc=True, ifdata=ANY, portdata=ANY), + call('br1', 'eth0.200', promisc=True, ifdata=ANY, portdata=ANY)] self.add_bridge_port.assert_has_calls(calls, any_order=True) @patch('charmhelpers.contrib.openstack.context.list_nics', @@ -367,12 +372,23 @@ class TestNeutronUtils(CharmTestCase): # assumed) self.test_config.set('data-port', 'br-eth0') neutron_utils.configure_ovs() + + # Also check that new bridges and ports are marked as managed by us: + expected_brdata = { + 'external-ids': {'charm-neutron-gateway': 'managed'}, + } + expected_ifdata = { + 'external-ids': {'charm-neutron-gateway': 'br-data'}, + } + expected_portdata = expected_ifdata + self.add_bridge.assert_has_calls([ - call('br-int'), - call('br-ex'), - call('br-data') + call('br-int', brdata=expected_brdata), + call('br-ex', brdata=expected_brdata), + call('br-data', brdata=expected_brdata) ]) - calls = [call('br-data', 'br-eth0')] + calls = [call('br-data', 'br-eth0', ifdata=expected_ifdata, + portdata=expected_portdata)] self.add_ovsbridge_linuxbridge.assert_has_calls(calls) @patch('charmhelpers.contrib.openstack.context.config') @@ -388,6 +404,36 @@ class TestNeutronUtils(CharmTestCase): call('br-data', '127.0.0.1:80'), ]) + @patch.object(neutron_utils, 'DataPortContext') + @patch('charmhelpers.contrib.openstack.context.config') + def test_configure_ovs_ensure_ext_port_overriden( + self, mock_config, mock_data_port_context): + mock_config.side_effect = self.test_config.get + self.config.side_effect = self.test_config.get + self.test_config.set('plugin', 'ovs') + self.get_bridges_and_ports_map.return_value = { + 'br-data': ['p4', 'p5'], + 'br1': ['p6'], + } + self.test_config.set( + 'data-port', + 'br-data:p4 br-data:p5 br1:p6') + self.test_config.set('bridge-mappings', 'net0:br-data net1:br1') + self.test_config.set('ext-port', None) + self.ExternalPortContext.return_value = \ + DummyExternalPortContext(return_value={'ext_port': 'eth0'}) + mock_data_port_context.return_value = \ + DummyDataPortContext(return_value={ + 'p4': 'br-data', + 'p5': 'br-data', + 'p6': 'br1', + }) + self.is_linuxbridge_interface.return_value = False + neutron_utils.configure_ovs() + # Ensure that ext-port was ignored. + self.assertNotIn(call('br-ex', 'eth0', ifdata=ANY, portdata=ANY), + self.add_bridge_port.call_args_list) + @patch.object(neutron_utils, 'register_configs') @patch('charmhelpers.contrib.openstack.templating.OSConfigRenderer') def test_do_openstack_upgrade(self, mock_renderer, @@ -1006,6 +1052,15 @@ class DummyExternalPortContext(): return self.return_value +class DummyDataPortContext(): + + def __init__(self, return_value): + self.return_value = return_value + + def __call__(self): + return self.return_value + + cluster1 = ['cluster1-machine1.internal'] cluster2 = ['cluster2-machine1.internal', 'cluster2-machine2.internal' 'cluster2-machine3.internal'] @@ -1035,7 +1090,7 @@ class TestNeutronAgentReallocation(CharmTestCase): ) @patch.object(neutron_utils, 'get_optional_interfaces') - @patch.object(neutron_utils, 'check_optional_relations') + @patch.object(neutron_utils, 'sequence_functions') @patch.object(neutron_utils, 'REQUIRED_INTERFACES') @patch.object(neutron_utils, 'services') @patch.object(neutron_utils, 'make_assess_status_func') @@ -1043,17 +1098,21 @@ class TestNeutronAgentReallocation(CharmTestCase): make_assess_status_func, services, REQUIRED_INTERFACES, - check_optional_relations, + sequence_functions, get_optional_interfaces): services.return_value = ['s1'] REQUIRED_INTERFACES.copy.return_value = {'int': ['test 1']} get_optional_interfaces.return_value = {'opt': ['test 2']} + sequence_functions.return_value = 'sequence_return' neutron_utils.assess_status_func('test-config') # ports=None whilst port checks are disabled. make_assess_status_func.assert_called_once_with( 'test-config', {'int': ['test 1'], 'opt': ['test 2']}, - charm_func=check_optional_relations, services=['s1'], ports=None) + charm_func='sequence_return', services=['s1'], ports=None) + sequence_functions.assert_called_once_with( + neutron_utils.check_optional_relations, + neutron_utils.check_ext_port_data_port_config) def test_pause_unit_helper(self): with patch.object(neutron_utils, '_pause_resume_helper') as prh: