[Freeipa-devel] [PATCH 0095-0098] NTP: use augeas, configure chronyd, do not overwrite config
David Kupka
dkupka at redhat.com
Tue Apr 26 08:09:59 UTC 2016
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.
--
David Kupka
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0095.1-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/20160426/4cdafd35/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0096.1-ntp-Add-module-for-NTP-configuration.patch
Type: text/x-patch
Size: 15332 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160426/4cdafd35/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0097.1-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/20160426/4cdafd35/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0098.1-ntp-install-Use-tasks-to-configure-NTP-daemon.patch
Type: text/x-patch
Size: 33116 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160426/4cdafd35/attachment-0003.bin>
More information about the Freeipa-devel
mailing list