Parse JSON in ovs_lib.get_vif_port_by_id

This patch replaces regex matching of text output with parsing
of JSON output in ovs_lib.get_vif_port_by_id.
This makes the code more reliable as subtle, possibly even
cosmetic, changes in ovs-vsctl output format could cause the
regular expression match to fail.

Also, this makes the code consistent with ovs_lib.get_vif_port_set
which already uses JSON output.

Finally this patch slightly changes the behaviour of
ovs_lib.get_vif_port_by_id returning None if elements such as
mac address or ofport were not available.

Change-Id: Ia985a130739c72b5b88414a79b2c6083ca6a0a00
Closes-Bug: #1280827

diff --git a/neutron/agent/linux/ovs_lib.py b/neutron/agent/linux/ovs_lib.py
index 876810147bdf4a47dee5e8ad1725a66c28de25e6..8a1e6dd088cb85b5263e05ebadc61e6005ba6682 100644 (file)
--- a/neutron/agent/linux/ovs_lib.py
+++ b/neutron/agent/linux/ovs_lib.py
@@ -108,23 +108,9 @@ class OVSBridge(BaseOVS):
     def __init__(self, br_name, root_helper):
         super(OVSBridge, self).__init__(root_helper)
         self.br_name = br_name
-        self.re_id = self.re_compile_id()
         self.defer_apply_flows = False
         self.deferred_flows = {'add': '', 'mod': '', 'del': ''}
 
-    def re_compile_id(self):
-        external = 'external_ids\s*'
-        mac = 'attached-mac="(?P<vif_mac>([a-fA-F\d]{2}:){5}([a-fA-F\d]{2}))"'
-        iface = 'iface-id="(?P<vif_id>[^"]+)"'
-        name = 'name\s*:\s"(?P<port_name>[^"]*)"'
-        port = 'ofport\s*:\s(?P<ofport>(-?\d+|\[\]))'
-        _re = ('%(external)s:\s{ ( %(mac)s,? | %(iface)s,? | . )* }'
-               ' \s+ %(name)s \s+ %(port)s' % {'external': external,
-                                               'mac': mac,
-                                               'iface': iface, 'name': name,
-                                               'port': port})
-        return re.compile(_re, re.M | re.X)
-
     def create(self):
         self.add_bridge(self.br_name)
 
@@ -391,33 +377,39 @@ class OVSBridge(BaseOVS):
         return edge_ports
 
     def get_vif_port_by_id(self, port_id):
-        args = ['--', '--columns=external_ids,name,ofport',
+        args = ['--format=json', '--', '--columns=external_ids,name,ofport',
                 'find', 'Interface',
                 'external_ids:iface-id="%s"' % port_id]
         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)
+        json_result = jsonutils.loads(result)
         try:
-            vif_mac = match.group('vif_mac')
-            vif_id = match.group('vif_id')
-            port_name = match.group('port_name')
-            # 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)
+            # Retrieve the indexes of the columns we're looking for
+            headings = json_result['headings']
+            ext_ids_idx = headings.index('external_ids')
+            name_idx = headings.index('name')
+            ofport_idx = headings.index('ofport')
+            # If data attribute is missing or empty the line below will raise
+            # 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]
+            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.info(_("Unable to parse regex results. Exception: %s"), e)
+            LOG.warn(_("Unable to parse interface details. Exception: %s"), e)
             return
 
     def delete_ports(self, all_ports=False):

diff --git a/neutron/tests/unit/openvswitch/test_ovs_lib.py b/neutron/tests/unit/openvswitch/test_ovs_lib.py
index ab8debd881995f968dddd00e4cc7a3a1e8c776d2..0035a7f34ac5ee9670464b08e7f1b724b6c68e58 100644 (file)
--- a/neutron/tests/unit/openvswitch/test_ovs_lib.py
+++ b/neutron/tests/unit/openvswitch/test_ovs_lib.py
@@ -514,21 +514,6 @@ class OVS_Lib_Test(base.BaseTestCase):
             ["ovs-vsctl", self.TO, "clear", "Port", pname, "tag"],
             root_helper=self.root_helper)
 
-    def test_port_id_regex(self):
-        result = ('external_ids        : {attached-mac="fa:16:3e:23:5b:f2",'
-                  ' iface-id="5c1321a7-c73f-4a77-95e6-9f86402e5c8f",'
-                  ' iface-status=active}\nname                :'
-                  ' "dhc5c1321a7-c7"\nofport              : 2\n')
-        match = self.br.re_id.search(result)
-        vif_mac = match.group('vif_mac')
-        vif_id = match.group('vif_id')
-        port_name = match.group('port_name')
-        ofport = int(match.group('ofport'))
-        self.assertEqual(vif_mac, 'fa:16:3e:23:5b:f2')
-        self.assertEqual(vif_id, '5c1321a7-c73f-4a77-95e6-9f86402e5c8f')
-        self.assertEqual(port_name, 'dhc5c1321a7-c7')
-        self.assertEqual(ofport, 2)
-
     def _test_iface_to_br(self, exp_timeout=None):
         iface = 'tap0'
         br = 'br-int'
@@ -620,35 +605,52 @@ class OVS_Lib_Test(base.BaseTestCase):
             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')
-
+    def _test_get_vif_port_by_id(self, iface_id, data):
+        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,
+            (mock.call(["ovs-vsctl", self.TO, "--format=json",
                         "--", "--columns=external_ids,name,ofport",
                         "find", "Interface",
-                        'external_ids:iface-id="tap99id"'],
+                        'external_ids:iface-id="%s"' % iface_id],
                        root_helper=self.root_helper),
-             expected_output % {'ofport': ofport or '[]'}),
-        ]
+             self._encode_ovs_json(headings, data))]
+
         tools.setup_mock_calls(self.execute, expected_calls_and_values)
+        vif_port = self.br.get_vif_port_by_id(iface_id)
 
-        vif_port = self.br.get_vif_port_by_id('tap99id')
+        tools.verify_mock_calls(self.execute, expected_calls_and_values)
+        return vif_port
+
+    def _test_get_vif_port_by_id_with_data(self, ofport=None, mac=None):
+        external_ids = [["iface-id", "tap99id"],
+                        ["iface-status", "active"]]
+        if mac:
+            external_ids.append(["attached-mac", mac])
+        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')
-        tools.verify_mock_calls(self.execute, expected_calls_and_values)
-        return vif_port
+        self.assertEqual(vif_port.ofport, ofport)
 
     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)
+        self._test_get_vif_port_by_id_with_data(
+            ofport=1, mac="aa:bb:cc:dd:ee:ff")
 
     def test_get_vif_by_port_id_without_ofport(self):
-        vif_port = self._test_get_vif_port_by_id()
-        self.assertIsNone(vif_port.ofport)
+        self._test_get_vif_port_by_id_with_data(mac="aa:bb:cc:dd:ee:ff")
+
+    def test_get_vif_by_port_id_with_invalid_ofport(self):
+        self._test_get_vif_port_by_id_with_data(
+            ofport=-1, mac="aa:bb:cc:dd:ee:ff")
+
+    def test_get_vif_by_port_id_without_mac(self):
+        self._test_get_vif_port_by_id_with_data(ofport=1)
+
+    def test_get_vif_by_port_id_with_no_data(self):
+        self.assertIsNone(self._test_get_vif_port_by_id('whatever', []))