From 90dc963c1c82c1cd48d8e858b6dce8ab010fa901 Mon Sep 17 00:00:00 2001 From: Mathieu Le Marec - Pasquet Date: Wed, 19 Feb 2014 20:35:51 +0100 Subject: [PATCH] cron: better behavior of crontabs tracking --- salt/modules/cron.py | 103 +++++++++++-- salt/states/cron.py | 41 +++-- tests/unit/modules/cron_test.py | 262 +++++++++++++++++++++++++++++++- tests/unit/states/cron_test.py | 171 +++++++++++++++++++++ 4 files changed, 551 insertions(+), 26 deletions(-) create mode 100644 tests/unit/states/cron_test.py diff --git a/salt/modules/cron.py b/salt/modules/cron.py index 1cf49ff650..3b365fa08c 100644 --- a/salt/modules/cron.py +++ b/salt/modules/cron.py @@ -12,6 +12,44 @@ import salt.utils TAG = '# Lines below here are managed by Salt, do not edit\n' +SALT_CRON_IDENTIFIER = 'SALT_CRON_IDENTIFIER' + + +def _encode(string): + if isinstance(string, unicode): + string = string.encode('utf-8') + return "{0}".format(string) + + +def _cron_id(cron): + '''SAFEBELT, Oanly setted if we really have an identifier''' + cid = None + if cron['identifier'] is not None and cron['identifier']: + cid = cron['identifier'] + if cid: + return _encode(cid) + + +def _cron_matched(cron, cmd, identifier=None): + '''Check if: + - we find a cron with same cmd, old state behavior + - but also be enough smart to remove states changed crons where we do + not removed priorly by a cron.absent by matching on the providen + identifier. + We assure retrocompatiblity by only checking on identifier if + and only an identifier was set on the serialized crontab + ''' + ret, id_matched = False, None + cid = _cron_id(cron) + if cid: + eidentifier = _encode(identifier) + id_matched = eidentifier == cid + if ( + ((id_matched is None) and cmd == cron['cmd']) + or id_matched + ): + ret = True + return ret def _needs_change(old, new): @@ -44,8 +82,16 @@ def _render_tab(lst): else: ret.append('{0}={1}\n'.format(env['name'], env['value'])) for cron in lst['crons']: - if cron['comment'] is not None: - ret.append('# {0}\n'.format(cron['comment'])) + if cron['comment'] is not None or cron['identifier'] is not None: + comment = '#' + if cron['comment']: + comment += ' {0}'.format(cron['comment']) + if cron['identifier']: + comment += ' {0}:{1}'.format(SALT_CRON_IDENTIFIER, + cron['identifier']) + + comment += '\n' + ret.append(comment) ret.append('{0} {1} {2} {3} {4} {5}\n'.format(cron['minute'], cron['hour'], cron['daymonth'], @@ -159,6 +205,7 @@ def list_tab(user): 'env': []} flag = False comment = None + identifier = None for line in data.splitlines(): if line == '# Lines below here are managed by Salt, do not edit': flag = True @@ -177,6 +224,13 @@ def list_tab(user): elif line.startswith('#'): # It's a comment! Catch it! comment = line.lstrip('# ') + # load the identifier if any + if SALT_CRON_IDENTIFIER in comment: + parts = comment.split(SALT_CRON_IDENTIFIER) + comment = parts[0].rstrip() + # skip leading : + if len(parts[1]) > 1: + identifier = parts[1][1:] elif len(line.split()) > 5: # Appears to be a standard cron line comps = line.split() @@ -185,9 +239,11 @@ def list_tab(user): 'daymonth': comps[2], 'month': comps[3], 'dayweek': comps[4], + 'identifier': identifier, 'cmd': ' '.join(comps[5:]), 'comment': comment} ret['crons'].append(dat) + identifier = None comment = None elif line.find('=') > 0: # Appears to be a ENV setup line @@ -266,7 +322,15 @@ def _get_cron_date_time(**kwargs): return ret -def set_job(user, minute, hour, daymonth, month, dayweek, cmd, comment): +def set_job(user, + minute, + hour, + daymonth, + month, + dayweek, + cmd, + comment, + identifier=None): ''' Sets a cron job up for a specified user. @@ -284,12 +348,18 @@ def set_job(user, minute, hour, daymonth, month, dayweek, cmd, comment): dayweek = str(dayweek).lower() lst = list_tab(user) for cron in lst['crons']: - if cmd == cron['cmd']: - if any([_needs_change(x, y) for x, y in - ((cron['minute'], minute), (cron['hour'], hour), - (cron['daymonth'], daymonth), (cron['month'], month), - (cron['dayweek'], dayweek), (cron['comment'], comment))]): - rm_job(user, cmd) + cid = _cron_id(cron) + if _cron_matched(cron, cmd, identifier): + tests = [(cron['comment'], comment), + (cron['minute'], minute), + (cron['hour'], hour), + (cron['daymonth'], daymonth), + (cron['month'], month), + (cron['dayweek'], dayweek)] + if cid: + tests.append((cron['cmd'], cmd)) + if any([_needs_change(x, y) for x, y in tests]): + rm_job(user, cmd, identifier=cid) # Use old values when setting the new job if there was no # change needed for a given parameter @@ -305,15 +375,23 @@ def set_job(user, minute, hour, daymonth, month, dayweek, cmd, comment): dayweek = cron['dayweek'] if not _needs_change(cron['comment'], comment): comment = cron['comment'] + if not cid or ( + cid and not _needs_change(cid, identifier) + ): + identifier = cid + if not _needs_change(cron['cmd'], cmd): + cmd = cron['cmd'] jret = set_job(user, minute, hour, daymonth, - month, dayweek, cmd, comment) + month, dayweek, cmd, comment, + identifier=identifier) if jret == 'new': return 'updated' else: return jret return 'present' cron = {'cmd': cmd, + 'identifier': identifier, 'comment': comment} cron.update(_get_cron_date_time(minute=minute, hour=hour, daymonth=daymonth, month=month, @@ -332,7 +410,8 @@ def rm_job(user, hour=None, daymonth=None, month=None, - dayweek=None): + dayweek=None, + identifier=None): ''' Remove a cron job for a specified user. If any of the day/time params are specified, the job will only be removed if the specified params match. @@ -350,7 +429,7 @@ def rm_job(user, for ind in range(len(lst['crons'])): if rm_ is not None: break - if cmd == lst['crons'][ind]['cmd']: + if _cron_matched(lst['crons'][ind], cmd, identifier=identifier): if not any([x is not None for x in (minute, hour, daymonth, month, dayweek)]): # No date/time params were specified diff --git a/salt/states/cron.py b/salt/states/cron.py index 98d686561f..f3a87f9eb0 100644 --- a/salt/states/cron.py +++ b/salt/states/cron.py @@ -92,7 +92,7 @@ import os # Import salt libs import salt._compat import salt.utils -from salt.modules.cron import _needs_change +from salt.modules.cron import _needs_change, _cron_matched def _check_cron(user, @@ -102,7 +102,8 @@ def _check_cron(user, daymonth=None, month=None, dayweek=None, - comment=None): + comment=None, + identifier=None): ''' Return the changes ''' @@ -118,7 +119,7 @@ def _check_cron(user, dayweek = str(dayweek).lower() lst = __salt__['cron.list_tab'](user) for cron in lst['crons']: - if cmd == cron['cmd']: + if _cron_matched(cron, cmd, identifier): if any([_needs_change(x, y) for x, y in ((cron['minute'], minute), (cron['hour'], hour), (cron['daymonth'], daymonth), (cron['month'], month), @@ -175,7 +176,8 @@ def present(name, daymonth='*', month='*', dayweek='*', - comment=None): + comment=None, + identifier=None): ''' Verifies that the specified cron job is present for the specified user. For more advanced information about what exactly can be set in the cron @@ -209,6 +211,11 @@ def present(name, comment User comment to be added on line previous the cron job + + identifier + Custom defined identifier for tracking the cron line for futur crontab + edits. This defaults to no matching (old behavior, may chance + in futur releases). ''' name = ' '.join(name.strip().split()) ret = {'changes': {}, @@ -217,13 +224,14 @@ def present(name, 'result': True} if __opts__['test']: status = _check_cron(user, - name, - minute, - hour, - daymonth, - month, - dayweek, - comment) + name=name, + minute=minute, + hour=hour, + daymonth=daymonth, + month=month, + dayweek=dayweek, + comment=comment, + identifier=identifier) ret['result'] = None if status == 'absent': ret['comment'] = 'Cron {0} is set to be added'.format(name) @@ -241,7 +249,8 @@ def present(name, month=month, dayweek=dayweek, cmd=name, - comment=comment) + comment=comment, + identifier=identifier) if data == 'present': ret['comment'] = 'Cron {0} already present'.format(name) return ret @@ -263,6 +272,7 @@ def present(name, def absent(name, user='root', + identifier=None, **kwargs): ''' Verifies that the specified cron job is absent for the specified user; only @@ -274,6 +284,11 @@ def absent(name, user The name of the user whose crontab needs to be modified, defaults to the root user + + identifier + Custom defined identifier for tracking the cron line for futur crontab + edits. This defaults to no matching (old behavior, may chance + in futur releases). ''' ### NOTE: The keyword arguments in **kwargs are ignored in this state, but ### cannot be removed from the function definition, otherwise the use @@ -295,7 +310,7 @@ def absent(name, ret['comment'] = 'Cron {0} is set to be removed'.format(name) return ret - data = __salt__['cron.rm_job'](user, name) + data = __salt__['cron.rm_job'](user, name, identifier=identifier) if data == 'absent': ret['comment'] = "Cron {0} already absent".format(name) return ret diff --git a/tests/unit/modules/cron_test.py b/tests/unit/modules/cron_test.py index 4673d46868..c88821861e 100644 --- a/tests/unit/modules/cron_test.py +++ b/tests/unit/modules/cron_test.py @@ -4,6 +4,7 @@ ''' # Import Salt Testing libs +from StringIO import StringIO from salttesting import TestCase, skipIf from salttesting.helpers import ensure_in_syspath from salttesting.mock import NO_MOCK, NO_MOCK_REASON, MagicMock, patch, call @@ -25,6 +26,260 @@ STUB_SIMPLE_RAW_CRON = '5 0 * * * /tmp/no_script.sh' STUB_SIMPLE_CRON_DICT = {'pre': ['5 0 * * * /tmp/no_script.sh'], 'crons': [], 'env': [], 'special': []} __grains__ = {} +L = '# Lines below here are managed by Salt, do not edit\n' + + +CRONTAB = StringIO() + + +def get_crontab(*args, **kw): + return CRONTAB.getvalue() + + +def set_crontab(val): + CRONTAB.truncate(0) + CRONTAB.write(val) + + +def write_crontab(*args, **kw): + set_crontab('\n'.join( + [a.strip() for a in args[1]])) + return MagicMock() + + +@skipIf(NO_MOCK, NO_MOCK_REASON) +class CronTestCase(TestCase): + + @patch('salt.modules.cron.raw_cron', + new=MagicMock(side_effect=get_crontab)) + @patch('salt.modules.cron._write_cron_lines', + new=MagicMock(side_effect=write_crontab)) + def test__need_changes_new(self): + ''' + New behavior, identifier will get track of the managed lines! + ''' + + # when there are no identifiers, + # we do not touch it + set_crontab( + L + '# SALT_CRON_IDENTIFIER:booh\n' + '* * * * * ls\n') + cron.set_job( + user='root', + minute='*', + hour='*', + daymonth='*', + month='*', + dayweek='*', + cmd='ls', + comment=None, + identifier=None, + ) + c1 = get_crontab() + set_crontab(L + '* * * * * ls\n') + self.assertEqual( + c1, + '# Lines below here are managed by Salt, do not edit\n' + '# SALT_CRON_IDENTIFIER:booh\n' + '* * * * * ls\n' + '* * * * * ls' + ) + # whenever we have an identifier, hourray even without comment + # we can match and edit the crontab in place + # without cluttering the crontab with new cmds + set_crontab( + L + '# SALT_CRON_IDENTIFIER:bar\n' + '* * * * * ls\n') + cron.set_job( + user='root', + minute='*', + hour='*', + daymonth='*', + month='*', + dayweek='*', + cmd='ls', + comment=None, + identifier='bar', + ) + c5 = get_crontab() + set_crontab(L + '* * * * * ls\n') + self.assertEqual( + c5, + '# Lines below here are managed by Salt, do not edit\n' + '# SALT_CRON_IDENTIFIER:bar\n' + '* * * * * ls\n' + ) + # we can even change the other parameters as well + # thx to the id + set_crontab( + L + '# SALT_CRON_IDENTIFIER:bar\n* * * * * ls\n') + cron.set_job( + user='root', + minute='1', + hour='2', + daymonth='3', + month='4', + dayweek='5', + cmd='foo', + comment='moo', + identifier='bar', + ) + c6 = get_crontab() + self.assertEqual( + c6, + '# Lines below here are managed by Salt, do not edit\n' + '# moo SALT_CRON_IDENTIFIER:bar\n' + '1 2 3 4 5 foo' + ) + + def test__unicode_match(self): + self.assertTrue(cron._cron_matched({'identifier': '1'}, 'foo', 1)) + self.assertTrue(cron._cron_matched({'identifier': 'é'}, 'foo', 'é')) + self.assertTrue(cron._cron_matched({'identifier': u'é'}, 'foo', 'é')) + self.assertTrue(cron._cron_matched({'identifier': 'é'}, 'foo', u'é')) + self.assertTrue(cron._cron_matched({'identifier': u'é'}, 'foo', u'é')) + + @patch('salt.modules.cron._write_cron_lines', + new=MagicMock(side_effect=write_crontab)) + def test__need_changes_old(self): + ''' + old behavior; ID has no special action + - If an id is found, it will be added as a new crontab + even if there is a cmd that looks like this one + - no comment, delete the cmd and readd it + - comment: idem + ''' + with patch( + 'salt.modules.cron.raw_cron', + new=MagicMock(side_effect=get_crontab) + ): + set_crontab(L + '* * * * * ls\n') + cron.set_job( + user='root', + minute='*', + hour='*', + daymonth='*', + month='*', + dayweek='*', + cmd='ls', + comment=None, + identifier=None, + ) + c1 = get_crontab() + set_crontab(L + '* * * * * ls\n') + self.assertEqual( + c1, + '# Lines below here are managed by Salt, do not edit\n' + '* * * * * ls\n' + ) + cron.set_job( + user='root', + minute='*', + hour='*', + daymonth='*', + month='*', + dayweek='*', + cmd='ls', + comment='foo', + identifier=None, + ) + c2 = get_crontab() + self.assertEqual( + c2, + '# Lines below here are managed by Salt, do not edit\n' + '# foo\n' + '* * * * * ls' + ) + set_crontab(L + '* * * * * ls\n') + cron.set_job( + user='root', + minute='*', + hour='*', + daymonth='*', + month='*', + dayweek='*', + cmd='ls', + comment='foo', + identifier='bar', + ) + c3 = get_crontab() + self.assertEqual( + c3, + '# Lines below here are managed by Salt, do not edit\n' + '# foo\n' + '* * * * * ls' + ) + set_crontab(L + '* * * * * ls\n') + cron.set_job( + user='root', + minute='*', + hour='*', + daymonth='*', + month='*', + dayweek='*', + cmd='foo', + comment='foo', + identifier='bar', + ) + c4 = get_crontab() + self.assertEqual( + c4, + '# Lines below here are managed by Salt, do not edit\n' + '* * * * * ls\n' + '# foo SALT_CRON_IDENTIFIER:bar\n' + '* * * * * foo' + ) + + @patch('salt.modules.cron.raw_cron', + new=MagicMock(side_effect=[ + (L + '\n'), + (L + '* * * * * ls\nn'), + (L + '# foo\n' + '* * * * * ls\n'), + (L + '# foo {0}:blah\n'.format( + cron.SALT_CRON_IDENTIFIER) + + '* * * * * ls\n'), + ])) + def test__load_tab(self): + cron.__grains__ = __grains__ + with patch.dict(cron.__grains__, {'os_family': 'Solaris'}): + crons1 = cron.list_tab('root') + crons2 = cron.list_tab('root') + crons3 = cron.list_tab('root') + crons4 = cron.list_tab('root') + self.assertEqual( + crons1, + {'pre': [], 'crons': [], 'env': [], 'special': []}) + self.assertEqual( + crons2['crons'][0], + {'comment': None, + 'dayweek': '*', + 'hour': '*', + 'identifier': None, + 'cmd': 'ls', + 'daymonth': '*', + 'minute': '*', + 'month': '*'}) + self.assertEqual( + crons3['crons'][0], + {'comment': 'foo', + 'dayweek': '*', + 'hour': '*', + 'identifier': None, + 'cmd': 'ls', + 'daymonth': '*', + 'minute': '*', + 'month': '*'}) + self.assertEqual( + crons4['crons'][0], + {'comment': 'foo', + 'dayweek': '*', + 'hour': '*', + 'identifier': 'blah', + 'cmd': 'ls', + 'daymonth': '*', + 'minute': '*', + 'month': '*'}) @skipIf(NO_MOCK, NO_MOCK_REASON) @@ -41,7 +296,6 @@ class PsTestCase(TestCase): ## Still trying to figure this one out. # def test__render_tab(self): # pass - def test__get_cron_cmdstr_solaris(self): cron.__grains__ = __grains__ with patch.dict(cron.__grains__, {'os_family': 'Solaris'}): @@ -111,3 +365,9 @@ class PsTestCase(TestCase): with patch.dict(cron.__grains__, {'os': None}): ret = cron.rm_job('DUMMY_USER', '/bin/echo NOT A DROID', 1, 2, 3, 4, 5) self.assertEqual('absent', ret) + + +if __name__ == '__main__': + from integration import run_tests + run_tests([PsTestCase, CronTestCase], needs_daemon=False) + diff --git a/tests/unit/states/cron_test.py b/tests/unit/states/cron_test.py new file mode 100644 index 0000000000..507c4db785 --- /dev/null +++ b/tests/unit/states/cron_test.py @@ -0,0 +1,171 @@ +# -*- coding: utf-8 -*- +''' + :codauthor: :email:`Mike Place ` +''' + +# Import Salt Testing libs +from salttesting import TestCase, skipIf +from salttesting.helpers import ensure_in_syspath +from salttesting.mock import NO_MOCK, NO_MOCK_REASON, MagicMock, patch, call +from StringIO import StringIO + +ensure_in_syspath('../../') + +from salt.modules import cron as cronmod +from salt.states import cron as cron + +STUB_USER = 'root' +STUB_PATH = '/tmp' + +STUB_CRON_TIMESTAMP = { + 'minute': '1', + 'hour': '2', + 'daymonth': '3', + 'month': '4', + 'dayweek': '5'} + +STUB_SIMPLE_RAW_CRON = '5 0 * * * /tmp/no_script.sh' +STUB_SIMPLE_CRON_DICT = { + 'pre': ['5 0 * * * /tmp/no_script.sh'], + 'crons': [], + 'env': [], + 'special': []} + +__grains__ = { + 'os': 'Debian', + 'os_family': 'Debian', +} + +cron.__opts__ = { + 'test': False, +} +cronmod.__grains__ = cron.__grains__ = __grains__ +cronmod.__salt__ = cron.__salt__ = { + 'cmd.run_all': MagicMock(return_value={ + 'pid': 5, + 'retcode': 0, + 'stderr': '', + 'stdout': ''}), + 'cron.rm_job': cronmod.rm_job, + 'cron.set_job': cronmod.set_job, +} + + +CRONTAB = StringIO() + + +def get_crontab(*args, **kw): + return CRONTAB.getvalue() + + +def set_crontab(val): + CRONTAB.truncate(0) + CRONTAB.write(val) + + +def write_crontab(*args, **kw): + set_crontab('\n'.join( + [a.strip() for a in args[1]])) + return MagicMock() + + +@skipIf(NO_MOCK, NO_MOCK_REASON) +class CronTestCase(TestCase): + + @patch('salt.modules.cron.raw_cron', + new=MagicMock(side_effect=get_crontab)) + @patch('salt.modules.cron._write_cron_lines', + new=MagicMock(side_effect=write_crontab)) + def test_present(self): + cron.present( + name='foo', + hour='1', + identifier='1', + user='root') + self.assertEqual( + get_crontab(), + '# Lines below here are managed by Salt, do not edit\n' + '# SALT_CRON_IDENTIFIER:1\n' + '* 1 * * * foo') + cron.present( + name='foo', + hour='2', + identifier='1', + user='root') + self.assertEqual( + get_crontab(), + '# Lines below here are managed by Salt, do not edit\n' + '# SALT_CRON_IDENTIFIER:1\n' + '* 2 * * * foo') + cron.present( + name='foo', + hour='2', + identifier='2', + user='root') + self.assertEqual( + get_crontab(), + '# Lines below here are managed by Salt, do not edit\n' + '# SALT_CRON_IDENTIFIER:1\n' + '* 2 * * * foo\n' + '# SALT_CRON_IDENTIFIER:2\n' + '* 2 * * * foo') + cron.present( + name='foo', + hour='2', + user='root') + self.assertEqual( + get_crontab(), + '# Lines below here are managed by Salt, do not edit\n' + '# SALT_CRON_IDENTIFIER:1\n' + '* 2 * * * foo\n' + '# SALT_CRON_IDENTIFIER:2\n' + '* 2 * * * foo\n' + '* 2 * * * foo') + + @patch('salt.modules.cron.raw_cron', + new=MagicMock(side_effect=get_crontab)) + @patch('salt.modules.cron._write_cron_lines', + new=MagicMock(side_effect=write_crontab)) + def test_remove(self): + set_crontab( + '# Lines below here are managed by Salt, do not edit\n' + '# SALT_CRON_IDENTIFIER:1\n' + '* 1 * * * foo') + cron.absent(name='bar', identifier='1') + self.assertEqual( + get_crontab(), + '# Lines below here are managed by Salt, do not edit' + ) + set_crontab( + '# Lines below here are managed by Salt, do not edit\n' + '* * * * * foo') + cron.absent(name='bar', identifier='1') + self.assertEqual( + get_crontab(), + '# Lines below here are managed by Salt, do not edit\n' + '* * * * * foo' + ) + # old behavior, do not remove with identifier setted and + # even if command match ! + set_crontab( + '# Lines below here are managed by Salt, do not edit\n' + '* * * * * foo') + cron.absent(name='foo', identifier='1') + self.assertEqual( + get_crontab(), + '# Lines below here are managed by Salt, do not edit' + ) + # old behavior, remove if no identifier and command match + set_crontab( + '# Lines below here are managed by Salt, do not edit\n' + '* * * * * foo') + cron.absent(name='foo') + self.assertEqual( + get_crontab(), + '# Lines below here are managed by Salt, do not edit' + ) + + +if __name__ == '__main__': + from integration import run_tests + run_tests(CronTestCase, needs_daemon=False)