Fix process increase #432

Merged
merged 6 commits into from Nov 3, 2016

Projects

None yet

2 participants

@pandax381
Contributor

Hi,

This is the proposal to the matter of mentioned in #429.

  • Script process does not respond to SIGTERM (fe9638b)

    Sep 26 16:31:03 lb01 Keepalived_healthcheckers[28302]: Process [28304] didn't respond to SIGTERM
    Sep 26 16:31:03 lb01 Keepalived_healthcheckers[28302]: pid 28304 exited due to signal 9
    Sep 26 16:31:06 lb01 Keepalived_healthcheckers[28302]: Process [28307] didn't respond to SIGTERM
    Sep 26 16:31:06 lb01 Keepalived_healthcheckers[28302]: pid 28307 exited due to signal 9
    Sep 26 16:31:09 lb01 Keepalived_healthcheckers[28302]: Process [28366] didn't respond to SIGTERM
    Sep 26 16:31:09 lb01 Keepalived_healthcheckers[28302]: pid 28366 exited due to signal 9
    

    I think there is a problem in the signal_handler_init().
    In signal_handler_init(), it remember the original disposition of signals.
    It's the first time will function normally. But, the second time it seems to unexpected behavior.
    Because, in the first round all of the signal is set to SIG_IGN. For this reason, dose not be restored in the signal_handler_script(). SIGTERM remains ignored.

    It has been modified to be stored only the first time.

  • When the script is blocked, process will increase (4c471b2)

    I wrote...

    MISC_CHECK is calling system(3) after fork(2). In check_misc.c, it's sending SIGTERM and SIGKILL on timeout. However, the signal is received only in the process that called system(3), the subsequent process is not propagated.

    Fixed to separate the process group. And it was to send a signal to the process group.

  • Signal propagation for the script process groups (f0c5783)

    Since the separation of the process group of script-related process, the signal has become not propagate. So, we propagate important signals. It's SIGTERM/SIGINT and SIGHUP.

Initially, I was thinking only of MISC_CHECK. Then, vrrp_script was also noticed that it is the same.

In this proposal, process was no longer increase.

What do you think about this proposal?
I want to listen to your opinion.

Best regards,
Masaya Yamamoto

@pqarmitage

I think this is a really good idea, and I have made some minor comments where I think there could be some slight improvements, or where there are some edge cases that occurred to me (I think the one about thread_child_handler() modifying the child thread queue while thread_fetch() is manipulating it has probably been around for a long time.

lib/signals.c
@@ -185,6 +185,7 @@ signal_handler_init(void)
int sig;
struct sigaction act, oact;
int n;
+ static int remember = 1;
@pqarmitage
pqarmitage Sep 27, 2016 Collaborator

Rather than making remember a static variable, I think this would be better as a parameter to signal_handle_init(), which is called from keepalived/vrrp/vrrp_daemon.c, keepalived/check/check_daemon.c and keepalived/core/main.c. Only main.c should call signal_handler_init with remember = true, since the vrrp and check children inherit the modified signal disposition that is set by the parent process.

@pandax381
pandax381 Sep 28, 2016 Contributor

I agree with you! (I was trying to suppress to minimal changes)
I'll fix so that it does not use a static variable.

lib/notify.c
@@ -105,6 +105,7 @@ system_call_script(thread_master_t *m, int (*func) (thread_t *), void * arg, lon
/* In case of this is parent process */
if (pid) {
+ setpgid(pid, pid);
@pqarmitage
pqarmitage Sep 27, 2016 Collaborator

This won't work reliably. Depending on the scheduling of the processes, the child process could have run between the fork() and calling setpgid() here, and while running could have forked another child. In that case, the pgid of the newest child will not be change to the new pgid.

The solution is not to call setpgid() here, but immediately after this if (pid) {} block, i.e. the first code that the child process executes, add setpgid(0, 0);

@pandax381
pandax381 Sep 28, 2016 Contributor

Oops... I'm sorry!
It's completely my mistake.

I agree with you. It should be setpgid() is child process.

+
+ p_pgid = getpgid(0);
+
+ while (thread) {
@pqarmitage
pqarmitage Sep 27, 2016 Collaborator

I think this loop needs to be called with SIGCHLD blocked, otherwise the thread_child_handler() signal handler may run and modify the child queue while we are modifying it.

@pandax381
pandax381 Sep 28, 2016 Contributor

Sorry, I forgot to block SIGCHLD.
Use sigprocmask().

@@ -1094,7 +1094,7 @@ vrrp_script_child_thread(thread_t * thread)
log_message(LOG_INFO, "VRRP_Script(%s) timed out", vscript->sname);
vscript->result = 0;
}
- kill(pid, SIGTERM);
+ kill(-pid, SIGTERM);
thread_add_child(thread->master, vrrp_script_child_timeout_thread,
@pqarmitage
pqarmitage Sep 27, 2016 Collaborator

There is a slight problem here. If the immediate child handles SIGTERM, but children that it forks ignore SIGTERM, then the parent of the child processes will terminate, but its children won't, and they will be left running as now.

The only solution that I can see is if a child is killed with SIGTERM we need to remember that it has been sent a SIGTERM, and then if thread_child_handler() detects the child's termination and it has been sent a SIGTERM, it must execute kill(-pid, SIGKILL) to ensure all it's child processes have definitely died.

P.S. While looking at the code, it occurred to me that there might be a potential race condition. If a SIGCHLD signal were raised while thread_fetch() is looking at the child list (under comment /* Timeout children */), then the child queue might be in the middle of being changed by thread_fetch() when the thread_child_handler() function is executed as the signal handler, and it also manipulates the child list. I'll have a look at this a bit further; what do you think?

@pandax381
pandax381 Sep 28, 2016 Contributor

Yes, We do not want immediate child process terminate ahead.

This is one of the solutions.

  • Immediate child process to handle the SIGTERM.
  • And send it to the PID rather than PGID.
  • Immediate child process re-sent a signal to the process group
  • Finaly, SIGKILL send to the process group

I also looks like there is a risk of race conditions around SIGCHLD.
Also, it is trivial, but I think also should not do a lot of things in the signal handler.
In report_child_status(), it looks like has been calls is not asynchronous-signal-safety function.

I think that it is best to run in a loop of thread_fetch.
Let me think carefully.

@pqarmitage
pqarmitage Sep 28, 2016 Collaborator

I think the original approach of sending SIGTERM to the pgid is right, since the child process may not handle SIGTERM in the way we want, and certainly may not be coded to send SIGTERM to all its children (it may just terminate and leave its children running); SIGTERM is just our best hope of killing all the children so I think sending it to the pgid is right.

I also think there has to be code to send SIGKILL to the pgid if the immediate child terminates after the SIGTERM, since we don't know that the child's children will have terminated.

One option, which I'm happy to add after the rest of this is sorted, would be to add an option in the configuration to specify what signal to send to the child to terminate it, and also whether it should be sent to the pid or the pgid.

keepalived/check/check_daemon.c
@@ -217,6 +219,7 @@ reload_check_thread(__attribute__((unused)) thread_t * thread)
#ifdef _WITH_VRRP_
kernel_netlink_close();
#endif
+ script_killall(master, SIGHUP);
@pqarmitage
pqarmitage Sep 27, 2016 edited Collaborator

This sends a SIGTERM to the child process if keepalived is stopping, and a SIGHUP to the child process if the config is being reloaded. Whichever of these two situations is occurring, does the child process need to know the difference. All we want is that it stops, although it might be possible that a child process wants to do something different depending on whether keepalived is stopping or reloading (but I can't think of a reason why), but then the problem is that if the child receives a SIGTERM, it can't distinguish between the keepalived process terminating and the child has been running for too long.

The same applies further down in misc_check_child_thread() and stop_vrrp().

@pandax381
pandax381 Sep 28, 2016 Contributor

I chose a SIGHUP when reload just in case.
However, we want that it terminate. So, it looks good in SIGTERM.

I thought as a child process is there is no need to understand the cause of SIGTERM. And I think so child process (as the script program) would be required to terminate with SIGTERM.

@pqarmitage
pqarmitage Sep 28, 2016 Collaborator

Agreed.

@pandax381
Contributor

Thank you for your review and comments.

I'll enhance the code.

@pandax381 pandax381 changed the title from Fix process increase to [WIP] Fix process increase Sep 28, 2016
pandax381 added some commits Sep 29, 2016
@pandax381 pandax381 Use argument instead of static variable
Signed-off-by: Masaya Yamamoto <yamamoto-ma@klab.com>
acb8ba2
@pandax381 pandax381 Fix bug around the process group
Signed-off-by: Masaya Yamamoto <yamamoto-ma@klab.com>
6ffd46f
@pandax381 pandax381 Use SIGTERM instead of SIGHUP
Signed-off-by: Masaya Yamamoto <yamamoto-ma@klab.com>
cebfbf5
@pandax381
Contributor

Hi,

I was fixed issue that has been pointed out to you with the exception of this one.

#432 (comment)

Regards,

@pqarmitage pqarmitage merged commit dc85fdf into acassen:master Nov 3, 2016
@pandax381 pandax381 changed the title from [WIP] Fix process increase to Fix process increase Nov 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment