(PUP-3939) Sync with upstream OpenBSD rcctl(1) changes

[jasperla]

The rcctl(1) tool recently changed the interface to use it; this PR puts Puppet back in sync.

Everything works, however there's one spec failure that I don't understand:

  Puppet::Type::Service::ProviderOpenbsd#enabled? should return :false if the service is disabled
     Failure/Error: provider.enabled?.should == :false
       expected: :false
            got: :true (using ==)
       Diff:
       @@ -1,2 +1,2 @@
       -:false
       +:true
     # ./spec/unit/provider/service/openbsd_spec.rb:120:in `block (3 levels) in <top (required)>'
     # util/rspec_runner:37:in `run'
     # util/rspec_runner:51:in `<main>'

Looking at the relevant code:

  def enabled?
    begin
      rcctl(:get, @resource[:name], :status)
    rescue Puppet::ExecutionFailure
      self.debug("Is disabled")
      return :false
    else
      self.debug("Is enabled")
      return :true
    end
  end

So the way the rcctl get sshd should work is return 1 if the service is not enabled, and 0 otherwise. It's now implemented as a begin/rescue/else block, but that doesn't seem to do the trick.
Could someone help me to get this sorted, or to let me know how to have execute() save the return code?

[puppetcla]

CLA signed by all contributors.

[joshcooper]

@jasperla Puppet will mixin Puppet::Util::Execution to every provider both at the instance-level and class-level. This way the provider can call execute in an instance method as is done in enabled? and can call execute in a class method, like self.instances. By default Puppet::Util::Execution.execute will raise if the command returns non-zero exit code. You can change the behavior by passing :failonfail => false as the last argument. Unfortunately for historical reasons, this will switch the default value of :combine to false. This means stdout and stderr won't be combined. If you care about having them combined you can explicit specify :combine => true. So given that you should be able to do something like the yum package provider:

output = Puppet::Util::Execution.execute(args, :failonfail => false, :combine => false)
...
if output.exitstatus == 100

Hope that helps!

[jasperla]

Thanks for the hint about output.exitstatus, however it seems something is still off.

In a regular shell:

% rcctl get sshd status ; echo $?
0
% rcctl get iked status ; echo $?
1
%

So sshd is running, whereas iked is not.

Puppet does the right thing and knows when it's enabled and when it's not:

% sudo puppet apply --debug -e 'service{"sshd": enable => true}'
[...]
Debug: Executing '/usr/sbin/rcctl check sshd'
Debug: Executing '/usr/sbin/rcctl get sshd status'
Debug: Service[sshd](provider=openbsd): Is enabled
[...]
% sudo puppet apply --debug -e 'service{"sshd": enable => false}'
[...]
Debug: Executing '/usr/sbin/rcctl check sshd'
Debug: Executing '/usr/sbin/rcctl get sshd status'
Debug: Service[sshd](provider=openbsd): Is enabled
Debug: Service[sshd](provider=openbsd): Disabling
Debug: Executing '/usr/sbin/rcctl disable sshd'
Notice: /Stage[main]/Main/Service[sshd]/enable: enable changed 'true' to 'false'
[...]
%  sudo puppet apply --debug -e 'service{"sshd": enable => true}'
Debug: Executing '/usr/sbin/rcctl check sshd'
Debug: Executing '/usr/sbin/rcctl get sshd status'
Debug: Service[sshd](provider=openbsd): Is disabled
Debug: Service[sshd](provider=openbsd): Enabling
Debug: Executing '/usr/sbin/rcctl enable sshd'
Notice: /Stage[main]/Main/Service[sshd]/enable: enable changed 'false' to 'true'

However it seems when the code is called from rspec it suddenly breaks? Looking at the current failure,
it actually returns :false when the service is enabled (should return :true if the service is enabled) and the other test (should return :false if the service is disabled) fails with an unsatisfied expectations. However if I modify #enabled? and swap the return values, the spec test's output swaps too... I'm a bit lost now..

[jasperla]

I've so far been unable to fix this myself, and help would be greatly appreciated or Puppet 4 won't be able to manage any services on OpenBSD :(

[ScottGarman]

@jasperla - I hope to have time to assist this week on this PR. I've created a ticket PUP-3939 to track the issue.

One thing I'm not clear on is if these changes are appropriately backward-compatible? I don't have an OpenBSD system to verify this on, so I'm proceeding cautiously here.

[jasperla]

Hi, thanks for that.

The changes are not quite backwards compatible as rcctl's changes are not as well. rcctl has not been in a proper OpenBSD release yet so I don't think that's too much of an issue right now.

[ScottGarman]

@jasperla ok, it sounds then like what should happen is the provider and spec should be updated to branch on the version of openbsd that the new interface will be the default for, and we should keep the old commands to be the default when the older variant of rcctl is used.

Also, the spec files in master have recently been updated to use the syntax in rspec v3, so I'm getting merge conflicts trying to cherry-pick these changes into an updated test branch I'm working from. Can you clean up those conflicts and add the appropriate branching in the provider & spec?

[jasperla]

@ScottGarman well, both checks will be for OpenBSD 5.7 then since rcctl itself was introduced after OpenBSD 5.6 was released and will thus first appear in OpenBSD 5.7. So in my opinion it's not quite needed to bother with backwards compat.

[jasperla]

The merge errors have been resolved now,

[ScottGarman]

@jasperla thanks for your patience - I've fixed up the rspec test and updated the commit message in a new PR-3663. I'll close out this PR in favor of the new one.

Since I made a trivial change to the provider code, could I ask you one last time to test the new PR and add a comment to it with the results of your testing? Thanks!