From 6b4d006344e38dcbbc0048b17ca41af16e13e5a2 Mon Sep 17 00:00:00 2001 From: Sergey Belous Date: Thu, 15 Jan 2015 18:19:51 +0300 Subject: [PATCH] Refactor checks for device existence The code calling driver.plug() shouldn't check for the device existence, it's a duplicate and it's an expensive call. Move check for device existence to base LinuxInterfaceDriver.plug() to remove code duplication. Make plug_new() abstract instead. Change-Id: Id118a64012ad10b197ba681ce5f1b2742eb135b4 Closes-Bug:1348703 --- neutron/agent/l3/dvr_fip_ns.py | 15 +- neutron/agent/l3/ha_router.py | 13 +- neutron/agent/l3/router_info.py | 23 +-- neutron/agent/linux/interface.py | 189 +++++++++--------- neutron/tests/unit/agent/l3/test_agent.py | 2 +- .../tests/unit/agent/linux/test_interface.py | 2 +- 6 files changed, 116 insertions(+), 128 deletions(-) diff --git a/neutron/agent/l3/dvr_fip_ns.py b/neutron/agent/l3/dvr_fip_ns.py index b959e5016..050cb6331 100644 --- a/neutron/agent/l3/dvr_fip_ns.py +++ b/neutron/agent/l3/dvr_fip_ns.py @@ -94,14 +94,13 @@ class FipNamespace(namespaces.Namespace): def _gateway_added(self, ex_gw_port, interface_name): """Add Floating IP gateway port.""" ns_name = self.get_name() - if not ip_lib.device_exists(interface_name, namespace=ns_name): - self.driver.plug(ex_gw_port['network_id'], - ex_gw_port['id'], - interface_name, - ex_gw_port['mac_address'], - bridge=self.agent_conf.external_network_bridge, - namespace=ns_name, - prefix=FIP_EXT_DEV_PREFIX) + self.driver.plug(ex_gw_port['network_id'], + ex_gw_port['id'], + interface_name, + ex_gw_port['mac_address'], + bridge=self.agent_conf.external_network_bridge, + namespace=ns_name, + prefix=FIP_EXT_DEV_PREFIX) ip_cidrs = common_utils.fixed_ip_cidrs(ex_gw_port['fixed_ips']) self.driver.init_l3(interface_name, ip_cidrs, namespace=ns_name) diff --git a/neutron/agent/l3/ha_router.py b/neutron/agent/l3/ha_router.py index ebccf32b4..ece0f0a31 100644 --- a/neutron/agent/l3/ha_router.py +++ b/neutron/agent/l3/ha_router.py @@ -257,13 +257,12 @@ class HaRouter(router.RouterInfo): port_id = port['id'] interface_name = self.get_internal_device_name(port_id) - if not ip_lib.device_exists(interface_name, namespace=self.ns_name): - self.driver.plug(port['network_id'], - port_id, - interface_name, - port['mac_address'], - namespace=self.ns_name, - prefix=router.INTERNAL_DEV_PREFIX) + self.driver.plug(port['network_id'], + port_id, + interface_name, + port['mac_address'], + namespace=self.ns_name, + prefix=router.INTERNAL_DEV_PREFIX) self._disable_ipv6_addressing_on_interface(interface_name) for ip_cidr in common_utils.fixed_ip_cidrs(port['fixed_ips']): diff --git a/neutron/agent/l3/router_info.py b/neutron/agent/l3/router_info.py index b9b54b610..3f7684b84 100644 --- a/neutron/agent/l3/router_info.py +++ b/neutron/agent/l3/router_info.py @@ -281,11 +281,9 @@ class RouterInfo(object): def _internal_network_added(self, ns_name, network_id, port_id, fixed_ips, mac_address, interface_name, prefix): - if not ip_lib.device_exists(interface_name, - namespace=ns_name): - self.driver.plug(network_id, port_id, interface_name, mac_address, - namespace=ns_name, - prefix=prefix) + self.driver.plug(network_id, port_id, interface_name, mac_address, + namespace=ns_name, + prefix=prefix) ip_cidrs = common_utils.fixed_ip_cidrs(fixed_ips) self.driver.init_l3(interface_name, ip_cidrs, namespace=ns_name) @@ -418,14 +416,13 @@ class RouterInfo(object): for ip in floating_ips] def _plug_external_gateway(self, ex_gw_port, interface_name, ns_name): - if not ip_lib.device_exists(interface_name, namespace=ns_name): - self.driver.plug(ex_gw_port['network_id'], - ex_gw_port['id'], - interface_name, - ex_gw_port['mac_address'], - bridge=self.agent_conf.external_network_bridge, - namespace=ns_name, - prefix=EXTERNAL_DEV_PREFIX) + self.driver.plug(ex_gw_port['network_id'], + ex_gw_port['id'], + interface_name, + ex_gw_port['mac_address'], + bridge=self.agent_conf.external_network_bridge, + namespace=ns_name, + prefix=EXTERNAL_DEV_PREFIX) def _external_gateway_added(self, ex_gw_port, interface_name, ns_name, preserve_ips): diff --git a/neutron/agent/linux/interface.py b/neutron/agent/linux/interface.py index 99654cfc8..2e0af0d60 100644 --- a/neutron/agent/linux/interface.py +++ b/neutron/agent/linux/interface.py @@ -179,9 +179,18 @@ class LinuxInterfaceDriver(object): ['sysctl', '-w', 'net.ipv6.conf.%s.accept_ra=2' % dev_name]) @abc.abstractmethod + def plug_new(self, network_id, port_id, device_name, mac_address, + bridge=None, namespace=None, prefix=None): + """Plug in the interface only for new devices that don't exist yet.""" + def plug(self, network_id, port_id, device_name, mac_address, bridge=None, namespace=None, prefix=None): - """Plug in the interface.""" + if not ip_lib.device_exists(device_name, + namespace=namespace): + self.plug_new(network_id, port_id, device_name, mac_address, + bridge, namespace, prefix) + else: + LOG.info(_LI("Device %s already exists"), device_name) @abc.abstractmethod def unplug(self, device_name, bridge=None, namespace=None, prefix=None): @@ -189,8 +198,8 @@ class LinuxInterfaceDriver(object): class NullDriver(LinuxInterfaceDriver): - def plug(self, network_id, port_id, device_name, mac_address, - bridge=None, namespace=None, prefix=None): + def plug_new(self, network_id, port_id, device_name, mac_address, + bridge=None, namespace=None, prefix=None): pass def unplug(self, device_name, bridge=None, namespace=None, prefix=None): @@ -224,48 +233,44 @@ class OVSInterfaceDriver(LinuxInterfaceDriver): ovs = ovs_lib.OVSBridge(bridge) ovs.replace_port(device_name, *attrs) - def plug(self, network_id, port_id, device_name, mac_address, - bridge=None, namespace=None, prefix=None): + def plug_new(self, network_id, port_id, device_name, mac_address, + bridge=None, namespace=None, prefix=None): """Plug in the interface.""" if not bridge: bridge = self.conf.ovs_integration_bridge - if not ip_lib.device_exists(device_name, namespace=namespace): + self.check_bridge_exists(bridge) - self.check_bridge_exists(bridge) + ip = ip_lib.IPWrapper() + tap_name = self._get_tap_name(device_name, prefix) - ip = ip_lib.IPWrapper() - tap_name = self._get_tap_name(device_name, prefix) + if self.conf.ovs_use_veth: + # Create ns_dev in a namespace if one is configured. + root_dev, ns_dev = ip.add_veth(tap_name, + device_name, + namespace2=namespace) + else: + ns_dev = ip.device(device_name) + internal = not self.conf.ovs_use_veth + self._ovs_add_port(bridge, tap_name, port_id, mac_address, + internal=internal) + + ns_dev.link.set_address(mac_address) + + if self.conf.network_device_mtu: + ns_dev.link.set_mtu(self.conf.network_device_mtu) if self.conf.ovs_use_veth: - # Create ns_dev in a namespace if one is configured. - root_dev, ns_dev = ip.add_veth(tap_name, - device_name, - namespace2=namespace) - else: - ns_dev = ip.device(device_name) - - internal = not self.conf.ovs_use_veth - self._ovs_add_port(bridge, tap_name, port_id, mac_address, - internal=internal) - - ns_dev.link.set_address(mac_address) - - if self.conf.network_device_mtu: - ns_dev.link.set_mtu(self.conf.network_device_mtu) - if self.conf.ovs_use_veth: - root_dev.link.set_mtu(self.conf.network_device_mtu) - - # Add an interface created by ovs to the namespace. - if not self.conf.ovs_use_veth and namespace: - namespace_obj = ip.ensure_namespace(namespace) - namespace_obj.add_device_to_namespace(ns_dev) - - ns_dev.link.set_up() - if self.conf.ovs_use_veth: - root_dev.link.set_up() - else: - LOG.info(_LI("Device %s already exists"), device_name) + root_dev.link.set_mtu(self.conf.network_device_mtu) + + # Add an interface created by ovs to the namespace. + if not self.conf.ovs_use_veth and namespace: + namespace_obj = ip.ensure_namespace(namespace) + namespace_obj.add_device_to_namespace(ns_dev) + + ns_dev.link.set_up() + if self.conf.ovs_use_veth: + root_dev.link.set_up() def unplug(self, device_name, bridge=None, namespace=None, prefix=None): """Unplug the interface.""" @@ -289,34 +294,30 @@ class OVSInterfaceDriver(LinuxInterfaceDriver): class MidonetInterfaceDriver(LinuxInterfaceDriver): - def plug(self, network_id, port_id, device_name, mac_address, - bridge=None, namespace=None, prefix=None): + def plug_new(self, network_id, port_id, device_name, mac_address, + bridge=None, namespace=None, prefix=None): """This method is called by the Dhcp agent or by the L3 agent when a new network is created """ - if not ip_lib.device_exists(device_name, namespace=namespace): - ip = ip_lib.IPWrapper() - tap_name = device_name.replace(prefix or n_const.TAP_DEVICE_PREFIX, - n_const.TAP_DEVICE_PREFIX) + ip = ip_lib.IPWrapper() + tap_name = device_name.replace(prefix or n_const.TAP_DEVICE_PREFIX, + n_const.TAP_DEVICE_PREFIX) - # Create ns_dev in a namespace if one is configured. - root_dev, ns_dev = ip.add_veth(tap_name, device_name, - namespace2=namespace) + # Create ns_dev in a namespace if one is configured. + root_dev, ns_dev = ip.add_veth(tap_name, device_name, + namespace2=namespace) - ns_dev.link.set_address(mac_address) - - # Add an interface created by ovs to the namespace. - namespace_obj = ip.ensure_namespace(namespace) - namespace_obj.add_device_to_namespace(ns_dev) + ns_dev.link.set_address(mac_address) - ns_dev.link.set_up() - root_dev.link.set_up() + # Add an interface created by ovs to the namespace. + namespace_obj = ip.ensure_namespace(namespace) + namespace_obj.add_device_to_namespace(ns_dev) - cmd = ['mm-ctl', '--bind-port', port_id, device_name] - utils.execute(cmd, run_as_root=True) + ns_dev.link.set_up() + root_dev.link.set_up() - else: - LOG.info(_LI("Device %s already exists"), device_name) + cmd = ['mm-ctl', '--bind-port', port_id, device_name] + utils.execute(cmd, run_as_root=True) def unplug(self, device_name, bridge=None, namespace=None, prefix=None): # the port will be deleted by the dhcp agent that will call the plugin @@ -348,33 +349,29 @@ class IVSInterfaceDriver(LinuxInterfaceDriver): cmd = ['ivs-ctl', 'add-port', device_name] utils.execute(cmd, run_as_root=True) - def plug(self, network_id, port_id, device_name, mac_address, - bridge=None, namespace=None, prefix=None): + def plug_new(self, network_id, port_id, device_name, mac_address, + bridge=None, namespace=None, prefix=None): """Plug in the interface.""" - if not ip_lib.device_exists(device_name, namespace=namespace): + ip = ip_lib.IPWrapper() + tap_name = self._get_tap_name(device_name, prefix) - ip = ip_lib.IPWrapper() - tap_name = self._get_tap_name(device_name, prefix) + root_dev, ns_dev = ip.add_veth(tap_name, device_name) - root_dev, ns_dev = ip.add_veth(tap_name, device_name) + self._ivs_add_port(tap_name, port_id, mac_address) - self._ivs_add_port(tap_name, port_id, mac_address) + ns_dev = ip.device(device_name) + ns_dev.link.set_address(mac_address) - ns_dev = ip.device(device_name) - ns_dev.link.set_address(mac_address) + if self.conf.network_device_mtu: + ns_dev.link.set_mtu(self.conf.network_device_mtu) + root_dev.link.set_mtu(self.conf.network_device_mtu) - if self.conf.network_device_mtu: - ns_dev.link.set_mtu(self.conf.network_device_mtu) - root_dev.link.set_mtu(self.conf.network_device_mtu) + if namespace: + namespace_obj = ip.ensure_namespace(namespace) + namespace_obj.add_device_to_namespace(ns_dev) - if namespace: - namespace_obj = ip.ensure_namespace(namespace) - namespace_obj.add_device_to_namespace(ns_dev) - - ns_dev.link.set_up() - root_dev.link.set_up() - else: - LOG.info(_LI("Device %s already exists"), device_name) + ns_dev.link.set_up() + root_dev.link.set_up() def unplug(self, device_name, bridge=None, namespace=None, prefix=None): """Unplug the interface.""" @@ -395,29 +392,25 @@ class BridgeInterfaceDriver(LinuxInterfaceDriver): DEV_NAME_PREFIX = 'ns-' - def plug(self, network_id, port_id, device_name, mac_address, - bridge=None, namespace=None, prefix=None): + def plug_new(self, network_id, port_id, device_name, mac_address, + bridge=None, namespace=None, prefix=None): """Plugin the interface.""" - if not ip_lib.device_exists(device_name, namespace=namespace): - ip = ip_lib.IPWrapper() + ip = ip_lib.IPWrapper() - # Enable agent to define the prefix - tap_name = device_name.replace(prefix or self.DEV_NAME_PREFIX, - n_const.TAP_DEVICE_PREFIX) - # Create ns_veth in a namespace if one is configured. - root_veth, ns_veth = ip.add_veth(tap_name, device_name, - namespace2=namespace) - ns_veth.link.set_address(mac_address) + # Enable agent to define the prefix + tap_name = device_name.replace(prefix or self.DEV_NAME_PREFIX, + n_const.TAP_DEVICE_PREFIX) + # Create ns_veth in a namespace if one is configured. + root_veth, ns_veth = ip.add_veth(tap_name, device_name, + namespace2=namespace) + ns_veth.link.set_address(mac_address) - if self.conf.network_device_mtu: - root_veth.link.set_mtu(self.conf.network_device_mtu) - ns_veth.link.set_mtu(self.conf.network_device_mtu) + if self.conf.network_device_mtu: + root_veth.link.set_mtu(self.conf.network_device_mtu) + ns_veth.link.set_mtu(self.conf.network_device_mtu) - root_veth.link.set_up() - ns_veth.link.set_up() - - else: - LOG.info(_LI("Device %s already exists"), device_name) + root_veth.link.set_up() + ns_veth.link.set_up() def unplug(self, device_name, bridge=None, namespace=None, prefix=None): """Unplug the interface.""" @@ -471,8 +464,8 @@ class MetaInterfaceDriver(LinuxInterfaceDriver): driver = self._get_driver_by_network_id(port.network_id) return driver.get_device_name(port) - def plug(self, network_id, port_id, device_name, mac_address, - bridge=None, namespace=None, prefix=None): + def plug_new(self, network_id, port_id, device_name, mac_address, + bridge=None, namespace=None, prefix=None): driver = self._get_driver_by_network_id(network_id) ret = driver.plug(network_id, port_id, device_name, mac_address, bridge=bridge, namespace=namespace, prefix=prefix) diff --git a/neutron/tests/unit/agent/l3/test_agent.py b/neutron/tests/unit/agent/l3/test_agent.py index 058452ce9..87b6afffb 100644 --- a/neutron/tests/unit/agent/l3/test_agent.py +++ b/neutron/tests/unit/agent/l3/test_agent.py @@ -764,7 +764,7 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): 'port_id': _uuid()}]} router[l3_constants.FLOATINGIP_KEY] = fake_fip['floatingips'] ri.external_gateway_updated(ex_gw_port, interface_name) - self.assertEqual(self.mock_driver.plug.call_count, 0) + self.assertEqual(1, self.mock_driver.plug.call_count) self.assertEqual(self.mock_driver.init_l3.call_count, 1) exp_arp_calls = [mock.call(ri.ns_name, interface_name, '20.0.0.30', mock.ANY)] diff --git a/neutron/tests/unit/agent/linux/test_interface.py b/neutron/tests/unit/agent/linux/test_interface.py index df605eb57..0cafc8add 100644 --- a/neutron/tests/unit/agent/linux/test_interface.py +++ b/neutron/tests/unit/agent/linux/test_interface.py @@ -27,7 +27,7 @@ from neutron.tests import base class BaseChild(interface.LinuxInterfaceDriver): - def plug(*args): + def plug_new(*args): pass def unplug(*args): -- 2.45.2