Clean up subprocess handling and make shell use optional #3509

kyhavlov · 2017-09-27T23:49:50Z

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.

slackpad

Made a quick pass through with some initial thoughts.

slackpad · 2017-09-28T21:14:34Z

agent/util_other.go

 	"os"
 	"os/exec"
+
+	"os/signal"


slackpad · 2017-09-28T21:15:35Z

agent/util_other.go

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


Can this be fed a logger?

slackpad · 2017-09-28T21:15:48Z

agent/watch_handler.go

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

+	"os/exec"


slackpad · 2017-09-28T21:18:42Z

agent/watch_handler.go

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


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

slackpad · 2017-09-28T21:21:27Z

command/lock.go

@@ -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,


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.

slackpad · 2017-09-28T21:22:34Z

agent/agent.go

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


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

preetapan · 2017-09-30T14:35:06Z

agent/util_other.go

+	// 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)


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?

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

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.

slackpad · 2017-10-02T20:18:26Z

agent/config/runtime_test.go

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

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


Looks like some debug code got left around.

slackpad · 2017-10-02T20:19:24Z

agent/structs/check_type.go

@@ -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 should call IsScript() to be more clear / dedup.

slackpad · 2017-10-02T20:22:04Z

agent/util_other.go

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


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

slackpad · 2017-10-02T20:27:08Z

agent/watch_handler.go

+func makeWatchHandler(logOutput io.Writer, handler interface{}) watch.HandlerFunc {
+	var args []string
+
+	// Figure out whether to run in shell or raw subprocess mode


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.

slackpad · 2017-10-02T20:27:44Z

command/lock.go

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

+	"os/exec"


This should sort up into the previous list.

slackpad · 2017-10-02T20:28:24Z

command/watch.go

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

+	"os/exec"


This should sort up into the previous list.

… new path.

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

kyhavlov · 2017-10-04T23:18:25Z

command/exec.go

@@ -161,10 +168,15 @@ func (c *ExecCommand) Run(args []string) int {
 			return 1
 		}
 		c.conf.script = buf.Bytes()
+	} else {
+		if !c.conf.shell {


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

kyhavlov added 2 commits September 27, 2017 16:46

Clean up handling of subprocesses and make using a shell optional

4248a34

Update docs for subprocess changes

e187c29

kyhavlov requested a review from slackpad September 27, 2017 23:49

Fix tests for new subprocess behavior

ac94e07

slackpad reviewed Sep 28, 2017

View reviewed changes

More cleanup of subprocesses

7489a7e

kyhavlov force-pushed the subprocess-cleanup branch from b02415a to 7489a7e Compare September 30, 2017 02:06

preetapan reviewed Sep 30, 2017

View reviewed changes

slackpad reviewed Oct 2, 2017

View reviewed changes

preetapan mentioned this pull request Oct 3, 2017

Implement HTTP Watch handler #3413

Merged

kyhavlov and others added 14 commits October 4, 2017 10:58

Minor adjustments and cleanup for subprocess logic

4343054

Makes the watch handler reload test use the new path.

dba2384

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

0c9f171

… new path.

Adds support for script args in Docker checks.

adf851d

Fixes the sanitize unit test.

08c23ea

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

f7f0b64

Adds shell option back to consul lock command.

0bc10d1

Adds shell option back to consul exec command.

262cb43

Adds shell back into consul watch command.

d6f33f9

Refactors signal forwarding and makes Windows-friendly.

af34efc

Adds a clarifying comment.

12c2d71

Changes error wording to a warning.

9373824

Scopes signals to interrupt and kill.

f343900

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

Adds an error for shell=false for consul exec.

c0bf614

kyhavlov commented Oct 4, 2017

View reviewed changes

slackpad added 2 commits October 4, 2017 16:39

Adds notes about the deprecated script and handler fields.

073ccc0

De-nests an if statement.

1051b46

slackpad merged commit 198ed60 into master Oct 4, 2017

slackpad deleted the subprocess-cleanup branch October 4, 2017 23:48

slackpad mentioned this pull request Oct 4, 2017

"consul watch" CLI command discards argv boundaries #3446

Closed

slackpad mentioned this pull request Oct 5, 2017

stdin for consul exec doesn't work on windows #1924

Closed

slackpad mentioned this pull request Oct 20, 2017

Incorrect babysitting of processes for Healthcheck with script #3597

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up subprocess handling and make shell use optional #3509

Clean up subprocess handling and make shell use optional #3509

kyhavlov commented Sep 27, 2017

slackpad left a comment

slackpad Sep 28, 2017

slackpad Sep 28, 2017

slackpad Sep 28, 2017

slackpad Sep 28, 2017

slackpad Sep 28, 2017

slackpad Sep 28, 2017

preetapan Sep 30, 2017

kyhavlov Sep 30, 2017

preetapan Oct 2, 2017

slackpad Oct 2, 2017

slackpad Oct 2, 2017

slackpad Oct 2, 2017

slackpad Oct 2, 2017

slackpad Oct 2, 2017

slackpad Oct 2, 2017

kyhavlov Oct 4, 2017

slackpad Oct 4, 2017

Clean up subprocess handling and make shell use optional #3509

Clean up subprocess handling and make shell use optional #3509

Conversation

kyhavlov commented Sep 27, 2017

slackpad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment