From 2702baed390d094b0eac07d0ae167ed236868d00 Mon Sep 17 00:00:00 2001 From: Salvatore Orlando Date: Mon, 13 Jan 2014 12:51:03 -0800 Subject: [PATCH] Avoid processing ports which are not yet ready This patch changes get_vif_port_set in order to not return OVS ports for which the ofport is not yet assigned, thus avoiding a regex match failure in get_vif_port_by_id. Because of this failure, treat_vif_port is unable to wire the port. As get_vif_port_by_id is also used elsewhere in the agent, it has been enhanced in order to tolerate situations in which ofport might have not yet been assigned. The ofport field is added to the list of those monitored by the SimpleInterfaceMonitor. This will guarantee an event is generated when the ofport is assigned to a port. Otherwise there is a risk a port would be never processed if it was not yet ready the first time is was detected. This change won't trigger any extra processing on the agent side. Finally, this patch avoids fetching device details from the plugin for ports which have disappeared from the OVS bridge. This is a little optimization which might be beneficial for short lived ports. Change-Id: Icf7f0c7d6fe5239a358567cc9dc9db8ec11c15be Partial-Bug: #1253896 --- neutron/agent/linux/ovs_lib.py | 49 +++++++++++++---- neutron/agent/linux/ovsdb_monitor.py | 2 +- .../openvswitch/agent/ovs_neutron_agent.py | 25 +++++++-- .../tests/unit/openvswitch/test_ovs_lib.py | 54 ++++++++++++++++--- .../openvswitch/test_ovs_neutron_agent.py | 17 ++++-- 5 files changed, 120 insertions(+), 27 deletions(-) diff --git a/neutron/agent/linux/ovs_lib.py b/neutron/agent/linux/ovs_lib.py index 6a48a4da3..f92189f36 100644 --- a/neutron/agent/linux/ovs_lib.py +++ b/neutron/agent/linux/ovs_lib.py @@ -118,7 +118,7 @@ class OVSBridge(BaseOVS): mac = 'attached-mac="(?P([a-fA-F\d]{2}:){5}([a-fA-F\d]{2}))"' iface = 'iface-id="(?P[^"]+)"' name = 'name\s*:\s"(?P[^"]*)"' - port = 'ofport\s*:\s(?P-?\d+)' + port = 'ofport\s*:\s(?P(-?\d+|\[\]))' _re = ('%(external)s:\s{ ( %(mac)s,? | %(iface)s,? | . )* }' ' \s+ %(name)s \s+ %(port)s' % {'external': external, 'mac': mac, @@ -356,7 +356,7 @@ class OVSBridge(BaseOVS): def get_vif_port_set(self): port_names = self.get_port_name_list() edge_ports = set() - args = ['--format=json', '--', '--columns=name,external_ids', + args = ['--format=json', '--', '--columns=name,external_ids,ofport', 'list', 'Interface'] result = self.run_vsctl(args, check_error=True) if not result: @@ -366,14 +366,29 @@ class OVSBridge(BaseOVS): if name not in port_names: continue external_ids = dict(row[1][1]) - if "iface-id" in external_ids and "attached-mac" in external_ids: - edge_ports.add(external_ids['iface-id']) - elif ("xs-vif-uuid" in external_ids and - "attached-mac" in external_ids): - # if this is a xenserver and iface-id is not automatically - # synced to OVS from XAPI, we grab it from XAPI directly - iface_id = self.get_xapi_iface_id(external_ids["xs-vif-uuid"]) - edge_ports.add(iface_id) + # Do not consider VIFs which aren't yet ready + # This can happen when ofport values are either [] or ["set", []] + # We will therefore consider only integer values for ofport + ofport = row[2] + try: + int_ofport = int(ofport) + except (ValueError, TypeError): + LOG.warn(_("Found not yet ready openvswitch port: %s"), row) + else: + if int_ofport > 0: + if ("iface-id" in external_ids and + "attached-mac" in external_ids): + edge_ports.add(external_ids['iface-id']) + elif ("xs-vif-uuid" in external_ids and + "attached-mac" in external_ids): + # if this is a xenserver and iface-id is not + # automatically synced to OVS from XAPI, we grab it + # from XAPI directly + iface_id = self.get_xapi_iface_id( + external_ids["xs-vif-uuid"]) + edge_ports.add(iface_id) + else: + LOG.warn(_("Found failed openvswitch port: %s"), row) return edge_ports def get_vif_port_by_id(self, port_id): @@ -383,12 +398,24 @@ class OVSBridge(BaseOVS): result = self.run_vsctl(args) if not result: return + # TODO(salv-orlando): consider whether it would be possible to use + # JSON formatting rather than doing regex parsing. match = self.re_id.search(result) try: vif_mac = match.group('vif_mac') vif_id = match.group('vif_id') port_name = match.group('port_name') - ofport = int(match.group('ofport')) + # Tolerate ports which might not have an ofport as they are not + # ready yet + # NOTE(salv-orlando): Consider returning None when ofport is not + # available. + try: + ofport = int(match.group('ofport')) + except ValueError: + LOG.warn(_("ofport for vif: %s is not a valid integer. " + "The port has not yet been configured by OVS"), + vif_id) + ofport = None return VifPort(port_name, ofport, vif_id, vif_mac, self) except Exception as e: LOG.info(_("Unable to parse regex results. Exception: %s"), e) diff --git a/neutron/agent/linux/ovsdb_monitor.py b/neutron/agent/linux/ovsdb_monitor.py index 5e58f52f7..40ba6114f 100644 --- a/neutron/agent/linux/ovsdb_monitor.py +++ b/neutron/agent/linux/ovsdb_monitor.py @@ -72,7 +72,7 @@ class SimpleInterfaceMonitor(OvsdbMonitor): def __init__(self, root_helper=None, respawn_interval=None): super(SimpleInterfaceMonitor, self).__init__( 'Interface', - columns=['name'], + columns=['name', 'ofport'], format='json', root_helper=root_helper, respawn_interval=respawn_interval, diff --git a/neutron/plugins/openvswitch/agent/ovs_neutron_agent.py b/neutron/plugins/openvswitch/agent/ovs_neutron_agent.py index 9ab6cfc24..ed67a67da 100644 --- a/neutron/plugins/openvswitch/agent/ovs_neutron_agent.py +++ b/neutron/plugins/openvswitch/agent/ovs_neutron_agent.py @@ -599,7 +599,7 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin, if cur_tag != str(lvm.vlan): self.int_br.set_db_attribute("Port", port.port_name, "tag", str(lvm.vlan)) - if int(port.ofport) != -1: + if port.ofport != -1: self.int_br.delete_flows(in_port=port.ofport) def port_unbound(self, vif_id, net_uuid=None): @@ -854,6 +854,13 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin, def treat_vif_port(self, vif_port, port_id, network_id, network_type, physical_network, segmentation_id, admin_state_up): + # When this function is called for a port, the port should have + # an OVS ofport configured, as only these ports were considered + # for being treated. If that does not happen, it is a potential + # error condition of which operators should be aware + if not vif_port.ofport: + LOG.warn(_("VIF port: %s has no ofport configured, and might not " + "be able to transmit"), vif_port.vif_id) if vif_port: if admin_state_up: self.port_bound(vif_port, network_id, network_type, @@ -918,7 +925,15 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin, def treat_devices_added_or_updated(self, devices): resync = False for device in devices: - LOG.debug(_("Processing port:%s"), device) + LOG.debug(_("Processing port %s"), device) + port = self.int_br.get_vif_port_by_id(device) + if not port: + # The port has disappeared and should not be processed + # There is no need to put the port DOWN in the plugin as + # it never went up in the first place + LOG.info(_("Port %s was not found on the integration bridge " + "and will therefore not be processed"), device) + continue try: # TODO(salv-orlando): Provide bulk API for retrieving # details for all devices in one call @@ -931,7 +946,6 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin, {'device': device, 'e': e}) resync = True continue - port = self.int_br.get_vif_port_by_id(details['device']) if 'port_id' in details: LOG.info(_("Port %(device)s updated. Details: %(details)s"), {'device': device, 'details': details}) @@ -950,9 +964,10 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin, LOG.debug(_("Setting status for %s to DOWN"), device) self.plugin_rpc.update_device_down( self.context, device, self.agent_id, cfg.CONF.host) + LOG.info(_("Configuration for device %s completed."), device) else: - LOG.debug(_("Device %s not defined on plugin"), device) - if (port and int(port.ofport) != -1): + LOG.warn(_("Device %s not defined on plugin"), device) + if (port and port.ofport != -1): self.port_dead(port) return resync diff --git a/neutron/tests/unit/openvswitch/test_ovs_lib.py b/neutron/tests/unit/openvswitch/test_ovs_lib.py index ee78f5b42..944912374 100644 --- a/neutron/tests/unit/openvswitch/test_ovs_lib.py +++ b/neutron/tests/unit/openvswitch/test_ovs_lib.py @@ -408,12 +408,13 @@ class OVS_Lib_Test(base.BaseTestCase): ovs_row = [] r["data"].append(ovs_row) for cell in row: - if isinstance(cell, str): + if isinstance(cell, (str, int, list)): ovs_row.append(cell) elif isinstance(cell, dict): ovs_row.append(["map", cell.items()]) else: - raise TypeError('%r not str or dict' % type(cell)) + raise TypeError('%r not int, str, list or dict' % + type(cell)) return jsonutils.dumps(r) def _test_get_vif_port_set(self, is_xen): @@ -425,11 +426,17 @@ class OVS_Lib_Test(base.BaseTestCase): headings = ['name', 'external_ids'] data = [ # A vif port on this bridge: - ['tap99', {id_key: 'tap99id', 'attached-mac': 'tap99mac'}], + ['tap99', {id_key: 'tap99id', 'attached-mac': 'tap99mac'}, 1], + # A vif port on this bridge not yet configured + ['tap98', {id_key: 'tap98id', 'attached-mac': 'tap98mac'}, []], + # Another vif port on this bridge not yet configured + ['tap97', {id_key: 'tap97id', 'attached-mac': 'tap97mac'}, + ['set', []]], + # A vif port on another bridge: - ['tap88', {id_key: 'tap88id', 'attached-mac': 'tap88id'}], + ['tap88', {id_key: 'tap88id', 'attached-mac': 'tap88id'}, 1], # Non-vif port on this bridge: - ['tun22', {}], + ['tun22', {}, 2], ] # Each element is a tuple of (expected mock call, return_value) @@ -438,7 +445,7 @@ class OVS_Lib_Test(base.BaseTestCase): root_helper=self.root_helper), 'tap99\ntun22'), (mock.call(["ovs-vsctl", self.TO, "--format=json", - "--", "--columns=name,external_ids", + "--", "--columns=name,external_ids,ofport", "list", "Interface"], root_helper=self.root_helper), self._encode_ovs_json(headings, data)), @@ -494,7 +501,7 @@ class OVS_Lib_Test(base.BaseTestCase): root_helper=self.root_helper), 'tap99\n'), (mock.call(["ovs-vsctl", self.TO, "--format=json", - "--", "--columns=name,external_ids", + "--", "--columns=name,external_ids,ofport", "list", "Interface"], root_helper=self.root_helper), RuntimeError()), @@ -615,3 +622,36 @@ class OVS_Lib_Test(base.BaseTestCase): return_value=mock.Mock(address=None)): with testtools.ExpectedException(Exception): self.br.get_local_port_mac() + + def _test_get_vif_port_by_id(self, ofport=None): + expected_output = ('external_ids : {attached-mac="aa:bb:cc:dd:ee:ff", ' + 'iface-id="tap99id",' + 'iface-status=active, ' + 'vm-uuid="tap99vm"}' + '\nname : "tap99"\nofport : %(ofport)s\n') + + # Each element is a tuple of (expected mock call, return_value) + expected_calls_and_values = [ + (mock.call(["ovs-vsctl", self.TO, + "--", "--columns=external_ids,name,ofport", + "find", "Interface", + 'external_ids:iface-id="tap99id"'], + root_helper=self.root_helper), + expected_output % {'ofport': ofport or '[]'}), + ] + tools.setup_mock_calls(self.execute, expected_calls_and_values) + + vif_port = self.br.get_vif_port_by_id('tap99id') + self.assertEqual(vif_port.vif_id, 'tap99id') + self.assertEqual(vif_port.vif_mac, 'aa:bb:cc:dd:ee:ff') + self.assertEqual(vif_port.port_name, 'tap99') + tools.verify_mock_calls(self.execute, expected_calls_and_values) + return vif_port + + def test_get_vif_by_port_id_with_ofport(self): + vif_port = self._test_get_vif_port_by_id('1') + self.assertEqual(vif_port.ofport, 1) + + def test_get_vif_by_port_id_without_ofport(self): + vif_port = self._test_get_vif_port_by_id() + self.assertIsNone(vif_port.ofport) diff --git a/neutron/tests/unit/openvswitch/test_ovs_neutron_agent.py b/neutron/tests/unit/openvswitch/test_ovs_neutron_agent.py index a2f94aee2..959101cab 100644 --- a/neutron/tests/unit/openvswitch/test_ovs_neutron_agent.py +++ b/neutron/tests/unit/openvswitch/test_ovs_neutron_agent.py @@ -248,8 +248,11 @@ class TestOvsNeutronAgent(base.BaseTestCase): self.assertEqual(expected, actual) def test_treat_devices_added_returns_true_for_missing_device(self): - with mock.patch.object(self.agent.plugin_rpc, 'get_device_details', - side_effect=Exception()): + with contextlib.nested( + mock.patch.object(self.agent.plugin_rpc, 'get_device_details', + side_effect=Exception()), + mock.patch.object(self.agent.int_br, 'get_vif_port_by_id', + return_value=mock.Mock())): self.assertTrue(self.agent.treat_devices_added_or_updated([{}])) def _mock_treat_devices_added_updated(self, details, port, func_name): @@ -284,7 +287,15 @@ class TestOvsNeutronAgent(base.BaseTestCase): self.assertTrue(self._mock_treat_devices_added_updated( mock.MagicMock(), port, 'port_dead')) - def test_treat_devices_added_updated_updates_known_port(self): + def test_treat_devices_added_does_not_process_missing_port(self): + with contextlib.nested( + mock.patch.object(self.agent.plugin_rpc, 'get_device_details'), + mock.patch.object(self.agent.int_br, 'get_vif_port_by_id', + return_value=None) + ) as (get_dev_fn, get_vif_func): + self.assertFalse(get_dev_fn.called) + + def test_treat_devices_added__updated_updates_known_port(self): details = mock.MagicMock() details.__contains__.side_effect = lambda x: True self.assertTrue(self._mock_treat_devices_added_updated( -- 2.45.2