feat(reaper): Add process reaper support (#7059)
##### Summary Add a child process reaper to the main netdata app if running as init (pid = 1). This prevents zombie processes when a child is re-parented to netdata when its running in a container. Also: * Few style cleanups to match surrounding code. Fixes: #6033 ##### Component Name netdata binary ##### Additional Information This re-purposes old commented out code in `popen.c`, which already implemented part of the required process tracking. Without this on a standard netdata docker install we saw at least one zombie `timeout` process straight after the container was started.
Steven Hartland committed
Oct 17, 2019 at 14:35 UTC
7ff016ea74261246def5500308f17cb575059856
8 files changed
+240
-62
CONTRIBUTORS.md
+1
@@ -129,5 +129,6 @@ This is the list of contributors that have signed this agreement:
129
|@skrzyp1|Jerzy S.||
130
|@akwan|Alan Kwan||
131
|@underhood|Timotej Šiškovič||
132
+|@stevenh|Steven Hartland|steven.hartland@multiplay.co.uk|
133
134
[](<>)
collectors/plugins.d/plugins_d.c
+2
-2
@@ -647,7 +647,7 @@ static void pluginsd_worker_thread_cleanup(void *arg) {
647
if (cd->pid) {
648
siginfo_t info;
649
info("killing child process pid %d", cd->pid);
650
- if (killpid(cd->pid, SIGTERM) != -1) {
650
+ if (killpid(cd->pid) != -1) {
651
info("waiting for child process pid %d to exit...", cd->pid);
652
waitid(P_PID, (id_t) cd->pid, &info, WEXITED);
653
}
@@ -738,7 +738,7 @@ void *pluginsd_worker_thread(void *arg) {
738
info("connected to '%s' running on pid %d", cd->fullfilename, cd->pid);
739
count = pluginsd_process(localhost, cd, fp, 0);
740
error("'%s' (pid %d) disconnected after %zu successful data collections (ENDs).", cd->fullfilename, cd->pid, count);
741
- killpid(cd->pid, SIGTERM);
741
+ killpid(cd->pid);
742
743
int worker_ret_code = mypclose(fp, cd->pid);
744
collectors/tc.plugin/plugin_tc.c
+1
-2
@@ -851,12 +851,11 @@ static void tc_main_cleanup(void *ptr) {
851
852
if(tc_child_pid) {
853
info("TC: killing with SIGTERM tc-qos-helper process %d", tc_child_pid);
854
- if(killpid(tc_child_pid, SIGTERM) != -1) {
854
+ if(killpid(tc_child_pid) != -1) {
855
siginfo_t info;
856
857
info("TC: waiting for tc plugin child process pid %d to exit...", tc_child_pid);
858
waitid(P_PID, (id_t) tc_child_pid, &info, WEXITED);
859
- // info("TC: finished tc plugin child process pid %d.", tc_child_pid);
859
}
860
861
tc_child_pid = 0;
daemon/main.c
+9
-27
@@ -146,46 +146,28 @@ void web_server_config_options(void) {
146
}
147
148
149
-int killpid(pid_t pid, int signal)
150
-{
151
- int ret = -1;
149
+// killpid kills pid with SIGTERM.
150
+int killpid(pid_t pid) {
151
+ int ret;
152
debug(D_EXIT, "Request to kill pid %d", pid);
153
154
errno = 0;
155
- if(kill(pid, 0) == -1) {
155
+ ret = kill(pid, SIGTERM);
156
+ if (ret == -1) {
157
switch(errno) {
158
case ESRCH:
158
- error("Request to kill pid %d, but it is not running.", pid);
159
- break;
159
+ // We wanted the process to exit so just let the caller handle.
160
+ return ret;
161
162
case EPERM:
162
- error("Request to kill pid %d, but I do not have enough permissions.", pid);
163
+ error("Cannot kill pid %d, but I do not have enough permissions.", pid);
164
break;
165
166
default:
166
- error("Request to kill pid %d, but I received an error.", pid);
167
+ error("Cannot kill pid %d, but I received an error.", pid);
168
break;
169
}
170
}
170
- else {
171
- errno = 0;
172
- ret = kill(pid, signal);
173
- if(ret == -1) {
174
- switch(errno) {
175
- case ESRCH:
176
- error("Cannot kill pid %d, but it is not running.", pid);
177
- break;
178
-
179
- case EPERM:
180
- error("Cannot kill pid %d, but I do not have enough permissions.", pid);
181
- break;
182
-
183
- default:
184
- error("Cannot kill pid %d, but I received an error.", pid);
185
- break;
186
- }
187
- }
188
- }
171
172
return ret;
173
}
daemon/main.h
+1
-1
@@ -41,7 +41,7 @@ struct netdata_static_thread {
41
};
42
43
extern void cancel_main_threads(void);
44
-extern int killpid(pid_t pid, int signal);
44
+extern int killpid(pid_t pid);
45
extern void netdata_cleanup_and_exit(int ret) NORETURN;
46
extern void send_statistics(const char *action, const char *action_result, const char *action_data);
47
daemon/signals.c
+100
-3
@@ -2,6 +2,8 @@
2
3
#include "common.h"
4
5
+static int reaper_enabled = 0;
6
+
7
typedef enum signal_action {
8
NETDATA_SIGNAL_END_OF_LIST,
9
NETDATA_SIGNAL_IGNORE,
@@ -10,6 +12,7 @@ typedef enum signal_action {
12
NETDATA_SIGNAL_LOG_ROTATE,
13
NETDATA_SIGNAL_RELOAD_HEALTH,
14
NETDATA_SIGNAL_FATAL,
15
+ NETDATA_SIGNAL_CHILD,
16
} SIGNAL_ACTION;
17
18
static struct {
@@ -26,6 +29,7 @@ static struct {
29
{ SIGUSR1, "SIGUSR1", 0, NETDATA_SIGNAL_SAVE_DATABASE },
30
{ SIGUSR2, "SIGUSR2", 0, NETDATA_SIGNAL_RELOAD_HEALTH },
31
{ SIGBUS, "SIGBUS", 0, NETDATA_SIGNAL_FATAL },
32
+ { SIGCHLD, "SIGCHLD", 0, NETDATA_SIGNAL_CHILD },
33
34
// terminator
35
{ 0, "NONE", 0, NETDATA_SIGNAL_END_OF_LIST }
@@ -42,7 +46,7 @@ static void signal_handler(int signo) {
46
char buffer[200 + 1];
47
snprintfz(buffer, 200, "\nSIGNAL HANLDER: received: %s. Oops! This is bad!\n", signals_waiting[i].name);
48
if(write(STDERR_FILENO, buffer, strlen(buffer)) == -1) {
45
- // nothing to do - we cannot write but there is no way to complaint about it
49
+ // nothing to do - we cannot write but there is no way to complain about it
50
;
51
}
52
}
@@ -74,15 +78,33 @@ void signals_init(void) {
78
struct sigaction sa;
79
sa.sa_flags = 0;
80
81
+ // Enable process tracking / reaper if running as init (pid == 1).
82
+ // This prevents zombie processes when running in a container.
83
+ if (getpid() == 1) {
84
+ info("SIGNAL: Enabling reaper");
85
+ myp_init();
86
+ reaper_enabled = 1;
87
+ } else {
88
+ info("SIGNAL: Not enabling reaper");
89
+ }
90
+
91
// ignore all signals while we run in a signal handler
92
sigfillset(&sa.sa_mask);
93
94
int i;
95
for (i = 0; signals_waiting[i].action != NETDATA_SIGNAL_END_OF_LIST; i++) {
82
- if(signals_waiting[i].action == NETDATA_SIGNAL_IGNORE)
96
+ switch (signals_waiting[i].action) {
97
+ case NETDATA_SIGNAL_IGNORE:
98
sa.sa_handler = SIG_IGN;
84
- else
99
+ break;
100
+ case NETDATA_SIGNAL_CHILD:
101
+ if (reaper_enabled == 0)
102
+ continue;
103
+ // FALLTHROUGH
104
+ default:
105
sa.sa_handler = signal_handler;
106
+ break;
107
+ }
108
109
if(sigaction(signals_waiting[i].signo, &sa, NULL) == -1)
110
error("SIGNAL: Failed to change signal handler for: %s", signals_waiting[i].name);
@@ -100,6 +122,76 @@ void signals_reset(void) {
122
if(sigaction(signals_waiting[i].signo, &sa, NULL) == -1)
123
error("SIGNAL: Failed to reset signal handler for: %s", signals_waiting[i].name);
124
}
125
+
126
+ if (reaper_enabled == 1)
127
+ myp_free();
128
+}
129
+
130
+// reap_child reaps the child identified by pid.
131
+static void reap_child(pid_t pid) {
132
+ siginfo_t i;
133
+
134
+ errno = 0;
135
+ debug(D_CHILDS, "SIGNAL: Reaping pid: %d...", pid);
136
+ if (waitid(P_PID, (id_t)pid, &i, WEXITED|WNOHANG) == -1) {
137
+ if (errno != ECHILD)
138
+ error("SIGNAL: Failed to wait for: %d", pid);
139
+ else
140
+ debug(D_CHILDS, "SIGNAL: Already reaped: %d", pid);
141
+ return;
142
+ } else if (i.si_pid == 0) {
143
+ // Process didn't exit, this shouldn't happen.
144
+ return;
145
+ }
146
+
147
+ switch (i.si_code) {
148
+ case CLD_EXITED:
149
+ debug(D_CHILDS, "SIGNAL: Child %d exited: %d", pid, i.si_status);
150
+ break;
151
+ case CLD_KILLED:
152
+ debug(D_CHILDS, "SIGNAL: Child %d killed by signal: %d", pid, i.si_status);
153
+ break;
154
+ case CLD_DUMPED:
155
+ debug(D_CHILDS, "SIGNAL: Child %d dumped core by signal: %d", pid, i.si_status);
156
+ break;
157
+ case CLD_STOPPED:
158
+ debug(D_CHILDS, "SIGNAL: Child %d stopped by signal: %d", pid, i.si_status);
159
+ break;
160
+ case CLD_TRAPPED:
161
+ debug(D_CHILDS, "SIGNAL: Child %d trapped by signal: %d", pid, i.si_status);
162
+ break;
163
+ case CLD_CONTINUED:
164
+ debug(D_CHILDS, "SIGNAL: Child %d continued by signal: %d", pid, i.si_status);
165
+ break;
166
+ default:
167
+ debug(D_CHILDS, "SIGNAL: Child %d gave us a SIGCHLD with code %d and status %d.", pid, i.si_code, i.si_status);
168
+ }
169
+}
170
+
171
+// reap_children reaps all pending children which are not managed by myp.
172
+static void reap_children() {
173
+ siginfo_t i;
174
+
175
+ while (1 == 1) {
176
+ // Identify which process caused the signal so we can determine
177
+ // if we need to reap a re-parented process.
178
+ i.si_pid = 0;
179
+ if (waitid(P_ALL, (id_t)0, &i, WEXITED|WNOHANG|WNOWAIT) == -1) {
180
+ if (errno != ECHILD) // This shouldn't happen with WNOHANG but does.
181
+ error("SIGNAL: Failed to wait");
182
+ return;
183
+ } else if (i.si_pid == 0) {
184
+ // No child exited.
185
+ return;
186
+ } else if (myp_reap(i.si_pid) == 0) {
187
+ // myp managed, sleep for a short time to avoid busy wait while
188
+ // this is handled by myp.
189
+ usleep(10000);
190
+ } else {
191
+ // Unknown process, likely a re-parented child, reap it.
192
+ reap_child(i.si_pid);
193
+ }
194
+ }
195
}
196
197
void signals_handle(void) {
@@ -157,6 +249,11 @@ void signals_handle(void) {
249
case NETDATA_SIGNAL_FATAL:
250
fatal("SIGNAL: Received %s. netdata now exits.", name);
251
252
+ case NETDATA_SIGNAL_CHILD:
253
+ debug(D_CHILDS, "SIGNAL: Received %s. Reaping...", name);
254
+ reap_children();
255
+ break;
256
+
257
default:
258
info("SIGNAL: Received %s. No signal handler configured. Ignoring it.", name);
259
break;
libnetdata/popen/popen.c
+123
-27
@@ -2,46 +2,79 @@
2
3
#include "../libnetdata.h"
4
5
-/*
5
+static pthread_mutex_t myp_lock;
6
+static int myp_tracking = 0;
7
+
8
struct mypopen {
9
pid_t pid;
8
- FILE *fp;
10
struct mypopen *next;
11
struct mypopen *prev;
12
};
13
14
static struct mypopen *mypopen_root = NULL;
15
15
-static void mypopen_add(FILE *fp, pid_t *pid) {
16
- struct mypopen *mp = malloc(sizeof(struct mypopen));
17
- if(!mp) {
18
- fatal("Cannot allocate %zu bytes", sizeof(struct mypopen))
16
+// myp_add_lock takes the lock if we're tracking.
17
+static void myp_add_lock(void) {
18
+ if (myp_tracking == 0)
19
return;
20
- }
20
22
- mp->fp = fp;
21
+ netdata_mutex_lock(&myp_lock);
22
+}
23
+
24
+// myp_add_unlock release the lock if we're tracking.
25
+static void myp_add_unlock(void) {
26
+ if (myp_tracking == 0)
27
+ return;
28
+
29
+ netdata_mutex_unlock(&myp_lock);
30
+}
31
+
32
+// myp_add_locked adds pid if we're tracking.
33
+// myp_add_lock must have been called previously.
34
+static void myp_add_locked(pid_t pid) {
35
+ struct mypopen *mp;
36
+
37
+ if (myp_tracking == 0)
38
+ return;
39
+
40
+ mp = mallocz(sizeof(struct mypopen));
41
mp->pid = pid;
24
- mp->next = popen_root;
42
+
43
+ mp->next = mypopen_root;
44
mp->prev = NULL;
26
- if(mypopen_root) mypopen_root->prev = mp;
45
+ if (mypopen_root != NULL)
46
+ mypopen_root->prev = mp;
47
mypopen_root = mp;
48
+ netdata_mutex_unlock(&myp_lock);
49
}
50
30
-static void mypopen_del(FILE *fp) {
51
+// myp_del deletes pid if we're tracking.
52
+static void myp_del(pid_t pid) {
53
struct mypopen *mp;
54
33
- for(mp = mypopen_root; mp; mp = mp->next)
34
- if(mp->fd == fp) break;
55
+ if (myp_tracking == 0)
56
+ return;
57
36
- if(!mp) error("Cannot find mypopen() file pointer in open childs.");
37
- else {
38
- if(mp->next) mp->next->prev = mp->prev;
39
- if(mp->prev) mp->prev->next = mp->next;
40
- if(mypopen_root == mp) mypopen_root = mp->next;
41
- free(mp);
58
+ netdata_mutex_lock(&myp_lock);
59
+ for (mp = mypopen_root; mp != NULL; mp = mp->next) {
60
+ if (mp->pid == pid) {
61
+ if (mp->next != NULL)
62
+ mp->next->prev = mp->prev;
63
+ if (mp->prev != NULL)
64
+ mp->prev->next = mp->next;
65
+ if (mypopen_root == mp)
66
+ mypopen_root = mp->next;
67
+ freez(mp);
68
+ break;
69
+ }
70
}
71
+
72
+ if (mp == NULL)
73
+ error("Cannot find pid %d.", pid);
74
+
75
+ netdata_mutex_unlock(&myp_lock);
76
}
44
-*/
77
+
78
#define PIPE_READ 0
79
#define PIPE_WRITE 1
80
@@ -58,7 +91,7 @@ static inline FILE *custom_popene(const char *command, volatile pid_t *pidptr, c
91
posix_spawnattr_t attr;
92
posix_spawn_file_actions_t fa;
93
61
- if(pipe(pipefd) == -1)
94
+ if (pipe(pipefd) == -1)
95
return NULL;
96
if ((fp = fdopen(pipefd[PIPE_READ], "r")) == NULL) {
97
goto error_after_pipe;
@@ -66,7 +99,7 @@ static inline FILE *custom_popene(const char *command, volatile pid_t *pidptr, c
99
100
// Mark all files to be closed by the exec() stage of posix_spawn()
101
int i;
69
- for(i = (int) (sysconf(_SC_OPEN_MAX) - 1); i >= 0; i--)
102
+ for (i = (int) (sysconf(_SC_OPEN_MAX) - 1); i >= 0; i--)
103
if(i != STDIN_FILENO && i != STDERR_FILENO)
104
(void)fcntl(i, F_SETFD, FD_CLOEXEC);
105
@@ -92,10 +125,16 @@ static inline FILE *custom_popene(const char *command, volatile pid_t *pidptr, c
125
} else {
126
error("posix_spawnattr_init() failed.");
127
}
128
+
129
+ // Take the lock while we fork to ensure we don't race with SIGCHLD
130
+ // delivery on a process which exits quickly.
131
+ myp_add_lock();
132
if (!posix_spawn(&pid, "/bin/sh", &fa, &attr, spawn_argv, env)) {
133
*pidptr = pid;
134
+ myp_add_locked(pid);
135
debug(D_CHILDS, "Spawned command: '%s' on pid %d from parent pid %d.", command, pid, getpid());
136
} else {
137
+ myp_add_unlock();
138
error("Failed to spawn command: '%s' from parent pid %d.", command, getpid());
139
fclose(fp);
140
fp = NULL;
@@ -128,6 +167,60 @@ error_after_pipe:
167
// See man environ
168
extern char **environ;
169
170
+// myp_init should be called by apps which act as init
171
+// (pid 1) so that processes created by mypopen and mypopene
172
+// are tracked. This enables the reaper to ignore processes
173
+// which will be handled internally, by calling myp_reap, to
174
+// avoid issues with already reaped processes during wait calls.
175
+//
176
+// Callers should call myp_free() to clean up resources.
177
+void myp_init(void) {
178
+ info("process tracking enabled.");
179
+ myp_tracking = 1;
180
+
181
+ if (netdata_mutex_init(&myp_lock) != 0) {
182
+ fatal("myp_init() mutex init failed.");
183
+ }
184
+}
185
+
186
+// myp_free cleans up any resources allocated for process
187
+// tracking.
188
+void myp_free(void) {
189
+ struct mypopen *mp, *next;
190
+
191
+ if (myp_tracking == 0)
192
+ return;
193
+
194
+ netdata_mutex_lock(&myp_lock);
195
+ for (mp = mypopen_root; mp != NULL; mp = next) {
196
+ next = mp->next;
197
+ freez(mp);
198
+ }
199
+
200
+ mypopen_root = NULL;
201
+ myp_tracking = 0;
202
+ netdata_mutex_unlock(&myp_lock);
203
+}
204
+
205
+// myp_reap returns 1 if pid should be reaped, 0 otherwise.
206
+int myp_reap(pid_t pid) {
207
+ struct mypopen *mp;
208
+
209
+ if (myp_tracking == 0)
210
+ return 0;
211
+
212
+ netdata_mutex_lock(&myp_lock);
213
+ for (mp = mypopen_root; mp != NULL; mp = mp->next) {
214
+ if (mp->pid == pid) {
215
+ netdata_mutex_unlock(&myp_lock);
216
+ return 0;
217
+ }
218
+ }
219
+ netdata_mutex_unlock(&myp_lock);
220
+
221
+ return 1;
222
+}
223
+
224
FILE *mypopen(const char *command, volatile pid_t *pidptr) {
225
return custom_popene(command, pidptr, environ);
226
}
@@ -137,9 +230,10 @@ FILE *mypopene(const char *command, volatile pid_t *pidptr, char **env) {
230
}
231
232
int mypclose(FILE *fp, pid_t pid) {
140
- debug(D_EXIT, "Request to mypclose() on pid %d", pid);
233
+ int ret;
234
+ siginfo_t info;
235
142
- /*mypopen_del(fp);*/
236
+ debug(D_EXIT, "Request to mypclose() on pid %d", pid);
237
238
// close the pipe fd
239
// this is required in musl
@@ -151,9 +245,11 @@ int mypclose(FILE *fp, pid_t pid) {
245
246
errno = 0;
247
154
- siginfo_t info;
155
- if(waitid(P_PID, (id_t) pid, &info, WEXITED) != -1) {
156
- switch(info.si_code) {
248
+ ret = waitid(P_PID, (id_t) pid, &info, WEXITED);
249
+ myp_del(pid);
250
+
251
+ if (ret != -1) {
252
+ switch (info.si_code) {
253
case CLD_EXITED:
254
if(info.si_status)
255
error("child pid %d exited with code %d.", info.si_pid, info.si_status);
libnetdata/popen/popen.h
+3
@@ -11,6 +11,9 @@
11
extern FILE *mypopen(const char *command, volatile pid_t *pidptr);
12
extern FILE *mypopene(const char *command, volatile pid_t *pidptr, char **env);
13
extern int mypclose(FILE *fp, pid_t pid);
14
+extern void myp_init(void);
15
+extern void myp_free(void);
16
+extern int myp_reap(pid_t pid);
17
18
extern void signals_unblock(void);
19
extern void signals_reset(void);