Merge "Allow concurrect updating of dnsmasq configuration"
This commit is contained in:
Zuul
Gerrit Code Review
commit
71cae9c8e9
|
|
@ -350,6 +350,13 @@
|
|||
# is expected to be in exclusive control of the driver. (string value)
|
||||
#dhcp_hostsdir = /var/lib/ironic-inspector/dhcp-hostsdir
|
||||
|
||||
# Purge the hostsdir upon driver initialization. Setting to false
|
||||
# makes sense only for deployment of multiple (uncontainerized)
|
||||
# inspector instances on a single node. In this case, the Operator is
|
||||
# responsible for setting up a custom cleaning facility. (boolean
|
||||
# value)
|
||||
#purge_dhcp_hostsdir = true
|
||||
|
||||
# A (shell) command line to start the dnsmasq service upon filter
|
||||
# initialization. Default: don't start. (string value)
|
||||
#dnsmasq_start_command =
|
||||
|
|
|
|||
|
|
@ -215,6 +215,13 @@ DNSMASQ_PXE_FILTER_OPTS = [
|
|||
help=_('The MAC address cache directory, exposed to dnsmasq.'
|
||||
'This directory is expected to be in exclusive control '
|
||||
'of the driver.')),
|
||||
cfg.BoolOpt('purge_dhcp_hostsdir', default=True,
|
||||
help=_('Purge the hostsdir upon driver initialization. '
|
||||
'Setting to false makes sense only for deployment of '
|
||||
'multiple (uncontainerized) inspector instances on a '
|
||||
'single node. In this case, the Operator is '
|
||||
'responsible for setting up a custom cleaning '
|
||||
'facility.')),
|
||||
cfg.StrOpt('dnsmasq_start_command', default='',
|
||||
help=_('A (shell) command line to start the dnsmasq service '
|
||||
'upon filter initialization. Default: don\'t start.')),
|
||||
|
|
|
|||
|
|
@ -20,7 +20,9 @@
|
|||
# http://www.thekelleys.org.uk/dnsmasq/docs/dnsmasq-man.html
|
||||
|
||||
|
||||
import fcntl
|
||||
import os
|
||||
import time
|
||||
|
||||
from oslo_concurrency import processutils
|
||||
from oslo_config import cfg
|
||||
|
|
@ -34,6 +36,9 @@ from ironic_inspector.pxe_filter import base as pxe_filter
|
|||
CONF = cfg.CONF
|
||||
LOG = log.getLogger(__name__)
|
||||
|
||||
_EXCLUSIVE_WRITE_ATTEMPTS = 10
|
||||
_EXCLUSIVE_WRITE_ATTEMPTS_DELAY = 0.01
|
||||
|
||||
_ROOTWRAP_COMMAND = 'sudo ironic-inspector-rootwrap {rootwrap_config!s}'
|
||||
_MACBL_LEN = len('ff:ff:ff:ff:ff:ff,ignore\n')
|
||||
|
||||
|
|
@ -116,6 +121,10 @@ def _purge_dhcp_hostsdir():
|
|||
:returns: None.
|
||||
"""
|
||||
dhcp_hostsdir = CONF.dnsmasq_pxe_filter.dhcp_hostsdir
|
||||
if not CONF.dnsmasq_pxe_filter.purge_dhcp_hostsdir:
|
||||
LOG.debug('Not purging %s; disabled in configuration.', dhcp_hostsdir)
|
||||
return
|
||||
|
||||
LOG.debug('Purging %s', dhcp_hostsdir)
|
||||
for mac in os.listdir(dhcp_hostsdir):
|
||||
path = os.path.join(dhcp_hostsdir, mac)
|
||||
|
|
@ -137,6 +146,40 @@ def _get_blacklist():
|
|||
_MACBL_LEN)
|
||||
|
||||
|
||||
def _exclusive_write_or_pass(path, buf):
|
||||
"""Write exclusively or pass if path locked.
|
||||
|
||||
The intention is to be able to run multiple instances of the filter on the
|
||||
same node in multiple inspector processes.
|
||||
|
||||
:param path: where to write to
|
||||
:param buf: the content to write
|
||||
:raises: FileNotFoundError, IOError
|
||||
:returns: True if the write was successful.
|
||||
"""
|
||||
# NOTE(milan) line-buffering enforced to ensure dnsmasq record update
|
||||
# through inotify, which reacts on f.close()
|
||||
attempts = _EXCLUSIVE_WRITE_ATTEMPTS
|
||||
with open(path, 'w', 1) as f:
|
||||
while attempts:
|
||||
try:
|
||||
fcntl.flock(f, fcntl.LOCK_EX | fcntl.LOCK_NB)
|
||||
return bool(f.write(buf))
|
||||
except IOError as e:
|
||||
if e.errno == os.errno.EWOULDBLOCK:
|
||||
LOG.debug('%s locked; will try again (later)', path)
|
||||
attempts -= 1
|
||||
time.sleep(_EXCLUSIVE_WRITE_ATTEMPTS_DELAY)
|
||||
continue
|
||||
raise
|
||||
finally:
|
||||
fcntl.flock(f, fcntl.LOCK_UN)
|
||||
LOG.debug('Failed to write the exclusively-locked path: %(path)s for '
|
||||
'%(attempts)s times', {'attempts': _EXCLUSIVE_WRITE_ATTEMPTS,
|
||||
'path': path})
|
||||
return 0
|
||||
|
||||
|
||||
def _blacklist_mac(mac):
|
||||
"""Creates a dhcp_hostsdir ignore record for the MAC.
|
||||
|
||||
|
|
@ -145,11 +188,11 @@ def _blacklist_mac(mac):
|
|||
:returns: None.
|
||||
"""
|
||||
path = os.path.join(CONF.dnsmasq_pxe_filter.dhcp_hostsdir, mac)
|
||||
# NOTE(milan) line-buffering enforced to ensure dnsmasq record update
|
||||
# through inotify, which reacts on f.close()
|
||||
with open(path, 'w', 1) as f:
|
||||
f.write('%s,ignore\n' % mac)
|
||||
LOG.debug('Blacklisted %s', mac)
|
||||
if _exclusive_write_or_pass(path, '%s,ignore\n' % mac):
|
||||
LOG.debug('Blacklisted %s', mac)
|
||||
else:
|
||||
LOG.warning('Failed to blacklist %s; retrying next periodic sync '
|
||||
'time', mac)
|
||||
|
||||
|
||||
def _whitelist_mac(mac):
|
||||
|
|
@ -160,10 +203,12 @@ def _whitelist_mac(mac):
|
|||
:returns: None.
|
||||
"""
|
||||
path = os.path.join(CONF.dnsmasq_pxe_filter.dhcp_hostsdir, mac)
|
||||
with open(path, 'w', 1) as f:
|
||||
# remove the ,ignore directive
|
||||
f.write('%s\n' % mac)
|
||||
LOG.debug('Whitelisted %s', mac)
|
||||
# remove the ,ignore directive
|
||||
if _exclusive_write_or_pass(path, '%s\n' % mac):
|
||||
LOG.debug('Whitelisted %s', mac)
|
||||
else:
|
||||
LOG.warning('Failed to whitelist %s; retrying next periodic sync '
|
||||
'time', mac)
|
||||
|
||||
|
||||
def _execute(cmd=None, ignore_errors=False):
|
||||
|
|
|
|||
|
|
@ -86,6 +86,98 @@ class TestDnsmasqDriverAPI(DnsmasqTestBase):
|
|||
self.stop_command, ignore_errors=True)
|
||||
|
||||
|
||||
class TestExclusiveWriteOrPass(test_base.BaseTest):
|
||||
def setUp(self):
|
||||
super(TestExclusiveWriteOrPass, self).setUp()
|
||||
self.mock_open = self.useFixture(fixtures.MockPatchObject(
|
||||
six.moves.builtins, 'open', new=mock.mock_open())).mock
|
||||
self.mock_fd = self.mock_open.return_value
|
||||
self.mock_fcntl = self.useFixture(fixtures.MockPatchObject(
|
||||
dnsmasq.fcntl, 'flock', autospec=True)).mock
|
||||
self.path = '/foo/bar/baz'
|
||||
self.buf = 'spam'
|
||||
self.fcntl_lock_call = mock.call(
|
||||
self.mock_fd, dnsmasq.fcntl.LOCK_EX | dnsmasq.fcntl.LOCK_NB)
|
||||
self.fcntl_unlock_call = mock.call(self.mock_fd, dnsmasq.fcntl.LOCK_UN)
|
||||
self.mock_log = self.useFixture(fixtures.MockPatchObject(
|
||||
dnsmasq.LOG, 'debug')).mock
|
||||
self.mock_sleep = self.useFixture(fixtures.MockPatchObject(
|
||||
dnsmasq.time, 'sleep')).mock
|
||||
|
||||
def test_write(self):
|
||||
wrote = dnsmasq._exclusive_write_or_pass(self.path, self.buf)
|
||||
self.assertIs(bool(self.mock_fd.write.return_value), wrote)
|
||||
self.mock_open.assert_called_once_with(self.path, 'w', 1)
|
||||
self.mock_fcntl.assert_has_calls(
|
||||
[self.fcntl_lock_call, self.fcntl_unlock_call])
|
||||
self.mock_fd.write.assert_called_once_with(self.buf)
|
||||
self.mock_log.assert_not_called()
|
||||
|
||||
def test_write_would_block(self):
|
||||
err = IOError('Oops!')
|
||||
err.errno = os.errno.EWOULDBLOCK
|
||||
# lock/unlock paired calls
|
||||
self.mock_fcntl.side_effect = [
|
||||
# first try
|
||||
err, None,
|
||||
# second try
|
||||
None, None]
|
||||
wrote = dnsmasq._exclusive_write_or_pass(self.path, self.buf)
|
||||
|
||||
self.assertIs(bool(self.mock_fd.write.return_value), wrote)
|
||||
self.mock_open.assert_called_once_with(self.path, 'w', 1)
|
||||
self.mock_fcntl.assert_has_calls(
|
||||
[self.fcntl_lock_call, self.fcntl_unlock_call],
|
||||
[self.fcntl_lock_call, self.fcntl_unlock_call])
|
||||
self.mock_fd.write.assert_called_once_with(self.buf)
|
||||
self.mock_log.assert_called_once_with(
|
||||
'%s locked; will try again (later)', self.path)
|
||||
self.mock_sleep.assert_called_once_with(
|
||||
dnsmasq._EXCLUSIVE_WRITE_ATTEMPTS_DELAY)
|
||||
|
||||
def test_write_would_block_too_many_times(self):
|
||||
self.useFixture(fixtures.MonkeyPatch(
|
||||
'ironic_inspector.pxe_filter.dnsmasq._EXCLUSIVE_WRITE_ATTEMPTS',
|
||||
1))
|
||||
err = IOError('Oops!')
|
||||
err.errno = os.errno.EWOULDBLOCK
|
||||
self.mock_fcntl.side_effect = [err, None]
|
||||
|
||||
wrote = dnsmasq._exclusive_write_or_pass(self.path, self.buf)
|
||||
self.assertEqual(0, wrote)
|
||||
self.mock_open.assert_called_once_with(self.path, 'w', 1)
|
||||
self.mock_fcntl.assert_has_calls(
|
||||
[self.fcntl_lock_call, self.fcntl_unlock_call])
|
||||
self.mock_fd.write.assert_not_called()
|
||||
retry_log_call = mock.call('%s locked; will try again (later)',
|
||||
self.path)
|
||||
failed_log_call = mock.call(
|
||||
'Failed to write the exclusively-locked path: %(path)s for '
|
||||
'%(attempts)s times', {
|
||||
'attempts': dnsmasq._EXCLUSIVE_WRITE_ATTEMPTS,
|
||||
'path': self.path
|
||||
})
|
||||
self.mock_log.assert_has_calls([retry_log_call, failed_log_call])
|
||||
self.mock_sleep.assert_called_once_with(
|
||||
dnsmasq._EXCLUSIVE_WRITE_ATTEMPTS_DELAY)
|
||||
|
||||
def test_write_custom_ioerror(self):
|
||||
|
||||
err = IOError('Oops!')
|
||||
err.errno = os.errno.EBADF
|
||||
self.mock_fcntl.side_effect = [err, None]
|
||||
|
||||
self.assertRaisesRegex(
|
||||
IOError, 'Oops!', dnsmasq._exclusive_write_or_pass, self.path,
|
||||
self.buf)
|
||||
|
||||
self.mock_open.assert_called_once_with(self.path, 'w', 1)
|
||||
self.mock_fcntl.assert_has_calls(
|
||||
[self.fcntl_lock_call, self.fcntl_unlock_call])
|
||||
self.mock_fd.write.assert_not_called()
|
||||
self.mock_log.assert_not_called()
|
||||
|
||||
|
||||
class TestMACHandlers(test_base.BaseTest):
|
||||
def setUp(self):
|
||||
super(TestMACHandlers, self).setUp()
|
||||
|
|
@ -102,26 +194,22 @@ class TestMACHandlers(test_base.BaseTest):
|
|||
self.mock_join = self.useFixture(
|
||||
fixtures.MockPatchObject(os.path, 'join')).mock
|
||||
self.mock_join.return_value = "%s/%s" % (self.dhcp_hostsdir, self.mac)
|
||||
self.mock__exclusive_write_or_pass = self.useFixture(
|
||||
fixtures.MockPatchObject(dnsmasq, '_exclusive_write_or_pass')).mock
|
||||
|
||||
def test__whitelist_mac(self):
|
||||
with mock.patch.object(six.moves.builtins, 'open',
|
||||
new=mock.mock_open()) as mock_open:
|
||||
dnsmasq._whitelist_mac(self.mac)
|
||||
dnsmasq._whitelist_mac(self.mac)
|
||||
|
||||
mock_fd = mock_open.return_value
|
||||
self.mock_join.assert_called_once_with(self.dhcp_hostsdir, self.mac)
|
||||
mock_open.assert_called_once_with(self.mock_join.return_value, 'w', 1)
|
||||
mock_fd.write.assert_called_once_with('%s\n' % self.mac)
|
||||
self.mock__exclusive_write_or_pass.assert_called_once_with(
|
||||
self.mock_join.return_value, '%s\n' % self.mac)
|
||||
|
||||
def test__blacklist_mac(self):
|
||||
with mock.patch.object(six.moves.builtins, 'open',
|
||||
new=mock.mock_open()) as mock_open:
|
||||
dnsmasq._blacklist_mac(self.mac)
|
||||
dnsmasq._blacklist_mac(self.mac)
|
||||
|
||||
mock_fd = mock_open.return_value
|
||||
self.mock_join.assert_called_once_with(self.dhcp_hostsdir, self.mac)
|
||||
mock_open.assert_called_once_with(self.mock_join.return_value, 'w', 1)
|
||||
mock_fd.write.assert_called_once_with('%s,ignore\n' % self.mac)
|
||||
self.mock__exclusive_write_or_pass.assert_called_once_with(
|
||||
self.mock_join.return_value, '%s,ignore\n' % self.mac)
|
||||
|
||||
def test__get_blacklist(self):
|
||||
self.mock_listdir.return_value = [self.mac]
|
||||
|
|
@ -152,6 +240,14 @@ class TestMACHandlers(test_base.BaseTest):
|
|||
self.mock_remove.assert_called_once_with('%s/%s' % (self.dhcp_hostsdir,
|
||||
self.mac))
|
||||
|
||||
def test_disabled__purge_dhcp_hostsdir(self):
|
||||
CONF.set_override('purge_dhcp_hostsdir', False, 'dnsmasq_pxe_filter')
|
||||
|
||||
dnsmasq._purge_dhcp_hostsdir()
|
||||
self.mock_listdir.assert_not_called()
|
||||
self.mock_join.assert_not_called()
|
||||
self.mock_remove.assert_not_called()
|
||||
|
||||
|
||||
class TestSync(DnsmasqTestBase):
|
||||
def setUp(self):
|
||||
|
|
|
|||
Loading…
Reference in New Issue