From 264d619457b73ec0877847df5a3d50fc0e91446f Mon Sep 17 00:00:00 2001 From: Soren Hansen Date: Wed, 17 Sep 2014 21:17:51 +0200 Subject: [PATCH 1/4] Pass exitCode by reference Arguments to defer statements are evaluated when the defer statement is evaluated, so pass exitCode by reference instead. Fixes issue #346 --- command/agent/remote_exec.go | 8 ++-- command/agent/remote_exec_test.go | 61 ++++++++++++++++++++++++++++++- 2 files changed, 64 insertions(+), 5 deletions(-) diff --git a/command/agent/remote_exec.go b/command/agent/remote_exec.go index 64fec4599a..e5022df9e0 100644 --- a/command/agent/remote_exec.go +++ b/command/agent/remote_exec.go @@ -137,7 +137,7 @@ func (a *Agent) handleRemoteExec(msg *UserEvent) { // Ensure we write out an exit code exitCode := 0 - defer a.remoteExecWriteExitCode(&event, exitCode) + defer a.remoteExecWriteExitCode(&event, &exitCode) // Check if this is a script, we may need to spill to disk var script string @@ -190,7 +190,7 @@ func (a *Agent) handleRemoteExec(msg *UserEvent) { err := cmd.Wait() writer.Flush() close(writer.BufCh) - if err != nil { + if err == nil { exitCh <- 0 return } @@ -287,8 +287,8 @@ func (a *Agent) remoteExecWriteOutput(event *remoteExecEvent, num int, output [] } // remoteExecWriteExitCode is used to write an exit code -func (a *Agent) remoteExecWriteExitCode(event *remoteExecEvent, exitCode int) bool { - val := []byte(strconv.FormatInt(int64(exitCode), 10)) +func (a *Agent) remoteExecWriteExitCode(event *remoteExecEvent, exitCode *int) bool { + val := []byte(strconv.FormatInt(int64(*exitCode), 10)) if err := a.remoteExecWriteKey(event, remoteExecExitSuffix, val); err != nil { a.logger.Printf("[ERR] agent: failed to write exit code for remote exec job: %v", err) return false diff --git a/command/agent/remote_exec_test.go b/command/agent/remote_exec_test.go index d431b1f809..2e49184cec 100644 --- a/command/agent/remote_exec_test.go +++ b/command/agent/remote_exec_test.go @@ -140,7 +140,8 @@ func TestRemoteExecWrites(t *testing.T) { t.Fatalf("bad") } - if !agent.remoteExecWriteExitCode(event, 1) { + exitCode := 1 + if !agent.remoteExecWriteExitCode(event, &exitCode) { t.Fatalf("bad") } @@ -227,6 +228,64 @@ func TestHandleRemoteExec(t *testing.T) { } } +func TestHandleRemoteExecFailed(t *testing.T) { + dir, agent := makeAgent(t, nextConfig()) + defer os.RemoveAll(dir) + defer agent.Shutdown() + testutil.WaitForLeader(t, agent.RPC, "dc1") + + event := &remoteExecEvent{ + Prefix: "_rexec", + Session: makeRexecSession(t, agent), + } + defer destroySession(t, agent, event.Session) + + spec := &remoteExecSpec{ + Command: "echo failing;exit 2", + Wait: time.Second, + } + buf, err := json.Marshal(spec) + if err != nil { + t.Fatalf("err: %v", err) + } + key := "_rexec/" + event.Session + "/job" + setKV(t, agent, key, buf) + + buf, err = json.Marshal(event) + if err != nil { + t.Fatalf("err: %v", err) + } + msg := &UserEvent{ + ID: generateUUID(), + Payload: buf, + } + + // Handle the event... + agent.handleRemoteExec(msg) + + // Verify we have an ack + key = "_rexec/" + event.Session + "/" + agent.config.NodeName + "/ack" + d := getKV(t, agent, key) + if d == nil || d.Session != event.Session { + t.Fatalf("bad ack: %#v", d) + } + + // Verify we have output + key = "_rexec/" + event.Session + "/" + agent.config.NodeName + "/out/00000" + d = getKV(t, agent, key) + if d == nil || d.Session != event.Session || + !bytes.Contains(d.Value, []byte("failing")) { + t.Fatalf("bad output: %#v", d) + } + + // Verify we have an exit code + key = "_rexec/" + event.Session + "/" + agent.config.NodeName + "/exit" + d = getKV(t, agent, key) + if d == nil || d.Session != event.Session || string(d.Value) != "2" { + t.Fatalf("bad output: %#v", d) + } +} + func makeRexecSession(t *testing.T, agent *Agent) string { args := structs.SessionRequest{ Datacenter: agent.config.Datacenter, From 8f5089a6613253b55d60b2a060ef79a17b91ecc0 Mon Sep 17 00:00:00 2001 From: Soren Hansen Date: Wed, 17 Sep 2014 21:33:03 +0200 Subject: [PATCH 2/4] Remove test code duplication --- command/agent/remote_exec_test.go | 70 +++++-------------------------- 1 file changed, 11 insertions(+), 59 deletions(-) diff --git a/command/agent/remote_exec_test.go b/command/agent/remote_exec_test.go index 2e49184cec..d91f6c2f24 100644 --- a/command/agent/remote_exec_test.go +++ b/command/agent/remote_exec_test.go @@ -170,7 +170,9 @@ func TestRemoteExecWrites(t *testing.T) { } } -func TestHandleRemoteExec(t *testing.T) { + + +func _TestHandleRemoteExec(t *testing.T, command string, expectedSubstring string, expectedReturnCode string) { dir, agent := makeAgent(t, nextConfig()) defer os.RemoveAll(dir) defer agent.Shutdown() @@ -183,7 +185,7 @@ func TestHandleRemoteExec(t *testing.T) { defer destroySession(t, agent, event.Session) spec := &remoteExecSpec{ - Command: "uptime", + Command: command, Wait: time.Second, } buf, err := json.Marshal(spec) @@ -216,74 +218,24 @@ func TestHandleRemoteExec(t *testing.T) { key = "_rexec/" + event.Session + "/" + agent.config.NodeName + "/out/00000" d = getKV(t, agent, key) if d == nil || d.Session != event.Session || - !bytes.Contains(d.Value, []byte("load")) { + !bytes.Contains(d.Value, []byte(expectedSubstring)) { t.Fatalf("bad output: %#v", d) } // Verify we have an exit code key = "_rexec/" + event.Session + "/" + agent.config.NodeName + "/exit" d = getKV(t, agent, key) - if d == nil || d.Session != event.Session || string(d.Value) != "0" { + if d == nil || d.Session != event.Session || string(d.Value) != expectedReturnCode { t.Fatalf("bad output: %#v", d) } } +func TestHandleRemoteExec(t *testing.T) { + _TestHandleRemoteExec(t, "uptime", "load", "0") +} + func TestHandleRemoteExecFailed(t *testing.T) { - dir, agent := makeAgent(t, nextConfig()) - defer os.RemoveAll(dir) - defer agent.Shutdown() - testutil.WaitForLeader(t, agent.RPC, "dc1") - - event := &remoteExecEvent{ - Prefix: "_rexec", - Session: makeRexecSession(t, agent), - } - defer destroySession(t, agent, event.Session) - - spec := &remoteExecSpec{ - Command: "echo failing;exit 2", - Wait: time.Second, - } - buf, err := json.Marshal(spec) - if err != nil { - t.Fatalf("err: %v", err) - } - key := "_rexec/" + event.Session + "/job" - setKV(t, agent, key, buf) - - buf, err = json.Marshal(event) - if err != nil { - t.Fatalf("err: %v", err) - } - msg := &UserEvent{ - ID: generateUUID(), - Payload: buf, - } - - // Handle the event... - agent.handleRemoteExec(msg) - - // Verify we have an ack - key = "_rexec/" + event.Session + "/" + agent.config.NodeName + "/ack" - d := getKV(t, agent, key) - if d == nil || d.Session != event.Session { - t.Fatalf("bad ack: %#v", d) - } - - // Verify we have output - key = "_rexec/" + event.Session + "/" + agent.config.NodeName + "/out/00000" - d = getKV(t, agent, key) - if d == nil || d.Session != event.Session || - !bytes.Contains(d.Value, []byte("failing")) { - t.Fatalf("bad output: %#v", d) - } - - // Verify we have an exit code - key = "_rexec/" + event.Session + "/" + agent.config.NodeName + "/exit" - d = getKV(t, agent, key) - if d == nil || d.Session != event.Session || string(d.Value) != "2" { - t.Fatalf("bad output: %#v", d) - } + _TestHandleRemoteExec(t, "echo failing;exit 2", "failing", "2") } func makeRexecSession(t *testing.T, agent *Agent) string { From 8605488598e0ffcd193d6798f0d68a92de22c6a9 Mon Sep 17 00:00:00 2001 From: Soren Hansen Date: Thu, 18 Sep 2014 11:04:20 +0200 Subject: [PATCH 3/4] Fix style issue in remote_exec_test --- command/agent/remote_exec_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command/agent/remote_exec_test.go b/command/agent/remote_exec_test.go index d91f6c2f24..b45ce0657d 100644 --- a/command/agent/remote_exec_test.go +++ b/command/agent/remote_exec_test.go @@ -172,7 +172,7 @@ func TestRemoteExecWrites(t *testing.T) { -func _TestHandleRemoteExec(t *testing.T, command string, expectedSubstring string, expectedReturnCode string) { +func testHandleRemoteExec(t *testing.T, command string, expectedSubstring string, expectedReturnCode string) { dir, agent := makeAgent(t, nextConfig()) defer os.RemoveAll(dir) defer agent.Shutdown() From caedef5cc1d0219f464a97b5efd4cb6630e537c7 Mon Sep 17 00:00:00 2001 From: Soren Hansen Date: Thu, 18 Sep 2014 12:55:09 +0200 Subject: [PATCH 4/4] Also change the call sites. *sigh* --- command/agent/remote_exec_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/command/agent/remote_exec_test.go b/command/agent/remote_exec_test.go index b45ce0657d..99722e6a02 100644 --- a/command/agent/remote_exec_test.go +++ b/command/agent/remote_exec_test.go @@ -231,11 +231,11 @@ func testHandleRemoteExec(t *testing.T, command string, expectedSubstring string } func TestHandleRemoteExec(t *testing.T) { - _TestHandleRemoteExec(t, "uptime", "load", "0") + testHandleRemoteExec(t, "uptime", "load", "0") } func TestHandleRemoteExecFailed(t *testing.T) { - _TestHandleRemoteExec(t, "echo failing;exit 2", "failing", "2") + testHandleRemoteExec(t, "echo failing;exit 2", "failing", "2") } func makeRexecSession(t *testing.T, agent *Agent) string {