From beb28439dc9706d52ac0220d0d94d8889adf51cb Mon Sep 17 00:00:00 2001 From: Salvatore Orlando Date: Sun, 16 Feb 2014 15:47:34 -0800 Subject: [PATCH] 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 --- neutron/agent/linux/ovs_lib.py | 60 +++++++--------- .../tests/unit/openvswitch/test_ovs_lib.py | 72 ++++++++++--------- 2 files changed, 63 insertions(+), 69 deletions(-) diff --git a/neutron/agent/linux/ovs_lib.py b/neutron/agent/linux/ovs_lib.py index f92189f369..cf84efcbc3 100644 --- a/neutron/agent/linux/ovs_lib.py +++ b/neutron/agent/linux/ovs_lib.py @@ -109,23 +109,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([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+|\[\]))' - _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) @@ -392,33 +378,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 944912374f..c1e926a248 100644 --- a/neutron/tests/unit/openvswitch/test_ovs_lib.py +++ b/neutron/tests/unit/openvswitch/test_ovs_lib.py @@ -517,21 +517,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' @@ -623,35 +608,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 '[]'}), - ] - tools.setup_mock_calls(self.execute, expected_calls_and_values) + 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') - 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_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') + 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', []))