Migrates exec tests to testing/testify and fixes exit condition

Before, we used an incorrect exit condition to determine if a process
still exists. Now, we look up the PID.

This converts test code to testing/testify per rationale in #130.
This upgrades testify to support error conditions.

Signed-off-by: Adrian Cole <adrian@tetrate.io>

go.mod

@@ -32,7 +32,7 @@ require (
 	github.com/shirou/gopsutil v0.0.0-20190731134726-d80c43f9c984
 	github.com/shurcooL/httpfs v0.0.0-20190707220628-8d4bc4ba7749
 	github.com/spf13/cobra v0.0.5
-	github.com/stretchr/testify v1.6.1
+	github.com/stretchr/testify v1.7.0
 	github.com/tetratelabs/getenvoy-package v0.0.0-20190730071641-da31aed4333e
 	github.com/tetratelabs/log v0.0.0-20190710134534-eb04d1e84fb8
 	github.com/tetratelabs/multierror v1.1.0

go.sum

@@ -738,8 +738,8 @@ github.com/stretchr/testify v0.0.0-20151208002404-e3a8ff8ce365/go.mod h1:a8OnRci
 github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=
 github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI=
 github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4=
-github.com/stretchr/testify v1.6.1 h1:hDPOHmpOpP40lSULcqw7IrRb/u7w6RpDC9399XyoNd0=
-github.com/stretchr/testify v1.6.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
+github.com/stretchr/testify v1.7.0 h1:nwc3DEeHmmLAfoZucVR881uASk0Mfjw8xYJ99tb5CcY=
+github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
 github.com/syndtr/gocapability v0.0.0-20170704070218-db04d3cc01c8/go.mod h1:hkRG7XYTFWNJGYcbNJQlaLq0fg1yr4J4t/NcTQtrfww=
 github.com/tetratelabs/getenvoy-package v0.0.0-20190730071641-da31aed4333e h1:P03rR9jXeD2ilkYVsRlDz/EyS2rTg5Nko8T/LzPri5k=
 github.com/tetratelabs/getenvoy-package v0.0.0-20190730071641-da31aed4333e/go.mod h1:IrGJ2YZz3+qia1dEU9GFg3QieuMUUwiJf5tjG8uhogg=

pkg/binary/envoy/debug/node_test.go

@@ -29,32 +29,29 @@ import (
 	"github.com/tetratelabs/getenvoy/pkg/binary/envoytest"
 )
 
-func Test_debugging_outputs(t *testing.T) {
-	t.Run("creates non-empty files", func(t *testing.T) {
-		r, _ := envoy.NewRuntime(EnableNodeCollection)
-		defer os.RemoveAll(r.DebugStore() + ".tar.gz")
-		defer os.RemoveAll(r.DebugStore())
-		envoytest.RunKill(r, filepath.Join("testdata", "null.yaml"), time.Second*10)
-
-		files := [...]string{"node/ps.txt", "node/network_interface.json", "node/connections.json"}
-		for _, file := range files {
-			path := filepath.Join(r.DebugStore(), file)
-			f, err := os.Stat(path)
-			require.NoError(t, err, "error stating %v", path)
-			if f.Size() < 1 {
-				t.Errorf("file %v was empty", path)
-			}
-			if strings.HasSuffix(file, ".json") {
-				raw, err := ioutil.ReadFile(path)
-				require.NoError(t, err, "error to read the file %v", path)
-				var is []interface{}
-				if err := json.Unmarshal(raw, &is); err != nil {
-					t.Errorf("error to unmarshal json string, %v: \"%v\"", err, raw)
-				}
-				if len(is) < 1 {
-					t.Errorf("unmarshaled content is empty, expected to be a non-empty array: \"%v\"", raw)
-				}
-			}
+func TestDebugCreatesNonEmptyFiles(t *testing.T) {
+	r, err := envoy.NewRuntime(EnableNodeCollection)
+	require.NoError(t, err, "error creating envoy runtime")
+	defer os.RemoveAll(r.DebugStore())
+
+	err = envoytest.RunKill(r, filepath.Join("testdata", "null.yaml"), time.Second*10)
+	require.NoError(t, err, "error running envoy")
+
+	files := [...]string{"node/ps.txt", "node/network_interface.json", "node/connections.json"}
+	for _, file := range files {
+		path := filepath.Join(r.DebugStore(), file)
+		f, err := os.Stat(path)
+		require.NoError(t, err, "error stating %v", path)
+		require.NotEmpty(t, f.Size(), "file %v was empty", path)
+
+		if strings.HasSuffix(file, ".json") {
+			raw, err := ioutil.ReadFile(path)
+			require.NoError(t, err, "error to read the file %v", path)
+			var is []interface{}
+
+			err = json.Unmarshal(raw, &is)
+			require.NoError(t, err, "error to unmarshal json string, %v: \"%v\"", err, raw)
+			require.NotEmpty(t, len(is), "unmarshaled content is empty, expected to be a non-empty array: \"%v\"", raw)
 		}
-	})
+	}
 }

pkg/util/exec/exec.go

@@ -17,6 +17,7 @@ package exec
 import (
 	"context"
 	"fmt"
+	"os"
 	"os/exec"
 	"syscall"
 	"time"
@@ -89,7 +90,7 @@ func Run(cmd *exec.Cmd, streams ioutil.StdStreams) error {
 		return newRunError(cmd, err)
 	}
 
-	// use a dedicated gorutine to wait for the command to complete
+	// use a dedicated goroutine to wait for the command to complete
 	errCh := make(chan error)
 	go func() {
 		defer close(errCh)
@@ -98,7 +99,7 @@ func Run(cmd *exec.Cmd, streams ioutil.StdStreams) error {
 		}
 	}()
 
-	// setup a cancellable handler for stop signals
+	// setup a cancelable handler for stop signals
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 	stopCh := setupSignalHandler(ctx)
@@ -111,43 +112,36 @@ func Run(cmd *exec.Cmd, streams ioutil.StdStreams) error {
 		}
 		return nil
 	case sig := <-stopCh:
-		terminate(cmd, errCh)
+		terminate(cmd)
 		return commonerrors.NewShutdownError(sig)
 	}
 }
 
-func terminate(cmd *exec.Cmd, errCh <-chan error) {
-	exited := func() bool {
-		select {
-		case <-errCh:
-			return true
-		default:
-			return false
-		}
-	}
-
-	defer func() {
-		// if the external process didn't exit gracefully, kill it
-		if !exited() {
-			if e := cmd.Process.Kill(); e != nil {
-				log.Warnf("failed to send SIGKILL to the external process %q: %v", cmd, e)
-			}
-		}
-	}()
-
-	if !exited() {
+func terminate(cmd *exec.Cmd) {
+	if isProcessRunning(cmd.Process.Pid) {
 		// first, give the external process a chance to exit gracefully,
 		// which is a must in case of `docker run` command.
 		if err := cmd.Process.Signal(syscall.SIGTERM); err != nil {
 			log.Warnf("failed to send SIGTERM to the external process %q: %v", cmd, err)
 		}
 	}
 
-	// wait for the external process to exit or a timeout to expire
-	select {
-	case <-errCh:
-		// don't warn about errors in response to SIGTERM
-	case <-time.After(killTimeout):
+	<-time.After(killTimeout)
+	if isProcessRunning(cmd.Process.Pid) {
 		log.Warnf("external process didn't exit gracefully within %s: %q", killTimeout, cmd)
+		if e := cmd.Process.Kill(); e != nil {
+			log.Warnf("failed to send SIGKILL to the external process %q: %v", cmd, e)
+		}
+	}
+	_ = cmd.Process.Release()
+}
+
+// Same as cgi.isProcessRunning.
+// See https://github.com/golang/go/issues/34396 for tracking long term solution.
+func isProcessRunning(pid int) bool {
+	p, err := os.FindProcess(pid)
+	if err != nil {
+		return false
 	}
+	return p.Signal(syscall.Signal(0)) == nil
 }