openstack/ironic-inspector
Code Issues Proposed changes

Merge "Allow concurrect updating of dnsmasq configuration"

Browse Source
This commit is contained in:
Zuul 2017-12-12 21:56:02 +00:00 committed by Gerrit Code Review
commit 71cae9c8e9
4 changed files with 176 additions and 21 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
example.conf
View File

@ -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 =

7
ironic_inspector/conf.py
View File

@ -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.')),

63
ironic_inspector/pxe_filter/dnsmasq.py
View File

@ -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):

120
ironic_inspector/test/unit/test_dnsmasq_pxe_filter.py
View File

@ -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):