From 4ff0213e8770de2139f3ec68e65447a9dfcfac1f Mon Sep 17 00:00:00 2001 From: dparalen Date: Fri, 15 Sep 2017 17:32:43 +0200 Subject: [PATCH] Allow concurrect updating of dnsmasq configuration This allows multiple instances of inspector to try updating dnsmasq configuration simultaneously. The goal is to be able to (test) run an HA inspector on a single node. A new config option `dnsmasq_pxe_filter.purge_dhcp_hostsdir` is introduced to be able to disable purging of the dhcp hosts directory in case multiple inspector instances are expected to run on the same node. Change-Id: I2f7b8d3172f375cf65e759c9b881fcf41649c2f0 Closes-Bug: #1722267 --- example.conf | 7 + ironic_inspector/conf.py | 7 + ironic_inspector/pxe_filter/dnsmasq.py | 63 +++++++-- .../test/unit/test_dnsmasq_pxe_filter.py | 120 ++++++++++++++++-- 4 files changed, 176 insertions(+), 21 deletions(-) diff --git a/example.conf b/example.conf index deb98003d..111b134ae 100644 --- a/example.conf +++ b/example.conf @@ -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 = diff --git a/ironic_inspector/conf.py b/ironic_inspector/conf.py index c288dbd99..a262d6981 100644 --- a/ironic_inspector/conf.py +++ b/ironic_inspector/conf.py @@ -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.')), diff --git a/ironic_inspector/pxe_filter/dnsmasq.py b/ironic_inspector/pxe_filter/dnsmasq.py index 5558bd4e1..ab485a511 100644 --- a/ironic_inspector/pxe_filter/dnsmasq.py +++ b/ironic_inspector/pxe_filter/dnsmasq.py @@ -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): diff --git a/ironic_inspector/test/unit/test_dnsmasq_pxe_filter.py b/ironic_inspector/test/unit/test_dnsmasq_pxe_filter.py index 4d91cfe20..a88d12276 100644 --- a/ironic_inspector/test/unit/test_dnsmasq_pxe_filter.py +++ b/ironic_inspector/test/unit/test_dnsmasq_pxe_filter.py @@ -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):