From: rossella Date: Thu, 9 Jul 2015 22:08:50 +0000 (+0000) Subject: Introduce get_ports_attributes in OVSBridge X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=e32e74553fe4947c7e7cc011ce277f45a4514ef5;p=openstack-build%2Fneutron-build.git Introduce get_ports_attributes in OVSBridge OVSBridge was inheriting db_list from BaseOVS, which was returning the information of all the ports on the machine, not only the ones belonging to the bridge. The OVSNeutronAgent was using that method with the assumption that ports were filtered by bridge. To avoid confusion, this patch add a new method to OVSBridge get_ports_attributes to query the info for all the ports belonging to the bridge. db_list is removed from BaseOVS since that method is already available in ovsdb/api.py ovs_lib methods that use db_list are refactored accordingly. Co-Authored-By: Assaf Muller Change-Id: I2ce6d232744f48ba7fc0f824a7db32e3655bc2aa Closes-Bug: 1473199 --- diff --git a/neutron/agent/common/ovs_lib.py b/neutron/agent/common/ovs_lib.py index de8cf3dd8..592466720 100644 --- a/neutron/agent/common/ovs_lib.py +++ b/neutron/agent/common/ovs_lib.py @@ -141,12 +141,6 @@ class BaseOVS(object): return self.ovsdb.db_get(table, record, column).execute( check_error=check_error, log_errors=log_errors) - def db_list(self, table, records=None, columns=None, - check_error=True, log_errors=True, if_exists=False): - return (self.ovsdb.db_list(table, records=records, columns=columns, - if_exists=if_exists). - execute(check_error=check_error, log_errors=log_errors)) - class OVSBridge(BaseOVS): def __init__(self, br_name): @@ -326,20 +320,24 @@ class OVSBridge(BaseOVS): "Exception: %(exception)s"), {'cmd': args, 'exception': e}) + def get_ports_attributes(self, table, columns=None, ports=None, + check_error=True, log_errors=True, + if_exists=False): + port_names = ports or self.get_port_name_list() + return (self.ovsdb.db_list(table, port_names, columns=columns, + if_exists=if_exists). + execute(check_error=check_error, log_errors=log_errors)) + # returns a VIF object for each VIF port def get_vif_ports(self): edge_ports = [] - port_names = self.get_port_name_list() - port_info = self.db_list( - 'Interface', columns=['name', 'external_ids', 'ofport']) - by_name = {x['name']: x for x in port_info} - for name in port_names: - if not by_name.get(name): - #NOTE(dprince): some ports (like bonds) won't have all - # these attributes so we skip them entirely - continue - external_ids = by_name[name]['external_ids'] - ofport = by_name[name]['ofport'] + port_info = self.get_ports_attributes( + 'Interface', columns=['name', 'external_ids', 'ofport'], + if_exists=True) + for port in port_info: + name = port['name'] + external_ids = port['external_ids'] + ofport = port['ofport'] if "iface-id" in external_ids and "attached-mac" in external_ids: p = VifPort(name, ofport, external_ids["iface-id"], external_ids["attached-mac"], self) @@ -356,9 +354,8 @@ class OVSBridge(BaseOVS): return edge_ports def get_vif_port_to_ofport_map(self): - port_names = self.get_port_name_list() - results = self.db_list( - 'Interface', port_names, ['name', 'external_ids', 'ofport'], + results = self.get_ports_attributes( + 'Interface', columns=['name', 'external_ids', 'ofport'], if_exists=True) port_map = {} for r in results: @@ -373,9 +370,8 @@ class OVSBridge(BaseOVS): def get_vif_port_set(self): edge_ports = set() - port_names = self.get_port_name_list() - results = self.db_list( - 'Interface', port_names, ['name', 'external_ids', 'ofport'], + results = self.get_ports_attributes( + 'Interface', columns=['name', 'external_ids', 'ofport'], if_exists=True) for result in results: if result['ofport'] == UNASSIGNED_OFPORT: @@ -413,22 +409,18 @@ class OVSBridge(BaseOVS): in the "Interface" table queried by the get_vif_port_set() method. """ - port_names = self.get_port_name_list() - results = self.db_list('Port', port_names, ['name', 'tag'], - if_exists=True) + results = self.get_ports_attributes( + 'Port', columns=['name', 'tag'], if_exists=True) return {p['name']: p['tag'] for p in results} def get_vifs_by_ids(self, port_ids): - interface_info = self.db_list( + interface_info = self.get_ports_attributes( "Interface", columns=["name", "external_ids", "ofport"]) by_id = {x['external_ids'].get('iface-id'): x for x in interface_info} - intfs_on_bridge = self.ovsdb.list_ports(self.br_name).execute( - check_error=True) result = {} for port_id in port_ids: result[port_id] = None - if (port_id not in by_id or - by_id[port_id]['name'] not in intfs_on_bridge): + if port_id not in by_id: LOG.info(_LI("Port %(port_id)s not present in bridge " "%(br_name)s"), {'port_id': port_id, 'br_name': self.br_name}) diff --git a/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_neutron_agent.py b/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_neutron_agent.py index fe9f8a15b..cf73bbc96 100644 --- a/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_neutron_agent.py +++ b/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_neutron_agent.py @@ -314,11 +314,13 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin, def _restore_local_vlan_map(self): cur_ports = self.int_br.get_vif_ports() - port_info = self.int_br.db_list( - "Port", columns=["name", "other_config", "tag"]) + port_names = [p.port_name for p in cur_ports] + port_info = self.int_br.get_ports_attributes( + "Port", columns=["name", "other_config", "tag"], ports=port_names) by_name = {x['name']: x for x in port_info} for port in cur_ports: - # if a port was deleted between get_vif_ports and db_lists, we + # if a port was deleted between get_vif_ports and + # get_ports_attributes, we # will get a KeyError try: local_vlan_map = by_name[port.port_name]['other_config'] @@ -741,8 +743,9 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin, def _bind_devices(self, need_binding_ports): devices_up = [] devices_down = [] - port_info = self.int_br.db_list( - "Port", columns=["name", "tag"]) + port_names = [p['vif_port'].port_name for p in need_binding_ports] + port_info = self.int_br.get_ports_attributes( + "Port", columns=["name", "tag"], ports=port_names) tags_by_name = {x['name']: x['tag'] for x in port_info} for port_detail in need_binding_ports: lvm = self.local_vlan_map.get(port_detail['network_id']) diff --git a/neutron/tests/functional/agent/test_ovs_lib.py b/neutron/tests/functional/agent/test_ovs_lib.py index b81e6a526..68872b3db 100644 --- a/neutron/tests/functional/agent/test_ovs_lib.py +++ b/neutron/tests/functional/agent/test_ovs_lib.py @@ -194,6 +194,22 @@ class OVSBridgeTestCase(OVSBridgeTestBase): self.assertEqual(sorted([x.port_name for x in vif_ports]), sorted([x.port_name for x in ports])) + def test_get_vif_ports_with_bond(self): + for i in range(2): + self.create_ovs_port() + vif_ports = [self.create_ovs_vif_port() for i in range(3)] + # bond ports don't have records in the Interface table but they do in + # the Port table + orig = self.br.get_port_name_list + new_port_name_list = lambda: orig() + ['bondport'] + mock.patch.object(self.br, 'get_port_name_list', + new=new_port_name_list).start() + ports = self.br.get_vif_ports() + self.assertEqual(3, len(ports)) + self.assertTrue(all([isinstance(x, ovs_lib.VifPort) for x in ports])) + self.assertEqual(sorted([x.port_name for x in vif_ports]), + sorted([x.port_name for x in ports])) + def test_get_vif_port_set(self): for i in range(2): self.create_ovs_port() @@ -215,6 +231,12 @@ class OVSBridgeTestCase(OVSBridgeTestBase): expected = set([vif_ports[0].vif_id]) self.assertEqual(expected, ports) + def test_get_ports_attributes(self): + port_names = [self.create_ovs_port()[0], self.create_ovs_port()[0]] + db_ports = self.br.get_ports_attributes('Interface', columns=['name']) + db_ports_names = [p['name'] for p in db_ports] + self.assertEqual(sorted(port_names), sorted(db_ports_names)) + def test_get_port_tag_dict(self): # Simple case tested in port test_set_get_clear_db_val pass diff --git a/neutron/tests/unit/agent/common/test_ovs_lib.py b/neutron/tests/unit/agent/common/test_ovs_lib.py index ee0c52eb4..b0b8180c1 100644 --- a/neutron/tests/unit/agent/common/test_ovs_lib.py +++ b/neutron/tests/unit/agent/common/test_ovs_lib.py @@ -473,28 +473,10 @@ class OVS_Lib_Test(base.BaseTestCase): vif_id = uuidutils.generate_uuid() mac = "ca:fe:de:ad:be:ef" id_field = 'xs-vif-uuid' if is_xen else 'iface-id' - external_ids = ('{"data":[[["map",[["attached-mac","%(mac)s"],' - '["%(id_field)s","%(vif)s"],' - '["iface-status","active"]]], ' - '"%(name)s", %(ofport)s]],' - '"headings":["external_ids", "name", "ofport"]}' % { - 'mac': mac, 'vif': vif_id, 'id_field': id_field, - 'name': pname, 'ofport': ofport}) - - # Each element is a tuple of (expected mock call, return_value) - expected_calls_and_values = [ - (self._vsctl_mock("list-ports", self.BR_NAME), "%s\n" % pname), - (self._vsctl_mock("--columns=name,external_ids,ofport", "list", - "Interface"), external_ids), - ] - if is_xen: - expected_calls_and_values.append( - (mock.call(["xe", "vif-param-get", "param-name=other-config", - "param-key=nicira-iface-id", "uuid=" + vif_id], - run_as_root=True), - vif_id) - ) - tools.setup_mock_calls(self.execute, expected_calls_and_values) + external_ids = {"attached-mac": mac, id_field: vif_id} + self.br.get_ports_attributes = mock.Mock(return_value=[{ + 'name': pname, 'ofport': ofport, 'external_ids': external_ids}]) + self.br.get_xapi_iface_id = mock.Mock(return_value=vif_id) ports = self.br.get_vif_ports() self.assertEqual(1, len(ports)) @@ -503,7 +485,10 @@ class OVS_Lib_Test(base.BaseTestCase): self.assertEqual(ports[0].vif_id, vif_id) self.assertEqual(ports[0].vif_mac, mac) self.assertEqual(ports[0].switch.br_name, self.BR_NAME) - tools.verify_mock_calls(self.execute, expected_calls_and_values) + self.br.get_ports_attributes.assert_called_once_with( + 'Interface', + columns=['name', 'external_ids', 'ofport'], + if_exists=True) def _encode_ovs_json(self, headings, data): # See man ovs-vsctl(8) for the encoding details. @@ -577,23 +562,6 @@ class OVS_Lib_Test(base.BaseTestCase): def test_get_vif_ports_xen(self): self._test_get_vif_ports(is_xen=True) - def test_get_vif_ports_with_bond(self): - pname = "bond0" - #NOTE(dprince): bond ports don't have records in the Interface table - external_ids = ('{"data":[], "headings":[]}') - - # Each element is a tuple of (expected mock call, return_value) - expected_calls_and_values = [ - (self._vsctl_mock("list-ports", self.BR_NAME), "%s\n" % pname), - (self._vsctl_mock("--columns=name,external_ids,ofport", "list", - "Interface"), external_ids), - ] - tools.setup_mock_calls(self.execute, expected_calls_and_values) - - ports = self.br.get_vif_ports() - self.assertEqual(0, len(ports)) - tools.verify_mock_calls(self.execute, expected_calls_and_values) - def test_get_vif_port_set_nonxen(self): self._test_get_vif_port_set(False) @@ -746,22 +714,17 @@ class OVS_Lib_Test(base.BaseTestCase): 'external_ids': {'iface-id': 'pid1', 'attached-mac': '11'}}, {'name': 'qvo2', 'ofport': 2, 'external_ids': {'iface-id': 'pid2', 'attached-mac': '22'}}, - {'name': 'qvo3', 'ofport': 3, - 'external_ids': {'iface-id': 'pid3', 'attached-mac': '33'}}, {'name': 'qvo4', 'ofport': -1, 'external_ids': {'iface-id': 'pid4', 'attached-mac': '44'}}, ] - self.br.db_list = mock.Mock(return_value=db_list_res) + self.br.get_ports_attributes = mock.Mock(return_value=db_list_res) self.br.ovsdb = mock.Mock() self.br.ovsdb.list_ports.return_value.execute.return_value = [ 'qvo1', 'qvo2', 'qvo4'] - by_id = self.br.get_vifs_by_ids(['pid1', 'pid2', 'pid3', - 'pid4', 'pid5']) - # pid3 isn't on bridge and pid4 doesn't have a valid ofport and pid5 - # isn't present in the db + by_id = self.br.get_vifs_by_ids(['pid1', 'pid2', 'pid3', 'pid4']) + # pid3 isn't on bridge and pid4 doesn't have a valid ofport self.assertIsNone(by_id['pid3']) self.assertIsNone(by_id['pid4']) - self.assertIsNone(by_id['pid5']) self.assertEqual('pid1', by_id['pid1'].vif_id) self.assertEqual('qvo1', by_id['pid1'].port_name) self.assertEqual(1, by_id['pid1'].ofport) diff --git a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_neutron_agent.py b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_neutron_agent.py index 9aaa3132f..e9b10c2c0 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_neutron_agent.py +++ b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_neutron_agent.py @@ -114,8 +114,9 @@ class TestOvsNeutronAgent(object): cfg.CONF.set_default('quitting_rpc_timeout', 10, 'AGENT') cfg.CONF.set_default('prevent_arp_spoofing', False, 'AGENT') kwargs = self.mod_agent.create_agent_config_map(cfg.CONF) - mock.patch('neutron.agent.common.ovs_lib.OVSBridge.db_list', - return_value=[]).start() + mock.patch( + 'neutron.agent.common.ovs_lib.OVSBridge.get_ports_attributes', + return_value=[]).start() with mock.patch.object(self.mod_agent.OVSNeutronAgent, 'setup_integration_br'),\ @@ -173,7 +174,7 @@ class TestOvsNeutronAgent(object): mock.patch.object(self.agent, 'provision_local_vlan') as \ provision_local_vlan: int_br.get_vif_ports.return_value = [port] - int_br.db_list.return_value = [{ + int_br.get_ports_attributes.return_value = [{ 'name': port.port_name, 'other_config': local_vlan_map, 'tag': tag }] @@ -466,7 +467,10 @@ class TestOvsNeutronAgent(object): self._mock_treat_devices_removed(False) def test_bind_port_with_missing_network(self): - self.agent._bind_devices([{'network_id': 'non-existent'}]) + vif_port = mock.Mock() + vif_port.name.return_value = 'port' + self.agent._bind_devices([{'network_id': 'non-existent', + 'vif_port': vif_port}]) def _test_process_network_ports(self, port_info): with mock.patch.object(self.agent.sg_agent, @@ -475,7 +479,7 @@ class TestOvsNeutronAgent(object): self.agent, "treat_devices_added_or_updated", return_value=([], [])) as device_added_updated,\ - mock.patch.object(self.agent.int_br, "db_list", + mock.patch.object(self.agent.int_br, "get_ports_attributes", return_value=[]),\ mock.patch.object(self.agent, "treat_devices_removed", @@ -1212,7 +1216,8 @@ class AncillaryBridgesTest(object): 'get_bridge_external_bridge_id', side_effect=pullup_side_effect),\ mock.patch( - 'neutron.agent.common.ovs_lib.OVSBridge.' 'db_list', + 'neutron.agent.common.ovs_lib.OVSBridge.' + 'get_ports_attributes', return_value=[]),\ mock.patch( 'neutron.agent.common.ovs_lib.OVSBridge.' 'get_vif_ports', @@ -1304,7 +1309,8 @@ class TestOvsDvrNeutronAgent(object): 'FixedIntervalLoopingCall', new=MockFixedIntervalLoopingCall),\ mock.patch( - 'neutron.agent.common.ovs_lib.OVSBridge.' 'db_list', + 'neutron.agent.common.ovs_lib.OVSBridge.' + 'get_ports_attributes', return_value=[]),\ mock.patch( 'neutron.agent.common.ovs_lib.OVSBridge.' 'get_vif_ports', diff --git a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_tunnel.py b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_tunnel.py index 5daad9998..fadcfa8fc 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_tunnel.py +++ b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_tunnel.py @@ -121,7 +121,7 @@ class TunnelTest(object): self.mock_int_bridge.add_patch_port.side_effect = ( lambda tap, peer: self.ovs_int_ofports[tap]) self.mock_int_bridge.get_vif_ports.return_value = [] - self.mock_int_bridge.db_list.return_value = [] + self.mock_int_bridge.get_ports_attributes.return_value = [] self.mock_int_bridge.db_get_val.return_value = {} self.mock_map_tun_bridge = self.ovs_bridges[self.MAP_TUN_BRIDGE] @@ -209,7 +209,8 @@ class TunnelTest(object): ] self.mock_int_bridge_expected += [ mock.call.get_vif_ports(), - mock.call.db_list('Port', columns=['name', 'other_config', 'tag']) + mock.call.get_ports_attributes( + 'Port', columns=['name', 'other_config', 'tag'], ports=[]) ] self.mock_tun_bridge_expected += [ @@ -601,7 +602,8 @@ class TunnelTestUseVethInterco(TunnelTest): ] self.mock_int_bridge_expected += [ mock.call.get_vif_ports(), - mock.call.db_list('Port', columns=['name', 'other_config', 'tag']) + mock.call.get_ports_attributes( + 'Port', columns=['name', 'other_config', 'tag'], ports=[]) ] self.mock_tun_bridge_expected += [ mock.call.delete_flows(),