cloud-init team mailing list archive

[Robert Schweikert <rjschwei@xxxxxxxx>]

Hi,

[1] is an initial rough implementation to support chrony and I'd
appreciate comments/feedback before I go on and fix the failing tests
and clean things up.

Some initial thoughts/questions from my side:
1.) For SUSE we will need to differentiate based on distribution release
whether we have chrony or ntp. Given the current implementation I'd say
other distros already have that choice to make between other clients.
  a.) I decided to read /etc/os-release which creates a complication
during testing, more on this below
  b.) An is_installable() test, as is currently implemented to
differentiate between ntp and timesyncd, is IMHO not reliable as the
user could have a repository that provides for installation of X, even
if the default way for time sync is Y for the given distribution
release. Meaning we either end up with an ambiguity or we make a choice
for the user. This conundrum exists as long as we we use the same
configuration "cc_ntp" for all time service clients, which is reasonable
as the data is really the same. IMHO the choice should be the
distribution default. Therefore there has to be a way for the code to
differentiate between version A and B and thus it becomes necessary to
read os-release.
  c.) Another option would be to add a "client" attribute to the
"cc_ntp" configuration. However, that brings us to the predicament of
handling configurations that do not set "client", meaning every existing
cloud.cfg file, and thus this could be considered an incompatibility. If
the new attribute is mandatory then every user has to touch their
existing configuration, if it is not mandatory we end up in the same
predicament as before of having to make a choice for the user and thus
we need to read os-release. Or if it is not set a distribution
implementation may choose to throw up it's hands and give up, not nice
either.

2.) With the changes I am proposing to add 'timesync_client' and
'timesync_service_name' to the distro implementation. While not every
distro implementation is moved to the new scheme we can easily test this
and fall back to the current configuration. The setting of these is then
distribution dependent which leads back to the comments in point 1.)
above. The 'timesync_service_name' is needed as the current code makes
the assumption that the package and the service name are the same since
is_installable() checks for 'ntp' and later on
reload_or_restart_service() is called also with 'ntp', well the
"service_name" variable value, but of course on SUSE distributions this
is really called ntpd.service to match the executable it starts rather
than the package name. Thus the current implementation is really broken
on openSUSE and SLES. Of course one could insert a "if cloud.distro.name
== ..." condition but that again leads to argument 1b.) where I content
that is_installable() is less than ideal for testing about which
timesync client should be used. Thus I think moving the setting to an
explicit setting in the distribution is the proper approach, unless of
course we decide that 1c.) is a more reasonable approach. 1.c) would
certainly be less complicated in the code ;)

3.) Testing is now more complicated as reading /etc/os-release in the
test environment needs to be handled. Thus I added write_os_release() to
the base class for all test classes. This would provide a relatively
straight forward way of creating the file in the temporary root that is
being used in many tests. I am not particularly fond of this but it does
the trick. One advantage, if we go down this path and every distro ends
up reading /etc/os-release is that for most test we could write a
generic os-release as in most cases it just has to be there and only for
some tests does the file have to have specific values. Thus the generic
file could be written in setup(). Anyway, in the end the content that is
being read from os-release needs to be supplied in some way. This could
happen via @patch or by writing the file. From my perspective, if I have
to write @patch(util....) and then mock the return to be the content of
the os-release file I might as well just write the file to the temporary
root and get it over with. The effort I think is the same.

I think this is a reasonable amount of food for thought and a starting
point for a discussion to figure out an approach that works for the
various distros and clients we have to deal with.

And of course needless to say it would be great if we can resolve this
in the not too distant future :)

Thanks,
Robert

-- 
Robert Schweikert                   MAY THE SOURCE BE WITH YOU
Distinguished Architect                       LINUX
Team Lead Public Cloud
rjschwei@xxxxxxxx
IRC: robjo

Attachment: signature.asc
Description: OpenPGP digital signature