12465 : manage the pid file using the service functions. #963
Conversation
| pidfile=${PIDFILE-/var/run/puppet/agent.pid} | ||
| puppetd=${PUPPETD-/usr/bin/puppet} | ||
| pidfile=${PIDFILE-/var/run/puppet.pid} | ||
| puppetd=${PUPPETD-/usr/sbin/puppetd} |
There was a problem hiding this comment.
Is puppetd a typo? We should either stop using the puppetd executable or we should use it as the exclusive way to start the agent. Having two executables is a burden to maintain.
There was a problem hiding this comment.
puppetd worked on 2.7, but isn't even present in 3.x, so this will fail hard.
It should be /usr/bin/puppet instead.
There was a problem hiding this comment.
puppetd is a typo, caused by merge of 2.7 into 3.x, I will change it and update.
| @@ -41,11 +40,9 @@ fi | |||
|
|
|||
| start() { | |||
| echo -n $"Starting puppet agent: " | |||
| daemon $daemonopts $puppetd ${PUPPET_OPTS} ${PUPPET_EXTRA_OPTS} | |||
| daemon $daemonopts "nohup $puppetd ${PUPPET_OPTS} ${PUPPET_EXTRA_OPTS} --no-daemonize >/dev/null 2>&1 & echo \$! > $pidfile" | |||
There was a problem hiding this comment.
Why nohup? This is a smell that Puppet isn't behaving properly.
Also, this appears to start a daemon process using the --no-daemonize option, which is surprising.
There was a problem hiding this comment.
We are asking the puppet to not fork and go to the back ground so that the initscript library is able to manage the daemonizing. This is done so that the init script can control the lifetime using a separate pidfile, and hence distinguish it from a command line invocation. This is also the reason for nohup.
There was a problem hiding this comment.
nohup worked sanely in the previous pull against 2.7 (and was there to ensure that if someone has changed some environment defaults it would still work) but no longer seems to now. Same is true with the --no-daemonize flag. Not sure if puppetd had some different defaults, but when using /usr/bin/puppet we also need to add the agent face to the call in 3.x.
So this line should probably be daemon $daemonopts $puppetd ${PUPPET_OPTS} ${PUPPET_EXTRA_OPTS}, as long as ${PUPPET_OPTS} includes agent and --daemonize.
Then RETVAL=$? to capture the success.
Then I would use pgrep -f '$puppetd ${PUPPET_OPTS} ${PUPPET_EXTRA_OPTS}' to get the pid # and write it to the pidfile (as daemon doesn't seem to actually write the pidfile out).
There was a problem hiding this comment.
I added --no-daemonize as separate from PUPPET_OPTS as it is used elsewhere in genconfig. Even though daemon works due to a quirk of bash, which disables SIGHUP, I felt that it was not safe to rely on it see so.q. I also felt that directly capturing the pid file after execution was less brittle than relying on pgrep (since pgrep relies on a pattern to match the pid).
|
Sorry @jeffmccune... you started commenting at the exact same time I merged this. It sounds like you have some valid concerns, so I can make sure these changes get in. |
No description provided.