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