Bug #12188

agent when run --no-daemonize tries to remove the pid of a running daemon

Added by R.I. Pienaar 4 months ago. Updated 3 months ago.

Status:Closed Start date:01/26/2012
Priority:Urgent Due date:
Assignee:Daniel Pittman % Done:

0%
Category:plumbing
Target version:2.7.11
Affected Puppet version: Branch:https://github.com/puppetlabs/puppet/pull/421
Keywords:
Votes: 0

Description

The fix in #5246 got the agent remove its pid after running. It’s unfortunately too big a hammer as when the agent is also running daemonized the pid removal will fail and log an err level message.

Before removing the pid file we should figure out if the pid file belongs to the current process and only then remove it.

This is confirmed on 2.7.10 but might also be the case in 2.6 master

Related issues

related to Puppet - Bug #5114: puppetd should not try to remove lock file in test mode Accepted 10/27/2010
duplicated by Puppet - Bug #12183: puppet agent --test --verbose --no-daemonize fails after ... Duplicate 01/26/2012

History

#1
Updated by R.I. Pienaar 4 months ago

  • Description updated (diff)

#2
Updated by Brice Figureau 4 months ago

Does the following (proudly untested) works:

diff --git a/lib/puppet/daemon.rb b/lib/puppet/daemon.rb
index 22630ff..a06780a 100755
--- a/lib/puppet/daemon.rb
+++ b/lib/puppet/daemon.rb
@@ -75,7 +75,7 @@ class Puppet::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?
+      locker.unlock or Puppet.err "Could not remove PID file #{pidfile}" if locker.locked? and locker.mine?
     end
   end

It’s possible this patch is the root cause of the issue: b434e3b9c31de63365e1a0f53d152be7e6e988ec which was merged as part of #3757.

#3
Updated by Alexander Swen 4 months ago

also see #12183

#4
Updated by James Turnbull 4 months ago

  • Category set to plumbing
  • Status changed from Unreviewed to Accepted
  • Assignee set to Daniel Pittman
  • Priority changed from Normal to Urgent

#5
Updated by Brice Figureau 4 months ago

Alexander Swen wrote:

also see #12183

Yes, this is a dup. Alexander confirmed that my above patch worked for him.

Also, this was introduced in: https://github.com/masterzen/puppet/commit/b434e3b9c31de63365e1a0f53d152be7e6e988ec

I really fail to see how the problem was introduced…

Note: I don’t really think it’s an urgent issue. This is just an harmless error message, it doesn’t break anything.

#6
Updated by James Turnbull 4 months ago

Brice – I think the same problem is in #12881 – I think it might be broader than just the error message.

#7
Updated by Brice Figureau 4 months ago

Brice Figureau wrote:

Alexander Swen wrote:

also see #12183

Yes, this is a dup. Alexander confirmed that my above patch worked for him.

Also, this was introduced in: https://github.com/masterzen/puppet/commit/b434e3b9c31de63365e1a0f53d152be7e6e988ec

I really fail to see how the problem was introduced…

Note: I don’t really think it’s an urgent issue. This is just an harmless error message, it doesn’t break anything.

In fact this was introduced in e0e31d571f38a3f0436e43ea6662f6d6c1e525c5 (from #5246). This patch added a daemon.stop call for no-daemonize which in turn tries to remove the pid and since it can’t (since it’s not ours), it errors. The fix I proposed is I believe the correct one.

#8
Updated by Daniel Pittman 4 months ago

  • Branch set to https://github.com/puppetlabs/puppet/pull/415

The proposed fixes are correct, but they hit the symptoms rather than the root cause of the problem. The code smell that we had to duplicate all the logic inside the unlock method outside it was what really tipped that off.

https://github.com/puppetlabs/puppet/pull/415 fixes the root cause, by pushing down the warning into the unlock method – now, if something goes wrong removing the PID file we warn, otherwise carry on.

This allows removing the warning logic from all the callers.

#9
Updated by Daniel Pittman 4 months ago

I should have said: testing and validation of the change in that code would be awesome, from other people who have reproduced this.

#10
Updated by Jeff McCune 4 months ago

  • Status changed from Accepted to Merged - Pending Release
  • Target version set to 2.7.11

#11
Updated by Daniel Pittman 4 months ago

  • Status changed from Merged - Pending Release to In Topic Branch Pending Review
  • Branch changed from https://github.com/puppetlabs/puppet/pull/415 to https://github.com/puppetlabs/puppet/pull/421

https://github.com/puppetlabs/puppet/pull/421 fixes a test break on Win32, where the absolute path wasn’t qualified with the drive letter.

#12
Updated by Daniel Pittman 4 months ago

  • Status changed from In Topic Branch Pending Review to Merged - Pending Release

#13
Updated by Matthaus Litteken 3 months ago

  • Status changed from Merged - Pending Release to Closed

released in Puppet 2.7.11

Also available in: Atom PDF