On Wed, Dec 14, 2011 at 10:06:38PM -0500, Stephen Gallagher wrote:
This patch depends on the SIGCHLD patches currently under review.
This
is the first consumer of those patches, and therefore can be used to
test them.
This changes the behavior of the monitor's child-monitoring code to use
the new sigchld handler. It also replaces some useless loop-checks that
we were doing to verify that the process was still up using waitpid().
It's less wasteful to simply rely on SIGCHLD.
I also revamped the restart logic so that now it will always restart
when the SIGCHLD handler is hit. To that end, I changed it so that the
ping timeout limit would kill the PID (thereby triggering the SIGCHLD
handler to take over).
Why was the logic to limit the number of service restarts removed? I
think it can be useful for cases where a domain is badly misconfigured
so that the monitor does not spin forever trying to restart a service.
Finally, I also added a 60s timer on monitor_kill_service() so that
it
will send a SIGKILL to the process if it hasn't exited gracefully a
minute after receiving SIGTERM. This is probably the most controversial
piece of this patch. In theory, if we SIGKILL during an
ldb_transaction_commit(), we could end up with corrupted data in the LDB
cache. But I think that if we're still writing after 60s, we have bigger
issues to address. I'm certainly willing to entertain counter-arguments
here.
This kinda concerns me. Think about the case where you are offline and
the cache contains your password..sssd might be writing to some very slow
device (NFS?) at the time.
Can't we just leave the process hanging around? With the recent symlink
patches, no requests will get to the DP even it if magically starts working
again because the symlink will already point to the "new" process even if
the "old" is still around.