[Freeipa-devel] [PATCH 0095-0098] NTP: use augeas, configure chronyd, do not overwrite config
Jan Cholasta
jcholast at redhat.com
Mon May 23 06:25:36 UTC 2016
On 20.5.2016 14:54, Martin Babinsky wrote:
> On 05/20/2016 02:29 PM, Martin Babinsky wrote:
>> 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
>>
> Also an attempt to install IPA server on F23 with chrony running
> resulted in this traceback:
>
> """
> 2016-05-20T12:51:14Z DEBUG Traceback (most recent call last):
> File "/usr/lib/python2.7/site-packages/ipaserver/install/service.py",
> line 447, in start_creation
> run_step(full_msg, method)
> File "/usr/lib/python2.7/site-packages/ipaserver/install/service.py",
> line 437, in run_step
> method()
> File
> "/usr/lib/python2.7/site-packages/ipaserver/install/ntpinstance.py",
> line 39, in __configure_server
> tasks.ntp_configure_server(self.sstore, self.fstore, self.force)
> File "/usr/lib/python2.7/site-packages/ipaplatform/tasks.py", line 52,
> in ntp_configure_server
> ntp.configure_server(sstore, fstore)
> File "/usr/lib/python2.7/site-packages/ipapython/ntp.py", line 303, in
> configure_server
> if not timesync['allow']:
> File "/usr/lib/python2.7/site-packages/ipapython/configfile.py", line
> 83, in __getitem__
> raise KeyError(loc)
> KeyError: 'allow'
>
> """
>
> That was probably caused by commented "allow" directive in default
> chrony config (http://paste.fedoraproject.org/368862/63748838).
>
Patch 95:
Rather than checking for existence of Augeas.copy each time your copy is
called, check it when the module is loaded:
try:
copy = augeas.Augeas.copy
except attributeError:
def copy(aug, src, dst):
...
It looks like only the Config class should be public, please underscore
everything else. Ditto for object attributes.
IMO the classes could be named to better reflect what they are doing. I
would go with Element -> Expression, Node -> Predicate, List -> Child.
Element.__call__ and Element.__getitem__ should require at least 1
argument and raise TypeError instead of returning None with invalid
arguments.
The "__nonzero__ = __bool__" should be right below __bool__ definition.
In Config, split the file path once in __init__. You should also
probably canonicalize it before splitting it.
Patch 97:
Could you use polymorphism here instead of copy-paste?
Patch 98:
You should keep --force-ntpd as an alias for --force-ntp for backward
compatibility.
Honza
--
Jan Cholasta
More information about the Freeipa-devel
mailing list