Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

B 2117 windows group process #4153

Merged
merged 2 commits into from
Apr 17, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
IMPROVEMENTS:
* client: Create new process group on process startup. [[GH-3572](https://github.com/hashicorp/nomad/issues/3572)]

BUG FIXES:
* driver/exec: Create process group for Windows process and send Ctrl-Break signal on Shutdown [[GH-2117](https://github.com/hashicorp/nomad/issues/2117)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use the pull request number instead of the issue number for these links. The issues in the URL is just a typo we haven't fixed in our make changelogfmt script because Github is nice enough to redirect it anyway. :)


## 0.8.0 (April 12, 2018)

__BACKWARDS INCOMPATIBILITIES:__
Expand Down
36 changes: 1 addition & 35 deletions client/driver/executor/executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -452,20 +452,6 @@ func ClientCleanup(ic *dstructs.IsolationConfig, pid int) error {
return clientCleanup(ic, pid)
}

// Cleanup any still hanging user processes
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 {
if err := syscall.Kill(-proc.Pid, syscall.SIGKILL); err != nil && err.Error() != noSuchProcessErr {
return err
}
return nil
} else {
return proc.Kill()
}
}

// Exit cleans up the alloc directory, destroys resource container and kills the
// user process
func (e *UniversalExecutor) Exit() error {
Expand Down Expand Up @@ -516,27 +502,7 @@ func (e *UniversalExecutor) ShutDown() error {
if err != nil {
return fmt.Errorf("executor.shutdown failed to find process: %v", err)
}
if runtime.GOOS == "windows" {
if err := proc.Kill(); err != nil && err.Error() != finishedErr {
return err
}
return nil
}

// Set default kill signal, as some drivers don't support configurable
// signals (such as rkt)
var osSignal os.Signal
if e.command.TaskKillSignal != nil {
osSignal = e.command.TaskKillSignal
} else {
osSignal = os.Interrupt
}

if err = proc.Signal(osSignal); err != nil && err.Error() != finishedErr {
return fmt.Errorf("executor.shutdown error: %v", err)
}

return nil
return e.shutdownProcess(proc)
}

// pidStats returns the resource usage stats per pid
Expand Down
38 changes: 33 additions & 5 deletions client/driver/executor/executor_unix.go
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
// +build darwin dragonfly freebsd linux netbsd openbsd solaris windows
// +build darwin dragonfly freebsd linux netbsd openbsd solaris

package executor

import (
"fmt"
"io"
"runtime"
"os"
"syscall"

syslog "github.com/RackSec/srslog"
Expand Down Expand Up @@ -53,12 +53,40 @@ 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{}
}
e.cmd.SysProcAttr.Setpgid = true
return nil
}

// Cleanup any still hanging user processes
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 {
if err := syscall.Kill(-proc.Pid, syscall.SIGKILL); err != nil && err.Error() != noSuchProcessErr {
return err
}
return nil
} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove else statement and just fall through to proc.Kill()

return proc.Kill()
}
}

func (e *UniversalExecutor) shutdownProcess(proc *os.Process) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment that this sends the shutdown signal and doesn't necessarily kill the process

// Set default kill signal, as some drivers don't support configurable
// signals (such as rkt)
var osSignal os.Signal
if e.command.TaskKillSignal != nil {
osSignal = e.command.TaskKillSignal
} else {
osSignal = os.Interrupt
}

if err := proc.Signal(osSignal); err != nil && err.Error() != finishedErr {
return fmt.Errorf("executor.shutdown error: %v", err)
}

return nil
}
111 changes: 111 additions & 0 deletions client/driver/executor/executor_windows.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
// +build windows
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary since the file has _windows.go


package executor

import (
"fmt"
"io"
"os"
"syscall"

syslog "github.com/RackSec/srslog"

"github.com/hashicorp/nomad/client/driver/logging"
)

func (e *UniversalExecutor) LaunchSyslogServer() (*SyslogServerState, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like you pointed out in the issue description I don't think there was any reason for this to be in the executor_unix.go file instead of executor.go proper. Would you mind moving it there to avoid duplication?

// Ensure the context has been set first
if e.ctx == nil {
return nil, fmt.Errorf("SetContext must be called before launching the Syslog Server")
}

e.syslogChan = make(chan *logging.SyslogMessage, 2048)
l, err := e.getListener(e.ctx.PortLowerBound, e.ctx.PortUpperBound)
if err != nil {
return nil, err
}
e.logger.Printf("[DEBUG] syslog-server: launching syslog server on addr: %v", l.Addr().String())
if err := e.configureLoggers(); err != nil {
return nil, err
}

e.syslogServer = logging.NewSyslogServer(l, e.syslogChan, e.logger)
go e.syslogServer.Start()
go e.collectLogs(e.lre, e.lro)
syslogAddr := fmt.Sprintf("%s://%s", l.Addr().Network(), l.Addr().String())
return &SyslogServerState{Addr: syslogAddr}, nil
}

func (e *UniversalExecutor) collectLogs(we io.Writer, wo io.Writer) {
for logParts := range e.syslogChan {
// If the severity of the log line is err then we write to stderr
// otherwise all messages go to stdout
if logParts.Severity == syslog.LOG_ERR {
e.lre.Write(logParts.Message)
e.lre.Write([]byte{'\n'})
} else {
e.lro.Write(logParts.Message)
e.lro.Write([]byte{'\n'})
}
}
}

// 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this comment

if e.cmd.SysProcAttr == nil {
e.cmd.SysProcAttr = &syscall.SysProcAttr{}
}
e.cmd.SysProcAttr.CreationFlags = syscall.CREATE_NEW_PROCESS_GROUP
return nil
}

// Cleanup any still hanging user processes
func (e *UniversalExecutor) cleanupChildProcesses(proc *os.Process) error {
// We must first verify if the process is still running.
// (Windows process often lingered around after being reported as killed).
handle, err := syscall.OpenProcess(syscall.PROCESS_TERMINATE|syscall.SYNCHRONIZE|syscall.PROCESS_QUERY_INFORMATION, false, uint32(proc.Pid))
if err != nil {
return os.NewSyscallError("OpenProcess", err)
}
defer syscall.CloseHandle(handle)

result, err := syscall.WaitForSingleObject(syscall.Handle(handle), 0)

switch result {
case syscall.WAIT_OBJECT_0:
return nil
case syscall.WAIT_TIMEOUT:
// Process still running. Just kill it.
return proc.Kill()
default:
return os.NewSyscallError("WaitForSingleObject", err)
}
}

// Send a Ctrl-Break signal for shutting down the process,
// like in https://golang.org/src/os/signal/signal_windows_test.go
func sendCtrlBreak(pid int) error {
dll, err := syscall.LoadDLL("kernel32.dll")
if err != nil {
return fmt.Errorf("Error loading kernel32.dll: %v", err)
}
proc, err := dll.FindProc("GenerateConsoleCtrlEvent")
if err != nil {
return fmt.Errorf("Cannot find procedure GenerateConsoleCtrlEvent: %v", err)
}
result, _, err := proc.Call(syscall.CTRL_BREAK_EVENT, uintptr(pid))
if result == 0 {
return fmt.Errorf("Error sending ctrl-break event: %v", err)
}
return nil
}

func (e *UniversalExecutor) shutdownProcess(proc *os.Process) error {
if err := sendCtrlBreak(proc.Pid); err != nil {
return fmt.Errorf("executor.shutdown error: %v", err)
}
e.logger.Printf("Sent Ctrl-Break to process %v", proc.Pid)

return nil
}