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 <amuller@redhat.com>

Change-Id: I2ce6d232744f48ba7fc0f824a7db32e3655bc2aa
Closes-Bug: 1473199

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})

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'])

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

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)

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',

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(),