On Mon, 2011-12-19 at 17:29 +0100, Jakub Hrozek wrote:
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.
Oops, that was an oversight. I forgot to add that back in. I'll get a
patch for this shortly.
> 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.
LDB doesn't work on NFS. It doesn't fully implement some parts of the
POSIX filesystem spec related to locking, IIRC.
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.
Well, I see a problem here with the possibility of process exhaustion on
very long-running systems.
Also, if processes remain hanging around, it can be difficult to tell
which one is actually active.
My thought was that if it didn't come back in 60s, it probably wasn't
ever going to come back, so sooner or later we were going to have to
SIGKILL it anyway. We can always make the 60s configurable in the future
if you prefer. But not right now, as I'm about to declare string-freeze
so we can try to get translation updates before release.