[PATCH/puppet 1/1] Fixes #2581. Use new 10.6 global launchd overrides file for service status/enabled

[Nigel Kersten]

Signed-off-by: Nigel Kersten <nig...@google.com>
---
lib/puppet/provider/service/launchd.rb | 92 ++++++++++++++++++++++++++++----
spec/unit/provider/service/launchd.rb | 58 ++++++++++++++++++++
2 files changed, 140 insertions(+), 10 deletions(-)

diff --git a/lib/puppet/provider/service/launchd.rb b/lib/puppet/provider/service/launchd.rb
index 16fc55b..770a7b1 100644
--- a/lib/puppet/provider/service/launchd.rb
+++ b/lib/puppet/provider/service/launchd.rb
@@ -37,6 +37,7 @@ Puppet::Type.type(:service).provide :launchd, :parent => :base do
"

commands :launchctl => "/bin/launchctl"
+ commands :sw_vers => "/usr/bin/sw_vers"

defaultfor :operatingsystem => :darwin
confine :operatingsystem => :darwin
@@ -48,6 +49,8 @@ Puppet::Type.type(:service).provide :launchd, :parent => :base do
"/System/Library/LaunchAgents",
"/System/Library/LaunchDaemons",]

+ Launchd_Overrides = "/var/db/launchd.db/com.apple.launchd/overrides.plist"
+

# returns a label => path map for either all jobs, or just a single
# job if the label is specified
@@ -86,6 +89,36 @@ Puppet::Type.type(:service).provide :launchd, :parent => :base do
new(:name => job, :provider => :launchd, :path => jobs[job])
end
end
+
+
+ def self.get_macosx_version_major
+ if defined? @macosx_version_major
+ return @macosx_version_major
+ end
+ begin
+ # Make sure we've loaded all of the facts
+ Facter.loadfacts
+
+ if Facter.value(:macosx_productversion_major)
+ product_version_major = Facter.value(:macosx_productversion_major)
+ else
+ # TODO: remove this code chunk once we require Facter 1.5.5 or higher.
+ Puppet.warning("DEPRECATION WARNING: Future versions of the launchd provider will require Facter 1.5.5 or newer.")
+ product_version = Facter.value(:macosx_productversion)
+ if product_version.nil?
+ fail("Could not determine OS X version from Facter")
+ end
+ product_version_major = product_version.scan(/(\d+)\.(\d+)./).join(".")
+ end
+ if %w{10.0 10.1 10.2 10.3}.include?(product_version_major)
+ fail("%s is not supported by the launchd provider" % product_version_major)
+ end
+ @macosx_version_major = product_version_major
+ return @macosx_version_major
+ rescue Puppet::ExecutionFailure => detail
+ fail("Could not determine OS X version: %s" % detail)
+ end
+ end


# finds the path for a given label and returns the path and parsed plist
@@ -171,33 +204,72 @@ Puppet::Type.type(:service).provide :launchd, :parent => :base do

# launchd jobs are enabled by default. They are only disabled if the key
# "Disabled" is set to true, but it can also be set to false to enable it.
+ # In 10.6, the Disabled key in the job plist is consulted, but only if there
+ # is no entry in the global overrides plist.
+ # We need to draw a distinction between undefined, true and false for both
+ # locations where the Disabled flag can be defined.
def enabled?
+ job_plist_disabled = nil
+ overrides_disabled = nil
+
job_path, job_plist = plist_from_label(resource[:name])
if job_plist.has_key?("Disabled")
- if job_plist["Disabled"] # inverse of disabled is enabled
- return :false
+ job_plist_disabled = job_plist["Disabled"]
+ end
+
+ if self.class.get_macosx_version_major == "10.6":
+ overrides = Plist::parse_xml(Launchd_Overrides)
+
+ unless overrides.nil?
+ if overrides.has_key?(resource[:name])
+ if overrides[resource[:name]].has_key?("Disabled")
+ overrides_disabled = overrides[resource[:name]]["Disabled"]
+ end
+ end
+ end
+ end
+
+ if overrides_disabled.nil?
+ if job_plist_disabled.nil? or job_plist_disabled == false
+ return :true
end
+ elsif overrides_disabled == false
+ return :true
end
- return :true
+ return :false
end


# enable and disable are a bit hacky. We write out the plist with the appropriate value
# rather than dealing with launchctl as it is unable to change the Disabled flag
# without actually loading/unloading the job.
+ # In 10.6 we need to write out a disabled key to the global overrides plist, in earlier
+ # versions this is stored in the job plist itself.
def enable
- job_path, job_plist = plist_from_label(resource[:name])
- if self.enabled? == :false
- job_plist.delete("Disabled")
- Plist::Emit.save_plist(job_plist, job_path)
+ if self.class.get_macosx_version_major == "10.6"
+ overrides = Plist::parse_xml(Launchd_Overrides)
+ overrides[resource[:name]] = { "Disabled" => false }
+ Plist::Emit.save_plist(overrides, Launchd_Overrides)
+ else
+ job_path, job_plist = plist_from_label(resource[:name])
+ if self.enabled? == :false
+ job_plist.delete("Disabled")
+ Plist::Emit.save_plist(job_plist, job_path)
+ end
end
end


def disable
- job_path, job_plist = plist_from_label(resource[:name])
- job_plist["Disabled"] = true
- Plist::Emit.save_plist(job_plist, job_path)
+ if self.class.get_macosx_version_major == "10.6"
+ overrides = Plist::parse_xml(Launchd_Overrides)
+ overrides[resource[:name]] = { "Disabled" => true }
+ Plist::Emit.save_plist(overrides, Launchd_Overrides)
+ else
+ job_path, job_plist = plist_from_label(resource[:name])
+ job_plist["Disabled"] = true
+ Plist::Emit.save_plist(job_plist, job_path)
+ end
end


diff --git a/spec/unit/provider/service/launchd.rb b/spec/unit/provider/service/launchd.rb
index b2c51a4..fa86a21 100755
--- a/spec/unit/provider/service/launchd.rb
+++ b/spec/unit/provider/service/launchd.rb
@@ -33,6 +33,10 @@ describe provider_class do
@provider.stubs(:plist_from_label).returns([@joblabel, @jobplist])
@provider.stubs(:execute).returns("")
@provider.stubs(:resource).returns @resource
+
+ # We stub this out for the normal case as 10.6 is "special".
+ provider_class.stubs(:get_macosx_version_major).returns("10.5")
+
end

it "should have a start method for #{@provider.object_id}" do
@@ -74,6 +78,42 @@ describe provider_class do
@provider.status.should == :running
end
end
+
+ describe "when checking whether the service is enabled" do
+ it "should return true if the job plist says disabled is false" do
+ @provider.stubs(:plist_from_label).returns(["foo", {"Disabled" => false}])
+ @provider.enabled?.should == :true
+ end
+ it "should return true if the job plist has no disabled key" do
+ @provider.stubs(:plist_from_label).returns(["foo", {}])
+ @provider.enabled?.should == :true
+ end
+ it "should return false if the job plist says disabled is true" do
+ @provider.stubs(:plist_from_label).returns(["foo", {"Disabled" => true}])
+ @provider.enabled?.should == :false
+ end
+ end
+
+ describe "when checking whether the service is enabled on OS X 10.6" do
+ it "should return true if the job plist says disabled is true and the global overrides says disabled is false" do
+ provider_class.stubs(:get_macosx_version_major).returns("10.6")
+ @provider.stubs(:plist_from_label).returns(["foo", {"Disabled" => true}])
+ Plist.stubs(:parse_xml).returns({@resource[:name] => {"Disabled" => false}})
+ @provider.enabled?.should == :true
+ end
+ it "should return false if the job plist says disabled is false and the global overrides says disabled is true" do
+ provider_class.stubs(:get_macosx_version_major).returns("10.6")
+ @provider.stubs(:plist_from_label).returns(["foo", {"Disabled" => false}])
+ Plist.stubs(:parse_xml).returns({@resource[:name] => {"Disabled" => true}})
+ @provider.enabled?.should == :false
+ end
+ it "should return true if the job plist and the global overrides have no disabled keys" do
+ provider_class.stubs(:get_macosx_version_major).returns("10.6")
+ @provider.stubs(:plist_from_label).returns(["foo", {}])
+ Plist.stubs(:parse_xml).returns({})
+ @provider.enabled?.should == :true
+ end
+ end

describe "when starting the service" do
it "should look for the relevant plist once" do
@@ -138,5 +178,23 @@ describe provider_class do
@provider.stop
end
end
+
+ describe "when enabling the service on OS X 10.6" do
+ it "should write to the global launchd overrides file once" do
+ provider_class.stubs(:get_macosx_version_major).returns("10.6")
+ Plist.stubs(:parse_xml).returns({})
+ Plist::Emit.expects(:save_plist).once
+ @provider.enable
+ end
+ end
+
+ describe "when disabling the service on OS X 10.6" do
+ it "should write to the global launchd overrides file once" do
+ provider_class.stubs(:get_macosx_version_major).returns("10.6")
+ Plist.stubs(:parse_xml).returns({})
+ Plist::Emit.expects(:save_plist).once
+ @provider.enable
+ end
+ end

end
--
1.6.4

[Nigel Kersten]

The big chunk of code for self.get_macosx_version_major is essentially
copied from the directoryservice provider, and we already had a
discussion about it then, concluding that we need to provide the
alternative code path but with a deprecation warning for older
versions of Facter that don't include the macosx_version_major fact.

To summarize this patch:

Launchd services have historically stored their "enabled?" state in a
Disabled key in their job plist.

In 10.6, Apple changed to using a global overrides plist instead. The
old location is still consulted, but only if the global overrides file
has no entry in it.

When you disable/enable a service in 10.6, you're modifying the global
overrides plist, not the job plist. I've followed this same model for
Puppet.

--
Nigel Kersten
nig...@google.com
System Administrator
Google, Inc.

[Luke Kanies]

I've got a few comments in here, but at this point, given that I'd
like to release 0.25 this week (since it was supposed to be out on
Monday), if these comments are much more than simple reorganization,
I'd rather merge the non-perfect code and release sooner than wait a
week for better code.

So take these with a bit of a grain of salt - if they're easy, I'd
like them done, but not if it's ridiculous.

This is essentially exactly the same code as in the DS provider,
right? Can we move it to a utility module, to remove the code
duplication?

This is acceptable, but might be a bit cleaner pulled into a private
method:

overrides_disabled = snow_leopard_disabled() if
self.class.get_macosx_version_major == "10.6":

...

private

def snow_leopard_disabled
...
end

Or even make it non-private so you can test it separately.

It's almost two providers here, given that everything branches for
10.6. Is it worth making a snowleopard subclass of this provider? It
should be relatively easy, and you can specify it's only valid on
10.6, so you'll get this top-level branch that chooses the right
provider rather than a bunch of branches in every method.

>
> diff --git a/spec/unit/provider/service/launchd.rb b/spec/unit/
> provider/service/launchd.rb

This :true/:false thing is hideous and must be fixed. Opening a
ticket for 0.26 right now. :/

--
The easiest way for your children to learn about money is for you
not to have any. -- Katharine Whitehorn
---------------------------------------------------------------------
Luke Kanies | http://reductivelabs.com | http://madstop.com

[Nigel Kersten]

We could, but these are the only two places that it should at all be
needed for. I'm more than happy to make it a utility function if we
come across another case?

sure. I can do that if you want.

so it's not that everything branches for 10.6, all the 10.4/5 code
paths need to be followed for Snow Leopard as well, but the overrides
is ultimately authoritative. For many cases there will be no override
entry.

I guess I could subclass the provider, and then call the super methods
first ... but how do I confine that subclassed provider to only OS X
10.6 if we aren't requiring that they have a Facter version that
provides macosx_major_version ?

Another factor to keep in mind is that we have Ruby access to the
ServiceManagement framework now in 10.6 so we can stop shelling out
all the time (yay!) and given that we have this for services and for
users/groups with the new Ruby bridge to DirectoryServices, I was
actually planning to rewrite both these types specially for Snow
Leopard and above using the native APIs.

I have a bit of that work done already, but didn't get to finish it
before Snow Leopard launched as they only got this working rather late
in the seed process.

oh please yes....

--
Nigel Kersten
nig...@google.com
System Administrator

Google Inc.

[Luke Kanies]

Given that you're refactoring this based on the new Ruby APIs, let's
just ship it like it is and fix these minor issues in the refactoring.

--
Happiness is not achieved by the conscious pursuit of happiness; it is
generally the by-product of other activities. -- Aldous Huxley