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

Martin Babinsky mbabinsk at redhat.com
Fri May 20 12:29:53 UTC 2016


On 05/16/2016 01:58 PM, David Kupka wrote:
> 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.
>

Hi David,

patch 0095:

I second Petr's opinion, at least some short examples of fetching and 
modifying configuration values for .e.g /etc/hosts would be welcome.

Also there is some leftover comment in the Element.__setitem__:
              # self.aug.copy(value.path, self(loc).path) 

              copy(self.aug, value.path, self(loc).path)


patch 0096:

1.)
+    def __init__(self):
+        self.service_name = ''
+        self.conf_file = ''
+        self.lens = ''
+        self.source_keywords = ['server']
+        self.server_options = {}
+        self.global_options = {}

I would rather rewrite this as a proper __init__ method with parameters:

def __init__(self, service_name = '', self.conf_file = '', ...)

This is clearer for me than calling empty __init__() of superclass and 
then setting instance attributes separately.

Also the self.lens attribute seems to be unused (leftover from previous 
revisions?)

2.)
+
+    def configure_client(self, ntp_servers=[], sstore=None, fstore=None):
+        service = services.service(self.service_name)
+
+        self._store_service_state(sstore, fstore)

Use 'ntp_servers=()' so that there is one less case of pylint 
complaining about using mutable object as default when we switch on this 
check in the future ;).

3.)
I would not turn "check_services" into instance method since it does not 
use instance variable at all. I would rather turn this into normal 
module-level function as was before and name it to 
'check_conflicting_timedate_services' or something like that.

In this way you wouldn't have to instantiate the whole class just to 
check whether there is a conflict between ntpd/chrony/whatever.

The same could be argued about 'force' and 'restore_forced' methods.

4.)
+
+class Ntp(NTPService):
+

according to pep8 style guide[1]:

"""
Note: When using abbreviations in CapWords, capitalize all the letters 
of the abbreviation. Thus HTTPServerError is better than HttpServerError.
"""
so name the class NTP please.

5.) "NTP" and "Chrony" do not need to override parent's 
'configure_client' method since the overriden one only calls 
super-method anyway.

Patch 0097

Double-check that you are not creating cyclic imports 
ipapython->ipaplatform. And please import only ipapython.ntp, not the 
whole package.

Patch 0098

1.)

+            tasks.ntp_check_conflict(options.force_ntp)
+        except ntp.NTPConflictingService as e:
+            print(e)

I know that this is ipa-client-install and people expect it to print 
errors into stdout, but it would be nice to also put the error into 
logger at warning level so that there is a trace of it in the logs in 
case of trouble. The same can be said about the check for conflicting 
NTP services in 'ipa-server/replica-install'.

Patch 101

Yay test and it even passes! It would be nice to have there some 
negative test cases but that can be extended by ^QE later.

[1] https://www.python.org/dev/peps/pep-0008/#descriptive-naming-styles

-- 
Martin^3 Babinsky




More information about the Freeipa-devel mailing list