[SSSD] [PATCH] MONITOR: use sigchld handler for monitoring SSSD services

Jan Zelený jzeleny at redhat.com
Tue Dec 20 07:45:58 UTC 2011


> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20111220/489e7712/attachment.sig>


More information about the sssd-devel mailing list