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 [email protected] IRC: robjo
signature.asc
Description: OpenPGP digital signature
-- Mailing list: https://launchpad.net/~cloud-init Post to : [email protected] Unsubscribe : https://launchpad.net/~cloud-init More help : https://help.launchpad.net/ListHelp
