Improve openvswitch and linuxbridge agents' parsing of mappings

Created generic quantum.utils.parse_mappings function that detects
duplicate key or value strings, and changed openvswitch and
linuxbridge agents to use this to parse their mappings from physical
networks to bridges or interfaces. Fixes bug 1067669.

Also fixed some typos in comments.

Change-Id: I342eaeeb6ff4c6e25d57d631f250faac082011b8

quantum/common/utils.py

@@ -134,3 +134,34 @@ def subprocess_popen(args, stdin=None, stdout=None, stderr=None, shell=False,
     return subprocess.Popen(args, shell=shell, stdin=stdin, stdout=stdout,
                             stderr=stderr, preexec_fn=_subprocess_setup,
                             env=env)
+
+
+def parse_mappings(mapping_list, unique_values=True):
+    """Parse a list of of mapping strings into a dictionary.
+
+    :param mapping_list: a list of strings of the form '<key>:<value>'
+    :param unique_values: values must be unique if True
+    :returns: a dict mapping keys to values
+    """
+    mappings = {}
+    for mapping in mapping_list:
+        mapping = mapping.strip()
+        if not mapping:
+            continue
+        split_result = mapping.split(':')
+        if len(split_result) != 2:
+            raise ValueError(_("Invalid mapping: '%s'") % mapping)
+        key = split_result[0].strip()
+        if not key:
+            raise ValueError(_("Missing key in mapping: '%s'") % mapping)
+        value = split_result[1].strip()
+        if not value:
+            raise ValueError(_("Missing value in mapping: '%s'") % mapping)
+        if key in mappings:
+            raise ValueError(_("Key %s in mapping: '%s' not unique") %
+                             (key, mapping))
+        if unique_values and value in mappings.itervalues():
+            raise ValueError(_("Value %s in mapping: '%s' not unique") %
+                             (value, mapping))
+        mappings[key] = value
+    return mappings

quantum/plugins/linuxbridge/agent/linuxbridge_quantum_agent.py

@@ -35,6 +35,7 @@
 from quantum.common import config as logging_config
 from quantum.common import constants
 from quantum.common import topics
+from quantum.common import utils as q_utils
 from quantum.openstack.common import cfg
 from quantum.openstack.common import context
 from quantum.openstack.common import log as logging
@@ -594,27 +595,24 @@ def main():
     # (TODO) gary - swap with common logging
     logging_config.setup_logging(cfg.CONF)
 
-    interface_mappings = {}
-    for mapping in cfg.CONF.LINUX_BRIDGE.physical_interface_mappings:
-        try:
-            physical_network, physical_interface = mapping.split(':')
-            interface_mappings[physical_network] = physical_interface
-            LOG.debug("physical network %s mapped to physical interface %s" %
-                      (physical_network, physical_interface))
-        except ValueError as ex:
-            LOG.error("Invalid physical interface mapping: %s - %s. "
-                      "Agent terminated!" %
-                      (mapping, ex))
-            sys.exit(1)
+    try:
+        interface_mappings = q_utils.parse_mappings(
+            cfg.CONF.LINUX_BRIDGE.physical_interface_mappings)
+    except ValueError as e:
+        LOG.error(_("Parsing physical_interface_mappings failed: %s."
+                    " Agent terminated!"), e)
+        sys.exit(1)
+    LOG.info(_("Interface mappings: %s") % interface_mappings)
 
     polling_interval = cfg.CONF.AGENT.polling_interval
     root_helper = cfg.CONF.AGENT.root_helper
     plugin = LinuxBridgeQuantumAgentRPC(interface_mappings,
                                         polling_interval,
                                         root_helper)
-    LOG.info("Agent initialized successfully, now running... ")
+    LOG.info(_("Agent initialized successfully, now running... "))
     plugin.daemon_loop()
     sys.exit(0)
 
+
 if __name__ == "__main__":
     main()

quantum/plugins/openvswitch/agent/ovs_quantum_agent.py

@@ -32,6 +32,7 @@
 from quantum.common import config as logging_config
 from quantum.common import constants as q_const
 from quantum.common import topics
+from quantum.common import utils as q_utils
 from quantum.openstack.common import cfg
 from quantum.openstack.common import context
 from quantum.openstack.common import log as logging
@@ -140,7 +141,7 @@ def __init__(self, integ_br, tun_br, local_ip,
         :param integ_br: name of the integration bridge.
         :param tun_br: name of the tunnel bridge.
         :param local_ip: local IP address of this hypervisor.
-        :param bridge_mappings: mappings from phyiscal interface to bridge.
+        :param bridge_mappings: mappings from physical network name to bridge.
         :param root_helper: utility to use when running shell cmds.
         :param polling_interval: interval (secs) to poll DB.
         :param enable_tunneling: if True enable GRE networks.
@@ -459,7 +460,7 @@ def setup_tunnel_br(self, tun_br):
     def setup_physical_bridges(self, bridge_mappings):
         '''Setup the physical network bridges.
 
-        Creates phyiscal network bridges and links them to the
+        Creates physical network bridges and links them to the
         integration bridge using veths.
 
         :param bridge_mappings: map physical network names to bridge names.'''
@@ -641,33 +642,17 @@ def daemon_loop(self):
         self.rpc_loop()
 
 
-def parse_bridge_mappings(bridge_mapping_list):
-    """Parse a list of physical network to bridge mappings.
-
-    :param bridge_mapping_list: a list of strings of the form
-                                '<physical network>:<bridge>'
-    :returns: a dict mapping physical networks to bridges
-    """
-    bridge_mappings = {}
-    for mapping in bridge_mapping_list:
-        mapping = mapping.strip()
-        if not mapping:
-            continue
-        split_result = [x.strip() for x in mapping.split(':', 1) if x.strip()]
-        if len(split_result) != 2:
-            raise ValueError('Invalid bridge mapping: %s.' % mapping)
-        physical_network, bridge = split_result
-        bridge_mappings[physical_network] = bridge
-    return bridge_mappings
-
-
 def create_agent_config_map(config):
     """Create a map of agent config parameters.
 
     :param config: an instance of cfg.CONF
     :returns: a map of agent configuration parameters
     """
-    bridge_mappings = parse_bridge_mappings(config.OVS.bridge_mappings)
+    try:
+        bridge_mappings = q_utils.parse_mappings(config.OVS.bridge_mappings)
+    except ValueError as e:
+        raise ValueError(_("Parsing bridge_mappings failed: %s.") % e)
+
     kwargs = dict(
         integ_br=config.OVS.integration_bridge,
         tun_br=config.OVS.tunnel_bridge,

quantum/tests/unit/openvswitch/test_ovs_quantum_agent.py

@@ -22,30 +22,6 @@
 from quantum.plugins.openvswitch.common import config
 
 
-class TestParseBridgeMappings(unittest.TestCase):
-
-    def parse(self, bridge_mapping_list):
-        return ovs_quantum_agent.parse_bridge_mappings(bridge_mapping_list)
-
-    def test_parse_bridge_mappings_fails_for_missing_separator(self):
-        with self.assertRaises(ValueError):
-            self.parse(['net'])
-
-    def test_parse_bridge_mappings_fails_for_missing_value(self):
-        with self.assertRaises(ValueError):
-            self.parse(['net:'])
-
-    def test_parse_bridge_mappings_succeeds_for_one_mapping(self):
-        self.assertEqual(self.parse(['net:br']), {'net': 'br'})
-
-    def test_parse_bridge_mappings_succeeds_for_n_mappings(self):
-        self.assertEqual(self.parse(['net:br', 'net1:br1']),
-                         {'net': 'br', 'net1': 'br1'})
-
-    def test_parse_bridge_mappings_succeeds_for_no_mappings(self):
-        self.assertEqual(self.parse(['']), {})
-
-
 class CreateAgentConfigMap(unittest.TestCase):
 
     def test_create_agent_config_map_succeeds(self):

quantum/tests/unit/test_common_utils.py

@@ -0,0 +1,60 @@
+# Copyright (c) 2012 OpenStack, LLC.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+import unittest2 as unittest
+
+from quantum.common import utils
+
+
+class TestParseMappings(unittest.TestCase):
+    def parse(self, mapping_list, unique_values=True):
+        return utils.parse_mappings(mapping_list, unique_values)
+
+    def test_parse_mappings_fails_for_missing_separator(self):
+        with self.assertRaises(ValueError):
+            self.parse(['key'])
+
+    def test_parse_mappings_fails_for_missing_key(self):
+        with self.assertRaises(ValueError):
+            self.parse([':val'])
+
+    def test_parse_mappings_fails_for_missing_value(self):
+        with self.assertRaises(ValueError):
+            self.parse(['key:'])
+
+    def test_parse_mappings_fails_for_extra_separator(self):
+        with self.assertRaises(ValueError):
+            self.parse(['key:val:junk'])
+
+    def test_parse_mappings_fails_for_duplicate_key(self):
+        with self.assertRaises(ValueError):
+            self.parse(['key:val1', 'key:val2'])
+
+    def test_parse_mappings_fails_for_duplicate_value(self):
+        with self.assertRaises(ValueError):
+            self.parse(['key1:val', 'key2:val'])
+
+    def test_parse_mappings_succeeds_for_one_mapping(self):
+        self.assertEqual(self.parse(['key:val']), {'key': 'val'})
+
+    def test_parse_mappings_succeeds_for_n_mappings(self):
+        self.assertEqual(self.parse(['key1:val1', 'key2:val2']),
+                         {'key1': 'val1', 'key2': 'val2'})
+
+    def test_parse_mappings_succeeds_for_duplicate_value(self):
+        self.assertEqual(self.parse(['key1:val', 'key2:val'], False),
+                         {'key1': 'val', 'key2': 'val'})
+
+    def test_parse_mappings_succeeds_for_no_mappings(self):
+        self.assertEqual(self.parse(['']), {})