From e9244d1e9bfea35bbe904989f44dbed2dafb7d50 Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Tue, 6 Apr 2021 12:43:22 +0800 Subject: [PATCH] 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 --- go.mod | 3 +- go.sum | 7 +- pkg/binary/envoy/debug/node_test.go | 51 ++-- pkg/util/exec/exec.go | 50 ++-- pkg/util/exec/exec_suite_test.go | 27 -- pkg/util/exec/exec_test.go | 268 ++++++++++-------- pkg/util/exec/testdata/test_sigkill.sh | 21 -- .../{test_sigterm.sh => test_trap_sigterm.sh} | 4 + 8 files changed, 204 insertions(+), 227 deletions(-) delete mode 100644 pkg/util/exec/exec_suite_test.go delete mode 100755 pkg/util/exec/testdata/test_sigkill.sh rename pkg/util/exec/testdata/{test_sigterm.sh => test_trap_sigterm.sh} (91%) diff --git a/go.mod b/go.mod index 8fa594c0..8b0daa76 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index 2525e58d..c13a8bf7 100644 --- a/go.sum +++ b/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= diff --git a/pkg/binary/envoy/debug/node_test.go b/pkg/binary/envoy/debug/node_test.go index d725000d..9bb849e6 100644 --- a/pkg/binary/envoy/debug/node_test.go +++ b/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) } - }) + } } diff --git a/pkg/util/exec/exec.go b/pkg/util/exec/exec.go index 20989264..cf645fac 100644 --- a/pkg/util/exec/exec.go +++ b/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,31 +112,13 @@ 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 { @@ -143,11 +126,22 @@ func terminate(cmd *exec.Cmd, errCh <-chan error) { } } - // 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 } diff --git a/pkg/util/exec/exec_suite_test.go b/pkg/util/exec/exec_suite_test.go deleted file mode 100644 index 26e2832f..00000000 --- a/pkg/util/exec/exec_suite_test.go +++ /dev/null @@ -1,27 +0,0 @@ -// Copyright 2020 Tetrate -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package exec_test - -import ( - "testing" - - . "github.com/onsi/ginkgo" - . "github.com/onsi/gomega" -) - -func TestExec(t *testing.T) { - RegisterFailHandler(Fail) - RunSpecs(t, "Exec Suite") -} diff --git a/pkg/util/exec/exec_test.go b/pkg/util/exec/exec_test.go index 053f136a..6136f630 100644 --- a/pkg/util/exec/exec_test.go +++ b/pkg/util/exec/exec_test.go @@ -19,138 +19,166 @@ package exec import ( "bytes" "context" - stderrors "errors" "os" "os/exec" + "strings" "syscall" + "testing" "time" - . "github.com/onsi/ginkgo" - . "github.com/onsi/gomega" + "github.com/stretchr/testify/require" commonerrors "github.com/tetratelabs/getenvoy/pkg/errors" ioutil "github.com/tetratelabs/getenvoy/pkg/util/io" ) -var _ = Describe("Run()", func() { - - var stopCh chan os.Signal - var setupSignalHandlerBackup func(ctx context.Context) <-chan os.Signal - var killTimeoutBackup time.Duration - - BeforeEach(func() { - setupSignalHandlerBackup = setupSignalHandler - stopCh = make(chan os.Signal, 1) - setupSignalHandler = func(ctx context.Context) <-chan os.Signal { - return stopCh - } - killTimeoutBackup = killTimeout - killTimeout = 3 * time.Second - }) - - AfterEach(func() { - killTimeout = killTimeoutBackup - setupSignalHandler = setupSignalHandlerBackup - }) - - var stdin *bytes.Buffer - var stdout *bytes.Buffer - var stderr *bytes.Buffer - var stdio ioutil.StdStreams - - BeforeEach(func() { - stdin = new(bytes.Buffer) - stdout = new(bytes.Buffer) - stderr = new(bytes.Buffer) - stdio = ioutil.StdStreams{ - In: stdin, - Out: stdout, - Err: stderr, - } - }) - - It("should properly pipe standard I/O", func() { - c := exec.Command("testdata/test_stdio.sh", "0", "stderr") - stdin.WriteString("stdin\n") - - err := Run(c, stdio) - - Expect(err).ToNot(HaveOccurred()) - Expect(stdout.String()).To(Equal(`stdin`)) - Expect(stderr.String()).To(Equal(`stderr`)) - }) - - It("should return a meaningful error when a command cannot start", func() { - c := exec.Command("testdata/test_stdio.sh", "123") - c.Dir = "testdata" - - err := Run(c, stdio) - - Expect(err).To(HaveOccurred()) - Expect(err).To(MatchError(`failed to execute an external command "testdata/test_stdio.sh 123": ` + - `fork/exec testdata/test_stdio.sh: no such file or directory`)) - - var runErr *RunError - Expect(stderrors.As(err, &runErr)).To(BeTrue()) - Expect(runErr.Cmd()).To(Equal("testdata/test_stdio.sh 123")) - - var pathErr *os.PathError - Expect(stderrors.As(runErr.Cause(), &pathErr)).To(BeTrue()) - }) - - It("should return a meaningful error when a command exits with a non-0 code", func() { - c := exec.Command("testdata/test_stdio.sh", "123") - - err := Run(c, stdio) - - Expect(err).To(HaveOccurred()) - Expect(err).To(MatchError(`failed to execute an external command "testdata/test_stdio.sh 123": exit status 123`)) - - var runErr *RunError - Expect(stderrors.As(err, &runErr)).To(BeTrue()) - Expect(runErr.Cmd()).To(Equal("testdata/test_stdio.sh 123")) - - var exitErr *exec.ExitError - Expect(stderrors.As(runErr.Cause(), &exitErr)).To(BeTrue()) - }) - - It("on shutdown, should gracefully terminate (SIGTERM) a command", func() { - cmd := exec.Command("testdata/test_sigterm.sh", "10") - - errCh := make(chan error) - go func() { - defer close(errCh) - if err := Run(cmd, stdio); err != nil { - errCh <- err - } - }() - - stopCh <- syscall.SIGINT - close(stopCh) - - var err error - Eventually(errCh, "5s", "100ms").Should(Receive(&err)) - Expect(err).To(Equal(commonerrors.NewShutdownError(syscall.SIGINT))) - }) - - It("on shutdown, should forcefully kill (SIGKILL) a command if the latter didn't exit timely after receiving SIGTERM", func() { - cmd := exec.Command("testdata/test_sigkill.sh", "10") - - errCh := make(chan error) - go func() { - defer close(errCh) - if err := Run(cmd, stdio); err != nil { - errCh <- err +func TestRunPipesStdoutAndStderr(t *testing.T) { + stdin, stdout, stderr := new(bytes.Buffer), new(bytes.Buffer), new(bytes.Buffer) + stdin.WriteString("input to stdin\n") + + cmd := exec.Command("testdata/test_stdio.sh", "0", "stderr") + err := Run(cmd, ioutil.StdStreams{In: stdin, Out: stdout, Err: stderr}) + + require.NoError(t, err, `error running [%v]`, cmd) + require.Equal(t, `input to stdin`, stdout.String(), `invalid stdout running [%v]`, cmd) + require.Equal(t, `stderr`, stderr.String(), `invalid stderr running [%v]`, cmd) +} + +func TestRunErrorWrapsCause(t *testing.T) { + tests := []struct { + name string + path string + expectedErr string + expectedErrTarget error + }{ + { + name: "invalid path", + path: "/invalid/path", + expectedErr: `failed to execute an external command "/invalid/path 123": fork/exec /invalid/path: no such file or directory`, + expectedErrTarget: new(os.PathError), + }, + { + name: "exit status", + path: "testdata/test_stdio.sh", + expectedErr: `failed to execute an external command "testdata/test_stdio.sh 123": exit status 123`, + expectedErrTarget: new(exec.ExitError), + }, + } + + for _, test := range tests { + tt := test // pin! see https://github.com/kyoh86/scopelint for why + + t.Run(tt.name, func(t *testing.T) { + stdin, stdout, stderr := new(bytes.Buffer), new(bytes.Buffer), new(bytes.Buffer) + + cmd := exec.Command(tt.path, "123") + err := Run(cmd, ioutil.StdStreams{In: stdin, Out: stdout, Err: stderr}) + + // Verify the command failed with the expected error. + require.EqualError(t, err, tt.expectedErr, `expected an error running [%v]`, cmd) + + var runErr *RunError + require.ErrorAs(t, err, &runErr, `expected a RunError running [%v]`, cmd) + require.Equal(t, tt.path+" 123", runErr.Cmd(), `expected RunError.Cmd() to contain path and args`) + require.ErrorAs(t, runErr.Cause(), &tt.expectedErrTarget, `expected RunError.Cause() to wrap the original error`) + }) + } +} + +func TestRunShutdownError(t *testing.T) { + for _, s := range []os.Signal{syscall.SIGINT, syscall.SIGTERM} { + s := s // pin! see https://github.com/kyoh86/scopelint for why + + t.Run(s.String(), func(t *testing.T) { + cmd := exec.Command("sleep", "1") + + // Fake the actual signal handler. Signals sent to stopCh aren't actually sent to the process. + stopCh := make(chan os.Signal, 1) + revertSetupSignalHandler := overrideSetupSignalHandler(func(ctx context.Context) <-chan os.Signal { + return stopCh + }) + defer revertSetupSignalHandler() + + // Since we are only testing signals manifest in errors, kill quickly. + revertKillTimeout := overrideKillTimeout(1 * time.Millisecond) + defer revertKillTimeout() + + // We run the command in a goroutine as we need to signal the process, which results in an error. + errCh := make(chan error) + go func() { + defer close(errCh) + errCh <- Run(cmd, ioutil.StdStreams{In: new(bytes.Buffer), Out: new(bytes.Buffer), Err: new(bytes.Buffer)}) + }() + + stopCh <- s // Trigger exec.terminate() + close(stopCh) + + // Verify the process shutdown and raised an error due to the signal we caught + for err := range errCh { + require.Equal(t, commonerrors.NewShutdownError(s), err) } - }() + }) + } +} - Eventually(stdout.String).Should(Equal("running")) +func TestRunSendsSIGTERMIfProcessStillRunningAfterStopSignal(t *testing.T) { + stdout, stderr := new(bytes.Buffer), new(bytes.Buffer) + stdio := ioutil.StdStreams{In: new(bytes.Buffer), Out: stdout, Err: stderr} - stopCh <- syscall.SIGINT - close(stopCh) + // Sleep for 2 seconds, which should be enough to know that the SIGTERM was issued + cmd := exec.Command("testdata/test_trap_sigterm.sh", "2") - var err error - Eventually(errCh, "5s", "100ms").Should(Receive(&err)) - Expect(err).To(Equal(commonerrors.NewShutdownError(syscall.SIGINT))) + // Fake the actual signal handler. Signals sent to stopCh aren't actually sent to the process. + stopCh := make(chan os.Signal, 1) + revertSetupSignalHandler := overrideSetupSignalHandler(func(ctx context.Context) <-chan os.Signal { + return stopCh }) -}) + defer revertSetupSignalHandler() + revertKillTimeout := overrideKillTimeout(2 * time.Second) + defer revertKillTimeout() + + // We run the command in a goroutine as we need to SIGTERM the process, which results in an error. + errCh := make(chan error) + go func() { + errCh <- Run(cmd, stdio) + close(errCh) + }() + + // Wait for shell script to start + require.Eventually(t, func() bool { + return strings.Contains(stdout.String(), "running") + }, 1*time.Second, 10*time.Millisecond) + + // Send a fake signal to our signal handler which invokes the stop channel + fakeSignal := syscall.Signal(0) + stopCh <- fakeSignal + close(stopCh) + + // Wait for the err channel which means the process shutdown. + for err := range errCh { + require.Equal(t, commonerrors.NewShutdownError(fakeSignal), err) + } + + // Verify the program received a SIGTERM because the process was still alive upon exec.terminate() + require.Equal(t, "SIGTERM caught!\n", stderr.String(), `invalid stderr running [%v]`, cmd) +} + +// overrideSetupSignalHandler sets setupSignalHandler to a function. Doing so prevents the process from seeing signals. +// The function returned reverts the original value. +func overrideSetupSignalHandler(override func(ctx context.Context) <-chan os.Signal) func() { + previous := setupSignalHandler + setupSignalHandler = override + return func() { + setupSignalHandler = previous + } +} + +// overrideKillTimeout sets killTimeout to a short value as when we override setupSignalHandler, we need to kill faster. +// The function returned reverts the original value. +func overrideKillTimeout(override time.Duration) func() { + previous := killTimeout + killTimeout = override + return func() { + killTimeout = previous + } +} diff --git a/pkg/util/exec/testdata/test_sigkill.sh b/pkg/util/exec/testdata/test_sigkill.sh deleted file mode 100755 index c238c016..00000000 --- a/pkg/util/exec/testdata/test_sigkill.sh +++ /dev/null @@ -1,21 +0,0 @@ -#!/usr/bin/env bash - -# Copyright 2020 Tetrate -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - -printf >&1 '%s' "running" - -trap 'echo "SIGTERM caught!"' TERM - -sleep "$1" diff --git a/pkg/util/exec/testdata/test_sigterm.sh b/pkg/util/exec/testdata/test_trap_sigterm.sh similarity index 91% rename from pkg/util/exec/testdata/test_sigterm.sh rename to pkg/util/exec/testdata/test_trap_sigterm.sh index eae9623f..4f7b7bd6 100755 --- a/pkg/util/exec/testdata/test_sigterm.sh +++ b/pkg/util/exec/testdata/test_trap_sigterm.sh @@ -14,4 +14,8 @@ # See the License for the specific language governing permissions and # limitations under the License. +echo running + +trap 'echo >&2 "SIGTERM caught!"' TERM + sleep "$1"