Iterate over same port_id if more than one exists

In certain cases where multiple ports can have the same
external_ids:iface_id property, the ovs agent will arbitrarily choose
one and ignore the rest. In case the chosen port isn't on the
integration bridge the ovs agent is managing, an error is returned to
the calling function. This is faulty since one of the other ports may
belong to the correct bridge and it should be chosen instead.

This is interesting for future L3 HA integration tests, where 2
different instances of l3 agents are needed to run on the same machine.
In this case, both agents will register ports which have the same
iface_id property, but obviously only one of the ports is relevant for
each agent.

Closes-bug: #1370914
Related-bug: #1374947
Change-Id: I05d70417e0f78ae51a9ce377fc93a3f12046b313

diff --git a/neutron/agent/linux/ovs_lib.py b/neutron/agent/linux/ovs_lib.py
index 2013ba170c60cbfa36ef0fd787acaa34d1d41ddc..64cf4eebbba0e84304158d241423476ec38ace37 100644 (file)
--- a/neutron/agent/linux/ovs_lib.py
+++ b/neutron/agent/linux/ovs_lib.py
@@ -22,6 +22,7 @@ from neutron.agent.linux import ip_lib
 from neutron.agent.linux import utils
 from neutron.common import exceptions
 from neutron.openstack.common import excutils
+from neutron.openstack.common.gettextutils import _LI, _LW
 from neutron.openstack.common import jsonutils
 from neutron.openstack.common import log as logging
 from neutron.plugins.common import constants
@@ -401,29 +402,28 @@ class OVSBridge(BaseOVS):
             # an exeception which will be captured in this block.
             # We won't deal with the possibility of ovs-vsctl return multiple
             # rows since the interface identifier is unique
-            data = json_result['data'][0]
-            port_name = data[name_idx]
-            switch = get_bridge_for_iface(self.root_helper, port_name)
-            if switch != self.br_name:
-                LOG.info(_("Port: %(port_name)s is on %(switch)s,"
-                           " not on %(br_name)s"), {'port_name': port_name,
-                                                    'switch': switch,
-                                                    'br_name': self.br_name})
-                return
-            ofport = data[ofport_idx]
-            # ofport must be integer otherwise return None
-            if not isinstance(ofport, int) or ofport == -1:
-                LOG.warn(_("ofport: %(ofport)s for VIF: %(vif)s is not a "
-                           "positive integer"), {'ofport': ofport,
-                                                 'vif': port_id})
-                return
-            # Find VIF's mac address in external ids
-            ext_id_dict = dict((item[0], item[1]) for item in
-                               data[ext_ids_idx][1])
-            vif_mac = ext_id_dict['attached-mac']
-            return VifPort(port_name, ofport, port_id, vif_mac, self)
-        except Exception as e:
-            LOG.warn(_("Unable to parse interface details. Exception: %s"), e)
+            for data in json_result['data']:
+                port_name = data[name_idx]
+                switch = get_bridge_for_iface(self.root_helper, port_name)
+                if switch != self.br_name:
+                    continue
+                ofport = data[ofport_idx]
+                # ofport must be integer otherwise return None
+                if not isinstance(ofport, int) or ofport == -1:
+                    LOG.warn(_LW("ofport: %(ofport)s for VIF: %(vif)s is not a"
+                                 " positive integer"), {'ofport': ofport,
+                                                        'vif': port_id})
+                    return
+                # Find VIF's mac address in external ids
+                ext_id_dict = dict((item[0], item[1]) for item in
+                                   data[ext_ids_idx][1])
+                vif_mac = ext_id_dict['attached-mac']
+                return VifPort(port_name, ofport, port_id, vif_mac, self)
+            LOG.info(_LI("Port %(port_id)s not present in bridge %(br_name)s"),
+                     {'port_id': port_id, 'br_name': self.br_name})
+        except Exception as error:
+            LOG.warn(_LW("Unable to parse interface details. Exception: %s"),
+                     error)
             return
 
     def delete_ports(self, all_ports=False):

diff --git a/neutron/tests/unit/agent/linux/test_ovs_lib.py b/neutron/tests/unit/agent/linux/test_ovs_lib.py
index 8d2444acce922b22d15d909ead749eedf7853435..5af128cd9bcdabcccb48c115df6380ec2b973e2e 100644 (file)
--- a/neutron/tests/unit/agent/linux/test_ovs_lib.py
+++ b/neutron/tests/unit/agent/linux/test_ovs_lib.py
@@ -822,8 +822,10 @@ class OVS_Lib_Test(base.BaseTestCase):
             with testtools.ExpectedException(Exception):
                 self.br.get_local_port_mac()
 
-    def _test_get_vif_port_by_id(self, iface_id, data, br_name=None):
+    def _test_get_vif_port_by_id(self, iface_id, data, br_name=None,
+                                 extra_calls_and_values=None):
         headings = ['external_ids', 'name', 'ofport']
+
         # Each element is a tuple of (expected mock call, return_value)
         expected_calls_and_values = [
             (mock.call(["ovs-vsctl", self.TO, "--format=json",
@@ -836,9 +838,15 @@ class OVS_Lib_Test(base.BaseTestCase):
             if not br_name:
                 br_name = self.BR_NAME
 
+            # Only the last information list in 'data' is used, so if more
+            # than one vif is described in data, the rest must be declared
+            # in the argument 'expected_calls_and_values'.
+            if extra_calls_and_values:
+                expected_calls_and_values.extend(extra_calls_and_values)
+
             expected_calls_and_values.append(
                 (mock.call(["ovs-vsctl", self.TO,
-                            "iface-to-br", data[0][headings.index('name')]],
+                            "iface-to-br", data[-1][headings.index('name')]],
                            root_helper=self.root_helper),
                  br_name))
         tools.setup_mock_calls(self.execute, expected_calls_and_values)
@@ -847,6 +855,15 @@ class OVS_Lib_Test(base.BaseTestCase):
         tools.verify_mock_calls(self.execute, expected_calls_and_values)
         return vif_port
 
+    def _assert_vif_port(self, vif_port, ofport=None, mac=None):
+        if not ofport or ofport == -1 or not mac:
+            self.assertIsNone(vif_port)
+            return
+        self.assertEqual('tap99id', vif_port.vif_id)
+        self.assertEqual(mac, vif_port.vif_mac)
+        self.assertEqual('tap99', vif_port.port_name)
+        self.assertEqual(ofport, vif_port.ofport)
+
     def _test_get_vif_port_by_id_with_data(self, ofport=None, mac=None):
         external_ids = [["iface-id", "tap99id"],
                         ["iface-status", "active"]]
@@ -855,13 +872,7 @@ class OVS_Lib_Test(base.BaseTestCase):
         data = [[["map", external_ids], "tap99",
                  ofport if ofport else '["set",[]]']]
         vif_port = self._test_get_vif_port_by_id('tap99id', data)
-        if not ofport or ofport == -1 or not mac:
-            self.assertIsNone(vif_port)
-            return
-        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')
-        self.assertEqual(vif_port.ofport, ofport)
+        self._assert_vif_port(vif_port, ofport, mac)
 
     def test_get_vif_by_port_id_with_ofport(self):
         self._test_get_vif_port_by_id_with_data(
@@ -887,6 +898,22 @@ class OVS_Lib_Test(base.BaseTestCase):
         self.assertIsNone(self._test_get_vif_port_by_id('tap99id', data,
                                                         "br-ext"))
 
+    def test_get_vif_by_port_id_multiple_vifs(self):
+        external_ids = [["iface-id", "tap99id"],
+                        ["iface-status", "active"],
+                        ["attached-mac", "de:ad:be:ef:13:37"]]
+        data = [[["map", external_ids], "dummytap", 1],
+                [["map", external_ids], "tap99", 1337]]
+        extra_calls_and_values = [
+                (mock.call(["ovs-vsctl", self.TO,
+                            "iface-to-br", "dummytap"],
+                           root_helper=self.root_helper),
+                 "br-ext")]
+
+        vif_port = self._test_get_vif_port_by_id(
+                'tap99id', data, extra_calls_and_values=extra_calls_and_values)
+        self._assert_vif_port(vif_port, ofport=1337, mac="de:ad:be:ef:13:37")
+
 
 class TestDeferredOVSBridge(base.BaseTestCase):