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.
I agree with you on this. However I'd prefer the timeout to be configurable as
you mentioned.
Jan