Add systemd-timesyncd support #43

bastelfreak · 2017-10-02T22:39:30Z

No description provided.

ekohl · 2017-10-02T23:05:26Z

spec/classes/init_spec.rb

@@ -23,6 +23,14 @@
          it { is_expected.to create_service('systemd-networkd').with_ensure('running') }
          it { is_expected.to create_service('systemd-networkd').with_enable(true) }
        end
+        context 'when enabling timesyncd' do


Maybe extend the existing test where both resolved and networkd are enabled? Or at least add the is_expected.to_not to the plain test?

I meant to add is_expected.not_to on line 13 as well.

ups. added that as well

ekohl

Looks good. Have you considered also managing the config file?

wyardley · 2017-10-04T20:27:02Z

manifests/init.pp

+  Enum['stopped','running']        $networkd_ensure  = 'running',
+  Boolean                          $manage_timesyncd = false,
+  Enum['stopped','running']        $timesyncd_ensure = 'running',
+  Optional[String] $ntp_server                       = undef,


Why not an array?

I thougt about it. Systemd expects a comma separated list. What do you think about an array which I join()?

yep, I think that's better. The API you expose through puppet should make sense in puppet land. Use join in the template or wherever.

I think that would be a cleaner interface, personally (having it be structured data in Puppet, and join it later). Not sure whether it should require an array for a single element list, or whether it should allow you to pass in a string and force to list context.

If you're being really friendly, I suppose you might also accept a string with no commas in instead of forcing people to use arrays of size 1.

And if it's an array, the params should probably be ntp_servers and fallback_ntp_server?
This also makes it clear that the params can take multiple values

@wyardley +1 Agreed. If you expect an array, pluralise the parameter name.

I would like to keep ntp_server and fallback_ntp_serve because those are the keys in the ini file. I updated the code to accept a single string or arrays. What do you think about that?

I personally think it should be plural, will defer to @alexjfisher

trevor-vaughan · 2017-10-04T20:39:13Z

Not all systems have timesyncd. I think that enabling the timesyncd class should be predicated on timesyncd actually being present on the system.

The same goes for networkd

bastelfreak · 2017-10-04T21:00:38Z

@trevor-vaughan it defaults to off/dont manage. Is that okay? Is there a way to check the presence without a fact?

trevor-vaughan · 2017-10-04T21:05:26Z

@bastelfreak I'm just worried that someone will turn it on without knowing that it's not present on their system and "bad things"(tm) will happen.

For instance, my Fedora system has both of these, but my RHEL 7 system does not. If I have something in Hiera based on osfamily=RedHat, then it's not going to be pretty for my Fedora systems.

While, technically, this is the fault of the user, it would be best to fail safe and perhaps notify them that they're trying to do something that won't work.

Unfortunately, you can't base it off of existing facts from what I can tell, you're going to have to actually query systemd itself to see what capabilities it actually has.

bastelfreak · 2017-10-04T21:23:08Z

@trevor-vaughan uff. This needs a huge amount of code. I agree that it would be nice. I don't know a sane way to query systemd about it. One solution would be to check for /usr/lib/systemd/systemd-timesyncd, but the path isn't the same on all distributions. Should this improvement be moved into its own pull request?

trevor-vaughan · 2017-10-04T21:24:49Z

@bastelfreak It's a systemd service. systemctl list-unit-files systemd-timesyncd.service appears to work just fine.

trevor-vaughan · 2017-10-04T21:28:30Z

@bastelfreak Did a bit more poking and this seems to give a good list that would make a great facter hash: systemctl list-unit-files --plain systemd-* | grep -v @ | grep -v static

bastelfreak · 2017-10-04T21:33:00Z

oh right, I totally forgot that :D

bastelfreak · 2017-10-04T22:05:20Z

systemctl list-unit-files --no-pager | awk '!/static|@/ && $1 ~ /systemd-.*.service/ { print $1 " " $2}'

I can wrap that up tomorrow in a fact (or do it in pure ruby)

trevor-vaughan · 2017-10-05T01:46:53Z

Pure ruby would probably be less fragile over time, but that would be ideal, thanks!

bastelfreak · 2017-10-05T08:39:48Z

Jo @trevor-vaughan! The awesome @jreinert and I added a ruby implementation of the structured fact. Output is:

{
  systemd-fsck-root.service => "enabled-runtime",
  systemd-journal-gatewayd.service => "indirect",
  systemd-journal-remote.service => "indirect",
  systemd-journal-upload.service => "disabled",
  systemd-networkd-wait-online.service => "enabled",
  systemd-networkd.service => "enabled",
  systemd-nspawn@.service => "disabled",
  systemd-resolved.service => "enabled",
  systemd-timesyncd.service => "disabled"
}

What do you think about that?

ekohl · 2017-10-05T08:43:01Z

lib/facter/systemd.rb

-# Purpose: 
-#   Determine whether systemd is the init system on the node
+# Purpose:
+#   Determine whether SystemD is the init system on the node


it's plain systemd, not SystemD according to https://www.freedesktop.org/wiki/Software/systemd/

oh dang. I did some copy and paste issues. my patch for this file isn't based on master. I will fix that

ekohl · 2017-10-05T08:43:19Z

lib/facter/systemd.rb

 Facter.add(:systemd) do
  confine :kernel => :linux
  setcode do
-    Facter.value(:service_provider) == 'systemd'
+    Facter::Util::Resolution.exec('ps -p 1 -o comm=') == 'systemd'


Please don't revert this

ekohl · 2017-10-05T08:48:17Z

manifests/timesyncd.pp

+    enable => $_enable_timesyncd,
+  }
+
+  if $ntp_server {


Should it otherwise be ensured to be absent?

the linux distribution may place default values in there. so IMO we should not touch it.

EmilienM · 2017-10-12T13:56:22Z

👍

trevor-vaughan · 2017-10-12T18:34:31Z

manifests/init.pp

@@ -32,11 +50,15 @@
    create_resources('systemd::service_limits', $service_limits)
  }

-  if $manage_resolved {
+  if $manage_resolved and $facts['systemd_internal_services']['systemd-resolved.service'] {


This is going to fail hard if the fact could not be populated for some reason.

Should probably be: if $mange_resolved and $facts['systemd_internal_services'] and $facts['systemd_internal_services']['systemd-resolved.service']

trevor-vaughan

👁️ 👍

ekohl · 2017-10-16T08:04:36Z

manifests/init.pp

@@ -32,11 +50,15 @@
    create_resources('systemd::service_limits', $service_limits)
  }

-  if $manage_resolved {
+  if $manage_resolved and $facts['systemd_internal_services'] and $facts['systemd_internal_services']['systemd-resolved.service'] {


Maybe a bit late to the party but if manage_resolved is true and the system can't use it, should that be an error?

this would add another layer, not sure if this is actually needed. Is it safe to assume that only people that know their systems try to enable it? (Or at least know if they run systemd or not)

Also using the module would be a bit more complicated/requires more logic. Currently I apply it to all servers in our org, no matter if its a freebsd, old centos or something with systemd. Fail safely vs fail hard

On the other hand, if you don't manage this you might silently not manage something when you expect it to. Makes me wonder if it should be a tristate: off, auto and on where on has checks if it's actually enabled.

I thought about this, but unsure of the needed complexity is worth it

The plugin class `yum::plugin::versionlock` should not be `contain`ed from every instance of the `yum::versionlock` defined type. Doing so makes it impossible to use the type in multiple dependent classes/profiles. `contain` implies that the resource 'owns' the plugin. Using `require` should be just as effective at solving voxpupuli#43 without introducing dependency cycles. With `contain`... ```puppet class profile::foo { yum::versionlock { '0:bash-4.1.2-9.el6_2.*':} } class profile::bar { yum::versionlock { '2:vim-enhanced-7.4.629-6.el7.x86_64':} } include profile::foo include profile::bar Class['profile::foo'] -> Class['profile::bar'] ``` fails with ``` Error: Found 1 dependency cycle: (Augeas[yum.conf_plugins] => Yum::Config[plugins] => Yum::Plugin[versionlock] => Class[Yum::Plugin::Versionlock] => Yum::Versionlock[0:bash-4.1.2-9.el6_2.*] => Class[Profile::Foo] => Class[Profile::Bar] => Yum::Versionlock[2:vim-enhanced-7.4.629-6.el7.x86_64] => Class[Yum::Plugin::Versionlock] => Yum::Plugin[versionlock] => Package[yum-plugin-versionlock] => Yum::Plugin[versionlock])\nTry the '--graph' option and opening the resulting '.dot' file in OmniGraffle or GraphViz Error: Failed to apply catalog: One or more resource dependency cycles detected in graph ``` This commit also refactors the type with: * puppet-strings style docs. * A simple `unless` instead of a `case` for `ensure`. * `assert_type` instead of `is_a`. Fixes voxpupuli#43

ekohl reviewed Oct 2, 2017

View reviewed changes

bastelfreak force-pushed the timesyncd branch from f73a0ee to eb14c6d Compare October 2, 2017 23:40

ekohl approved these changes Oct 3, 2017

View reviewed changes

bastelfreak added 7 commits October 4, 2017 21:50

add support for systemd-timesyncd

e6d3b40

fix typo in the networkd class

f3b30f8

add docs for the timesyncd class

a6e3f86

add tests for timesyncd

79e0cec

extend unit tests

fc9605d

allow to configure ntp settings for timesyncd

17706ef

fix typos on resolved/networkd

d5bf0e8

bastelfreak force-pushed the timesyncd branch from eb14c6d to d5bf0e8 Compare October 4, 2017 19:55

wyardley reviewed Oct 4, 2017

View reviewed changes

allow string+array for ntp values

9fa085b

bastelfreak force-pushed the timesyncd branch from 9adca8a to 9fa085b Compare October 4, 2017 21:06

ekohl reviewed Oct 5, 2017

View reviewed changes

bastelfreak force-pushed the timesyncd branch 3 times, most recently from 7e86ce5 to 3a69968 Compare October 12, 2017 05:09

EmilienM approved these changes Oct 12, 2017

View reviewed changes

bastelfreak added 3 commits October 12, 2017 11:20

add structured fact to list systemd services + state

cab471c

mock systemd_internal_services fact

0324535

include mocked facts in rpsec-puppet-facts

61e28c1

bastelfreak force-pushed the timesyncd branch from 3a69968 to 74cfb58 Compare October 12, 2017 18:20

trevor-vaughan reviewed Oct 12, 2017

View reviewed changes

only contain classes if the needed services exists

0d1b764

bastelfreak force-pushed the timesyncd branch from 74cfb58 to 0d1b764 Compare October 12, 2017 18:38

trevor-vaughan approved these changes Oct 12, 2017

View reviewed changes

bastelfreak merged commit a1ca247 into voxpupuli:master Oct 12, 2017

bastelfreak deleted the timesyncd branch October 12, 2017 18:55

ekohl reviewed Oct 16, 2017

View reviewed changes

raphink added the enhancement New feature or request label Aug 21, 2020

Add systemd-timesyncd support #43

Add systemd-timesyncd support #43

Conversation

bastelfreak commented Oct 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekohl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexjfisher Oct 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trevor-vaughan commented Oct 4, 2017

bastelfreak commented Oct 4, 2017

trevor-vaughan commented Oct 4, 2017

bastelfreak commented Oct 4, 2017

trevor-vaughan commented Oct 4, 2017

trevor-vaughan commented Oct 4, 2017

bastelfreak commented Oct 4, 2017

bastelfreak commented Oct 4, 2017

trevor-vaughan commented Oct 5, 2017

bastelfreak commented Oct 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EmilienM commented Oct 12, 2017

trevor-vaughan Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

trevor-vaughan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexjfisher Oct 4, 2017 •

edited

Loading

trevor-vaughan Oct 12, 2017 •

edited

Loading