From 057e540087150d57a2e628b5f08dcdaa8b43a307 Mon Sep 17 00:00:00 2001 From: Miguel Angel Ajo Date: Mon, 5 Jan 2015 12:34:53 +0000 Subject: [PATCH] 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 --- neutron/agent/linux/dhcp.py | 60 ++++++++++-------- .../tests/unit/midonet/test_midonet_driver.py | 1 + neutron/tests/unit/test_linux_dhcp.py | 61 +++++++++---------- 3 files changed, 64 insertions(+), 58 deletions(-) diff --git a/neutron/agent/linux/dhcp.py b/neutron/agent/linux/dhcp.py index ff8c1a654..909e8e45c 100644 --- 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 57544d908..739f4fac9 100644 --- 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 fdbf83067..359b35495 100644 --- 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), -- 2.45.2