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).
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.
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.
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.
On Mon, 2011-12-19 at 11:36 -0500, Stephen Gallagher wrote:
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.
New patch attached re-adds support for service startup limitation. I tested it by setting 'command = /bin/false' in the [pam] section of sssd.conf.
On Mon, Dec 19, 2011 at 03:22:31PM -0500, Stephen Gallagher wrote:
On Mon, 2011-12-19 at 11:36 -0500, Stephen Gallagher wrote:
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.
New patch attached re-adds support for service startup limitation. I tested it by setting 'command = /bin/false' in the [pam] section of sssd.conf.
Ack
On Tue, 2011-12-20 at 11:05 +0100, Jakub Hrozek wrote:
On Mon, Dec 19, 2011 at 03:22:31PM -0500, Stephen Gallagher wrote:
On Mon, 2011-12-19 at 11:36 -0500, Stephen Gallagher wrote:
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.
New patch attached re-adds support for service startup limitation. I tested it by setting 'command = /bin/false' in the [pam] section of sssd.conf.
Ack
Pushed to master and opened https://fedorahosted.org/sssd/ticket/1119 to track making the SIGKILL timer configurable.
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
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Jan Zelený -
Stephen Gallagher