From 708d74cb7a379a3b560b4cfbad25ee4b1e810621 Mon Sep 17 00:00:00 2001 From: Marcin Matlaszek Date: Fri, 17 Nov 2017 02:26:25 +0100 Subject: [PATCH 1/4] Create new process group on process startup. Clean up by sending SIGKILL to the whole process group. --- client/driver/executor/executor.go | 5 +++++ client/driver/executor/executor_basic.go | 4 ++++ client/driver/executor/executor_linux.go | 9 +++++++++ client/driver/raw_exec.go | 4 ++-- client/driver/utils.go | 10 ++++++++++ 5 files changed, 30 insertions(+), 2 deletions(-) diff --git a/client/driver/executor/executor.go b/client/driver/executor/executor.go index 9735daab01d..41153730b56 100644 --- a/client/driver/executor/executor.go +++ b/client/driver/executor/executor.go @@ -227,6 +227,11 @@ func (e *UniversalExecutor) LaunchCmd(command *ExecCommand) (*ProcessState, erro // set the task dir as the working directory for the command e.cmd.Dir = e.ctx.TaskDir + // start command in separate process group + if err := e.setNewProcessGroup(); err != nil { + return nil, err + } + // configuring the chroot, resource container, and start the plugin // process in the chroot. if err := e.configureIsolation(); err != nil { diff --git a/client/driver/executor/executor_basic.go b/client/driver/executor/executor_basic.go index 123ed47032e..65030d45b44 100644 --- a/client/driver/executor/executor_basic.go +++ b/client/driver/executor/executor_basic.go @@ -29,6 +29,10 @@ func (e *UniversalExecutor) configureIsolation() error { return nil } +func (e *UniversalExecutor) setNewProcessGroup() error { + return nil +} + func (e *UniversalExecutor) Stats() (*cstructs.TaskResourceUsage, error) { pidStats, err := e.pidStats() if err != nil { diff --git a/client/driver/executor/executor_linux.go b/client/driver/executor/executor_linux.go index ef1de7ebcf6..fe9600eb9cc 100644 --- a/client/driver/executor/executor_linux.go +++ b/client/driver/executor/executor_linux.go @@ -331,6 +331,15 @@ func DestroyCgroup(groups *cgroupConfig.Cgroup, cgPaths map[string]string, execu return mErrs.ErrorOrNil() } +// configure new process group for child process +func (e *UniversalExecutor) setNewProcessGroup() error { + if e.cmd.SysProcAttr == nil { + e.cmd.SysProcAttr = &syscall.SysProcAttr{} + } + e.cmd.SysProcAttr.Setpgid = true + return nil +} + // getCgroupManager returns the correct libcontainer cgroup manager. func getCgroupManager(groups *cgroupConfig.Cgroup, paths map[string]string) cgroups.Manager { return &cgroupFs.Manager{Cgroups: groups, Paths: paths} diff --git a/client/driver/raw_exec.go b/client/driver/raw_exec.go index 0cea2ea554e..e8f6c5bd202 100644 --- a/client/driver/raw_exec.go +++ b/client/driver/raw_exec.go @@ -298,8 +298,8 @@ func (h *rawExecHandle) run() { ps, werr := h.executor.Wait() close(h.doneCh) if ps.ExitCode == 0 && werr != nil { - if e := killProcess(h.userPid); e != nil { - h.logger.Printf("[ERR] driver.raw_exec: error killing user process: %v", e) + if e := killProcessGroup(h.userPid); e != nil { + h.logger.Printf("[ERR] driver.raw_exec: error killing user process group: %v", e) } } diff --git a/client/driver/utils.go b/client/driver/utils.go index 5fba8071f2f..c4038700735 100644 --- a/client/driver/utils.go +++ b/client/driver/utils.go @@ -8,6 +8,7 @@ import ( "os/exec" "path/filepath" "strings" + "syscall" "time" "github.com/hashicorp/consul-template/signals" @@ -110,6 +111,15 @@ func killProcess(pid int) error { return proc.Kill() } +// killProcessGroup kills a process group with the given pid +func killProcessGroup(pid int) error { + proc, err := os.FindProcess(pid) + if err != nil { + return err + } + return syscall.Kill(-proc.Pid, syscall.SIGKILL) +} + // destroyPlugin kills the plugin with the given pid and also kills the user // process func destroyPlugin(pluginPid int, userPid int) error { From 76a8978c3e487d32b80b8cb55c07e69b10453a95 Mon Sep 17 00:00:00 2001 From: Marcin Matlaszek Date: Wed, 22 Nov 2017 00:51:39 +0100 Subject: [PATCH 2/4] Make starting & cleaning process group Windows compatible. --- client/driver/executor/executor.go | 13 ++++++++++++- client/driver/executor/executor_linux.go | 9 --------- client/driver/executor/executor_unix.go | 10 ++++++++++ client/driver/raw_exec.go | 4 ++-- client/driver/utils.go | 10 ---------- 5 files changed, 24 insertions(+), 22 deletions(-) diff --git a/client/driver/executor/executor.go b/client/driver/executor/executor.go index 41153730b56..62a67c1bdfa 100644 --- a/client/driver/executor/executor.go +++ b/client/driver/executor/executor.go @@ -448,6 +448,17 @@ func ClientCleanup(ic *dstructs.IsolationConfig, pid int) error { return clientCleanup(ic, pid) } +// Cleanup any still hanging user processes +func (e *UniversalExecutor) cleanupUserLeftovers(proc *os.Process) error { + // If new process group was created upon command execution + // we can kill the whole process group now to cleanup any leftovers. + if e.cmd.SysProcAttr != nil && e.cmd.SysProcAttr.Setpgid { + return syscall.Kill(-proc.Pid, syscall.SIGKILL) + } else { + return proc.Kill() + } +} + // Exit cleans up the alloc directory, destroys resource container and kills the // user process func (e *UniversalExecutor) Exit() error { @@ -475,7 +486,7 @@ func (e *UniversalExecutor) Exit() error { if err != nil { e.logger.Printf("[ERR] executor: can't find process with pid: %v, err: %v", e.cmd.Process.Pid, err) - } else if err := proc.Kill(); err != nil && err.Error() != finishedErr { + } else if err := e.cleanupUserLeftovers(proc); err != nil && err.Error() != finishedErr { merr.Errors = append(merr.Errors, fmt.Errorf("can't kill process with pid: %v, err: %v", e.cmd.Process.Pid, err)) } diff --git a/client/driver/executor/executor_linux.go b/client/driver/executor/executor_linux.go index fe9600eb9cc..ef1de7ebcf6 100644 --- a/client/driver/executor/executor_linux.go +++ b/client/driver/executor/executor_linux.go @@ -331,15 +331,6 @@ func DestroyCgroup(groups *cgroupConfig.Cgroup, cgPaths map[string]string, execu return mErrs.ErrorOrNil() } -// configure new process group for child process -func (e *UniversalExecutor) setNewProcessGroup() error { - if e.cmd.SysProcAttr == nil { - e.cmd.SysProcAttr = &syscall.SysProcAttr{} - } - e.cmd.SysProcAttr.Setpgid = true - return nil -} - // getCgroupManager returns the correct libcontainer cgroup manager. func getCgroupManager(groups *cgroupConfig.Cgroup, paths map[string]string) cgroups.Manager { return &cgroupFs.Manager{Cgroups: groups, Paths: paths} diff --git a/client/driver/executor/executor_unix.go b/client/driver/executor/executor_unix.go index 90efa32e6fa..cf2fc3f3344 100644 --- a/client/driver/executor/executor_unix.go +++ b/client/driver/executor/executor_unix.go @@ -5,6 +5,7 @@ package executor import ( "fmt" "io" + "syscall" syslog "github.com/RackSec/srslog" @@ -47,3 +48,12 @@ func (e *UniversalExecutor) collectLogs(we io.Writer, wo io.Writer) { } } } + +// configure new process group for child process +func (e *UniversalExecutor) setNewProcessGroup() error { + if e.cmd.SysProcAttr == nil { + e.cmd.SysProcAttr = &syscall.SysProcAttr{} + } + e.cmd.SysProcAttr.Setpgid = true + return nil +} diff --git a/client/driver/raw_exec.go b/client/driver/raw_exec.go index e8f6c5bd202..0cea2ea554e 100644 --- a/client/driver/raw_exec.go +++ b/client/driver/raw_exec.go @@ -298,8 +298,8 @@ func (h *rawExecHandle) run() { ps, werr := h.executor.Wait() close(h.doneCh) if ps.ExitCode == 0 && werr != nil { - if e := killProcessGroup(h.userPid); e != nil { - h.logger.Printf("[ERR] driver.raw_exec: error killing user process group: %v", e) + if e := killProcess(h.userPid); e != nil { + h.logger.Printf("[ERR] driver.raw_exec: error killing user process: %v", e) } } diff --git a/client/driver/utils.go b/client/driver/utils.go index c4038700735..5fba8071f2f 100644 --- a/client/driver/utils.go +++ b/client/driver/utils.go @@ -8,7 +8,6 @@ import ( "os/exec" "path/filepath" "strings" - "syscall" "time" "github.com/hashicorp/consul-template/signals" @@ -111,15 +110,6 @@ func killProcess(pid int) error { return proc.Kill() } -// killProcessGroup kills a process group with the given pid -func killProcessGroup(pid int) error { - proc, err := os.FindProcess(pid) - if err != nil { - return err - } - return syscall.Kill(-proc.Pid, syscall.SIGKILL) -} - // destroyPlugin kills the plugin with the given pid and also kills the user // process func destroyPlugin(pluginPid int, userPid int) error { From 963c10e584a1522cfa5da92bafcecd22a3ba55a8 Mon Sep 17 00:00:00 2001 From: Marcin Matlaszek Date: Thu, 23 Nov 2017 11:57:26 +0100 Subject: [PATCH 3/4] Fix errors when trying to kill whole process group. --- client/driver/executor/executor.go | 9 ++++++++- client/driver/executor/executor_basic.go | 4 ---- client/driver/executor/executor_unix.go | 5 +++++ 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/client/driver/executor/executor.go b/client/driver/executor/executor.go index 62a67c1bdfa..c416d511c3f 100644 --- a/client/driver/executor/executor.go +++ b/client/driver/executor/executor.go @@ -440,6 +440,10 @@ var ( // finishedErr is the error message received when trying to kill and already // exited process. finishedErr = "os: process already finished" + + // noSuchProcessErr is the error message received when trying to kill a non + // existing process (e.g. when killing a process group). + noSuchProcessErr = "no such process" ) // ClientCleanup is the cleanup routine that a Nomad Client uses to remove the @@ -453,7 +457,10 @@ func (e *UniversalExecutor) cleanupUserLeftovers(proc *os.Process) error { // If new process group was created upon command execution // we can kill the whole process group now to cleanup any leftovers. if e.cmd.SysProcAttr != nil && e.cmd.SysProcAttr.Setpgid { - return syscall.Kill(-proc.Pid, syscall.SIGKILL) + if err := syscall.Kill(-proc.Pid, syscall.SIGKILL); err != nil && err.Error() != noSuchProcessErr { + return err + } + return nil } else { return proc.Kill() } diff --git a/client/driver/executor/executor_basic.go b/client/driver/executor/executor_basic.go index 65030d45b44..123ed47032e 100644 --- a/client/driver/executor/executor_basic.go +++ b/client/driver/executor/executor_basic.go @@ -29,10 +29,6 @@ func (e *UniversalExecutor) configureIsolation() error { return nil } -func (e *UniversalExecutor) setNewProcessGroup() error { - return nil -} - func (e *UniversalExecutor) Stats() (*cstructs.TaskResourceUsage, error) { pidStats, err := e.pidStats() if err != nil { diff --git a/client/driver/executor/executor_unix.go b/client/driver/executor/executor_unix.go index cf2fc3f3344..81b79e6f4bd 100644 --- a/client/driver/executor/executor_unix.go +++ b/client/driver/executor/executor_unix.go @@ -5,6 +5,7 @@ package executor import ( "fmt" "io" + "runtime" "syscall" syslog "github.com/RackSec/srslog" @@ -51,6 +52,10 @@ func (e *UniversalExecutor) collectLogs(we io.Writer, wo io.Writer) { // configure new process group for child process func (e *UniversalExecutor) setNewProcessGroup() error { + // We need to check that as build flags includes windows for this file + if runtime.GOOS == "windows" { + return nil + } if e.cmd.SysProcAttr == nil { e.cmd.SysProcAttr = &syscall.SysProcAttr{} } From 9b5a5922e2e100108862d56aae571ac71652061e Mon Sep 17 00:00:00 2001 From: Marcin Matlaszek Date: Tue, 6 Feb 2018 21:08:14 +0100 Subject: [PATCH 4/4] Make raw_exec processes cleanup function more precise. --- client/driver/executor/executor.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/client/driver/executor/executor.go b/client/driver/executor/executor.go index c416d511c3f..e7134c1ce12 100644 --- a/client/driver/executor/executor.go +++ b/client/driver/executor/executor.go @@ -453,7 +453,7 @@ func ClientCleanup(ic *dstructs.IsolationConfig, pid int) error { } // Cleanup any still hanging user processes -func (e *UniversalExecutor) cleanupUserLeftovers(proc *os.Process) error { +func (e *UniversalExecutor) cleanupChildProcesses(proc *os.Process) error { // If new process group was created upon command execution // we can kill the whole process group now to cleanup any leftovers. if e.cmd.SysProcAttr != nil && e.cmd.SysProcAttr.Setpgid { @@ -493,7 +493,7 @@ func (e *UniversalExecutor) Exit() error { if err != nil { e.logger.Printf("[ERR] executor: can't find process with pid: %v, err: %v", e.cmd.Process.Pid, err) - } else if err := e.cleanupUserLeftovers(proc); err != nil && err.Error() != finishedErr { + } else if err := e.cleanupChildProcesses(proc); err != nil && err.Error() != finishedErr { merr.Errors = append(merr.Errors, fmt.Errorf("can't kill process with pid: %v, err: %v", e.cmd.Process.Pid, err)) }