Merge "Fix for race condition during netns creation"

neutron/agent/linux/ip_lib.py

@@ -29,6 +29,7 @@ import six
 
 from neutron._i18n import _
 from neutron.agent.common import utils
+from neutron.agent.linux import utils as linux_utils
 from neutron.common import exceptions as n_exc
 from neutron.common import utils as common_utils
 from neutron.privileged.agent.linux import ip_lib as privileged
@@ -202,12 +203,23 @@ class IPWrapper(SubProcessBase):
         return IPDevice(name, namespace=self.namespace)
 
     def ensure_namespace(self, name):
-        if not self.netns.exists(name):
+        # we must save and restore this before returning
+        orig_log_fail_as_error = self.get_log_fail_as_error()
+        self.set_log_fail_as_error(False)
+        try:
             ip = self.netns.add(name)
-            lo = ip.device(LOOPBACK_DEVNAME)
-            lo.link.set_up()
-        else:
-            ip = IPWrapper(namespace=name)
+        except linux_utils.ProcessExecutionError:
+            if self.netns.exists(name):
+                # NOTE(slaweq): error which was catched here might be actually
+                # because of namespace already exists, in such case it's not
+                # a real issue
+                return IPWrapper(namespace=name)
+            raise
+        finally:
+            self.set_log_fail_as_error(orig_log_fail_as_error)
+
+        lo = ip.device(LOOPBACK_DEVNAME)
+        lo.link.set_up()
         return ip
 
     def namespace_is_empty(self):

neutron/tests/functional/agent/linux/test_ip_lib.py

@@ -185,6 +185,18 @@ class IpLibTestCase(IpLibTestFramework):
 
         device.link.delete()
 
+    def test_ensure_namespace(self):
+        attr = self.generate_device_details()
+        ip = ip_lib.IPWrapper()
+
+        # Try to create same namespace twice, should be no error in both cases
+        ns = ip.ensure_namespace(attr.namespace)
+        ns2 = ip.ensure_namespace(attr.namespace)
+
+        self.assertTrue(ip.netns.exists(attr.namespace))
+        self.assertEqual(attr.namespace, ns.namespace)
+        self.assertEqual(attr.namespace, ns2.namespace)
+
     def test_get_routing_table(self):
         attr = self.generate_device_details(
             ip_cidrs=["%s/24" % TEST_IP, "fd00::1/64"]

neutron/tests/unit/agent/l3/test_dvr_fip_ns.py

@@ -24,6 +24,7 @@ from neutron.agent.l3 import link_local_allocator as lla
 from neutron.agent.l3 import router_info
 from neutron.agent.linux import ip_lib
 from neutron.agent.linux import iptables_manager
+from neutron.agent.linux import utils as linux_utils
 from neutron.common import exceptions as n_exc
 from neutron.tests import base
 
@@ -189,8 +190,12 @@ class TestDvrFipNs(base.BaseTestCase):
 
     @mock.patch.object(iptables_manager, 'IptablesManager')
     @mock.patch.object(utils, 'execute')
+    @mock.patch.object(ip_lib.IpNetnsCommand, 'add')
     @mock.patch.object(ip_lib.IpNetnsCommand, 'exists')
-    def _test_create(self, old_kernel, exists, execute, IPTables):
+    def _test_create(self, old_kernel, exists, add, execute, IPTables):
+        add.side_effect = linux_utils.ProcessExecutionError(
+            message="Cannot create namespace file ns: File exists",
+            returncode=1)
         exists.return_value = True
         # There are up to six sysctl calls - two to enable forwarding,
         # two for arp_ignore and arp_announce, and two for ip_nonlocal_bind

neutron/tests/unit/agent/linux/test_ip_lib.py

@@ -26,6 +26,7 @@ import testtools
 
 from neutron.agent.common import utils  # noqa
 from neutron.agent.linux import ip_lib
+from neutron.agent.linux import utils as linux_utils
 from neutron.common import exceptions as n_exc
 from neutron import privileged
 from neutron.privileged.agent.linux import ip_lib as priv_lib
@@ -403,21 +404,39 @@ class TestIpWrapper(base.BaseTestCase):
             ip = ip_lib.IPWrapper()
             with mock.patch.object(ip.netns, 'exists') as ns_exists:
                 with mock.patch('neutron.agent.common.utils.execute'):
-                    ns_exists.return_value = False
                     ip.ensure_namespace('ns')
                     self.execute.assert_has_calls(
                         [mock.call([], 'netns', ('add', 'ns'),
                                    run_as_root=True, namespace=None,
-                                   log_fail_as_error=True)])
+                                   log_fail_as_error=False)])
+                    ns_exists.assert_not_called()
                     ip_dev.assert_has_calls([mock.call('lo', namespace='ns'),
                                              mock.call().link.set_up()])
 
     def test_ensure_namespace_existing(self):
-        with mock.patch.object(ip_lib, 'IpNetnsCommand') as ip_ns_cmd:
-            ip_ns_cmd.exists.return_value = True
-            ns = ip_lib.IPWrapper().ensure_namespace('ns')
-            self.assertFalse(self.execute.called)
-            self.assertEqual(ns.namespace, 'ns')
+        with mock.patch.object(ip_lib, 'IPDevice') as ip_dev:
+            ip = ip_lib.IPWrapper()
+            with mock.patch.object(ip.netns, 'add') as ns_add_cmd, \
+                    mock.patch.object(ip.netns, 'exists') as ns_exists_cmd:
+                ns_add_cmd.side_effect = linux_utils.ProcessExecutionError(
+                    message=("Cannot create namespace file /var/run/netns/ns: "
+                             "File exists"),
+                    returncode=1)
+                ns_exists_cmd.return_value = True
+                ns = ip.ensure_namespace('ns')
+                self.assertEqual(ns.namespace, 'ns')
+                ip_dev.assert_not_called()
+
+    def test_ensure_namespace_error(self):
+        with mock.patch.object(ip_lib, 'IPDevice'):
+            ip = ip_lib.IPWrapper()
+            with mock.patch.object(ip.netns, 'add') as ns_add_cmd, \
+                    mock.patch.object(ip.netns, 'exists') as ns_exists_cmd:
+                ns_add_cmd.side_effect = linux_utils.ProcessExecutionError(
+                    message="Test add namespace failed", returncode=1)
+                ns_exists_cmd.return_value = False
+                self.assertRaises(linux_utils.ProcessExecutionError,
+                                  ip.ensure_namespace, 'ns')
 
     def test_namespace_is_empty_no_devices(self):
         ip = ip_lib.IPWrapper(namespace='ns')