Hi Luke,
First, thanks for the many comments :-)
On Sun, 2008-08-24 at 18:15 -0500, Luke Kanies wrote:
> On Aug 24, 2008, at 1:41 PM, Brice Figureau wrote:
>
> >
> >
> > This path adds a service provider to manage daemontools[1]
> > services directly from puppet.
> >
> > The daemons directories should be placed in /var/lib/service
> > (overridable with the service path parameter).
> > When the service is enabled, the daemon directory is symlinked
> > into the service directory which defaults to /service
> > (or /etc/service on debian). The daemon is then started by svscan.
> > Services are disabled by removing this symlink.
> > Start and stop are almost directly mapped to enable/disable.
> > This provider supports restart and status directly
>
> Cool, I'm glad to see someone supporting this.
>
> As James mentioned, we can't accept this into the main repo without
> tests, but we can work with you to get those done, too.
I didn't had a look to rspec or the other unit test for the moment, but
the question I had is that how can you perform automated testing of the
provider code without using daemontools on the testing platform?
Or should I only test that "enable" wants to create a symlink, or that
"restart" calls the right command, but not issue it?
> > Signed-off-by: Brice Figureau <[EMAIL PROTECTED]>
> > ---
> > lib/puppet/provider/service/daemontools.rb | 147 +++++++++++++++++++
> > +++++++++
> > 1 files changed, 147 insertions(+), 0 deletions(-)
> > create mode 100644 lib/puppet/provider/service/daemontools.rb
> >
> > diff --git a/lib/puppet/provider/service/daemontools.rb b/lib/puppet/
> > provider/service/daemontools.rb
> > new file mode 100644
> > index 0000000..194bbbe
> > --- /dev/null
> > +++ b/lib/puppet/provider/service/daemontools.rb
> > @@ -0,0 +1,147 @@
> > +# Daemontools service management
> > +#
> > +# author Brice Figureau <[EMAIL PROTECTED]>
> > +Puppet::Type.type(:service).provide :daemontools, :parent => :base do
> > + desc "Daemontools service management. The daemons directories
> > should be
> > + placed in ``/var/lib/service`` (overridable with the service
> > path). When enabled
> > + the daemon directories are symlinked to the service directory
> > (and the daemon started
> > + by svscan). Services are disabled by unlinking this symlink.
> > + Start and stop are almost directly mapped to enable/disable.
> > + This provider supports restart and status directly"
> > +
> > + commands :svc => "/usr/bin/svc"
> > + commands :svstat => "/usr/bin/svstat"
> > +
> > + class << self
> > + attr_accessor :defpath
> > + end
> > +
> > + @defpath = "/var/lib/service"
> > + case Facter["operatingsystem"].value
> > + when "Debian":
> > + @@enabledpath = "/etc/service"
> > + else
> > + @@enabledpath = "/service"
> > + end
>
> Why the split between defpath and enabledpath? Am I just missing
> something about them?
In fact, here is the reasoning:
* daemontools daemon are just directories containing a "run" script
that is running under supervision.
* as soon as such directory exist in what is called the service dir
(usually /service or /etc/service on debian), the daemon is started.
* to stop a daemon you have to remove this directory, and then tell
daemontools to stop the service.
* it is more practical to put the daemon directory elsewhere, and to
symlink it to the service directory.
Hence the provider defines too things:
* the service directory (@enabledpath).
* the place where the daemon is (@defpath), which is the one
that can be overriden by @resource[:path].
The question that remains is: how can I let @enabledpath to be changed
by either the user by the way of the resource (or any other way)?
> >
> > + # returns all providers for all existing services in @defpath
> > + # ie enabled or not
> > + def self.instances
> > + path = self.defpath
> > + unless FileTest.directory?(path)
> > + Puppet.notice "Service path %s does not exist" % path
> > + next
> > + end
> > +
> > + check = [:ensure]
> > +
> > + if public_method_defined? :enabled?
> > + check << :enable
> > + end
>
> This 'check' variable isn't actually used anywhere that I can see...
This is copied from the init service. It doesn't seem to be used in the
init service too. Should I remove it?
> >
> > + Dir.entries(path).reject { |e|
> > + fullpath = File.join(path, e)
> > + e =~ /^\./ or ! FileTest.directory?(fullpath)
> > + }.collect do |name|
> > + new(:name => name, :path => path)
> > + end
> > + end
> > +
> > + # Mark that our services supports 'status' commands
> > unconditionally
> > + def hasstatus=(value)
> > + @parameters[:hasstatus] = true
> > + end
>
> Doesn't daemontools always have status? In that case, you should
> probably have a different default for this value.
Yes daemontools always has a status. That's why I don't honor hasstatus.
Or maybe I didn't understood what you meant.
> >
> > + # Where is our appdir
> > + def appdir
> > + if defined? @appdir
> > + return @appdir
> > + else
> > + @appdir = self.search(@resource[:name], @resource[:path])
> > + end
> > + end
> > +
> > + # Where is our daemon if it was enabled
> > + # which doesn't mean there is always one
> > + def servicedir
> > + if defined? @servicedir
> > + return @servicedir
> > + else
> > + @servicedir = File.join( @@enabledpath,
> > @resource[:name] )
> > + end
> > + end
>
> I'd prefer to see an accessor like the defpath above used for
> enabledpath.
That was my initial code, but since I couldn't call it in
enable/enabled?/disable I ended up with this @@ things :-(
Now that I understood why it didn't work, I'll put the accessor again.
It is still not clear why self.defpath can work in self.instances and
not in enabled. Ruby strangeness ?
> In fact, I'd even prefer to see the 'resource' accessor
> used rather than @resource.
Ah OK. That answers a question I asked you in the other thread :-)
I wasn't aware that there was an accessor for the resource.
> Both are stylistic, but they're what I'd do, at least partially
> because they make testing much easier.
>
> >
> > + def restartcmd
> > + [ command(:svc), "-t", self.servicedir]
> > + end
> > +
> > + # find a service by its name an the path in which it is
> > + def search(name, path)
> > + path = [path] unless path.is_a?(Array)
> > + path.each { |path|
> > + fqname = File.join(path,name)
> > + begin
> > + stat = File.stat(fqname)
> > + rescue
> > + # should probably rescue specific errors...
> > + self.debug("Could not find %s in %s" % [name,path])
> > + next
> > + end
> > +
> > + # if we've gotten this far, we found a valid script
> > + return fqname
> > + }
> > + raise Puppet::Error, "Could not find service link in
> > service directory for '%s'" % name
> > + end
> > +
> > + # The start command does nothing, service are automatically
> > started
> > + # when enabled by svscan. But, forces an enable if necessary
> > + def start
> > + # to start make sure the sevice is enabled
> > + self.enable unless self.enabled?
> > + end
>
> You shouldn't need the 'unless' here, afaik; Puppet's transaction
> should only call this if it's convinced it's not currently started.
I'm not quite sure, but I think my test shown that start could be called
even if the service was not yet enabled. But maybe that was when the
code didn't support that enable couldn't be called twice in a row (ie in
enable and then in start).
> >
> > + def status
> > + begin
> > + output = svstat self.servicedir
> > + rescue Puppet::ExecutionFailure
> > + warning "Could not get status on service %s" % self.name
> > + return :stopped
>
> You're probably going to want to throw an exception here, rather than
> just warning, since it's unlikely that Puppet will be able to do
> anything like start the service if it can't even check the status.
OK. I just copied some parts of the smf provider.
> >
> > + end
> > + return :running if output =~ /\bup\b/
> > + return :stopped
> > + end
> > +
> > + # unfortunately it is not possible
> > + # to stop without disabling the service
> > + def stop
> > + self.disable
> > + end
> > +
> > + # disable by stopping the service
> > + # and removing the symlink so that svscan
> > + # doesn't restart our service behind our back
> > + def disable
> > + # should stop the service
> > + # stop log if any
> > + log = File.join(self.servicedir, "log")
> > + texecute("stop log", [ command(:svc) , '-dx', log] ) if
> > FileTest.directory?(log)
>
> Does this "stop log" work? I think that texecute expects an array and
> does a direct system call, which would mean looking for a binary named
> 'stop log'.
The "stop log" string is just for display purpose when something goes
wrong (see the base provider). The texecute just sends the command array
to execute.
> Also, if 'stop' is an executable used by the provider, it should be
> listed as a command at the top. Your standard 'stop' method will
> still work here, I'm pretty sure, although I don't think I've had that
> kind of collision before. If it's an optional command, use
> 'optional_commands :stop => foo'.
>
> >
> > + # stop resource
> > + texecute("stop", [command(:svc), '-dx', self.servicedir] )
> > +
> > + # unlink the symlinks
> > + appdir = self.servicedir
> > + File.unlink(appdir) if FileTest.symlink?(appdir)
> > + end
> > +
> > + def enabled?
> > + FileTest.symlink?(self.servicedir)
> > + end
> > +
> > + def enable
> > + srcdir = self.appdir
> > + enableddir = self.servicedir
> > + File.symlink(srcdir, enableddir) if ! FileTest.symlink?
> > (enableddir)
> > + end
> > +end
Thanks for all the comments.
I'll try to send a revised (with test) version later this week.
Oh, I read in the development lifecycle wiki page that a ticket was
necessary. Should I create one that I assign to myself?
And what milestone should I use (0.24.x, or 0.25) ?
Should I base the code on the current HEAD or the 0.24.x branch ?
Thanks for all the answers :-)
--
Brice Figureau <[EMAIL PROTECTED]>
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Puppet Developers" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to [EMAIL PROTECTED]
For more options, visit this group at
http://groups.google.com/group/puppet-dev?hl=en
-~----------~----~----~----~------~----~------~--~---
