From 589205a6c2cca4e78ba4d14008df22c077dde1d9 Mon Sep 17 00:00:00 2001 From: Terry Wilson Date: Thu, 11 Dec 2014 11:10:37 -0700 Subject: [PATCH] Move non-bridge-related OVSBridge methods to BaseOVS This patch moves several methods in OVSBridge that don't really have anything to do with OVS bridges to BaseOVS where they are a much better fit. Since OVSBridge inherits from BaseOVS, no modules using ovs_lib will have to change to support this move. Also, several module-level functions that essentially re-implement BaseOVS.run_vsctl() are moved to BaseOVS and converted to use run_vsctl. In order to avoid changing the API, the module-level functions are then reimplemented by creating a BaseOVS instance and calling the associated method. Partially-implements: blueprint vsctl-to-ovsdb Change-Id: I94aaa0bef68e407649b421db53bb59d007335f5c --- neutron/agent/linux/ovs_lib.py | 114 ++++++++---------- neutron/cmd/netns_cleanup.py | 3 +- neutron/cmd/ovs_cleanup.py | 3 +- .../openvswitch/agent/ovs_neutron_agent.py | 11 +- .../tests/unit/agent/linux/test_ovs_lib.py | 10 +- .../openvswitch/test_ovs_neutron_agent.py | 15 ++- .../tests/unit/openvswitch/test_ovs_tunnel.py | 7 +- neutron/tests/unit/test_netns_cleanup.py | 10 +- neutron/tests/unit/test_ovs_cleanup.py | 2 +- 9 files changed, 82 insertions(+), 93 deletions(-) diff --git a/neutron/agent/linux/ovs_lib.py b/neutron/agent/linux/ovs_lib.py index 1fcea9dfa..33801f2e2 100644 --- a/neutron/agent/linux/ovs_lib.py +++ b/neutron/agent/linux/ovs_lib.py @@ -127,7 +127,8 @@ class BaseOVS(object): def get_bridge_name_for_port_name(self, port_name): try: - return self.run_vsctl(['port-to-br', port_name], check_error=True) + return self.run_vsctl( + ['port-to-br', port_name], check_error=True).strip() except RuntimeError as e: with excutils.save_and_reraise_exception() as ctxt: if 'Exit code: 1\n' in str(e): @@ -136,6 +137,49 @@ class BaseOVS(object): def port_exists(self, port_name): return bool(self.get_bridge_name_for_port_name(port_name)) + def get_bridge_for_iface(self, iface): + res = self.run_vsctl(['iface-to-br', iface]) + if res: + return res.strip() + + def get_bridges(self): + res = self.run_vsctl(['list-br'], check_error=True) + return res.strip().split('\n') if res else [] + + def get_bridge_external_bridge_id(self, bridge): + res = self.run_vsctl(['br-get-external-id', bridge, 'bridge-id']) + if res: + return res.strip() + + def set_db_attribute(self, table_name, record, column, value): + args = ["set", table_name, record, "%s=%s" % (column, value)] + self.run_vsctl(args) + + def clear_db_attribute(self, table_name, record, column): + args = ["clear", table_name, record, column] + self.run_vsctl(args) + + def _db_get_map(self, table, record, column, check_error=False): + def db_str_to_map(full_str): + entries = full_str.strip("{}").split(", ") + ret = {} + for e in entries: + key, sep, value = e.partition('=') + if sep != "": + ret[key] = value.strip('"') + return ret + + output = self.db_get_val(table, record, column, + check_error=check_error) + if output: + return db_str_to_map(output) + return {} + + def db_get_val(self, table, record, column, check_error=False): + output = self.run_vsctl(["get", table, record, column], check_error) + if output: + return output.rstrip("\n\r") + class OVSBridge(BaseOVS): def __init__(self, br_name, root_helper): @@ -201,14 +245,6 @@ class OVSBridge(BaseOVS): self.run_vsctl(["--", "--if-exists", "del-port", self.br_name, port_name]) - def set_db_attribute(self, table_name, record, column, value): - args = ["set", table_name, record, "%s=%s" % (column, value)] - self.run_vsctl(args) - - def clear_db_attribute(self, table_name, record, column): - args = ["clear", table_name, record, column] - self.run_vsctl(args) - def run_ofctl(self, cmd, args, process_input=None): full_args = ["ovs-ofctl", cmd, self.br_name] + args try: @@ -294,28 +330,6 @@ class OVSBridge(BaseOVS): ] return self.add_port(local_name, *options) - def db_get_map(self, table, record, column, check_error=False): - output = self.run_vsctl(["get", table, record, column], check_error) - if output: - output_str = output.rstrip("\n\r") - return self.db_str_to_map(output_str) - return {} - - def db_get_val(self, table, record, column, check_error=False): - output = self.run_vsctl(["get", table, record, column], check_error) - if output: - return output.rstrip("\n\r") - - def db_str_to_map(self, full_str): - list = full_str.strip("{}").split(", ") - ret = {} - for e in list: - if e.find("=") == -1: - continue - arr = e.split("=") - ret[arr[0]] = arr[1].strip("\"") - return ret - def get_port_name_list(self): res = self.run_vsctl(["list-ports", self.br_name], check_error=True) if res: @@ -323,7 +337,7 @@ class OVSBridge(BaseOVS): return [] def get_port_stats(self, port_name): - return self.db_get_map("Interface", port_name, "statistics") + return self._db_get_map("Interface", port_name, "statistics") def get_xapi_iface_id(self, xs_vif_uuid): args = ["xe", "vif-param-get", "param-name=other-config", @@ -341,8 +355,8 @@ class OVSBridge(BaseOVS): edge_ports = [] port_names = self.get_port_name_list() for name in port_names: - external_ids = self.db_get_map("Interface", name, "external_ids", - check_error=True) + external_ids = self._db_get_map("Interface", name, "external_ids", + check_error=True) ofport = self.db_get_val("Interface", name, "ofport", check_error=True) if "iface-id" in external_ids and "attached-mac" in external_ids: @@ -439,7 +453,7 @@ class OVSBridge(BaseOVS): # rows since the interface identifier is unique for data in json_result['data']: port_name = data[name_idx] - switch = get_bridge_for_iface(self.root_helper, port_name) + switch = self.get_bridge_for_iface(port_name) if switch != self.br_name: continue ofport = data[ofport_idx] @@ -558,36 +572,6 @@ class DeferredOVSBridge(object): self.br.br_name) -def get_bridge_for_iface(root_helper, iface): - args = ["ovs-vsctl", "--timeout=%d" % cfg.CONF.ovs_vsctl_timeout, - "iface-to-br", iface] - try: - return utils.execute(args, root_helper=root_helper).strip() - except Exception: - LOG.exception(_LE("Interface %s not found."), iface) - return None - - -def get_bridges(root_helper): - args = ["ovs-vsctl", "--timeout=%d" % cfg.CONF.ovs_vsctl_timeout, - "list-br"] - try: - return utils.execute(args, root_helper=root_helper).strip().split("\n") - except Exception as e: - with excutils.save_and_reraise_exception(): - LOG.exception(_LE("Unable to retrieve bridges. Exception: %s"), e) - - -def get_bridge_external_bridge_id(root_helper, bridge): - args = ["ovs-vsctl", "--timeout=2", "br-get-external-id", - bridge, "bridge-id"] - try: - return utils.execute(args, root_helper=root_helper).strip() - except Exception: - LOG.exception(_LE("Bridge %s not found."), bridge) - return None - - def _build_flow_expr_str(flow_dict, cmd): flow_expr_arr = [] actions = None diff --git a/neutron/cmd/netns_cleanup.py b/neutron/cmd/netns_cleanup.py index 5f3d6dca6..5f38353fd 100644 --- a/neutron/cmd/netns_cleanup.py +++ b/neutron/cmd/netns_cleanup.py @@ -109,7 +109,8 @@ def unplug_device(conf, device): except RuntimeError: root_helper = agent_config.get_root_helper(conf) # Maybe the device is OVS port, so try to delete - bridge_name = ovs_lib.get_bridge_for_iface(root_helper, device.name) + ovs = ovs_lib.BaseOVS(root_helper) + bridge_name = ovs.get_bridge_for_iface(device.name) if bridge_name: bridge = ovs_lib.OVSBridge(bridge_name, root_helper) bridge.delete_port(device.name) diff --git a/neutron/cmd/ovs_cleanup.py b/neutron/cmd/ovs_cleanup.py index 63e4e2949..9af8702ec 100644 --- a/neutron/cmd/ovs_cleanup.py +++ b/neutron/cmd/ovs_cleanup.py @@ -86,7 +86,8 @@ def main(): configuration_bridges = set([conf.ovs_integration_bridge, conf.external_network_bridge]) - ovs_bridges = set(ovs_lib.get_bridges(conf.AGENT.root_helper)) + ovs = ovs_lib.BaseOVS(conf.AGENT.root_helper) + ovs_bridges = set(ovs.get_bridges()) available_configuration_bridges = configuration_bridges & ovs_bridges if conf.ovs_all_ports: diff --git a/neutron/plugins/openvswitch/agent/ovs_neutron_agent.py b/neutron/plugins/openvswitch/agent/ovs_neutron_agent.py index 898c0b4fb..2b6339a27 100644 --- a/neutron/plugins/openvswitch/agent/ovs_neutron_agent.py +++ b/neutron/plugins/openvswitch/agent/ovs_neutron_agent.py @@ -723,7 +723,8 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin, def setup_ancillary_bridges(self, integ_br, tun_br): '''Setup ancillary bridges - for example br-ex.''' - ovs_bridges = set(ovs_lib.get_bridges(self.root_helper)) + ovs = ovs_lib.BaseOVS(self.root_helper) + ovs_bridges = set(ovs.get_bridges()) # Remove all known bridges ovs_bridges.remove(integ_br) if self.enable_tunneling: @@ -735,9 +736,8 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin, # bridge-id's configured br_names = [] for bridge in ovs_bridges: - id = ovs_lib.get_bridge_external_bridge_id(self.root_helper, - bridge) - if id != bridge: + bridge_id = ovs.get_bridge_external_bridge_id(bridge) + if bridge_id != bridge: br_names.append(bridge) ovs_bridges.difference_update(br_names) ancillary_bridges = [] @@ -883,7 +883,8 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin, self.int_ofports = {} self.phys_ofports = {} ip_wrapper = ip_lib.IPWrapper(self.root_helper) - ovs_bridges = ovs_lib.get_bridges(self.root_helper) + ovs = ovs_lib.BaseOVS(self.root_helper) + ovs_bridges = ovs.get_bridges() for physical_network, bridge in bridge_mappings.iteritems(): LOG.info(_LI("Mapping physical network %(physical_network)s to " "bridge %(bridge)s"), diff --git a/neutron/tests/unit/agent/linux/test_ovs_lib.py b/neutron/tests/unit/agent/linux/test_ovs_lib.py index 4112f38ef..158d2bdcf 100644 --- a/neutron/tests/unit/agent/linux/test_ovs_lib.py +++ b/neutron/tests/unit/agent/linux/test_ovs_lib.py @@ -764,9 +764,11 @@ class OVS_Lib_Test(base.BaseTestCase): iface = 'tap0' br = 'br-int' root_helper = 'sudo' + if exp_timeout: + self.br.vsctl_timeout = exp_timeout self.execute.return_value = 'br-int' exp_timeout_str = self._build_timeout_opt(exp_timeout) - self.assertEqual(ovs_lib.get_bridge_for_iface(root_helper, iface), br) + self.assertEqual(self.br.get_bridge_for_iface(iface), br) self.execute.assert_called_once_with( ["ovs-vsctl", exp_timeout_str, "iface-to-br", iface], root_helper=root_helper) @@ -784,7 +786,7 @@ class OVS_Lib_Test(base.BaseTestCase): root_helper = 'sudo' self.execute.side_effect = Exception - self.assertIsNone(ovs_lib.get_bridge_for_iface(root_helper, iface)) + self.assertIsNone(self.br.get_bridge_for_iface(iface)) self.execute.assert_called_once_with( ["ovs-vsctl", self.TO, "iface-to-br", iface], root_helper=root_helper) @@ -825,9 +827,11 @@ class OVS_Lib_Test(base.BaseTestCase): def _test_get_bridges(self, exp_timeout=None): bridges = ['br-int', 'br-ex'] root_helper = 'sudo' + if exp_timeout: + self.br.vsctl_timeout = exp_timeout self.execute.return_value = 'br-int\nbr-ex\n' timeout_str = self._build_timeout_opt(exp_timeout) - self.assertEqual(ovs_lib.get_bridges(root_helper), bridges) + self.assertEqual(self.br.get_bridges(), bridges) self.execute.assert_called_once_with( ["ovs-vsctl", timeout_str, "list-br"], root_helper=root_helper) diff --git a/neutron/tests/unit/openvswitch/test_ovs_neutron_agent.py b/neutron/tests/unit/openvswitch/test_ovs_neutron_agent.py index b6451f637..473479eac 100644 --- a/neutron/tests/unit/openvswitch/test_ovs_neutron_agent.py +++ b/neutron/tests/unit/openvswitch/test_ovs_neutron_agent.py @@ -125,8 +125,7 @@ class TestOvsNeutronAgent(base.BaseTestCase): return_value='00:00:00:00:00:01'), mock.patch('neutron.agent.linux.utils.get_interface_mac', return_value='00:00:00:00:00:01'), - mock.patch('neutron.agent.linux.ovs_lib.' - 'get_bridges'), + mock.patch('neutron.agent.linux.ovs_lib.BaseOVS.get_bridges'), mock.patch('neutron.openstack.common.loopingcall.' 'FixedIntervalLoopingCall', new=MockFixedIntervalLoopingCall)): @@ -1130,7 +1129,7 @@ class TestOvsNeutronAgent(base.BaseTestCase): mock.patch.object(ip_lib.IpLinkCommand, "delete"), mock.patch.object(ip_lib.IpLinkCommand, "set_up"), mock.patch.object(ip_lib.IpLinkCommand, "set_mtu"), - mock.patch.object(ovs_lib, "get_bridges") + mock.patch.object(ovs_lib.BaseOVS, "get_bridges") ) as (devex_fn, sysexit_fn, utilsexec_fn, remflows_fn, ovs_addfl_fn, ovs_addport_fn, ovs_delport_fn, br_addport_fn, br_delport_fn, addveth_fn, linkdel_fn, linkset_fn, linkmtu_fn, get_br_fn): @@ -1635,7 +1634,7 @@ class AncillaryBridgesTest(base.BaseTestCase): def _test_ancillary_bridges(self, bridges, ancillary): device_ids = ancillary[:] - def pullup_side_effect(self, *args): + def pullup_side_effect(*args): # Check that the device_id exists, if it does return it # if it does not return None try: @@ -1655,11 +1654,11 @@ class AncillaryBridgesTest(base.BaseTestCase): return_value='00:00:00:00:00:01'), mock.patch('neutron.agent.linux.ovs_lib.OVSBridge.' 'set_secure_mode'), - mock.patch('neutron.agent.linux.ovs_lib.get_bridges', + mock.patch('neutron.agent.linux.ovs_lib.BaseOVS.get_bridges', return_value=bridges), - mock.patch( - 'neutron.agent.linux.ovs_lib.get_bridge_external_bridge_id', - side_effect=pullup_side_effect)): + mock.patch('neutron.agent.linux.ovs_lib.BaseOVS.' + 'get_bridge_external_bridge_id', + side_effect=pullup_side_effect)): self.agent = ovs_neutron_agent.OVSNeutronAgent(**self.kwargs) self.assertEqual(len(ancillary), len(self.agent.ancillary_brs)) if ancillary: diff --git a/neutron/tests/unit/openvswitch/test_ovs_tunnel.py b/neutron/tests/unit/openvswitch/test_ovs_tunnel.py index 940008395..e729db5ed 100644 --- a/neutron/tests/unit/openvswitch/test_ovs_tunnel.py +++ b/neutron/tests/unit/openvswitch/test_ovs_tunnel.py @@ -127,7 +127,8 @@ class TunnelTest(base.BaseTestCase): add_veth = self.ipwrapper.return_value.add_veth add_veth.return_value = [self.inta, self.intb] - self.get_bridges = mock.patch.object(ovs_lib, 'get_bridges').start() + self.get_bridges = mock.patch.object(ovs_lib.BaseOVS, + 'get_bridges').start() self.get_bridges.return_value = [self.INT_BRIDGE, self.TUN_BRIDGE, self.MAP_TUN_BRIDGE] @@ -242,7 +243,7 @@ class TunnelTest(base.BaseTestCase): self.ipdevice_expected = [] self.ipwrapper_expected = [mock.call('sudo')] - self.get_bridges_expected = [mock.call('sudo'), mock.call('sudo')] + self.get_bridges_expected = [mock.call(), mock.call()] self.inta_expected = [] self.intb_expected = [] @@ -669,7 +670,7 @@ class TunnelTestUseVethInterco(TunnelTest): 'phy-%s' % self.MAP_TUN_BRIDGE) ] - self.get_bridges_expected = [mock.call('sudo'), mock.call('sudo')] + self.get_bridges_expected = [mock.call(), mock.call()] self.inta_expected = [mock.call.link.set_up()] self.intb_expected = [mock.call.link.set_up()] diff --git a/neutron/tests/unit/test_netns_cleanup.py b/neutron/tests/unit/test_netns_cleanup.py index 59383f428..9c6ebd889 100644 --- a/neutron/tests/unit/test_netns_cleanup.py +++ b/neutron/tests/unit/test_netns_cleanup.py @@ -103,7 +103,7 @@ class TestNetnsCleanup(base.BaseTestCase): with mock.patch('neutron.agent.linux.ovs_lib.OVSBridge') as ovs_br_cls: br_patch = mock.patch( - 'neutron.agent.linux.ovs_lib.get_bridge_for_iface') + 'neutron.agent.linux.ovs_lib.BaseOVS.get_bridge_for_iface') with br_patch as mock_get_bridge_for_iface: mock_get_bridge_for_iface.return_value = 'br-int' ovs_bridge = mock.Mock() @@ -111,8 +111,7 @@ class TestNetnsCleanup(base.BaseTestCase): util.unplug_device(conf, device) - mock_get_bridge_for_iface.assert_called_once_with( - conf.AGENT.root_helper, 'tap1') + mock_get_bridge_for_iface.assert_called_once_with('tap1') ovs_br_cls.assert_called_once_with('br-int', conf.AGENT.root_helper) ovs_bridge.assert_has_calls( @@ -128,7 +127,7 @@ class TestNetnsCleanup(base.BaseTestCase): with mock.patch('neutron.agent.linux.ovs_lib.OVSBridge') as ovs_br_cls: br_patch = mock.patch( - 'neutron.agent.linux.ovs_lib.get_bridge_for_iface') + 'neutron.agent.linux.ovs_lib.BaseOVS.get_bridge_for_iface') with br_patch as mock_get_bridge_for_iface: with mock.patch.object(util.LOG, 'debug') as debug: mock_get_bridge_for_iface.return_value = None @@ -137,8 +136,7 @@ class TestNetnsCleanup(base.BaseTestCase): util.unplug_device(conf, device) - mock_get_bridge_for_iface.assert_called_once_with( - conf.AGENT.root_helper, 'tap1') + mock_get_bridge_for_iface.assert_called_once_with('tap1') self.assertEqual(ovs_br_cls.mock_calls, []) self.assertTrue(debug.called) diff --git a/neutron/tests/unit/test_ovs_cleanup.py b/neutron/tests/unit/test_ovs_cleanup.py index de7900039..2e474c127 100644 --- a/neutron/tests/unit/test_ovs_cleanup.py +++ b/neutron/tests/unit/test_ovs_cleanup.py @@ -49,7 +49,7 @@ class TestOVSCleanup(base.BaseTestCase): mock.patch('neutron.common.config.setup_logging'), mock.patch('neutron.cmd.ovs_cleanup.setup_conf', return_value=conf), - mock.patch('neutron.agent.linux.ovs_lib.get_bridges', + mock.patch('neutron.agent.linux.ovs_lib.BaseOVS.get_bridges', return_value=bridges), mock.patch('neutron.agent.linux.ovs_lib.OVSBridge'), mock.patch.object(util, 'collect_neutron_ports', -- 2.45.2