From 90dc4691c888944d3d1691d06a739e8dc54857d6 Mon Sep 17 00:00:00 2001 From: Bob Kukura Date: Tue, 13 Nov 2012 13:50:53 -0500 Subject: [PATCH] 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 | 31 ++++++++++ .../agent/linuxbridge_quantum_agent.py | 24 ++++---- .../openvswitch/agent/ovs_quantum_agent.py | 31 +++------- .../openvswitch/test_ovs_quantum_agent.py | 24 -------- quantum/tests/unit/test_common_utils.py | 60 +++++++++++++++++++ 5 files changed, 110 insertions(+), 60 deletions(-) create mode 100644 quantum/tests/unit/test_common_utils.py diff --git a/quantum/common/utils.py b/quantum/common/utils.py index 8b9015cf5f..8df274ade2 100644 --- a/quantum/common/utils.py +++ b/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 ':' + :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 diff --git a/quantum/plugins/linuxbridge/agent/linuxbridge_quantum_agent.py b/quantum/plugins/linuxbridge/agent/linuxbridge_quantum_agent.py index 89ad6941ed..3f10e97d1a 100755 --- a/quantum/plugins/linuxbridge/agent/linuxbridge_quantum_agent.py +++ b/quantum/plugins/linuxbridge/agent/linuxbridge_quantum_agent.py @@ -35,6 +35,7 @@ from quantum.agent import rpc as agent_rpc 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() diff --git a/quantum/plugins/openvswitch/agent/ovs_quantum_agent.py b/quantum/plugins/openvswitch/agent/ovs_quantum_agent.py index a76da62724..c7f46c2085 100755 --- a/quantum/plugins/openvswitch/agent/ovs_quantum_agent.py +++ b/quantum/plugins/openvswitch/agent/ovs_quantum_agent.py @@ -32,6 +32,7 @@ from quantum.agent import rpc as agent_rpc 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 @@ class OVSQuantumAgent(object): :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 @@ class OVSQuantumAgent(object): 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 @@ class OVSQuantumAgent(object): 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 - ':' - :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, diff --git a/quantum/tests/unit/openvswitch/test_ovs_quantum_agent.py b/quantum/tests/unit/openvswitch/test_ovs_quantum_agent.py index f7c78c60d4..0067d4d2ab 100644 --- a/quantum/tests/unit/openvswitch/test_ovs_quantum_agent.py +++ b/quantum/tests/unit/openvswitch/test_ovs_quantum_agent.py @@ -22,30 +22,6 @@ from quantum.plugins.openvswitch.agent import ovs_quantum_agent 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): diff --git a/quantum/tests/unit/test_common_utils.py b/quantum/tests/unit/test_common_utils.py new file mode 100644 index 0000000000..762c90e6e4 --- /dev/null +++ b/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(['']), {})