[PATCHES] Allow daemontools.rb to stop a service without removing it
Bryan Allen
2146-unified.diff is a unified diff of all changes.
--
bda
cyberpunk is dead. long live cyberpunk.
Luke Kanies
commit, using rebase -i.
Also, this patch is missing any tests; could you add some?
Comments below.
> provider/service/daemontools.rb
> index e8d116a..e45e0b7 100644
> --- a/lib/puppet/provider/service/daemontools.rb
> +++ b/lib/puppet/provider/service/daemontools.rb
> @@ -26,11 +26,16 @@
> Puppet::Type.type(:service).provide :daemontools, :parent => :base do
>
> This provider supports out of the box:
>
> - * start/stop (mapped to enable/disable)
> + * start/stop
> * enable/disable
> * restart
> * status
>
> + If a service has ensure => \"running\", it will link /path/to/
> daemon to
> + /path/to/service, which will automatically enable the service.
> +
> + If a service has ensure => \"stopped\", it will only down the
> service, not
> + remove the /path/to/service link.
>
> """
>
> @@ -113,12 +118,9 @@
> Puppet::Type.type(:service).provide :daemontools, :parent => :base do
> [ command(:svc), "-t", self.service]
> 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
> - # start is then automatic
> + def startcmd
> + self.enable if ! FileTest.symlink?(self.service)
> + [command(:svc), "-u", self.service ]
> end
>
> def status
> @@ -131,33 +133,28 @@
> Puppet::Type.type(:service).provide :daemontools, :parent => :base do
> return :stopped
> end
>
> - # unfortunately it is not possible
> - # to stop without disabling the service
> - def stop
> - self.disable
> + def stopcmd
> + [ command(:svc), "-d", self.service ]
> end
>
> - # disable by stopping the service
> - # and removing the symlink so that svscan
> - # doesn't restart our service behind our back
> + # Stop and remove the service symlink so the service is not
> restarted after
> + # reboot.
> def disable
> - # should stop the service
> - # stop the log subservice if any
> - log = File.join(self.service, "log")
> - texecute("stop log", [ command(:svc) , '-dx', log] ) if
> FileTest.directory?(log)
> -
> - # stop the main resource
> - texecute("stop", [command(:svc), '-dx', self.service] )
> -
> - # unlink the daemon symlink to disable it
> + [ command(:svc), "-d", self.service ]
You'll want to just call 'stop()' here.
The service stuff is a bit confusing; it was some of the first code
written and hasn't been significantly remodeled since then, and it
could reuse it.
Basically, the base class has methods for the different verbs (start,
stop, enable, disable), and each of them looks for a '<verb>cmd'
method. If that method is defined, then its result is executed, and
if not, then the default method is used.
The reason for this was so you don't have to do all of the command
execution and escaping, but there are better ways.
So your class has a functional 'stop' method, which you should use.
>
> File.unlink(self.service) if FileTest.symlink?(self.service)
> end
>
> def enabled?
> - FileTest.symlink?(self.service)
> + case self.status
> + when :running:
> + return :true
> + else
> + return :false
> + end
> end
Aren't you differentiating between enable/disable and start/stop in
this patch? In that case, shouldn't this be testing for the symlink
if it's enabled, and whether it's running to see, um, if it's running?
>
> def enable
> + Puppet.notice "Enabling %s: linking %s -> %s" %
> [ self.service, self.daemon, self.service ]
If you do this, only have it be an info message; there should already
be a transaction log if this method gets called.
>
> File.symlink(self.daemon, self.service) if !
> FileTest.symlink?(self.service)
> end
> end
> From 77f9e316d0e09863c737754028dd9c7bf7116856 Mon Sep 17 00:00:00 2001
> From: Bryan Allen <b...@icgroup.com>
> Date: Wed, 8 Apr 2009 12:34:53 -0400
> Subject: [PATCH 4086/4091] Do not need to stop the log service, svc -
> d does that.
>
> ---
> lib/puppet/provider/service/daemontools.rb | 15 +++------------
> 1 files changed, 3 insertions(+), 12 deletions(-)
>
> diff --git a/lib/puppet/provider/service/daemontools.rb b/lib/puppet/
> provider/service/daemontools.rb
> index e8d116a..73449d1 100644
> --- a/lib/puppet/provider/service/daemontools.rb
> +++ b/lib/puppet/provider/service/daemontools.rb
> @@ -137,19 +137,10 @@
> Puppet::Type.type(:service).provide :daemontools, :parent => :base do
> self.disable
> end
>
> - # disable by stopping the service
> - # and removing the symlink so that svscan
> - # doesn't restart our service behind our back
> + # Stop and remove the service symlink so the service is not
> restarted after
> + # reboot.
> def disable
> - # should stop the service
> - # stop the log subservice if any
> - log = File.join(self.service, "log")
> - texecute("stop log", [ command(:svc) , '-dx', log] ) if
> FileTest.directory?(log)
> -
> - # stop the main resource
> - texecute("stop", [command(:svc), '-dx', self.service] )
> -
> - # unlink the daemon symlink to disable it
> + [command(:svc), "-d", self.service ]
Same comments as above.
>
> File.unlink(self.service) if FileTest.symlink?(self.service)
> end
>
> --
> 1.6.1.1
>
> From 9dea54455d4a352dc14c577035bb0aded3a0b0d6 Mon Sep 17 00:00:00 2001
> From: Bryan Allen <b...@icgroup.com>
> Date: Wed, 8 Apr 2009 12:35:43 -0400
> Subject: [PATCH 4087/4091] We can stop a service without disabling it.
>
> ---
> lib/puppet/provider/service/daemontools.rb | 6 ++----
> 1 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/lib/puppet/provider/service/daemontools.rb b/lib/puppet/
> provider/service/daemontools.rb
> index 73449d1..ca265b4 100644
> --- a/lib/puppet/provider/service/daemontools.rb
> +++ b/lib/puppet/provider/service/daemontools.rb
> @@ -131,10 +131,8 @@
> Puppet::Type.type(:service).provide :daemontools, :parent => :base do
> return :stopped
> end
>
> - # unfortunately it is not possible
> - # to stop without disabling the service
> - def stop
> - self.disable
> + def stopcmd
> + [ command(:svc), "-d", self.service ]
> end
>
> # Stop and remove the service symlink so the service is not
> restarted after
> --
> 1.6.1.1
>
> From ed2429e87dfd7a613260e88c1a4ac812a81b8669 Mon Sep 17 00:00:00 2001
> From: Bryan Allen <b...@icgroup.com>
> Date: Wed, 8 Apr 2009 12:35:56 -0400
> Subject: [PATCH 4088/4091] style
>
> ---
> lib/puppet/provider/service/daemontools.rb | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/lib/puppet/provider/service/daemontools.rb b/lib/puppet/
> provider/service/daemontools.rb
> index ca265b4..9bb0508 100644
> --- a/lib/puppet/provider/service/daemontools.rb
> +++ b/lib/puppet/provider/service/daemontools.rb
> @@ -138,7 +138,7 @@
> Puppet::Type.type(:service).provide :daemontools, :parent => :base do
> # Stop and remove the service symlink so the service is not
> restarted after
> # reboot.
> def disable
> - [command(:svc), "-d", self.service ]
> + [ command(:svc), "-d", self.service ]
> File.unlink(self.service) if FileTest.symlink?(self.service)
> end
>
> --
> 1.6.1.1
>
> From b4f32dd0bc58b9b426f92ea2a1e7d588ea9f4a7b Mon Sep 17 00:00:00 2001
> From: Bryan Allen <b...@icgroup.com>
> Date: Wed, 8 Apr 2009 12:40:27 -0400
> Subject: [PATCH 4089/4091] Start the service if it exists, enable it
> otherwise; note when a service must be enabled
>
> ---
> lib/puppet/provider/service/daemontools.rb | 10 ++++------
> 1 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/lib/puppet/provider/service/daemontools.rb b/lib/puppet/
> provider/service/daemontools.rb
> index 9bb0508..4b4210c 100644
> --- a/lib/puppet/provider/service/daemontools.rb
> +++ b/lib/puppet/provider/service/daemontools.rb
> @@ -113,12 +113,9 @@
> Puppet::Type.type(:service).provide :daemontools, :parent => :base do
> [ command(:svc), "-t", self.service]
> 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
> - # start is then automatic
> + def startcmd
> + self.enable if ! FileTest.symlink?(self.service)
Shouldn't you be able to do:
self.enable unless enabled?
>
> + [command(:svc), "-u", self.service ]
> end
>
> def status
> @@ -147,6 +144,7 @@
> Puppet::Type.type(:service).provide :daemontools, :parent => :base do
> end
>
> def enable
> + Puppet.notice "Enabling %s: linking %s -> %s" %
> [ self.service, self.daemon, self.service ]
Again, this will be seen as log spam.
>
> File.symlink(self.daemon, self.service) if !
> FileTest.symlink?(self.service)
> end
> end
> --
> 1.6.1.1
>
> From c5aa27bfee24c1e2e0f2186a5cbc4854f74fafec Mon Sep 17 00:00:00 2001
> From: Bryan Allen <b...@icgroup.com>
> Date: Wed, 8 Apr 2009 12:44:24 -0400
> Subject: [PATCH 4090/4091] enabled? should be a test, not an action
> to enable the service
>
> ---
> lib/puppet/provider/service/daemontools.rb | 7 ++++++-
> 1 files changed, 6 insertions(+), 1 deletions(-)
>
> diff --git a/lib/puppet/provider/service/daemontools.rb b/lib/puppet/
> provider/service/daemontools.rb
> index 4b4210c..ce5518a 100644
> --- a/lib/puppet/provider/service/daemontools.rb
> +++ b/lib/puppet/provider/service/daemontools.rb
> @@ -140,7 +140,12 @@
> Puppet::Type.type(:service).provide :daemontools, :parent => :base do
> end
>
> def enabled?
> - FileTest.symlink?(self.service)
> + case self.status
> + when :running:
> + return :true
> + else
> + return :false
> + end
> end
Isn't this a duplicate change?
>
> def enable
> --
> 1.6.1.1
>
> From 9f2b7e9c4ed76c0203ccb8ee8943a7f964cdef73 Mon Sep 17 00:00:00 2001
> From: Bryan Allen <b...@icgroup.com>
> Date: Wed, 8 Apr 2009 12:55:04 -0400
> Subject: [PATCH 4091/4091] refs #2146, documentation updated to note
> behavior changes
>
> Signed-off-by: Bryan Allen <b...@icgroup.com>
> ---
> lib/puppet/provider/service/daemontools.rb | 7 ++++++-
> 1 files changed, 6 insertions(+), 1 deletions(-)
>
> diff --git a/lib/puppet/provider/service/daemontools.rb b/lib/puppet/
> provider/service/daemontools.rb
> index ce5518a..e45e0b7 100644
> --- a/lib/puppet/provider/service/daemontools.rb
> +++ b/lib/puppet/provider/service/daemontools.rb
> @@ -26,11 +26,16 @@
> Puppet::Type.type(:service).provide :daemontools, :parent => :base do
>
> This provider supports out of the box:
>
> - * start/stop (mapped to enable/disable)
> + * start/stop
> * enable/disable
> * restart
> * status
>
> + If a service has ensure => \"running\", it will link /path/to/
> daemon to
> + /path/to/service, which will automatically enable the service.
> +
> + If a service has ensure => \"stopped\", it will only down the
> service, not
> + remove the /path/to/service link.
>
> """
>
> --
> 1.6.1.1
>
--
That was just a drill of the emergency y2k system. Had this been a
real emergency, we would've also dumped a bucket of spiders on you and
yelled out "civilization is collapsing!"
---------------------------------------------------------------------
Luke Kanies | http://reductivelabs.com | http://madstop.com
Brice Figureau
> Patches attached (via git format-patch).
>
> 2146-unified.diff is a unified diff of all changes.
>
Please send your patches with git send-email after they've been git
formatted, because my MUA sees them attached and I can't reply to you
with the patch included as Luke did (hey what MUA are you using?).
See:
http://reductivelabs.com/trac/puppet/wiki/DevelopmentLifecycle#id12
I caught a few things, though:
> diff --git a/lib/puppet/provider/service/daemontools.rb b/lib/puppet/provider/service/daemontools.rb
> index e8d116a..e45e0b7 100644
> --- a/lib/puppet/provider/service/daemontools.rb
> +++ b/lib/puppet/provider/service/daemontools.rb
> @@ -26,11 +26,16 @@ Puppet::Type.type(:service).provide :daemontools, :parent => :base do
>
> This provider supports out of the box:
>
> - * start/stop (mapped to enable/disable)
> + * start/stop
> * enable/disable
> * restart
> * status
>
> + If a service has ensure => \"running\", it will link /path/to/daemon to
> + /path/to/service, which will automatically enable the service.
> +
> + If a service has ensure => \"stopped\", it will only down the service, not
> + remove the /path/to/service link.
>
> """
>
> @@ -113,12 +118,9 @@ Puppet::Type.type(:service).provide :daemontools, :parent => :base do
> [ command(:svc), "-t", self.service]
> 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
> - # start is then automatic
> + def startcmd
> + self.enable if ! FileTest.symlink?(self.service)
> + [command(:svc), "-u", self.service ]
> end
Is it necessary to svc -u the service?
Also I don't think it is possible to put something like self.enable in
startcmd , because you don't have the guarantee startcmd will be called
at the moment puppet wants to start the service (hence you might enable
the service at the wrong moment).
And prefer unless to if not, it's easier to read.
Make it:
texecute("stop", [command(:svc), '-d', self.service] )
> File.unlink(self.service) if FileTest.symlink?(self.service)
> end
>
> def enabled?
> - FileTest.symlink?(self.service)
> + case self.status
> + when :running:
> + return :true
> + else
> + return :false
> + end
> end
This looks suspicious: enabled should not check if the service is
running, it should check if the service is enabled, or did I get your
logic wrong?
> def enable
> + Puppet.notice "Enabling %s: linking %s -> %s" % [ self.service, self.daemon, self.service ]
> File.symlink(self.daemon, self.service) if ! FileTest.symlink?(self.service)
> end
> end
--
Brice Figureau
Days of Wonder
http://www.daysofwonder.com
Luke Kanies
transaction system doesn't know that stop() calls stopcmd(), that's
all internal to the provider. So in noop mode, we just don't call
stop() (instead we say "would have stopped" or something similar).
I kinda overlook this type of problem because the service base class
provides all of this, for no good reason, but until it's all
refactored, people are bound to make some coding compromises.
Otherwise, your comments were spot on.
The surest sign that intelligent life exists elsewhere in the universe
is that it has never tried to contact us.
--Calvin and Hobbes (Bill Watterson)
Bryan Allen
+------------------------------------------------------------------------------
| On 2009-04-11 08:57:05, Luke Kanies wrote:
|
| I'd appreciate it if you'd squash all of these commits into one
| commit, using rebase -i.
Will do, after below.
| Also, this patch is missing any tests; could you add some?
I have no idea how to write tests for Ruby/Puppet. I assume the rspec thread
from last week/weekend will be useful?
| > def enabled?
| > - FileTest.symlink?(self.service)
| > + case self.status
| > + when :running:
| > + return :true
| > + else
| > + return :false
| > + end
| > end
|
| Aren't you differentiating between enable/disable and start/stop in
| this patch? In that case, shouldn't this be testing for the symlink
| if it's enabled, and whether it's running to see, um, if it's running?
So, then:
http://github.com/bda/puppet/commit/d74e20d63b5da4edeb69251568037c7ed03b733e
Bryan Allen
| On 2009-04-12 00:14:08, Brice Figureau wrote:
|
| Is it necessary to svc -u the service?
If the service has been manually downed, and your policy says the service
should be running, I assume startcmd is the proper place to up the serivce.
| Also I don't think it is possible to put something like self.enable in
| startcmd , because you don't have the guarantee startcmd will be called
| at the moment puppet wants to start the service (hence you might enable
| the service at the wrong moment).
| And prefer unless to if not, it's easier to read.
| > + [ command(:svc), "-d", self.service ]
|
| Make it:
| texecute("stop", [command(:svc), '-d', self.service] )
Done. I assume start and restart should also use texecute?
| This looks suspicious: enabled should not check if the service is
| running, it should check if the service is enabled, or did I get your
| logic wrong?
You (and Luke) are correct. Fixed in my branch, will post patches after I
figure out rspec.
Luke Kanies
>
> Sorry for the lag. Easter weekend, Catholic girlfriend.
>
> +
> ------------------------------------------------------------------------------
> | On 2009-04-11 08:57:05, Luke Kanies wrote:
> |
> | I'd appreciate it if you'd squash all of these commits into one
> | commit, using rebase -i.
>
> Will do, after below.
>
> | Also, this patch is missing any tests; could you add some?
>
> I have no idea how to write tests for Ruby/Puppet. I assume the
> rspec thread
> from last week/weekend will be useful?
It was mostly about the tools (how to run the tests, etc.).
Rspec is actually pretty damn easy; just look at other tests for
similar classes, and ask if you have problems.
>
> | > def enabled?
> | > - FileTest.symlink?(self.service)
> | > + case self.status
> | > + when :running:
> | > + return :true
> | > + else
> | > + return :false
> | > + end
> | > end
> |
> | Aren't you differentiating between enable/disable and start/stop in
> | this patch? In that case, shouldn't this be testing for the symlink
> | if it's enabled, and whether it's running to see, um, if it's
> running?
>
> So, then:
>
> http://github.com/bda/puppet/commit/d74e20d63b5da4edeb69251568037c7ed03b733e
Because Puppet is stupidly converting true/false into symbols, you're
probably-correctly returning the symbols rather than the booleans.
However, you're enabled? method is returning :true/:false, so it will
always evaluate to true (because :false is true).
--
The reasonable man adapts himself to the world; the unreasonable one
persists in trying to adapt the world to himself. Therefore all
progress depends on the unreasonable man. -- George Bernard Shaw