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,10 +32,11 @@ 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
+	gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b // indirect
 	gotest.tools v2.2.0+incompatible
 	istio.io/api v0.0.0-20200227213531-891bf31f3c32
 	istio.io/istio v0.0.0-20200304114959-c3c353285578

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=
@@ -1057,8 +1057,9 @@ gopkg.in/yaml.v2 v2.2.2/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=
 gopkg.in/yaml.v2 v2.2.4/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=
 gopkg.in/yaml.v2 v2.2.7 h1:VUgggvou5XRW9mHwD/yXxIYSMtY0zoKQf/v226p2nyo=
 gopkg.in/yaml.v2 v2.2.7/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=
-gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c h1:dUUwHk2QECo/6vqA44rthZ8ie2QXMNeKRTHCNY2nXvo=
 gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
+gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b h1:h8qDotaEPuJATrMmW04NCwg7v22aHH28wwpauUhK9Oo=
+gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
 gotest.tools v2.2.0+incompatible h1:VsBPFP1AI068pPrMxtb/S8Zkgf9xEmTLJjfM+P5UIEo=
 gotest.tools v2.2.0+incompatible/go.mod h1:DsYFclhRJ6vuDpmuTbkuFWG+y2sxOXAzmJt81HFBacw=
 honnef.co/go/tools v0.0.0-20180728063816-88497007e858/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4=

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
 }