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

Clean up subprocess handling and make shell use optional #3509

Merged
merged 20 commits into from Oct 4, 2017

Conversation

Projects
None yet
3 participants
@kyhavlov
Copy link
Member

commented Sep 27, 2017

This aims at fixing the issues in #2999 around Consul's handling of child processes spawned from the exec/lock/watch commands and checks/watches run by the consul agent.

kyhavlov added some commits Sep 27, 2017

@kyhavlov kyhavlov requested a review from slackpad Sep 27, 2017

@slackpad
Copy link
Contributor

left a comment

Made a quick pass through with some initial thoughts.

"os"
"os/exec"

"os/signal"

This comment has been minimized.

Copy link
@slackpad

slackpad Sep 28, 2017

Contributor

Sort up.

select {
case sig := <-signalCh:
if err := cmd.Process.Signal(sig); err != nil {
fmt.Println("Error relaying signal to subprocess: ", err)

This comment has been minimized.

Copy link
@slackpad

slackpad Sep 28, 2017

Contributor

Can this be fed a logger?

@@ -9,6 +9,8 @@ import (
"os"
"strconv"

"os/exec"

This comment has been minimized.

Copy link
@slackpad

slackpad Sep 28, 2017

Contributor

Sort up.

script, isScript := handler.(string)
args, isArgs := handler.([]string)
if isArgs {
script = fmt.Sprintf("%v", args)

This comment has been minimized.

Copy link
@slackpad

slackpad Sep 28, 2017

Contributor

This is nasty since we don't use this (and we super don't want to treat the args as a big string. Can you maybe always run the below code based on an args array, so there's less switching around, like this:

var args []string
switch h := hander.(type) {
case string:
    // figure out the shell and stuff from the environment
    args = []string{theShell, "-c", h...}
case []string:
    args = h
}

// then down below you just always use ExecSubprocess with the args local
@@ -101,6 +104,9 @@ func (c *LockCommand) run(args []string, lu **LockUnlock) int {
"is generated based on the provided child command.")
f.BoolVar(&passStdin, "pass-stdin", false,
"Pass stdin to the child process.")
f.BoolVar(&shell, "shell", false,

This comment has been minimized.

Copy link
@slackpad

slackpad Sep 28, 2017

Contributor

If we are going to move health checks away from using the shell (via having the user put the shell in themselves via args then we could omit this part of the change.

h := wp.Exempt["handler"]
if _, ok := h.(string); h == nil || !ok {
// Get the handler and subprocess arguments
handler, hasHandler := wp.Exempt["handler"]

This comment has been minimized.

Copy link
@slackpad

slackpad Sep 28, 2017

Contributor

How about issuing a deprecation warning for handler here and in health checks?

@kyhavlov kyhavlov force-pushed the subprocess-cleanup branch from b02415a to 7489a7e Sep 30, 2017

// If enabled, start a goroutine to relay signals to the subprocess and
// another to watch for its shutdown.
if echoSignals {
signalCh := make(chan os.Signal, 4)

This comment has been minimized.

Copy link
@preetapan

preetapan Sep 30, 2017

Member

How did you come up with 4 as the buffer size here?

Asking because the documentation on signal.Notify states that Package signal will not block sending to c: the caller must ensure that c has sufficient buffer space to keep up with the expected signal rate. 4 seemed like an oddly specific choice, e.g what If I sent the subprocess a bunch of SIGHUPs in quick succession?

This comment has been minimized.

Copy link
@kyhavlov

kyhavlov Sep 30, 2017

Author Member

I took the same value we use in listening for signals on the agent: https://github.com/hashicorp/consul/blob/master/command/agent.go#L366

This comment has been minimized.

Copy link
@preetapan

preetapan Oct 2, 2017

Member

That begs the question of why its set to 4 there as well.

I would set this buffer size to something like 10. Not that we can hypothesize anything about the incoming signal rate in how people would use this, but atleast 10 is conservatively large enough. If you are sending more than 10 signals at a time to the subprocess, you are probably doing something wrong.

With leaving it as 4, there is the possibility of missing a signal because Signal.Notify won't block when the buffer is full, and thus your go routine below will never see it.

@@ -3344,6 +3387,7 @@ func TestFullConfig(t *testing.T) {
}

// check the warnings
t.Log(b.Warnings)

This comment has been minimized.

Copy link
@slackpad

slackpad Oct 2, 2017

Contributor

Looks like some debug code got left around.

@@ -59,7 +60,7 @@ func (c *CheckType) IsTTL() bool {

// IsMonitor checks if this is a Monitor type
func (c *CheckType) IsMonitor() bool {
return c.Script != "" && c.DockerContainerID == "" && c.Interval != 0
return (c.Script != "" || len(c.ScriptArgs) > 0) && c.DockerContainerID == "" && c.Interval != 0

This comment has been minimized.

Copy link
@slackpad

slackpad Oct 2, 2017

Contributor

This should call IsScript() to be more clear / dedup.

// another to watch for its shutdown.
if echoSignals {
signalCh := make(chan os.Signal, 4)
shutdownCh := make(chan struct{}, 0)

This comment has been minimized.

Copy link
@slackpad

slackpad Oct 2, 2017

Contributor

I don't think you need the , 0 argument here.

func makeWatchHandler(logOutput io.Writer, handler interface{}) watch.HandlerFunc {
var args []string

// Figure out whether to run in shell or raw subprocess mode

This comment has been minimized.

Copy link
@slackpad

slackpad Oct 2, 2017

Contributor

Ugh I'm sorry I just realized that ExecScript has special stuff on Windows (there's util_windows.go). I know there are some open issues for Windows that will likely be cleaned up by just using args, so we should probably keep using ExecScript in this path as well until we just deprecate it, that way we don't break more than we have to. Sorry to flip back on this.

@@ -9,6 +9,8 @@ import (
"syscall"
"time"

"os/exec"

This comment has been minimized.

Copy link
@slackpad

slackpad Oct 2, 2017

Contributor

This should sort up into the previous list.

@@ -8,6 +8,8 @@ import (
"strconv"
"strings"

"os/exec"

This comment has been minimized.

Copy link
@slackpad

slackpad Oct 2, 2017

Contributor

This should sort up into the previous list.

kyhavlov and others added some commits Oct 4, 2017

Scopes signals to interrupt and kill.
This avoids us trying to send SIGCHILD to the dead process.
@@ -161,10 +168,15 @@ func (c *ExecCommand) Run(args []string) int {
return 1
}
c.conf.script = buf.Bytes()
} else {
if !c.conf.shell {

This comment has been minimized.

Copy link
@kyhavlov

kyhavlov Oct 4, 2017

Author Member

this can be else if !c.conf.shell { on line 171, I think

This comment has been minimized.

Copy link
@slackpad

slackpad Oct 4, 2017

Contributor

fixed

slackpad added some commits Oct 4, 2017

@slackpad slackpad merged commit 198ed60 into master Oct 4, 2017

0 of 2 checks passed

default Finished TeamCity Build Consul :: Consul OSS :: PR builds : Exit code 2
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@slackpad slackpad deleted the subprocess-cleanup branch Oct 4, 2017

HadarGreinsmark added a commit to HadarGreinsmark/consul that referenced this pull request Oct 19, 2017

Clean up subprocess handling and make shell use optional (hashicorp#3509
)

* Clean up handling of subprocesses and make using a shell optional

* Update docs for subprocess changes

* Fix tests for new subprocess behavior

* More cleanup of subprocesses

* Minor adjustments and cleanup for subprocess logic

* Makes the watch handler reload test use the new path.

* Adds check tests for new args path, and updates existing tests to use new path.

* Adds support for script args in Docker checks.

* Fixes the sanitize unit test.

* Adds panic for unknown watch type, and reverts back to Run().

* Adds shell option back to consul lock command.

* Adds shell option back to consul exec command.

* Adds shell back into consul watch command.

* Refactors signal forwarding and makes Windows-friendly.

* Adds a clarifying comment.

* Changes error wording to a warning.

* Scopes signals to interrupt and kill.

This avoids us trying to send SIGCHILD to the dead process.

* Adds an error for shell=false for consul exec.

* Adds notes about the deprecated script and handler fields.

* De-nests an if statement.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.