[Freeipa-devel] [PATCH 0095-0098] NTP: use augeas, configure chronyd, do not overwrite config

[Martin Basti]

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