(PUP-10102) fix service crash

[gimmyxd]

When running on distributions that do not have a
default service provider puppet will try to
evaluate runit and set defpath default
before verifying that runit is suitable. Raising an
error when setting defpath results in a state that should
not be reached.

This commit removes the raise exception from defpath
and let puppet decide if the provide is suitable or not.

[gimmyxd]

❯ puppet resource service salam ensure=running provider=systemd
Error: /Service[salam]: Provider systemd is not functional on this host
Error: Could not run: Provider systemd is not functional on this host

❯ puppet resource service salam ensure=running provider=daemontools
Error: /Service[salam]: Provider daemontools is not functional on this host
Error: Could not run: Provider daemontools is not functional on this host

❯ puppet resource service salam ensure=running provider=runit
Error: /Service[salam]: Provider runit is not functional on this host
Error: Could not run: Provider runit is not functional on this host

[GabrielNagy]

👍

[puppetcla]

CLA signed by all contributors.

[joshcooper]

Ideally we could confine the provider based on the existence on one of the daemon directories, so that the framework never calls the unsuitable provider. Something like:

confine :exists => defpath

And then change defpath to:

def self.defpath
  ["/var/lib/service", "/etc"].find do |path|
    Puppet::FileSystem.exist?(path) && FileTest.directory?(path)
  end
end

If we can't confine based on those directories (see the init provider's comment), then I think we should check for defpath returning nil, otherwise we'll call FileTest.directory?(nil) which happens to be nil-safe. Suggest checking for nil explicitly:

def self.instances
  path = self.defpath
  unless path
    Puppet.info("daemontools is unsuitable because service directory is nil")
  end
  ...
end

[gimmyxd]

There are no other confines on defpath so i opt on using the second options as i do not have enough context on why the confine on defpath is problematic. Having the attr_writer :defpath means that @defpath can be set from different places.

https://github.com/puppetlabs/puppet/blob/5.5.x/lib/puppet/provider/service/daemontools.rb#L45

[TheMeier]

The first version led to this error for me running unit tests in debian docker containers:
Could not autoload puppet/type/service: Could not autoload puppet/provider/service/runit: /builds/pocu/xymonclient/vendor/bundle/ruby/2.5.0/gems/puppet-6.10.1/lib/puppet/provider/service/runit.rb:110: syntax error, unexpected end-of-input, expecting keyword_end
Actually the same error I get when i set

Puppet::Type.type(:service).provide(:systemd).instance_eval do
  defaultfor :operatingsystem => :debian
end

in spec_helper.rb

[gimmyxd]

@TheMeier that is a syntax error, make sure that the content of /lib/puppet/provider/service/runit.rb is ok.

[gimmyxd]

There is still an issue because if the path parameter is not set and no default is found, nil is sent around for the path value.

[TheMeier]

@TheMeier that is a syntax error, make sure that the content of /lib/puppet/provider/service/runit.rb is ok.

That is freshly installed via gem every time in the docker container. So it's quite strange and I don't understand where it comes from...

[gimmyxd]

@TheMeier could you try to apply https://patch-diff.githubusercontent.com/raw/puppetlabs/puppet/pull/7794.patch after the regular gem install?

[TheMeier]

@gimmyxd I will try tomorrow

[TheMeier]

D'oh got an explanation for the syntax error. Seems gitlab-ci caches led to me applying my monkey patches twice .. :/

[TheMeier]

@gimmyxd the patch works fine. I have to validate the the issues we were having are not related to gitlab-ci caching in the first place

[TheMeier]

Without the patch and with cache disabled the error also occurs. So I am somewhat confident to say this patch fixes my issues