Please review pull request #415: Bug/2.7.x/12188 pid handling warning cleanup opened by (daniel-pittman)
Description:
Previously everyone who invoked the unlock method of our pid file code would
duplicate all the internal logic to issue a warning, but only if we tried and
failed to delete the PID file.
This seems redundant compared to just pushing that down to the code that
actually fails, which this commit does.
Along the way it helps ensure that we don't warn unnecessarily about PID file
deletion, and adds tests to validate that.
Signed-off-by: Daniel Pittman [email protected]
- Opened: Fri Jan 27 01:36:01 UTC 2012
- Based on: puppetlabs:2.7.x (21eb8638d519406716820ba70564f7731dc556e3)
- Requested merge: daniel-pittman:bug/2.7.x/12188-pid-handling-warning-cleanup (3310470edfe327f5d3a5bfbacc5ba7c14cf5d7f2)
Diff follows:
diff --git a/lib/puppet/daemon.rb b/lib/puppet/daemon.rb
index 22630ff..a6945c8 100755
--- a/lib/puppet/daemon.rb
+++ b/lib/puppet/daemon.rb
@@ -74,8 +74,7 @@ def reload
# Remove the pid file for our daemon.
def remove_pidfile
Puppet::Util.synchronize_on(Puppet[:name],Sync::EX) do
- locker = Puppet::Util::Pidlock.new(pidfile)
- locker.unlock or Puppet.err "Could not remove PID file #{pidfile}" if locker.locked?
+ Puppet::Util::Pidlock.new(pidfile).unlock
end
end
diff --git a/lib/puppet/network/server.rb b/lib/puppet/network/server.rb
index e4de07d..e83259c 100644
--- a/lib/puppet/network/server.rb
+++ b/lib/puppet/network/server.rb
@@ -40,8 +40,7 @@ def create_pidfile
# Remove the pid file for our daemon.
def remove_pidfile
Puppet::Util.synchronize_on(Puppet[:name],Sync::EX) do
- locker = Puppet::Util::Pidlock.new(pidfile)
- locker.unlock or Puppet.err "Could not remove PID file #{pidfile}" if locker.locked?
+ Puppet::Util::Pidlock.new(pidfile).unlock
end
end
diff --git a/lib/puppet/util/pidlock.rb b/lib/puppet/util/pidlock.rb
index 9ed8635..9eb1bd2 100644
--- a/lib/puppet/util/pidlock.rb
+++ b/lib/puppet/util/pidlock.rb
@@ -25,7 +25,18 @@ def lock
def unlock(opts = {})
if mine?
- File.unlink(@lockfile)
+ 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
true
else
false
diff --git a/spec/unit/daemon_spec.rb b/spec/unit/daemon_spec.rb
index fc43d93..9eedef7 100755
--- a/spec/unit/daemon_spec.rb
+++ b/spec/unit/daemon_spec.rb
@@ -173,36 +173,20 @@ def without_warnings
end
it "should do nothing if the pidfile is not present" do
- pidfile = mock 'pidfile', :locked? => false
- Puppet::Util::Pidlock.expects(:new).with("/my/file").returns pidfile
+ pidfile = mock 'pidfile', :unlock => false
- Puppet.settings.stubs(:value).with(:name).returns "eh"
- Puppet.settings.stubs(:value).with(:pidfile).returns "/my/file"
+ Puppet[:pidfile] = "/my/file"
+ Puppet::Util::Pidlock.expects(:new).with("/my/file").returns pidfile
- pidfile.expects(:unlock).never
@daemon.remove_pidfile
end
it "should unlock the pidfile using the Pidlock class" do
- pidfile = mock 'pidfile', :locked? => true
- Puppet::Util::Pidlock.expects(:new).with("/my/file").returns pidfile
- pidfile.expects(:unlock).returns true
-
- Puppet.settings.stubs(:value).with(:name).returns "eh"
- Puppet.settings.stubs(:value).with(:pidfile).returns "/my/file"
-
- @daemon.remove_pidfile
- end
+ pidfile = mock 'pidfile', :unlock => true
- it "should warn if it cannot remove the pidfile" do
- pidfile = mock 'pidfile', :locked? => true
+ Puppet[:pidfile] = "/my/file"
Puppet::Util::Pidlock.expects(:new).with("/my/file").returns pidfile
- pidfile.expects(:unlock).returns false
-
- Puppet.settings.stubs(:value).with(:name).returns "eh"
- Puppet.settings.stubs(:value).with(:pidfile).returns "/my/file"
- Puppet.expects :err
@daemon.remove_pidfile
end
end
diff --git a/spec/unit/network/server_spec.rb b/spec/unit/network/server_spec.rb
index b38e82b..80cf18c 100755
--- a/spec/unit/network/server_spec.rb
+++ b/spec/unit/network/server_spec.rb
@@ -196,36 +196,18 @@
end
it "should do nothing if the pidfile is not present" do
- pidfile = mock 'pidfile', :locked? => false
+ pidfile = mock 'pidfile', :unlock => false
Puppet::Util::Pidlock.expects(:new).with("/my/file").returns pidfile
-
- Puppet.settings.stubs(:value).with(:name).returns "eh"
Puppet.settings.stubs(:value).with(:pidfile).returns "/my/file"
- pidfile.expects(:unlock).never
@server.remove_pidfile
end
it "should unlock the pidfile using the Pidlock class" do
- pidfile = mock 'pidfile', :locked? => true
- Puppet::Util::Pidlock.expects(:new).with("/my/file").returns pidfile
- pidfile.expects(:unlock).returns true
-
- Puppet.settings.stubs(:value).with(:name).returns "eh"
- Puppet.settings.stubs(:value).with(:pidfile).returns "/my/file"
-
- @server.remove_pidfile
- end
-
- it "should warn if it cannot remove the pidfile" do
- pidfile = mock 'pidfile', :locked? => true
+ pidfile = mock 'pidfile', :unlock => true
Puppet::Util::Pidlock.expects(:new).with("/my/file").returns pidfile
- pidfile.expects(:unlock).returns false
-
- Puppet.settings.stubs(:value).with(:name).returns "eh"
Puppet.settings.stubs(:value).with(:pidfile).returns "/my/file"
- Puppet.expects :err
@server.remove_pidfile
end
end
diff --git a/spec/unit/util/pidlock_spec.rb b/spec/unit/util/pidlock_spec.rb
old mode 100644
new mode 100755
index 37f3ca4..962745b
--- a/spec/unit/util/pidlock_spec.rb
+++ b/spec/unit/util/pidlock_spec.rb
@@ -88,6 +88,27 @@
@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
@@ -177,6 +198,11 @@
@lock.unlock
@lock.should_not be_mine
end
+
+ it "should not warn" do
+ Puppet.expects(:err).never
+ @lock.unlock
+ end
end
end
-end
\ No newline at end of file
+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.
