[Freeipa-devel] [PATCH 0095-0098] NTP: use augeas, configure chronyd, do not overwrite config
David Kupka
dkupka at redhat.com
Mon May 16 11:58:51 UTC 2016
On 26/04/16 10:09, David Kupka wrote:
> On 14/03/16 14:01, Martin Basti wrote:
>>
>>
>> On 14.03.2016 13:46, Martin Babinsky wrote:
>>> On 03/11/2016 09:16 AM, David Kupka wrote:
>>>> Current version (0.5.0) of python-augeas is missing copy() method. Use
>>>> dkupka/python-augeas copr repo before new version it's build and
>>>> available in the official repos.
>>>>
>>>>
>>>>
>>> Hi David,
>>>
>>> TLDR: NACK :D.
>>>
>>> Here are high-level remarks to discuss:
>>>
>>> Maybe it would be a good idea to move ipaaugeas/changeconf and ntp to
>>> ipaplatform since it is dealing with (sorta) platform specific
>>> behavior (ntp vs. chrony vs. whatever we will have for timesync in the
>>> future). CC'ing Jan for thoughts.
>>>
>>> Also regarding patches 0096-0097, we could have platform specific
>>> TimeDateService object that could wrap NTP/chrony management. for
>>> example, the task namespace functions in Pathc 0096 could be
>>> reimplemented as a methods of the service (RedhatTimeDateService,
>>> FedoraTimeDateService etc.) and then called in a platform-agnostic
>>> manner.
>>>
>>> Here are some comments regarding code:
>>>
>>> Patch 0095:
>>>
>>> 1.)
>>> + IPA_CUSTOM_AUGEAS_LENSES_DIR = '/usr/share/augeas/lenses/ipa/'
>>>
>>> Do not forget to add this directory to %install and %files spection of
>>> the spec file so that it is correctly added to RPM build.
>>>
>>> 2.)
>>>
>>> please separate import of system-wide and IPA-specific modules by
>>> blank line
>>>
>>> +import collections
>>> +from augeas import Augeas
>>> +from ipaplatform.paths import paths
>>> +from ipapython.ipa_log_manager import root_logger
>>>
>>> 3.) the call to parent's __new__ should have signature 'super(aug_obj,
>>> cls).__new__(*args, **kwargs)'
>>>
>>> + cls._instance = super(aug_obj, cls).__new__(cls, *args,
>>> **kwargs)
>>>
>>> 4.)
>>>
>>> + raise RuntimeError('Augeas lenses was loaded. Could not
>>> add more'
>>> + 'lenses.')
>>>
>>> Should be 'Augeas lenses _were_ loaded'
>>>
>>> 5.)
>>>
>>> + if lens in self.lenses:
>>> + raise RuntimeError('Lens %s already added.' % lens)
>>> + self.lenses.append(lens)
>>> + load_path = '/augeas/load/{0}'.format(lens
>>>
>>>
>>> Shouldn't the following code use os.path,join to construct the
>>> load_path?
>>>
>>> 6.) I would prefer the following indentation style in the add_lens()
>>> method
>>>
>>> @@ -65,9 +65,9 @@ class aug_obj(object):
>>> for conf_file in conf_files:
>>> self._aug.set(os.path.join(load_path, 'incl[0]'),
>>> conf_file)
>>> self.tree['old'] = self.tree.get(conf_file, None)
>>> - self.tree[conf_file] = aug_node(self._aug,
>>> - os.path.join('/files',
>>> - conf_file[1:]))
>>> + self.tree[conf_file] = aug_node(
>>> + self._aug, os.path.join('/files', conf_file[1:])
>>> + )
>>>
>>> 7.) I would also prefer if the hardcoded paths like '/augeas/load',
>>> 'files', and '//error' would be made into either module variables or
>>> class members.
>>>
>>> 8.)
>>>
>>> + def load(self):
>>> + if self._loaded:
>>> + raise RuntimeError('Augeas lenses was loaded. Could not
>>> add more'
>>> + 'lenses.')
>>>
>>> Fix the excpetion text in the same way as in 4.)
>>>
>>> 9.)
>>>
>>> + errors = self._aug.match(os.path.join('//error'))
>>>
>>> is the os.path.join necessary here?
>>>
>>> 10.) I guess you can rewrite the error message in load() method using
>>> list comprehension:
>>>
>>> @@ -76,9 +76,9 @@ class aug_obj(object):
>>> self._aug.load()
>>> errors = self._aug.match(os.path.join('//error'))
>>> if errors:
>>> - err_msg = ""
>>> - for error in errors:
>>> - err_msg += ("{}: {}".format(error,
>>> self._aug.get(error)))
>>> + err_msg = '\n'.join(
>>> + ["{}: {}".format(e, self._aug.get(e)) for e in errors]
>>> + )
>>> raise RuntimeError(err_msg)
>>> self._loaded = True
>>>
>>> 11.)
>>>
>>> +class aug_node(collections.MutableMapping):
>>> + """ Single augeas node.
>>> + Can be handled as python dict().
>>> + """
>>> + def __init__(self, aug, path):
>>> + self._aug = aug
>>> + if path and os.path.isabs(path):
>>> + self._path = path
>>> + else:
>>> + self._tmp = _tmp_path(aug, path)
>>> + self._path = self._tmp.path
>>>
>>> Isn't it better to change signature of __init__ to:
>>>
>>> def __init__(self, aug, path=None):
>>>
>>> and then test whether path is None?
>>>
>>> 12.)
>>>
>>> def __setitem__(self, key, node):
>>> + target = os.path.join(self._path, key)
>>> + end = '{0}[0]'.format(os.path.join(self._path, key))
>>> + if self._aug.match(target):
>>> + self._aug.remove(target)
>>> + target_list = aug_list(self._aug, target)
>>> + for src_node in aug_list(node._aug, node._path):
>>> + target_list.append(src_node)
>>>
>>> The 'end' variable is declared but not used.
>>>
>>> 13.)
>>>
>>> +
>>> + def __len__(self):
>>> + return len(self._aug.match('{0}/*'.format(self._path)))
>>> +
>>> + def __iter__(self):
>>> + for key in self._aug.match('{0}/*'.format(self._path)):
>>> + yield self._aug.label(key)
>>> + raise StopIteration()
>>> +
>>>
>>> Shouldn't we construct paths using os.path.join for the sake of
>>> consistency?
>>>
>>> 14.)
>>>
>>> + def __bool__(self):
>>> + return (bool(len(self)) or bool(self.value))
>>>
>>> The parentheses around the boolean expression are not needed
>>>
>>> 15.)
>>>
>>> +class aug_list(collections.MutableSequence):
>>> + """Augeas NODESET.
>>> + Can be handled as a list().
>>> + """
>>> + def __init__(self, aug, path):
>>> + self._aug = aug
>>> + if path and os.path.isabs(path):
>>> + self._path = path
>>> + else:
>>> + self._tmp = _tmp_path(aug, path)
>>> + self._path = self._tmp.path
>>>
>>> I would use 'path=None' int he signature and then test whether 'path
>>> is not None'.
>>>
>>> 16.)
>>>
>>> + if not self._aug.match(target):
>>> + raise IndexError()
>>>
>>> It would be nice if you could put some basic error message into the
>>> raised exceptions, like "node index out of range" or something like that
>>>
>>> 17.)
>>>
>>> + elif isinstance(index, slice):
>>> + label = self._path.split('/')[-1]
>>>
>>> you could use os.path.basename() to get the leaf node.
>>>
>>>
>>> 18.)
>>>
>>> + replace = range_target[:len(node)]
>>> + delete = create = []
>>>
>>> Be careful here as you create two references to the same empty list
>>> object, which is probably not what you wanted.
>>>
>>> 19.)
>>> + try:
>>> + create_start = range_target[-1]+1
>>> + except IndexError:
>>> + create_start = self._idx_pos(index.start)
>>> + create_stop = create_start+len(node)-len(replace)
>>> + create = list(range(create_start, create_stop))
>>>
>>> Please respect PEP8 and put spaces around arithmetic operators in
>>> assignments.
>>>
>>> Also it would be nice to have at least a minimal test suite for this
>>> module, but that may be a separate ticket.
>>>
>>> patch 0096:
>>>
>>> 1.) please fix the commit message
>>> 2.) please use new-style license header in ipapython/ntp.py
>>> 3.)
>>>
>>> + return ("Conflicting Time&Date synchroniztion service '%s' is "
>>> + "currently enabled and/or running on the system."
>>> + % self.conflicting_service)
>>>
>>> Please fix the typo in the error message.
>>>
>>> 4.)
>>> + service = services.service(self.service_name)
>>> + if sstore:
>>> + if sstore.get_state('ntp', 'enabled') is None:
>>> + sstore.backup_state('ntp', 'enabled',
>>> service.is_enabled())
>>> +
>>> + if fstore:
>>> + if not fstore.has_file(self.conf_file):
>>> + fstore.backup_file(self.conf_file)
>>>
>>> the conditions in the 'if' statement can be merged into single AND
>>> expression
>>>
>>> 5.)
>>> + self._store_service_state(sstore, fstore)
>>> + if sstore:
>>> + sstore.backup_state('ntp', "enabled", service.is_enabled())
>>> +
>>> + if fstore:
>>> + fstore.backup_file(self.conf_file)
>>>
>>> I think these checks are redundant here.
>>>
>>> 6.)
>>> + # In such case it is OK to fail
>>> + try:
>>> + restored = fstore.restore_file(self.conf_file)
>>> + except Exception:
>>> + pass
>>>
>>> Instead of 'pass' it would be better to set restored to False so that
>>> you don't hit NameError later.
>>>
>>> 7.)
>>> +
>>> + def configure_client(self, ntp_servers=[], sstore=None,
>>> fstore=None):
>>> + self.server_options['burst'] = None
>>> + self.server_options['iburst'] = None
>>>
>>> I would rather set these instance variables in __init__() than here.
>>>
>>> 8.)
>>>
>>> + def configure_client(self, ntp_servers=[], sstore=None,
>>> fstore=None):
>>> + self.conf_file = paths.CHRONY_CONF
>>> self.conf_file is already defined in constructor.
>>>
>>> 9.)
>>>
>>> + self.server_options['iburst'] = None
>>> this should be moved to __init__()
>>>
>>> 10.)
>>> + with ipaaugeas.aug_obj() as aug:
>>> + try:
>>> + aug.add_lens(self.lens, [self.conf_file])
>>> + aug.load()
>>> + except RuntimeError as e:
>>> + raise NTPConfigurationError(e)
>>>
>>> This code is repeated quite a few times, maybe it would be a good idea
>>> to factor it out to a method of NTPService object.
>>>
>>>
>>> Patch 0097:
>>>
>>> 1.) please fix a typo here:
>>>
>>> + description="Disble any other Time synchronization services."
>>>
>>> 2.)
>>>
>>> + installutils, kra, krbinstance, memcacheinstance, ntpinstance,
>>> you have 2 spaces before 'ntpinstance'
>>>
>>
>> I'm adding my nitpicks too :)
>>
>> 1)
>> +#!/usr/bin/python
>>
>> This should not be in modules, only in executable files
>>
>> 2)
>> Missing license in ipaaugeas.py
>>
>> Martin^2
>
> Hello,
> after offline discussion with Honza I've rewritten the augeas wrapper
> and modified ipautil/ntp.py accordingly. The rest should be pretty much
> the same.
> Patches attached.
>
Rebased and added minimal test suite.
--
David Kupka
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-43-dkupka-0095.2-augeas-add-wrapper-around-python-binding.patch
Type: text/x-patch
Size: 4812 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160516/29df5c44/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-43-dkupka-0096.2-ntp-Add-module-for-NTP-configuration.patch
Type: text/x-patch
Size: 15414 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160516/29df5c44/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-43-dkupka-0097.2-ntp-Add-platform-specific-tasks.patch
Type: text/x-patch
Size: 4182 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160516/29df5c44/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-43-dkupka-0098.2-ntp-install-Use-tasks-to-configure-NTP-daemon.patch
Type: text/x-patch
Size: 30596 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160516/29df5c44/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-43-dkupka-0101.0-test-Basic-tests-for-configfile-module.patch
Type: text/x-patch
Size: 3627 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160516/29df5c44/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0095.2-augeas-add-wrapper-around-python-binding.patch
Type: text/x-patch
Size: 4812 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160516/29df5c44/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0096.2-ntp-Add-module-for-NTP-configuration.patch
Type: text/x-patch
Size: 15304 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160516/29df5c44/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0097.2-ntp-Add-platform-specific-tasks.patch
Type: text/x-patch
Size: 3667 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160516/29df5c44/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0098.2-ntp-install-Use-tasks-to-configure-NTP-daemon.patch
Type: text/x-patch
Size: 33082 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160516/29df5c44/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0101.0-test-Basic-tests-for-configfile-module.patch
Type: text/x-patch
Size: 3627 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160516/29df5c44/attachment-0009.bin>
More information about the Freeipa-devel
mailing list