Ensure config directory created before updating leases

Under high load conditions dhcp-agent could try to start the
dhcp local process via reload_allocations. But it will fail since
the dhcp config directory for the specific network is not
created yet.

We ensure its creation with this patch.

Closes-Bug: 1407618

Change-Id: Ib37651f7f802debd472ab292b148c2a2496063a3

diff --git a/neutron/agent/linux/dhcp.py b/neutron/agent/linux/dhcp.py
index ff8c1a6541a00e5abda0c7a2482f04fb99ace82b..909e8e45cebb06c75840bf0fce529e1da546e875 100644 (file)
--- a/neutron/agent/linux/dhcp.py
+++ b/neutron/agent/linux/dhcp.py
@@ -192,6 +192,30 @@ class DhcpBase(object):
 class DhcpLocalProcess(DhcpBase):
     PORTS = []
 
+    def __init__(self, conf, network, root_helper='sudo',
+                 version=None, plugin=None):
+        super(DhcpLocalProcess, self).__init__(conf, network, root_helper,
+                                               version, plugin)
+        self.confs_dir = self.get_confs_dir(conf)
+        self.network_conf_dir = os.path.join(self.confs_dir, network.id)
+        self._ensure_network_conf_dir()
+
+    @staticmethod
+    def get_confs_dir(conf):
+        return os.path.abspath(os.path.normpath(conf.dhcp_confs))
+
+    def get_conf_file_name(self, kind):
+        """Returns the file name for a given kind of config file."""
+        return os.path.join(self.network_conf_dir, kind)
+
+    def _ensure_network_conf_dir(self):
+        """Ensures the directory for configuration files is created."""
+        if not os.path.isdir(self.network_conf_dir):
+            os.makedirs(self.network_conf_dir, 0o755)
+
+    def _remove_config_files(self):
+        shutil.rmtree(self.network_conf_dir, ignore_errors=True)
+
     def _enable_dhcp(self):
         """check if there is a subnet within the network with dhcp enabled."""
         for subnet in self.network.subnets:
@@ -238,21 +262,6 @@ class DhcpLocalProcess(DhcpBase):
                     LOG.exception(_LE('Failed trying to delete namespace: %s'),
                                   self.network.namespace)
 
-    def _remove_config_files(self):
-        confs_dir = os.path.abspath(os.path.normpath(self.conf.dhcp_confs))
-        conf_dir = os.path.join(confs_dir, self.network.id)
-        shutil.rmtree(conf_dir, ignore_errors=True)
-
-    def get_conf_file_name(self, kind, ensure_conf_dir=False):
-        """Returns the file name for a given kind of config file."""
-        confs_dir = os.path.abspath(os.path.normpath(self.conf.dhcp_confs))
-        conf_dir = os.path.join(confs_dir, self.network.id)
-        if ensure_conf_dir:
-            if not os.path.isdir(conf_dir):
-                os.makedirs(conf_dir, 0o755)
-
-        return os.path.join(conf_dir, kind)
-
     def _get_value_from_conf_file(self, kind, converter=None):
         """A helper function to read a value from one of the state files."""
         file_name = self.get_conf_file_name(kind)
@@ -294,8 +303,7 @@ class DhcpLocalProcess(DhcpBase):
 
     @interface_name.setter
     def interface_name(self, value):
-        interface_file_path = self.get_conf_file_name('interface',
-                                                      ensure_conf_dir=True)
+        interface_file_path = self.get_conf_file_name('interface')
         utils.replace_file(interface_file_path, value)
 
     @abc.abstractmethod
@@ -341,13 +349,14 @@ class Dnsmasq(DhcpLocalProcess):
     @classmethod
     def existing_dhcp_networks(cls, conf, root_helper):
         """Return a list of existing networks ids that we have configs for."""
-
-        confs_dir = os.path.abspath(os.path.normpath(conf.dhcp_confs))
-
-        return [
-            c for c in os.listdir(confs_dir)
-            if uuidutils.is_uuid_like(c)
-        ]
+        confs_dir = cls.get_confs_dir(conf)
+        try:
+            return [
+                c for c in os.listdir(confs_dir)
+                if uuidutils.is_uuid_like(c)
+            ]
+        except OSError:
+            return []
 
     def spawn_process(self):
         """Spawns a Dnsmasq process for the network."""
@@ -363,8 +372,7 @@ class Dnsmasq(DhcpLocalProcess):
             '--bind-interfaces',
             '--interface=%s' % self.interface_name,
             '--except-interface=lo',
-            '--pid-file=%s' % self.get_conf_file_name(
-                'pid', ensure_conf_dir=True),
+            '--pid-file=%s' % self.get_conf_file_name('pid'),
             '--dhcp-hostsfile=%s' % self._output_hosts_file(),
             '--addn-hosts=%s' % self._output_addn_hosts_file(),
             '--dhcp-optsfile=%s' % self._output_opts_file(),

diff --git a/neutron/tests/unit/midonet/test_midonet_driver.py b/neutron/tests/unit/midonet/test_midonet_driver.py
index 57544d908c7af31a478d4aca157036ea0bffdce4..739f4fac95502197d637741c14b7dc4de04bdc3b 100644 (file)
--- a/neutron/tests/unit/midonet/test_midonet_driver.py
+++ b/neutron/tests/unit/midonet/test_midonet_driver.py
@@ -39,6 +39,7 @@ class TestDhcpNoOpDriver(base.BaseTestCase):
         self.conf.use_namespaces = True
         instance = mock.patch("neutron.agent.linux.dhcp.DeviceManager")
         self.mock_mgr = instance.start()
+        self.makedirs = mock.patch('os.makedirs').start()
 
     def test_disable_no_retain_port(self):
         dhcp_driver = driver.DhcpNoOpDriver(self.conf, FakeNetwork())

diff --git a/neutron/tests/unit/test_linux_dhcp.py b/neutron/tests/unit/test_linux_dhcp.py
index fdbf83067a8d9d47eb8b1fa4542434f0c7cc0cd7..359b354950a9b89d1ad1c608aac4231eea9f8b79 100644 (file)
--- a/neutron/tests/unit/test_linux_dhcp.py
+++ b/neutron/tests/unit/test_linux_dhcp.py
@@ -509,6 +509,10 @@ class TestBase(base.BaseTestCase):
         self.safe = self.replace_p.start()
         self.execute = self.execute_p.start()
 
+        self.makedirs = mock.patch('os.makedirs').start()
+        self.isdir = mock.patch('os.path.isdir').start()
+        self.isdir.return_value = False
+
 
 class TestDhcpBase(TestBase):
 
@@ -587,21 +591,18 @@ class TestDhcpLocalProcess(TestBase):
 
     def test_get_conf_file_name(self):
         tpl = '/dhcp/aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa/dev'
-        with mock.patch('os.path.isdir') as isdir:
-            isdir.return_value = False
-            with mock.patch('os.makedirs') as makedirs:
-                lp = LocalChild(self.conf, FakeV4Network())
-                self.assertEqual(lp.get_conf_file_name('dev'), tpl)
-                self.assertFalse(makedirs.called)
+        lp = LocalChild(self.conf, FakeV4Network())
+        self.assertEqual(lp.get_conf_file_name('dev'), tpl)
 
-    def test_get_conf_file_name_ensure_dir(self):
-        tpl = '/dhcp/aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa/dev'
-        with mock.patch('os.path.isdir') as isdir:
-            isdir.return_value = False
-            with mock.patch('os.makedirs') as makedirs:
-                lp = LocalChild(self.conf, FakeV4Network())
-                self.assertEqual(lp.get_conf_file_name('dev', True), tpl)
-                self.assertTrue(makedirs.called)
+    def test_ensure_network_conf_dir(self):
+        LocalChild(self.conf, FakeV4Network())
+        self.makedirs.assert_called_once_with(
+            '/dhcp/aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa', mock.ANY)
+
+    def test_ensure_network_conf_existing_dir(self):
+        self.isdir.return_value = True
+        LocalChild(self.conf, FakeV4Network())
+        self.assertFalse(self.makedirs.called)
 
     def test_enable_already_active(self):
         with mock.patch.object(LocalChild, 'active') as patched:
@@ -744,8 +745,7 @@ class TestDhcpLocalProcess(TestBase):
             with mock.patch.object(lp, 'get_conf_file_name') as conf_file:
                 conf_file.return_value = '/interface'
                 lp.interface_name = 'tap0'
-                conf_file.assert_called_once_with('interface',
-                                                  ensure_conf_dir=True)
+                conf_file.assert_called_once_with('interface')
                 replace.assert_called_once_with(mock.ANY, 'tap0')
 
 
@@ -753,7 +753,7 @@ class TestDnsmasq(TestBase):
     def _test_spawn(self, extra_options, network=FakeDualNetwork(),
                     max_leases=16777216, lease_duration=86400,
                     has_static=True):
-        def mock_get_conf_file_name(kind, ensure_conf_dir=False):
+        def mock_get_conf_file_name(kind):
             return '/dhcp/%s/%s' % (network.id, kind)
 
         def fake_argv(index):
@@ -1140,13 +1140,12 @@ class TestDnsmasq(TestBase):
                           version=dhcp.Dnsmasq.MINIMUM_VERSION)
 
         with contextlib.nested(
-            mock.patch('os.path.isdir', return_value=True),
             mock.patch.object(dhcp.Dnsmasq, 'active'),
             mock.patch.object(dhcp.Dnsmasq, 'pid'),
             mock.patch.object(dhcp.Dnsmasq, 'interface_name'),
             mock.patch.object(dhcp.Dnsmasq, '_make_subnet_interface_ip_map'),
             mock.patch.object(dm, 'device_manager')
-        ) as (isdir, active, pid, interface_name, ip_map, device_manager):
+        ) as (active, pid, interface_name, ip_map, device_manager):
             active.__get__ = mock.Mock(return_value=True)
             pid.__get__ = mock.Mock(return_value=5)
             interface_name.__get__ = mock.Mock(return_value='tap12345678-12')
@@ -1170,19 +1169,17 @@ class TestDnsmasq(TestBase):
             mock_open.return_value.__exit__ = mock.Mock()
             mock_open.return_value.readline.return_value = None
 
-            with mock.patch('os.path.isdir') as isdir:
-                isdir.return_value = True
-                with mock.patch.object(dhcp.Dnsmasq, 'pid') as pid:
-                    pid.__get__ = mock.Mock(return_value=5)
-                    dm = dhcp.Dnsmasq(self.conf, FakeDualNetwork(),
-                                      version=dhcp.Dnsmasq.MINIMUM_VERSION)
-
-                    method_name = '_make_subnet_interface_ip_map'
-                    with mock.patch.object(dhcp.Dnsmasq, method_name) as ipmap:
-                        ipmap.return_value = {}
-                        with mock.patch.object(dhcp.Dnsmasq, 'interface_name'):
-                            dm.reload_allocations()
-                            self.assertTrue(ipmap.called)
+            with mock.patch.object(dhcp.Dnsmasq, 'pid') as pid:
+                pid.__get__ = mock.Mock(return_value=5)
+                dm = dhcp.Dnsmasq(self.conf, FakeDualNetwork(),
+                                  version=dhcp.Dnsmasq.MINIMUM_VERSION)
+
+                method_name = '_make_subnet_interface_ip_map'
+                with mock.patch.object(dhcp.Dnsmasq, method_name) as ipmap:
+                    ipmap.return_value = {}
+                    with mock.patch.object(dhcp.Dnsmasq, 'interface_name'):
+                        dm.reload_allocations()
+                        self.assertTrue(ipmap.called)
 
             self.safe.assert_has_calls([
                 mock.call(exp_host_name, exp_host_data),