diff options
author | Steven Hartland <steven.hartland@multiplay.co.uk> | 2019-10-17 14:35:08 +0100 |
---|---|---|
committer | Chris Akritidis <43294513+cakrit@users.noreply.github.com> | 2019-10-17 15:35:08 +0200 |
commit | 7ff016ea74261246def5500308f17cb575059856 (patch) | |
tree | fdb15a3a92ede70ffc41d74bddf4e64b740d7b30 /libnetdata/popen | |
parent | 4b9cc50adc5d3cd0a644ebbd2d5db4ae66e4208e (diff) |
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.
Diffstat (limited to 'libnetdata/popen')
-rw-r--r-- | libnetdata/popen/popen.c | 150 | ||||
-rw-r--r-- | libnetdata/popen/popen.h | 3 |
2 files changed, 126 insertions, 27 deletions
diff --git a/libnetdata/popen/popen.c b/libnetdata/popen/popen.c index 906b105357..1c4ae64d6c 100644 --- a/libnetdata/popen/popen.c +++ b/libnetdata/popen/popen.c @@ -2,46 +2,79 @@ #include "../libnetdata.h" -/* +static pthread_mutex_t myp_lock; +static int myp_tracking = 0; + struct mypopen { pid_t pid; - FILE *fp; struct mypopen *next; struct mypopen *prev; }; static struct mypopen *mypopen_root = NULL; -static void mypopen_add(FILE *fp, pid_t *pid) { - struct mypopen *mp = malloc(sizeof(struct mypopen)); - if(!mp) { - fatal("Cannot allocate %zu bytes", sizeof(struct mypopen)) +// myp_add_lock takes the lock if we're tracking. +static void myp_add_lock(void) { + if (myp_tracking == 0) return; - } - mp->fp = fp; + netdata_mutex_lock(&myp_lock); +} + +// myp_add_unlock release the lock if we're tracking. +static void myp_add_unlock(void) { + if (myp_tracking == 0) + return; + + netdata_mutex_unlock(&myp_lock); +} + +// myp_add_locked adds pid if we're tracking. +// myp_add_lock must have been called previously. +static void myp_add_locked(pid_t pid) { + struct mypopen *mp; + + if (myp_tracking == 0) + return; + + mp = mallocz(sizeof(struct mypopen)); mp->pid = pid; - mp->next = popen_root; + + mp->next = mypopen_root; mp->prev = NULL; - if(mypopen_root) mypopen_root->prev = mp; + if (mypopen_root != NULL) + mypopen_root->prev = mp; mypopen_root = mp; + netdata_mutex_unlock(&myp_lock); } -static void mypopen_del(FILE *fp) { +// myp_del deletes pid if we're tracking. +static void myp_del(pid_t pid) { struct mypopen *mp; - for(mp = mypopen_root; mp; mp = mp->next) - if(mp->fd == fp) break; + if (myp_tracking == 0) + return; - if(!mp) error("Cannot find mypopen() file pointer in open childs."); - else { - if(mp->next) mp->next->prev = mp->prev; - if(mp->prev) mp->prev->next = mp->next; - if(mypopen_root == mp) mypopen_root = mp->next; - free(mp); + netdata_mutex_lock(&myp_lock); + for (mp = mypopen_root; mp != NULL; mp = mp->next) { + if (mp->pid == pid) { + if (mp->next != NULL) + mp->next->prev = mp->prev; + if (mp->prev != NULL) + mp->prev->next = mp->next; + if (mypopen_root == mp) + mypopen_root = mp->next; + freez(mp); + break; + } } + + if (mp == NULL) + error("Cannot find pid %d.", pid); + + netdata_mutex_unlock(&myp_lock); } -*/ + #define PIPE_READ 0 #define PIPE_WRITE 1 @@ -58,7 +91,7 @@ static inline FILE *custom_popene(const char *command, volatile pid_t *pidptr, c posix_spawnattr_t attr; posix_spawn_file_actions_t fa; - if(pipe(pipefd) == -1) + if (pipe(pipefd) == -1) return NULL; if ((fp = fdopen(pipefd[PIPE_READ], "r")) == NULL) { goto error_after_pipe; @@ -66,7 +99,7 @@ static inline FILE *custom_popene(const char *command, volatile pid_t *pidptr, c // Mark all files to be closed by the exec() stage of posix_spawn() int i; - for(i = (int) (sysconf(_SC_OPEN_MAX) - 1); i >= 0; i--) + for (i = (int) (sysconf(_SC_OPEN_MAX) - 1); i >= 0; i--) if(i != STDIN_FILENO && i != STDERR_FILENO) (void)fcntl(i, F_SETFD, FD_CLOEXEC); @@ -92,10 +125,16 @@ static inline FILE *custom_popene(const char *command, volatile pid_t *pidptr, c } else { error("posix_spawnattr_init() failed."); } + + // Take the lock while we fork to ensure we don't race with SIGCHLD + // delivery on a process which exits quickly. + myp_add_lock(); if (!posix_spawn(&pid, "/bin/sh", &fa, &attr, spawn_argv, env)) { *pidptr = pid; + myp_add_locked(pid); debug(D_CHILDS, "Spawned command: '%s' on pid %d from parent pid %d.", command, pid, getpid()); } else { + myp_add_unlock(); error("Failed to spawn command: '%s' from parent pid %d.", command, getpid()); fclose(fp); fp = NULL; @@ -128,6 +167,60 @@ error_after_pipe: // See man environ extern char **environ; +// myp_init should be called by apps which act as init +// (pid 1) so that processes created by mypopen and mypopene +// are tracked. This enables the reaper to ignore processes +// which will be handled internally, by calling myp_reap, to +// avoid issues with already reaped processes during wait calls. +// +// Callers should call myp_free() to clean up resources. +void myp_init(void) { + info("process tracking enabled."); + myp_tracking = 1; + + if (netdata_mutex_init(&myp_lock) != 0) { + fatal("myp_init() mutex init failed."); + } +} + +// myp_free cleans up any resources allocated for process +// tracking. +void myp_free(void) { + struct mypopen *mp, *next; + + if (myp_tracking == 0) + return; + + netdata_mutex_lock(&myp_lock); + for (mp = mypopen_root; mp != NULL; mp = next) { + next = mp->next; + freez(mp); + } + + mypopen_root = NULL; + myp_tracking = 0; + netdata_mutex_unlock(&myp_lock); +} + +// myp_reap returns 1 if pid should be reaped, 0 otherwise. +int myp_reap(pid_t pid) { + struct mypopen *mp; + + if (myp_tracking == 0) + return 0; + + netdata_mutex_lock(&myp_lock); + for (mp = mypopen_root; mp != NULL; mp = mp->next) { + if (mp->pid == pid) { + netdata_mutex_unlock(&myp_lock); + return 0; + } + } + netdata_mutex_unlock(&myp_lock); + + return 1; +} + FILE *mypopen(const char *command, volatile pid_t *pidptr) { return custom_popene(command, pidptr, environ); } @@ -137,9 +230,10 @@ FILE *mypopene(const char *command, volatile pid_t *pidptr, char **env) { } int mypclose(FILE *fp, pid_t pid) { - debug(D_EXIT, "Request to mypclose() on pid %d", pid); + int ret; + siginfo_t info; - /*mypopen_del(fp);*/ + debug(D_EXIT, "Request to mypclose() on pid %d", pid); // close the pipe fd // this is required in musl @@ -151,9 +245,11 @@ int mypclose(FILE *fp, pid_t pid) { errno = 0; - siginfo_t info; - if(waitid(P_PID, (id_t) pid, &info, WEXITED) != -1) { - switch(info.si_code) { + ret = waitid(P_PID, (id_t) pid, &info, WEXITED); + myp_del(pid); + + if (ret != -1) { + switch (info.si_code) { case CLD_EXITED: if(info.si_status) error("child pid %d exited with code %d.", info.si_pid, info.si_status); diff --git a/libnetdata/popen/popen.h b/libnetdata/popen/popen.h index 90d4b829b0..32f64e460b 100644 --- a/libnetdata/popen/popen.h +++ b/libnetdata/popen/popen.h @@ -11,6 +11,9 @@ extern FILE *mypopen(const char *command, volatile pid_t *pidptr); extern FILE *mypopene(const char *command, volatile pid_t *pidptr, char **env); extern int mypclose(FILE *fp, pid_t pid); +extern void myp_init(void); +extern void myp_free(void); +extern int myp_reap(pid_t pid); extern void signals_unblock(void); extern void signals_reset(void); |