Please review pull request #550: Bug/2.7.x/12844 agent enable doesnt remove old lockfile opened by (cprice-puppet)
Description:
This pull request handles the initial issues around lockfiles (ticket #12844). I'm still planning to add another commit to deal with ticket #11057, so it's probably best not to merge this yet. I just wanted to get it up here so that folks could get a head start on reviewing, and so that if the clock runs out for the 2.7.12rc tag this stuff will be available.
It will probably be easier to review this request by commits rather than as a whole.
The first commit backs out the initial backward-compatibility patch for #12844, which did not address mcollective compatibility concerns.
The second commit backs out the original merge that changed the agent lockfile behavior, which is what caused the mcollective compatibility concerns. Reverting this affects tickets #3757, #11057, and #4836.
The third commit provides backward compatibility between 2.7.12+ and 2.7.10/2.7.11 with regards to the agent lockfiles.
Planned forthcoming fourth commit will address ticket #11057.
- Opened: Fri Mar 02 20:11:18 UTC 2012
- Based on: puppetlabs:master (372c3982888d9048be0a08921e39c81dab3a5dec)
- Requested merge: cprice-puppet:bug/2.7.x/12844-agent-enable-doesnt-remove-old-lockfile (30855bddaaf809e06aa3b80013d1946aed2900af)
Diff follows:
diff --git a/lib/puppet/agent.rb b/lib/puppet/agent.rb
index a16b756..6c998a4 100644
--- a/lib/puppet/agent.rb
+++ b/lib/puppet/agent.rb
@@ -8,9 +8,6 @@ class Puppet::Agent
require 'puppet/agent/locker'
include Puppet::Agent::Locker
- require 'puppet/agent/disabler'
- include Puppet::Agent::Disabler
-
attr_reader :client_class, :client, :splayed
# Just so we can specify that we are "the" instance.
@@ -34,10 +31,6 @@ def run(*args)
Puppet.notice "Run of #{client_class} already in progress; skipping"
return
end
- if disabled?
- Puppet.notice "Skipping run of #{client_class}; administratively disabled: #{disable_message}"
- return
- end
result = nil
block_run = Puppet::Application.controlled_run do
splay
diff --git a/lib/puppet/agent/disabler.rb b/lib/puppet/agent/disabler.rb
deleted file mode 100644
index 34ab94b..0000000
--- a/lib/puppet/agent/disabler.rb
+++ /dev/null
@@ -1,27 +0,0 @@
-require 'puppet/util/anonymous_filelock'
-
-module Puppet::Agent::Disabler
- # Let the daemon run again, freely in the filesystem.
- def enable
- disable_lockfile.unlock
- end
-
- # Stop the daemon from making any catalog runs.
- def disable(msg='')
- disable_lockfile.lock(msg)
- end
-
- def disable_lockfile
- @disable_lockfile ||= Puppet::Util::AnonymousFilelock.new(lockfile_path+".disabled")
-
- @disable_lockfile
- end
-
- def disabled?
- disable_lockfile.locked?
- end
-
- def disable_message
- disable_lockfile.message
- end
-end
diff --git a/lib/puppet/agent/locker.rb b/lib/puppet/agent/locker.rb
index d41b125..98f5b38 100644
--- a/lib/puppet/agent/locker.rb
+++ b/lib/puppet/agent/locker.rb
@@ -3,6 +3,16 @@
# Break out the code related to locking the agent. This module is just
# included into the agent, but having it here makes it easier to test.
module Puppet::Agent::Locker
+ # Let the daemon run again, freely in the filesystem.
+ def enable
+ lockfile.unlock(:anonymous => true)
+ end
+
+ # Stop the daemon from making any catalog runs.
+ def disable
+ lockfile.lock(:anonymous => true)
+ end
+
# Yield if we get a lock, else do nothing. Return
# true/false depending on whether we get the lock.
def lock
diff --git a/lib/puppet/application/agent.rb b/lib/puppet/application/agent.rb
index 001342e..eab02d0 100644
--- a/lib/puppet/application/agent.rb
+++ b/lib/puppet/application/agent.rb
@@ -40,12 +40,7 @@ def preinit
end
option("--centrallogging")
-
- option("--disable [MESSAGE]") do |message|
- options[:disable] = true
- options[:disable_message] = message
- end
-
+ option("--disable")
option("--enable")
option("--debug","-d")
option("--fqdn FQDN","-f")
@@ -107,7 +102,7 @@ def help
USAGE
-----
puppet agent [--certname <name>] [-D|--daemonize|--no-daemonize]
- [-d|--debug] [--detailed-exitcodes] [--digest <digest>] [--disable [message]] [--enable]
+ [-d|--debug] [--detailed-exitcodes] [--digest <digest>] [--disable] [--enable]
[--fingerprint] [-h|--help] [-l|--logdest syslog|<file>|console]
[--no-client] [--noop] [-o|--onetime] [--serve <handler>] [-t|--test]
[-v|--verbose] [-V|--version] [-w|--waitforcert <seconds>]
@@ -211,9 +206,6 @@ def help
not want the central configuration to override the local state until
everything is tested and committed.
- Disable can also take an optional message that will be reported by the
- 'puppet agent' at the next disabled run.
-
'puppet agent' uses the same lock file while it is running, so no more
than one 'puppet agent' process is working at a time.
@@ -381,7 +373,7 @@ def enable_disable_client(agent)
if options[:enable]
agent.enable
elsif options[:disable]
- agent.disable(options[:disable_message] || 'reason not specified')
+ agent.disable
end
exit(0)
end
diff --git a/lib/puppet/util/anonymous_filelock.rb b/lib/puppet/util/anonymous_filelock.rb
deleted file mode 100644
index ff09c5d..0000000
--- a/lib/puppet/util/anonymous_filelock.rb
+++ /dev/null
@@ -1,36 +0,0 @@
-
-class Puppet::Util::AnonymousFilelock
- attr_reader :lockfile
-
- def initialize(lockfile)
- @lockfile = lockfile
- end
-
- def anonymous?
- true
- end
-
- def lock(msg = '')
- return false if locked?
-
- File.open(@lockfile, 'w') { |fd| fd.print(msg) }
- true
- end
-
- def unlock
- if locked?
- File.unlink(@lockfile)
- true
- else
- false
- end
- end
-
- def locked?
- File.exists? @lockfile
- end
-
- def message
- return File.read(@lockfile) if locked?
- end
-end
\ No newline at end of file
diff --git a/lib/puppet/util/pidlock.rb b/lib/puppet/util/pidlock.rb
index 9eb1bd2..a0fe99e 100644
--- a/lib/puppet/util/pidlock.rb
+++ b/lib/puppet/util/pidlock.rb
@@ -1,10 +1,20 @@
require 'fileutils'
-require 'puppet/util/anonymous_filelock'
-class Puppet::Util::Pidlock < Puppet::Util::AnonymousFilelock
+class Puppet::Util::Pidlock
+ attr_reader :lockfile
+
+ def initialize(lockfile)
+ @lockfile = lockfile
+ end
def locked?
clear_if_stale
+ return true if File.exists? @lockfile
+
+ # HACK! There was a temporary change to the lockfile behavior introduced in 2.7.10 and 2.7.11, and reverted
+ # in 2.7.12. We need to pull some chicanery to be backwards-compatible with those versions. For more info,
+ # see the comments on the method... and this hack should be removed for the 3.x series.
+ handle_2_7_10_disabled_lockfile
File.exists? @lockfile
end
@@ -13,36 +23,39 @@ def mine?
end
def anonymous?
- false
+ return false unless File.exists?(@lockfile)
+ File.read(@lockfile) == ""
end
- def lock
- return mine? if locked?
+ def lock(opts = {})
+ opts = {:anonymous => false}.merge(opts)
- File.open(@lockfile, "w") { |fd| fd.write(Process.pid) }
- true
+ if locked?
+ mine?
+ else
+ if opts[:anonymous]
+ File.open(@lockfile, 'w') { |fd| true }
+ else
+ File.open(@lockfile, "w") { |fd| fd.write(Process.pid) }
+ end
+ true
+ end
end
def unlock(opts = {})
- if mine?
- begin
- File.unlink(@lockfile)
- rescue Errno::ENOENT
- # Someone deleted it for us ...and so we do nothing. No point whining
- # about a problem that the user can't actually do anything about.
- rescue SystemCallError => e
- # This one is a real failure though. No idea what went wrong, but it
- # is most likely "read only file(system)" or wrong permissions or
- # something like that.
- Puppet.err "Could not remove PID file #{@lockfile}: #{e}"
- puts e.backtrace if Puppet[:trace]
- end
+ return false unless locked?
+
+ opts = {:anonymous => false}.merge(opts)
+
+ if mine? or (opts[:anonymous] and anonymous?)
+ File.unlink(@lockfile)
true
else
false
end
end
+ private
def lock_pid
if File.exists? @lockfile
File.read(@lockfile).to_i
@@ -51,7 +64,6 @@ def lock_pid
end
end
- private
def clear_if_stale
return if lock_pid.nil?
@@ -65,4 +77,41 @@ def clear_if_stale
File.unlink(@lockfile)
end
end
+
+
+ ######################################################################################
+ # Backwards compatibility hack
+ ######################################################################################
+ # A change to lockfile behavior was introduced in 2.7.10 and 2.7.11; basically,
+ # instead of using a single lockfile to indicate both administrative disabling of
+ # the agent *and* the case where an agent run is already in progress, we started using
+ # two separate lockfiles: the 'normal' one for the "run in progress" case, and a
+ # separate one with a ".disabled" extension to indicate administrative disabling.
+ #
+ # This was determined to cause incompatibilities with mcollective, so the behavior
+ # was reverted for 2.7.12. Unfortunately this leaves the possibility that someone
+ # may have run "agent --disable" to administratively disable a 2.7.10 or 2.7.11
+ # agent, and then upgraded to a newer version. This method exists only to
+ # provide backwards compatibility. Basically, it just recognizes the 2.7.10/2.7.11
+ # ".disabled" lock file, warns, and cleans it up.
+ #
+ # This should be removed for the 3.x series.
+ #
+ # For more information, please see tickets #12844, #3757, #4836, and #11057
+ #
+ # -- cprice 2012-03-01
+ #
+ def handle_2_7_10_disabled_lockfile
+ disabled_lockfile_path = @lockfile + ".disabled"
+ if (File.exists?(disabled_lockfile_path))
+ Puppet.warning("Found special lockfile '#{disabled_lockfile_path}'; this file was " +
+ "generated by a call to 'puppet agent --disable' in puppet 2.7.10 or 2.7.11. " +
+ "The expected lockfile path is '#{@lockfile}'; renaming the lock file.")
+ File.rename(disabled_lockfile_path, @lockfile)
+ end
+ end
+ private :handle_2_7_10_disabled_lockfile
+ ######################################################################################
+ # End backwards compatibility hack
+ ######################################################################################
end
diff --git a/spec/unit/agent/disabler_spec.rb b/spec/unit/agent/disabler_spec.rb
deleted file mode 100644
index 5e43cf9..0000000
--- a/spec/unit/agent/disabler_spec.rb
+++ /dev/null
@@ -1,60 +0,0 @@
-#!/usr/bin/env rspec
-require 'spec_helper'
-require 'puppet/agent'
-require 'puppet/agent/locker'
-
-class LockerTester
- include Puppet::Agent::Disabler
-end
-
-describe Puppet::Agent::Disabler do
- before do
- @locker = LockerTester.new
- @locker.stubs(:lockfile_path).returns "/my/lock"
- end
-
- it "should use an AnonymousFilelock instance as its disable_lockfile" do
- @locker.disable_lockfile.should be_instance_of(Puppet::Util::AnonymousFilelock)
- end
-
- it "should use 'lockfile_path' to determine its disable_lockfile path" do
- @locker.expects(:lockfile_path).returns "/my/lock"
- lock = Puppet::Util::AnonymousFilelock.new("/my/lock")
- Puppet::Util::AnonymousFilelock.expects(:new).with("/my/lock.disabled").returns lock
-
- @locker.disable_lockfile
- end
-
- it "should reuse the same lock file each time" do
- @locker.disable_lockfile.should equal(@locker.disable_lockfile)
- end
-
- it "should lock the anonymous lock when disabled" do
- @locker.disable_lockfile.expects(:lock)
-
- @locker.disable
- end
-
- it "should disable with a message" do
- @locker.disable_lockfile.expects(:lock).with("disabled because")
-
- @locker.disable("disabled because")
- end
-
- it "should unlock the anonymous lock when enabled" do
- @locker.disable_lockfile.expects(:unlock)
-
- @locker.enable
- end
-
- it "should check the lock if it is disabled" do
- @locker.disable_lockfile.expects(:locked?)
-
- @locker.disabled?
- end
-
- it "should report the disable message when disabled" do
- @locker.disable_lockfile.expects(:message).returns("message")
- @locker.disable_message.should == "message"
- end
-end
diff --git a/spec/unit/agent/locker_spec.rb b/spec/unit/agent/locker_spec.rb
index 9b530c0..341859e 100755
--- a/spec/unit/agent/locker_spec.rb
+++ b/spec/unit/agent/locker_spec.rb
@@ -29,6 +29,18 @@ class LockerTester
@locker.lockfile.should equal(@locker.lockfile)
end
+ it "should use the lock file to anonymously lock the process when disabled" do
+ @locker.lockfile.expects(:lock).with(:anonymous => true)
+
+ @locker.disable
+ end
+
+ it "should use the lock file to anonymously unlock the process when enabled" do
+ @locker.lockfile.expects(:unlock).with(:anonymous => true)
+
+ @locker.enable
+ end
+
it "should have a method that yields when a lock is attained" do
@locker.lockfile.expects(:lock).returns true
diff --git a/spec/unit/agent_backward_compatibility_spec.rb b/spec/unit/agent_backward_compatibility_spec.rb
new file mode 100644
index 0000000..1cd587f
--- /dev/null
+++ b/spec/unit/agent_backward_compatibility_spec.rb
@@ -0,0 +1,154 @@
+#!/usr/bin/env rspec
+require 'spec_helper'
+require 'puppet/agent'
+
+
+############################################################################
+# NOTE #
+############################################################################
+# #
+# This entire spec is only here for backwards compatibility from 2.7.12+ #
+# with 2.7.10 and 2.7.11. The entire file should be able to be removed #
+# for the 3.x series. #
+# #
+# For more info, see the comments on the #handle_2_7_10_disabled_lockfile #
+# method in pidlock.rb #
+# #
+# --cprice 2012-03-01 #
+############################################################################
+
+class AgentTestClient
+ def run
+ # no-op
+ end
+ def stop
+ # no-op
+ end
+end
+
+describe Puppet::Agent do
+ include PuppetSpec::Files
+
+ let(:agent) { Puppet::Agent.new(AgentTestClient) }
+
+ describe "in order to be backwards-compatibility with versions 2.7.10 and 2.7.11" do
+
+ describe "when the 2.7.10/2.7.11 'disabled' lockfile exists" do
+
+ # the "normal" lockfile
+ let(:lockfile_path) { tmpfile("agent_spec_lockfile") }
+
+ # the 2.7.10/2.7.11 "disabled" lockfile
+ # (can't use PuppetSpec::Files.tmpfile here because we need the ".disabled" file to have *exactly* the same
+ # path/name as the original file, plus the ".disabled" suffix.)
+ let(:disabled_lockfile_path) { lockfile_path + ".disabled" }
+
+ # some regexes to match log messages
+ let(:warning_regex) { /^Found special lockfile '#{disabled_lockfile_path}'.*renaming/ }
+ let(:run_in_progress_regex) { /^Run of .* already in progress; skipping/ }
+
+ before(:each) do
+ # create the 2.7.10 "disable" lockfile.
+ FileUtils.touch(disabled_lockfile_path)
+
+ # stub in our temp lockfile path.
+ AgentTestClient.expects(:lockfile_path).returns lockfile_path
+ end
+
+ after(:each) do
+ # manually clean up the files that we didn't create via PuppetSpec::Files.tmpfile
+ begin
+ File.unlink(disabled_lockfile_path)
+ rescue Errno::ENOENT
+ # some of the tests expect for the agent code to take care of deleting this file,
+ # so it may (validly) not exist.
+ end
+ end
+
+ describe "when the 'regular' lockfile also exists" do
+ # the logic here is that if a 'regular' lockfile already exists, then there is some state that the
+ # current version of puppet is responsible for dealing with. All of the tests in this block are
+ # simply here to make sure that our backwards-compatibility hack does *not* interfere with this.
+ #
+ # Even if the ".disabled" lockfile exists--it can be dealt with at another time, when puppet is
+ # in *exactly* the state that we want it to be in (mostly meaning that the 'regular' lockfile
+ # does not exist.)
+
+ before(:each) do
+ # create the "regular" lockfile
+ FileUtils.touch(lockfile_path)
+ end
+
+ it "should be recognized as 'running'" do
+ agent.should be_running
+ end
+
+ it "should not try to start a new agent run" do
+ AgentTestClient.expects(:new).never
+ Puppet.expects(:notice).with { |msg| msg =~ run_in_progress_regex }
+
+ agent.run
+ end
+
+ it "should not delete the 2.7.10/2.7.11 lockfile" do
+ agent.run
+
+ File.exists?(disabled_lockfile_path).should == true
+ end
+
+ it "should not print the warning message" do
+ Puppet.expects(:warning).with {|msg| msg =~ warning_regex } .never
+
+ agent.run
+ end
+ end
+
+ describe "when the 'regular' lockfile does not exist" do
+ # this block of tests is for actually testing the backwards compatibility hack. This
+ # is where we're in a clean state and we know it's safe(r) to muck with the lockfile
+ # situation.
+
+ it "should recognize that the agent is disabled" do
+ # TODO change this to use "be_disabled" when that is re-introduced.
+ agent.should be_running
+ end
+
+ describe "when an agent run is requested" do
+ it "should not try to start a new agent run" do
+ AgentTestClient.expects(:new).never
+ Puppet.expects(:notice).with { |msg| msg =~ run_in_progress_regex }
+
+ agent.run
+ end
+
+ it "should warn, remove the 2.7.10/2.7.11 lockfile, and create the 'normal' lockfile" do
+ Puppet.expects(:warning).with { |msg| msg =~ warning_regex }
+
+ agent.run
+
+ File.exists?(disabled_lockfile_path).should == false
+ File.exists?(lockfile_path).should == true
+ end
+ end
+
+ describe "when running --enable" do
+ it "should recognize that the agent is disabled" do
+ # TODO change this to use "be_disabled" when that is re-introduced.
+ agent.should be_running
+ end
+
+ it "should warn and clean up the 2.7.10/2.7.11 lockfile" do
+ Puppet.expects(:warning).with { |msg| msg =~ warning_regex }
+
+ agent.enable
+
+ File.exists?(disabled_lockfile_path).should == false
+ File.exists?(lockfile_path).should == false
+ end
+ end
+ end
+ end
+ end
+
+
+end
diff --git a/spec/unit/agent_spec.rb b/spec/unit/agent_spec.rb
index f5342f8..8d837e1 100755
--- a/spec/unit/agent_spec.rb
+++ b/spec/unit/agent_spec.rb
@@ -24,7 +24,6 @@ def without_warnings
# So we don't actually try to hit the filesystem.
@agent.stubs(:lock).yields
- @agent.stubs(:disabled?).returns(false)
# make Puppet::Application safe for stubbing; restore in an :after block; silence warnings for this.
without_warnings { Puppet::Application = Class.new(Puppet::Application) }
@@ -77,7 +76,6 @@ def controlled_run(&block)
describe "when being run" do
before do
- AgentTestClient.stubs(:lockfile_path).returns "/my/lock"
@agent.stubs(:running?).returns false
end
@@ -94,12 +92,6 @@ def controlled_run(&block)
@agent.run
end
- it "should do nothing if disabled" do
- @agent.expects(:disabled?).returns(true)
- AgentTestClient.expects(:new).never
- @agent.run
- end
-
it "(#11057) should notify the user about why a run is skipped" do
Puppet::Application.stubs(:controlled_run).returns(false)
Puppet::Application.stubs(:run_status).returns('MOCK_RUN_STATUS')
diff --git a/spec/unit/application/agent_spec.rb b/spec/unit/application/agent_spec.rb
index 44162bf..c89f538 100755
--- a/spec/unit/application/agent_spec.rb
+++ b/spec/unit/application/agent_spec.rb
@@ -91,7 +91,7 @@
@puppetd.command_line.stubs(:args).returns([])
end
- [:centrallogging, :enable, :debug, :fqdn, :test, :verbose, :digest].each do |option|
+ [:centrallogging, :disable, :enable, :debug, :fqdn, :test, :verbose, :digest].each do |option|
it "should declare handle_#{option} method" do
@puppetd.should respond_to("handle_#{option}".to_sym)
end
@@ -102,24 +102,6 @@
end
end
- describe "when handling --disable" do
- it "should declare handle_disable method" do
- @puppetd.should respond_to(:handle_disable)
- end
-
- it "should set disable to true" do
- @puppetd.options.stubs(:[]=)
- @puppetd.options.expects(:[]=).with(:disable, true)
- @puppetd.handle_disable('')
- end
-
- it "should store disable message" do
- @puppetd.options.stubs(:[]=)
- @puppetd.options.expects(:[]=).with(:disable_message, "message")
- @puppetd.handle_disable('message')
- end
- end
-
it "should set an existing handler on server" do
Puppet::Network::Handler.stubs(:handler).with("handler").returns(true)
@@ -367,20 +349,6 @@
end
end
- it "should pass the disable message when disabling" do
- @puppetd.options.stubs(:[]).with(:disable).returns(true)
- @puppetd.options.stubs(:[]).with(:disable_message).returns("message")
- @agent.expects(:disable).with("message")
- expect { @puppetd.enable_disable_client(@agent) }.to exit_with 0
- end
-
- it "should pass the default disable message when disabling without a message" do
- @puppetd.options.stubs(:[]).with(:disable).returns(true)
- @puppetd.options.stubs(:[]).with(:disable_message).returns(nil)
- @agent.expects(:disable).with("reason not specified")
- expect { @puppetd.enable_disable_client(@agent) }.to exit_with 0
- end
-
it "should finally exit" do
expect { @puppetd.enable_disable_client(@agent) }.to exit_with 0
end
diff --git a/spec/unit/util/anonymous_filelock_spec.rb b/spec/unit/util/anonymous_filelock_spec.rb
deleted file mode 100644
index 784ac0f..0000000
--- a/spec/unit/util/anonymous_filelock_spec.rb
+++ /dev/null
@@ -1,78 +0,0 @@
-#!/usr/bin/env rspec
-require 'spec_helper'
-
-require 'puppet/util/anonymous_filelock'
-
-describe Puppet::Util::AnonymousFilelock do
- require 'puppet_spec/files'
- include PuppetSpec::Files
-
- before(:each) do
- @lockfile = tmpfile("lock")
- @lock = Puppet::Util::AnonymousFilelock.new(@lockfile)
- end
-
- it "should be anonymous" do
- @lock.should be_anonymous
- end
-
- describe "#lock" do
- it "should return false if already locked" do
- @lock.stubs(:locked?).returns(true)
- @lock.lock.should be_false
- end
-
- it "should return true if it successfully locked" do
- @lock.lock.should be_true
- end
-
- it "should create a lock file" do
- @lock.lock
-
- File.should be_exists(@lockfile)
- end
-
- it "should create a lock file containing a message" do
- @lock.lock("message")
-
- File.read(@lockfile).should == "message"
- end
- end
-
- describe "#unlock" do
- it "should return true when unlocking" do
- @lock.lock
- @lock.unlock.should be_true
- end
-
- it "should return false when not locked" do
- @lock.unlock.should be_false
- end
-
- it "should clear the lock file" do
- File.open(@lockfile, 'w') { |fd| fd.print("locked") }
- @lock.unlock
- File.should_not be_exists(@lockfile)
- end
- end
-
- it "should be locked when locked" do
- @lock.lock
- @lock.should be_locked
- end
-
- it "should not be locked when not locked" do
- @lock.should_not be_locked
- end
-
- it "should not be locked when unlocked" do
- @lock.lock
- @lock.unlock
- @lock.should_not be_locked
- end
-
- it "should return the lock message" do
- @lock.lock("lock message")
- @lock.message.should == "lock message"
- end
-end
\ No newline at end of file
diff --git a/spec/unit/util/pidlock_spec.rb b/spec/unit/util/pidlock_spec.rb
deleted file mode 100755
index 962745b..0000000
--- a/spec/unit/util/pidlock_spec.rb
+++ /dev/null
@@ -1,208 +0,0 @@
-#!/usr/bin/env rspec
-require 'spec_helper'
-
-require 'puppet/util/pidlock'
-
-describe Puppet::Util::Pidlock do
- require 'puppet_spec/files'
- include PuppetSpec::Files
-
- before(:each) do
- @lockfile = tmpfile("lock")
- @lock = Puppet::Util::Pidlock.new(@lockfile)
- end
-
- it "should not be anonymous" do
- @lock.should_not be_anonymous
- end
-
- describe "#lock" do
- it "should not be locked at start" do
- @lock.should_not be_locked
- end
-
- it "should not be mine at start" do
- @lock.should_not be_mine
- end
-
- it "should become locked" do
- @lock.lock
- @lock.should be_locked
- end
-
- it "should become mine" do
- @lock.lock
- @lock.should be_mine
- end
-
- it "should be possible to lock multiple times" do
- @lock.lock
- lambda { @lock.lock }.should_not raise_error
- end
-
- it "should return true when locking" do
- @lock.lock.should be_true
- end
-
- it "should return true if locked by me" do
- @lock.lock
- @lock.lock.should be_true
- end
-
- it "should return false if locked by someone else" do
- Process.stubs(:kill)
- File.open(@lockfile, "w") { |fd| fd.print('0') }
-
- @lock.lock.should be_false
- end
-
- it "should create a lock file" do
- @lock.lock
- File.should be_exists(@lockfile)
- end
-
- it "should create a lock file containing our pid" do
- @lock.lock
- File.read(@lockfile).to_i.should == Process.pid.to_i
- end
- end
-
- describe "#unlock" do
- it "should not be locked anymore" do
- @lock.lock
- @lock.unlock
- @lock.should_not be_locked
- end
-
- it "should return false if not locked" do
- @lock.unlock.should be_false
- end
-
- it "should return true if properly unlocked" do
- @lock.lock
- @lock.unlock.should be_true
- end
-
- it "should get rid of the lock file" do
- @lock.lock
- @lock.unlock
- File.should_not be_exists(@lockfile)
- end
-
- it "should not warn if the lockfile was deleted by someone else" do
- @lock.lock
- File.unlink(@lockfile)
-
- Puppet.expects(:err).never # meh
- @lock.unlock
- end
-
- it "should warn if the lockfile can't be deleted" do
- @lock.lock
- File.expects(:unlink).with(@lockfile).raises(Errno::EIO)
- Puppet.expects(:err).with do |argument|
- argument.should =~ /Input\/output error/
- end
- @lock.unlock
-
- # This is necessary because our cleanup code uses File.unlink
- File.unstub(:unlink)
- @lock.unlock
- end
- end
-
- describe "#locked?" do
- it "should return true if locked" do
- @lock.lock
- @lock.should be_locked
- end
- end
-
- describe "with a stale lock" do
- before(:each) do
- Process.stubs(:kill).with(0, 6789)
- Process.stubs(:kill).with(0, 1234).raises(Errno::ESRCH)
- Process.stubs(:pid).returns(6789)
- File.open(@lockfile, 'w') { |fd| fd.write("1234") }
- end
-
- it "should not be locked" do
- @lock.should_not be_locked
- end
-
- describe "#lock" do
- it "should clear stale locks" do
- @lock.locked?
- File.should_not be_exists(@lockfile)
- end
-
- it "should replace with new locks" do
- @lock.lock
- File.should be_exists(@lockfile)
- @lock.lock_pid.should == 6789
- @lock.should be_mine
- @lock.should be_locked
- end
- end
-
- describe "#unlock" do
- it "should not be allowed" do
- @lock.unlock.should be_false
- end
-
- it "should not remove the lock file" do
- @lock.unlock
- File.should be_exists(@lockfile)
- end
- end
- end
-
- describe "with another process lock" do
- before(:each) do
- Process.stubs(:kill).with(0, 6789)
- Process.stubs(:kill).with(0, 1234)
- Process.stubs(:pid).returns(6789)
- File.open(@lockfile, 'w') { |fd| fd.write("1234") }
- end
-
- it "should be locked" do
- @lock.should be_locked
- end
-
- it "should not be mine" do
- @lock.should_not be_mine
- end
-
- describe "#lock" do
- it "should not be possible" do
- @lock.lock.should be_false
- end
-
- it "should not overwrite the lock" do
- @lock.lock
- @lock.should_not be_mine
- end
- end
-
- describe "#unlock" do
- it "should not be possible" do
- @lock.unlock.should be_false
- end
-
- it "should not remove the lock file" do
- @lock.unlock
- File.should be_exists(@lockfile)
- end
-
- it "should still not be our lock" do
- @lock.unlock
- @lock.should_not be_mine
- end
-
- it "should not warn" do
- Puppet.expects(:err).never
- @lock.unlock
- end
- end
- end
-end
diff --git a/test/util/pidlock.rb b/test/util/pidlock.rb
new file mode 100755
index 0000000..beaff10
--- /dev/null
+++ b/test/util/pidlock.rb
@@ -0,0 +1,126 @@
+require File.expand_path(File.dirname(__FILE__) + '/../lib/puppettest')
+
+require 'puppet/util/pidlock'
+require 'fileutils'
+
+# This is *fucked* *up*
+Puppet.debug = false
+
+class TestPuppetUtilPidlock < Test::Unit::TestCase
+ include PuppetTest
+
+ def setup
+ super
+ @workdir = tstdir
+ end
+
+ def teardown
+ super
+ FileUtils.rm_rf(@workdir)
+ end
+
+ def test_00_basic_create
+ l = nil
+ assert_nothing_raised { l = Puppet::Util::Pidlock.new(@workdir + '/nothingmuch') }
+
+ assert_equal Puppet::Util::Pidlock, l.class
+
+ assert_equal @workdir + '/nothingmuch', l.lockfile
+ end
+
+ def test_10_uncontended_lock
+ l = Puppet::Util::Pidlock.new(@workdir + '/test_lock')
+
+ assert !l.locked?
+ assert !l.mine?
+ assert l.lock
+ assert l.locked?
+ assert l.mine?
+ assert !l.anonymous?
+ # It's OK to call lock multiple times
+ assert l.lock
+ assert l.unlock
+ assert !l.locked?
+ assert !l.mine?
+ end
+
+ def test_20_someone_elses_lock
+ childpid = nil
+ l = Puppet::Util::Pidlock.new(@workdir + '/someone_elses_lock')
+
+ # First, we need a PID that's guaranteed to be (a) used, (b) someone
+ # else's, and (c) around for the life of this test.
+ childpid = fork { loop do; sleep 10; end }
+
+ File.open(l.lockfile, 'w') { |fd| fd.write(childpid) }
+
+ assert l.locked?
+ assert !l.mine?
+ assert !l.lock
+ assert l.locked?
+ assert !l.mine?
+ assert !l.unlock
+ assert l.locked?
+ assert !l.mine?
+ ensure
+ Process.kill("KILL", childpid) unless childpid.nil?
+ end
+
+ def test_30_stale_lock
+ # This is a bit hard to guarantee, but we need a PID that is definitely
+ # unused, and will stay so for the the life of this test. Our best
+ # bet is to create a process, get it's PID, let it die, and *then*
+ # lock on it.
+ childpid = fork { exit }
+
+ # Now we can't continue until we're sure that the PID is dead
+ Process.wait(childpid)
+
+ l = Puppet::Util::Pidlock.new(@workdir + '/stale_lock')
+
+ # locked? should clear the lockfile
+ File.open(l.lockfile, 'w') { |fd| fd.write(childpid) }
+ assert File.exists?(l.lockfile)
+ assert !l.locked?
+ assert !File.exists?(l.lockfile)
+
+ # lock should replace the lockfile with our own
+ File.open(l.lockfile, 'w') { |fd| fd.write(childpid) }
+ assert File.exists?(l.lockfile)
+ assert l.lock
+ assert l.locked?
+ assert l.mine?
+
+ # unlock should fail, and should *not* molest the existing lockfile,
+ # despite it being stale
+ File.open(l.lockfile, 'w') { |fd| fd.write(childpid) }
+ assert File.exists?(l.lockfile)
+ assert !l.unlock
+ assert File.exists?(l.lockfile)
+ end
+
+ def test_40_not_locked_at_all
+ l = Puppet::Util::Pidlock.new(@workdir + '/not_locked')
+
+ assert !l.locked?
+ # We can't unlock if we don't hold the lock
+ assert !l.unlock
+ end
+
+ def test_50_anonymous_lock
+ l = Puppet::Util::Pidlock.new(@workdir + '/anonymous_lock')
+
+ assert !l.locked?
+ assert l.lock(:anonymous => true)
+ assert l.locked?
+ assert l.anonymous?
+ assert !l.mine?
+ assert "", File.read(l.lockfile)
+ assert !l.unlock
+ assert l.locked?
+ assert l.anonymous?
+ assert l.unlock(:anonymous => true)
+ assert !File.exists?(l.lockfile)
+ end
+end
+
-- 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.
