diff --git a/neutron/agent/linux/ovs_lib.py b/neutron/agent/linux/ovs_lib.py index 6a48a4da3e..f92189f369 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 5e58f52f76..40ba6114ff 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 9ab6cfc243..ed67a67dae 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 ee78f5b42e..944912374f 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 a2f94aee2d..959101cab1 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(