[PATCH 1/1] Provides #3723. Add ability for execs to have several attempts at a successful execution and fix minor bug with logoutput and returns as an array..

[Nigel Kersten]

* Add 'tries' and 'try_sleep' parameters
* Fix bug where returns is specified as an array and logoutput on
* failure.
* unit tests for both cases above.

Signed-off-by: Nigel Kersten <nig...@google.com>
---
lib/puppet/type/exec.rb | 62 +++++++++++++++++++++++++++++++++++++++++++---
spec/unit/type/exec.rb | 42 +++++++++++++++++++++++++++++--
2 files changed, 97 insertions(+), 7 deletions(-)

diff --git a/lib/puppet/type/exec.rb b/lib/puppet/type/exec.rb
index 47d85da..ff6317a 100755
--- a/lib/puppet/type/exec.rb
+++ b/lib/puppet/type/exec.rb
@@ -111,9 +111,20 @@ module Puppet
dir = self.resource[:cwd] || Dir.pwd

event = :executed_command
+ tries = self.resource[:tries]
+ try_sleep = self.resource[:try_sleep]

begin
- @output, status = @resource.run(self.resource[:command])
+ tries.times do |try|
+ # Only add debug messages for tries > 1 to reduce log spam.
+ debug("Exec try #{try+1}/#{tries}") if tries > 1
+ @output, @status = @resource.run(self.resource[:command])
+ break if self.should.include?(@status.exitstatus.to_s)
+ if try_sleep > 0 and tries > 1
+ debug("Sleeping for #{try_sleep} seconds between tries")
+ sleep try_sleep
+ end
+ end
rescue Timeout::Error
self.fail "Command exceeded timeout" % value.inspect
end
@@ -123,7 +134,7 @@ module Puppet
when :true
log = @resource[:loglevel]
when :on_failure
- if status.exitstatus.to_s != self.should.to_s
+ unless self.should.include?(@status.exitstatus.to_s)
log = @resource[:loglevel]
else
log = :false
@@ -136,9 +147,9 @@ module Puppet
end
end

- unless self.should.include?(status.exitstatus.to_s)
+ unless self.should.include?(@status.exitstatus.to_s)
self.fail("%s returned %s instead of one of [%s]" %
- [self.resource[:command], status.exitstatus, self.should.join(",")])
+ [self.resource[:command], @status.exitstatus, self.should.join(",")])
end

return event
@@ -276,6 +287,49 @@ module Puppet
defaultto 300
end

+ newparam(:tries) do
+ desc "The number of times execution of the command should be tried.
+ Defaults to '1'. This many attempts will be made to execute
+ the command until an acceptable return code is returned.
+ Note that the timeout paramater applies to each try rather than
+ to the complete set of tries."
+
+ munge do |value|
+ if value.is_a?(String)
+ unless value =~ /^[\d]+$/
+ raise ArgumentError, "Tries must be an integer"
+ end
+ value = Integer(value)
+ end
+ if value < 1
+ raise ArgumentError, "Tries must be an integer >= 1"
+ end
+ value
+ end
+
+ defaultto 1
+ end
+
+ newparam(:try_sleep) do
+ desc "The time to sleep in seconds between 'tries'."
+
+ munge do |value|
+ if value.is_a?(String)
+ unless value =~ /^[-\d.]+$/
+ raise ArgumentError, "try_sleep must be a number"
+ end
+ value = Float(value)
+ end
+ if value < 0
+ raise ArgumentError, "try_sleep cannot be a negative number"
+ end
+ value
+ end
+
+ defaultto 0
+ end
+
+
newcheck(:refreshonly) do
desc "The command should only be run as a
refresh mechanism for when a dependent object is changed. It only
diff --git a/spec/unit/type/exec.rb b/spec/unit/type/exec.rb
index 813bdae..17be1d0 100755
--- a/spec/unit/type/exec.rb
+++ b/spec/unit/type/exec.rb
@@ -3,7 +3,7 @@
require File.dirname(__FILE__) + '/../../spec_helper'

module ExecModuleTesting
- def create_resource(command, output, exitstatus, returns = [0])
+ def create_resource(command, output, exitstatus, returns = 0)
@user_name = 'some_user_name'
@group_name = 'some_group_name'
Puppet.features.stubs(:root?).returns(true)
@@ -15,8 +15,8 @@ module ExecModuleTesting
Puppet::Util::SUIDManager.expects(:run_and_capture).with([command], @user_name, @group_name).returns([output, status])
end

- def create_logging_resource(command, output, exitstatus, logoutput, loglevel)
- create_resource(command, output, exitstatus)
+ def create_logging_resource(command, output, exitstatus, logoutput, loglevel, returns = 0)
+ create_resource(command, output, exitstatus, returns)
@execer[:logoutput] = logoutput
@execer[:loglevel] = loglevel
end
@@ -99,6 +99,16 @@ describe Puppet::Type.type(:exec), " when logoutput=>on_failure is set," do
proc { @execer.refresh }.should raise_error(Puppet::Error)
end

+ it "should log the output on failure when returns is specified as an array" do
+ #Puppet::Util::Log.newdestination :console
+ command = "false"
+ output = "output1\noutput2\n"
+ create_logging_resource(command, output, 1, :on_failure, :err, [0, 100])
+ expect_output(output, :err)
+
+ proc { @execer.refresh }.should raise_error(Puppet::Error)
+ end
+
it "shouldn't log the output on success" do
#Puppet::Util::Log.newdestination :console
command = "true"
@@ -107,6 +117,32 @@ describe Puppet::Type.type(:exec), " when logoutput=>on_failure is set," do
@execer.property(:returns).expects(:err).never
@execer.refresh
end
+
+ it "shouldn't log the output on success when non-zero exit status is in a returns array" do
+ #Puppet::Util::Log.newdestination :console
+ command = "true"
+ output = "output1\noutput2\n"
+ create_logging_resource(command, output, 100, :on_failure, :err, [1,100])
+ @execer.property(:returns).expects(:err).never
+ @execer.refresh
+ end
+end
+
+describe Puppet::Type.type(:exec), " when multiple tries are set," do
+ include ExecModuleTesting
+
+ it "should repeat the command attempt 'tries' times on failure and produce an error" do
+ Puppet.features.stubs(:root?).returns(true)
+ command = "false"
+ user = "user"
+ group = "group"
+ tries = 5
+ retry_exec = Puppet::Type.type(:exec).new(:name => command, :path => %w{/usr/bin /bin}, :user => user, :group => group, :returns => 0, :tries => tries, :try_sleep => 0)
+ status = stub "process"
+ status.stubs(:exitstatus).returns(1)
+ Puppet::Util::SUIDManager.expects(:run_and_capture).with([command], user, group).times(tries).returns(["", status])
+ proc { retry_exec.refresh }.should raise_error(Puppet::Error)
+ end
end

describe Puppet::Type.type(:exec) do
--
1.7.0.4

[Luke Kanies]

I'm comfortable with this, although it's obviously a significant
enhancement to exec, enough that it's almost a functionally different
resource type.

+1

> --
> You received this message because you are subscribed to the Google
> Groups "Puppet Developers" group.
> To post to this group, send email to puppe...@googlegroups.com.
> To unsubscribe from this group, send email to puppet-dev+...@googlegroups.com
> .
> For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en
> .
>

--
All power corrupts, but we need the electricity.
-- Unknown
---------------------------------------------------------------------
Luke Kanies -|- http://puppetlabs.com -|- +1(615)594-8199

[Nigel Kersten]

On Mon, Jun 7, 2010 at 10:33 PM, Luke Kanies <lu...@puppetlabs.com> wrote:

I'm comfortable with this, although it's obviously a significant enhancement to exec, enough that it's almost a functionally different resource type.

I've been thinking that as well. I'm going to let this churn around for a bit before committing, as it feels like we could come up with something more expressive and less like a bash loop.

--
nigel

[Nigel Kersten]

On Mon, Jun 7, 2010 at 11:22 PM, Nigel Kersten <nig...@google.com> wrote:
>
>
> On Mon, Jun 7, 2010 at 10:33 PM, Luke Kanies <lu...@puppetlabs.com> wrote:
>>
>> I'm comfortable with this, although it's obviously a significant
>> enhancement to exec, enough that it's almost a functionally different
>> resource type.
>
> I've been thinking that as well. I'm going to let this churn around for a
> bit before committing, as it feels like we could come up with something more
> expressive and less like a bash loop.
>

I'm going to mark this as ready to commit now, as I haven't thought of
anything concrete.

--
nigel