Avoid unnecessarily checking the existence of a device

Plugging a device usually involves checking for the existence of the
device twice, once before calling plug and once after.  It turns out
that these calls are expensive, often taking a half second or more
each.  For that reason, it is worth the effort to make sure we check
only once.

The device driver is now responsible for cleanly plugging/unplugging
the device without knowing whether it exists or not.  Pushing this
responsibility to the device driver allows implementing it more
efficiently in terms of calls made out to the operating system.

This is targetted at the neutron-tempest-parallel bp because it shaves
time off the time to set up a router, something that hinders parallel
performance.

Change-Id: I391fafe68b76e1c620d2b25e8613ba507fd25dfd
Partial-Bug: #1287824

diff --git a/neutron/agent/l3_agent.py b/neutron/agent/l3_agent.py
index 5c8c8757e582b336ae379f0826a5b4aec71e6183..0d350a84f171ec95422317c269187811e07831c1 100644 (file)
--- a/neutron/agent/l3_agent.py
+++ b/neutron/agent/l3_agent.py
@@ -602,15 +602,12 @@ class L3NATAgent(firewall_l3_agent.FWaaSL3AgentRpcCallback, manager.Manager):
     def external_gateway_added(self, ri, ex_gw_port,
                                interface_name, internal_cidrs):
 
-        if not ip_lib.device_exists(interface_name,
-                                    root_helper=self.root_helper,
-                                    namespace=ri.ns_name()):
-            self.driver.plug(ex_gw_port['network_id'],
-                             ex_gw_port['id'], interface_name,
-                             ex_gw_port['mac_address'],
-                             bridge=self.conf.external_network_bridge,
-                             namespace=ri.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.conf.external_network_bridge,
+                         namespace=ri.ns_name(),
+                         prefix=EXTERNAL_DEV_PREFIX)
 
         # Compute a list of addresses this router is supposed to have.
         # This avoids unnecessarily removing those addresses and
@@ -639,13 +636,10 @@ class L3NATAgent(firewall_l3_agent.FWaaSL3AgentRpcCallback, manager.Manager):
     def external_gateway_removed(self, ri, ex_gw_port,
                                  interface_name, internal_cidrs):
 
-        if ip_lib.device_exists(interface_name,
-                                root_helper=self.root_helper,
-                                namespace=ri.ns_name()):
-            self.driver.unplug(interface_name,
-                               bridge=self.conf.external_network_bridge,
-                               namespace=ri.ns_name(),
-                               prefix=EXTERNAL_DEV_PREFIX)
+        self.driver.unplug(interface_name,
+                           bridge=self.conf.external_network_bridge,
+                           namespace=ri.ns_name(),
+                           prefix=EXTERNAL_DEV_PREFIX)
 
     def metadata_filter_rules(self):
         rules = []

diff --git a/neutron/agent/linux/interface.py b/neutron/agent/linux/interface.py
index c763b03a30d62771f73cb6be5ef979d64b3e12ac..c3a97ecb209613f5f5d26ddf724cc3edb5e938ee 100644 (file)
--- a/neutron/agent/linux/interface.py
+++ b/neutron/agent/linux/interface.py
@@ -162,12 +162,12 @@ class OVSInterfaceDriver(LinuxInterfaceDriver):
         if not bridge:
             bridge = self.conf.ovs_integration_bridge
 
-        self.check_bridge_exists(bridge)
-
         if not ip_lib.device_exists(device_name,
                                     self.root_helper,
                                     namespace=namespace):
 
+            self.check_bridge_exists(bridge)
+
             ip = ip_lib.IPWrapper(self.root_helper)
             tap_name = self._get_tap_name(device_name, prefix)
 
@@ -199,7 +199,7 @@ class OVSInterfaceDriver(LinuxInterfaceDriver):
             if self.conf.ovs_use_veth:
                 root_dev.link.set_up()
         else:
-            LOG.warn(_("Device %s already exists"), device_name)
+            LOG.info(_("Device %s already exists"), device_name)
 
     def unplug(self, device_name, bridge=None, namespace=None, prefix=None):
         """Unplug the interface."""
@@ -253,14 +253,17 @@ class MidonetInterfaceDriver(LinuxInterfaceDriver):
             utils.execute(cmd, self.root_helper)
 
         else:
-            LOG.warn(_("Device %s already exists"), device_name)
+            LOG.info(_("Device %s already exists"), device_name)
 
     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
         device = ip_lib.IPDevice(device_name,
                                  self.root_helper,
                                  namespace)
-        device.link.delete()
+        try:
+            device.link.delete()
+        except RuntimeError:
+            LOG.error(_("Failed unplugging interface '%s'"), device_name)
         LOG.debug(_("Unplugged interface '%s'"), device_name)
 
         ip_lib.IPWrapper(
@@ -312,7 +315,7 @@ class IVSInterfaceDriver(LinuxInterfaceDriver):
             ns_dev.link.set_up()
             root_dev.link.set_up()
         else:
-            LOG.warn(_("Device %s already exists"), device_name)
+            LOG.info(_("Device %s already exists"), device_name)
 
     def unplug(self, device_name, bridge=None, namespace=None, prefix=None):
         """Unplug the interface."""
@@ -361,7 +364,7 @@ class BridgeInterfaceDriver(LinuxInterfaceDriver):
             ns_veth.link.set_up()
 
         else:
-            LOG.warn(_("Device %s already exists"), device_name)
+            LOG.info(_("Device %s already exists"), device_name)
 
     def unplug(self, device_name, bridge=None, namespace=None, prefix=None):
         """Unplug the interface."""

diff --git a/neutron/tests/unit/test_l3_agent.py b/neutron/tests/unit/test_l3_agent.py
index 87f4df520ed3e18db3d045da0d7c049870591a2b..fe11deeb22909e7ef6635860474e7c282dc08f0b 100644 (file)
--- a/neutron/tests/unit/test_l3_agent.py
+++ b/neutron/tests/unit/test_l3_agent.py
@@ -388,6 +388,7 @@ class TestBasicRouterOperations(base.BaseTestCase):
         agent.process_router_floating_ip_nat_rules = mock.Mock()
         agent.process_router_floating_ip_addresses.return_value = {
             fake_fip_id: 'ACTIVE'}
+        agent.external_gateway_added = mock.Mock()
         router = self._prepare_router_data()
         fake_floatingips1 = {'floatingips': [
             {'id': fake_fip_id,
@@ -577,6 +578,7 @@ class TestBasicRouterOperations(base.BaseTestCase):
         router = self._prepare_router_data(enable_snat=True)
         ri = l3_agent.RouterInfo(router['id'], self.conf.root_helper,
                                  self.conf.use_namespaces, router=router)
+        agent.external_gateway_added = mock.Mock()
         # Process with NAT
         agent.process_router(ri)
         orig_nat_rules = ri.iptables_manager.ipv4['nat'].rules[:]
@@ -598,6 +600,7 @@ class TestBasicRouterOperations(base.BaseTestCase):
         router = self._prepare_router_data(enable_snat=False)
         ri = l3_agent.RouterInfo(router['id'], self.conf.root_helper,
                                  self.conf.use_namespaces, router=router)
+        agent.external_gateway_added = mock.Mock()
         # Process without NAT
         agent.process_router(ri)
         orig_nat_rules = ri.iptables_manager.ipv4['nat'].rules[:]
@@ -619,6 +622,7 @@ class TestBasicRouterOperations(base.BaseTestCase):
         router = self._prepare_router_data()
         ri = l3_agent.RouterInfo(router['id'], self.conf.root_helper,
                                  self.conf.use_namespaces, router=router)
+        agent.external_gateway_added = mock.Mock()
         # Process with NAT
         agent.process_router(ri)
         orig_nat_rules = ri.iptables_manager.ipv4['nat'].rules[:]
@@ -648,6 +652,7 @@ class TestBasicRouterOperations(base.BaseTestCase):
         router = self._prepare_router_data(num_internal_ports=2)
         ri = l3_agent.RouterInfo(router['id'], self.conf.root_helper,
                                  self.conf.use_namespaces, router=router)
+        agent.external_gateway_added = mock.Mock()
         # Process with NAT
         agent.process_router(ri)
         orig_nat_rules = ri.iptables_manager.ipv4['nat'].rules[:]
@@ -669,6 +674,7 @@ class TestBasicRouterOperations(base.BaseTestCase):
         router = self._prepare_router_data()
         ri = l3_agent.RouterInfo(router['id'], self.conf.root_helper,
                                  self.conf.use_namespaces, router=router)
+        agent.external_gateway_added = mock.Mock()
         with mock.patch.object(
                 l3_agent.L3NATAgent,
                 'internal_network_added') as internal_network_added:
@@ -694,6 +700,7 @@ class TestBasicRouterOperations(base.BaseTestCase):
         router = self._prepare_router_data()
         ri = l3_agent.RouterInfo(router['id'], self.conf.root_helper,
                                  self.conf.use_namespaces, router=router)
+        agent.external_gateway_added = mock.Mock()
         # add an internal port
         agent.process_router(ri)
 
@@ -734,6 +741,7 @@ class TestBasicRouterOperations(base.BaseTestCase):
 
             ri = l3_agent.RouterInfo(router['id'], self.conf.root_helper,
                                      self.conf.use_namespaces, router=router)
+            agent.external_gateway_added = mock.Mock()
             agent.process_router(ri)
             # Assess the call for putting the floating IP up was performed
             mock_update_fip_status.assert_called_once_with(

diff --git a/neutron/tests/unit/test_linux_interface.py b/neutron/tests/unit/test_linux_interface.py
index 09bf8df27fdc9a6c00835a393b8e905355eea1cd..7c84623fc0071e449cc1aa1912302ccfaa10395a 100644 (file)
--- a/neutron/tests/unit/test_linux_interface.py
+++ b/neutron/tests/unit/test_linux_interface.py
@@ -336,7 +336,7 @@ class TestBridgeInterfaceDriver(TestBase):
 
     def test_plug_dev_exists(self):
         self.device_exists.return_value = True
-        with mock.patch('neutron.agent.linux.interface.LOG.warn') as log:
+        with mock.patch('neutron.agent.linux.interface.LOG.info') as log:
             br = interface.BridgeInterfaceDriver(self.conf)
             br.plug('01234567-1234-1234-99',
                     'port-1234',