From: Tom Clegg <tom@curoverse.com>
Date: Sat, 7 Nov 2015 08:54:03 +0000 (-0500)
Subject: 5824: Fix Keep server shutdown, check errors, simplify stderr redirection.
X-Git-Tag: 1.1.0~1259^2~11
X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/1e10339657c1dee2b71b4d10eddffd0a35c949b3

5824: Fix Keep server shutdown, check errors, simplify stderr redirection.

(Oops, we forgot to actually Run() the python command for stop_keep.)
---

diff --git a/sdk/go/arvadostest/run_servers.go b/sdk/go/arvadostest/run_servers.go
index 27c552a4e1..c61b68b319 100644
--- a/sdk/go/arvadostest/run_servers.go
+++ b/sdk/go/arvadostest/run_servers.go
@@ -3,9 +3,6 @@ package arvadostest
 import (
 	"bufio"
 	"bytes"
-	"fmt"
-	"io"
-	"io/ioutil"
 	"log"
 	"os"
 	"os/exec"
@@ -68,24 +65,12 @@ func StartAPI() {
 	chdirToPythonTests()
 
 	cmd := exec.Command("python", "run_test_server.py", "start", "--auth", "admin")
-	stderr, err := cmd.StderrPipe()
-	if err != nil {
-		log.Fatal(err)
-	}
-	go io.Copy(os.Stderr, stderr)
-	stdout, err := cmd.StdoutPipe()
+	cmd.Stdin = nil
+	cmd.Stderr = os.Stderr
+
+	authScript, err := cmd.Output()
 	if err != nil {
-		log.Fatal(err)
-	}
-	if err = cmd.Start(); err != nil {
-		log.Fatal(err)
-	}
-	var authScript []byte
-	if authScript, err = ioutil.ReadAll(stdout); err != nil {
-		log.Fatal(err)
-	}
-	if err = cmd.Wait(); err != nil {
-		log.Fatal(err)
+		log.Fatalf("%+v: %s", cmd.Args, err)
 	}
 	ParseAuthSettings(authScript)
 	ResetEnv()
@@ -96,7 +81,7 @@ func StopAPI() {
 	defer os.Chdir(cwd)
 	chdirToPythonTests()
 
-	exec.Command("python", "run_test_server.py", "stop").Run()
+	bgRun(exec.Command("python", "run_test_server.py", "stop"))
 }
 
 // StartKeep starts the given number of keep servers,
@@ -112,16 +97,7 @@ func StartKeep(numKeepServers int, enforcePermissions bool) {
 		cmdArgs = append(cmdArgs, "--keep-enforce-permissions")
 	}
 
-	cmd := exec.Command("python", cmdArgs...)
-
-	stderr, err := cmd.StderrPipe()
-	if err != nil {
-		log.Fatalf("Setting up stderr pipe: %s", err)
-	}
-	go io.Copy(os.Stderr, stderr)
-	if err := cmd.Run(); err != nil {
-		panic(fmt.Sprintf("'python run_test_server.py start_keep' returned error %s", err))
-	}
+	bgRun(exec.Command("python", cmdArgs...))
 }
 
 // StopKeep stops keep servers that were started with StartKeep.
@@ -132,5 +108,27 @@ func StopKeep(numKeepServers int) {
 	defer os.Chdir(cwd)
 	chdirToPythonTests()
 
-	exec.Command("python", "run_test_server.py", "stop_keep", "--num-keep-servers", strconv.Itoa(numKeepServers))
+	cmd := exec.Command("python", "run_test_server.py", "stop_keep", "--num-keep-servers", strconv.Itoa(numKeepServers))
+	cmd.Stdin = nil
+	cmd.Stderr = os.Stderr
+	cmd.Stdout = os.Stderr
+	if err := cmd.Run(); err != nil {
+		log.Fatalf("%+v: %s", cmd.Args, err)
+	}
+}
+
+// Start cmd, with stderr and stdout redirected to our own
+// stderr. Return when the process exits, but do not wait for its
+// stderr and stdout to close: any grandchild processes will continue
+// writing to our stderr.
+func bgRun(cmd *exec.Cmd) {
+	cmd.Stdin = nil
+	cmd.Stderr = os.Stderr
+	cmd.Stdout = os.Stderr
+	if err := cmd.Start(); err != nil {
+		log.Fatalf("%+v: %s", cmd.Args, err)
+	}
+	if _, err := cmd.Process.Wait(); err != nil {
+		log.Fatalf("%+v: %s", cmd.Args, err)
+	}
 }
diff --git a/services/keepstore/pull_worker_integration_test.go b/services/keepstore/pull_worker_integration_test.go
index 3a3069ab77..52b59ec1ef 100644
--- a/services/keepstore/pull_worker_integration_test.go
+++ b/services/keepstore/pull_worker_integration_test.go
@@ -82,6 +82,8 @@ func TestPullWorkerIntegration_GetNonExistingLocator(t *testing.T) {
 	}
 
 	pullRequest := SetupPullWorkerIntegrationTest(t, testData, false)
+	defer arvadostest.StopAPI()
+	defer arvadostest.StopKeep(2)
 
 	performPullWorkerIntegrationTest(testData, pullRequest, t)
 }
@@ -97,6 +99,8 @@ func TestPullWorkerIntegration_GetExistingLocator(t *testing.T) {
 	}
 
 	pullRequest := SetupPullWorkerIntegrationTest(t, testData, true)
+	defer arvadostest.StopAPI()
+	defer arvadostest.StopKeep(2)
 
 	performPullWorkerIntegrationTest(testData, pullRequest, t)
 }
diff --git a/tools/keep-rsync/keep-rsync_test.go b/tools/keep-rsync/keep-rsync_test.go
index 9432a0d383..94281fa8bc 100644
--- a/tools/keep-rsync/keep-rsync_test.go
+++ b/tools/keep-rsync/keep-rsync_test.go
@@ -470,9 +470,11 @@ func (s *DoMainTestSuite) Test_doMainWithSrcAndDstConfig(c *C) {
 	args := []string{"-src", srcConfig.Name(), "-dst", dstConfig.Name()}
 	os.Args = append(os.Args, args...)
 
-	// Start keepservers. Since we are not doing any tweaking as in setupRsync func,
-	// kcSrc and kcDst will be the same and no actual copying to dst will happen, but that's ok.
+	// Start keepservers. Since we are not doing any tweaking as
+	// in setupRsync func, kcSrc and kcDst will be the same and no
+	// actual copying to dst will happen, but that's ok.
 	arvadostest.StartKeep(2, false)
+	defer arvadostest.StopKeep(2)
 
 	err := doMain()
 	c.Check(err, IsNil)