From c7ad89d9eb1d9f5979eb2aede62de6f7175a15aa Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Mon, 13 Sep 2021 15:37:30 -0700 Subject: [PATCH 1/6] Add tests calling signal to func-e This is a follow-up for https://github.com/tetratelabs/func-e/pull/371#pullrequestreview-752322738 to test all possible signals to func-e. A minor change on checking the stderr: this uses scanner instead of peek on the buffer since for some reason redirecting cmd.Stderr to a buffer doesn't give complete stderr after the process is terminated. Hence here we use scanner to io.ReadCloser of cmd.StderrPipe() instead. Signed-off-by: Dhi Aurrahman --- internal/moreos/moreos_func-e_test.go | 167 +++++++++++++++--------- internal/moreos/testdata/fake_func-e.go | 6 +- 2 files changed, 113 insertions(+), 60 deletions(-) diff --git a/internal/moreos/moreos_func-e_test.go b/internal/moreos/moreos_func-e_test.go index 8604efef..9b056c3c 100644 --- a/internal/moreos/moreos_func-e_test.go +++ b/internal/moreos/moreos_func-e_test.go @@ -23,6 +23,7 @@ import ( "path/filepath" "runtime" "strings" + "syscall" "testing" "time" @@ -45,68 +46,105 @@ var ( moreosSrcDir embed.FS ) -// TestProcessGroupAttr_Kill tests sending SIGKILL to fake func-e. -// On linux, we propagate SIGKILL to the child process as the configured SysProcAttr.Pdeathsig -// in proc_linux.go. -func TestProcessGroupAttr_Kill(t *testing.T) { - // This works only for linux, sending kill -9 on darwin will not kill the process, we need to kill - // via pgid or kill the child first. - if runtime.GOOS != "linux" { - t.Skip() +// Test_CallSignal tests sending signals to fake func-e. +func Test_CallSignal(t *testing.T) { + type testCase struct { + name string + signal func(*os.Process) error + skip bool + waitForExiting bool } - tempDir := t.TempDir() - // Build a fake envoy and pass the ENV hint so that fake func-e uses it - fakeEnvoy := filepath.Join(tempDir, "envoy"+Exe) - fakebinary.RequireFakeEnvoy(t, fakeEnvoy) - t.Setenv("ENVOY_PATH", fakeEnvoy) - - fakeFuncE := filepath.Join(tempDir, "func-e"+Exe) - requireFakeFuncE(t, fakeFuncE) - - stderr := new(bytes.Buffer) - stdout := new(bytes.Buffer) - - // With an arg so fakeFuncE runs fakeEnvoy as its child and doesn't exit. - arg := string(version.LastKnownEnvoy) - cmd := exec.Command(fakeFuncE, "run", arg, "-c") - cmd.Stdout = stdout - cmd.Stderr = stderr - - require.NoError(t, cmd.Start()) - - // Block until we reach an expected line or timeout. - reader := bufio.NewReader(stderr) - waitFor := "initializing epoch 0" - require.Eventually(t, func() bool { - b, _ := reader.Peek(512) // the error value is always EOF. - return strings.HasPrefix(string(b), waitFor) - }, 5*time.Second, 100*time.Millisecond, "timeout waiting for stderr to contain %q", waitFor) - - require.Equal(t, Sprintf("starting: %s %s -c\n", fakeEnvoy, arg), stdout.String()) - - fakeFuncEProcess, err := process.NewProcess(int32(cmd.Process.Pid)) - require.NoError(t, err) - - // Get all fake func-e children processes. - children, err := fakeFuncEProcess.Children() - require.NoError(t, err) - require.Equal(t, len(children), 1) // Should have only one child process i.e. the fake envoy process. - fakeEnvoyProcess := &os.Process{Pid: int(children[0].Pid)} - - // Kill the fake func-e process. - require.NoError(t, cmd.Process.Kill()) - - // Wait for the process to die; this could error due to the kill signal. - cmd.Wait() //nolint - - // Wait and check if fake func-e and envoy processes are killed. - requireFindProcessError(t, cmd.Process, process.ErrorProcessNotRunning) - requireFindProcessError(t, fakeEnvoyProcess, process.ErrorProcessNotRunning) + tests := []testCase{ + { + name: "interrupt", + signal: Interrupt, + waitForExiting: true, + }, + { + name: "SIGTERM", + signal: func(proc *os.Process) error { return proc.Signal(syscall.SIGTERM) }, + waitForExiting: true, + // On windows os.Process.Signal is not implemented; it will return an error instead of sending + // a signal. + skip: runtime.GOOS == OSWindows, + }, + { + name: "kill", + // On linux, we propagate SIGKILL to the child process as the configured SysProcAttr.Pdeathsig + // in proc_linux.go. + signal: func(proc *os.Process) error { return proc.Kill() }, + waitForExiting: false, // since the process is killed, it is immediately exit. + // This works only for Linux, sending kill -9 on Darwin will not kill the process, we need to + // kill via pgid or kill the child first. + skip: runtime.GOOS != OSLinux, + }, + } - // Ensure both processes are killed. - require.NoError(t, EnsureProcessDone(cmd.Process)) - require.NoError(t, EnsureProcessDone(fakeEnvoyProcess)) + for _, tc := range tests { + tc := tc + t.Run(tc.name, func(t *testing.T) { + if tc.skip { + t.Skip() + } + + tempDir := t.TempDir() + + // Build a fake envoy and pass the ENV hint so that fake func-e uses it + fakeEnvoy := filepath.Join(tempDir, "envoy"+Exe) + fakebinary.RequireFakeEnvoy(t, fakeEnvoy) + t.Setenv("ENVOY_PATH", fakeEnvoy) + + fakeFuncE := filepath.Join(tempDir, "func-e"+Exe) + requireFakeFuncE(t, fakeFuncE) + + stdout := new(bytes.Buffer) + + // With an arg so fakeFuncE runs fakeEnvoy as its child and doesn't exit. + arg := string(version.LastKnownEnvoy) + cmd := exec.Command(fakeFuncE, "run", arg, "-c") + cmd.Stdout = stdout + + stderr, err := cmd.StderrPipe() + require.NoError(t, err) + stderrScanner := bufio.NewScanner(stderr) + + require.NoError(t, cmd.Start()) + + // Block until we reach an expected line or timeout. + requireScannedWaitFor(t, stderrScanner, "starting main dispatch loop") + require.Equal(t, Sprintf("starting: %s %s -c\n", fakeEnvoy, arg), stdout.String()) + + fakeFuncEProcess, err := process.NewProcess(int32(cmd.Process.Pid)) + require.NoError(t, err) + + // Get all fake func-e children processes. + children, err := fakeFuncEProcess.Children() + require.NoError(t, err) + require.Equal(t, len(children), 1) // Should have only one child process i.e. the fake envoy process. + fakeEnvoyProcess := &os.Process{Pid: int(children[0].Pid)} + + tc.signal(cmd.Process) + if tc.waitForExiting { + requireScannedWaitFor(t, stderrScanner, "exiting") + } + + // Wait for the process to die; this could error due to the kill signal. + err = cmd.Wait() + if tc.waitForExiting { + // When it is terminated using interrupt or SIGTERM, we expect cmd.Wait to be properly stopped. + require.NoError(t, err) + } + + // Wait and check if fake func-e and envoy processes are stopped. + requireFindProcessError(t, cmd.Process, process.ErrorProcessNotRunning) + requireFindProcessError(t, fakeEnvoyProcess, process.ErrorProcessNotRunning) + + // Ensure both processes are killed. + require.NoError(t, EnsureProcessDone(cmd.Process)) + require.NoError(t, EnsureProcessDone(fakeEnvoyProcess)) + }) + } } // requireFakeFuncE builds a func-e binary only depends on fakeFuncESrc and the sources in this package. @@ -136,3 +174,14 @@ func requireFindProcessError(t *testing.T, proc *os.Process, expectedErr error) return err == expectedErr }, 100*time.Millisecond, 5*time.Millisecond, "timeout waiting for expected error %v", expectedErr) } + +func requireScannedWaitFor(t *testing.T, s *bufio.Scanner, waitFor string) { + require.Eventually(t, func() bool { + for s.Scan() { + if strings.HasPrefix(s.Text(), waitFor) { + return true + } + } + return false + }, 5*time.Second, 100*time.Millisecond, "timeout waiting for scanner to find %q", waitFor) +} diff --git a/internal/moreos/testdata/fake_func-e.go b/internal/moreos/testdata/fake_func-e.go index ed54d354..7147f055 100644 --- a/internal/moreos/testdata/fake_func-e.go +++ b/internal/moreos/testdata/fake_func-e.go @@ -39,7 +39,7 @@ func main() { // Like envoy.Run. waitCtx, waitCancel := context.WithCancel(context.Background()) - sigCtx, _ := signal.NotifyContext(waitCtx, syscall.SIGINT, syscall.SIGTERM) + sigCtx, stop := signal.NotifyContext(waitCtx, os.Interrupt, syscall.SIGTERM) defer waitCancel() moreos.Fprintf(os.Stdout, "starting: %s\n", strings.Join(cmd.Args, " ")) //nolint @@ -56,6 +56,10 @@ func main() { // Block until we receive SIGINT or are canceled because Envoy has died. <-sigCtx.Done() + stop() + + // Simulate handleShutdown like in envoy.Run. + _ = moreos.Interrupt(cmd.Process) // Block until it exits to ensure file descriptors are closed prior to archival. // Allow up to 5 seconds for a clean stop, killing if it can't for any reason. From 6447499d771053e9265a97b74fc733c245f107de Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Mon, 13 Sep 2021 16:12:21 -0700 Subject: [PATCH 2/6] Fix comment Signed-off-by: Dhi Aurrahman --- internal/moreos/moreos_func-e_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/moreos/moreos_func-e_test.go b/internal/moreos/moreos_func-e_test.go index 9b056c3c..15e3a402 100644 --- a/internal/moreos/moreos_func-e_test.go +++ b/internal/moreos/moreos_func-e_test.go @@ -65,13 +65,13 @@ func Test_CallSignal(t *testing.T) { name: "SIGTERM", signal: func(proc *os.Process) error { return proc.Signal(syscall.SIGTERM) }, waitForExiting: true, - // On windows os.Process.Signal is not implemented; it will return an error instead of sending + // On Windows, os.Process.Signal is not implemented; it will return an error instead of sending // a signal. skip: runtime.GOOS == OSWindows, }, { name: "kill", - // On linux, we propagate SIGKILL to the child process as the configured SysProcAttr.Pdeathsig + // On Linux, we propagate SIGKILL to the child process as the configured SysProcAttr.Pdeathsig // in proc_linux.go. signal: func(proc *os.Process) error { return proc.Kill() }, waitForExiting: false, // since the process is killed, it is immediately exit. From 19782d2563e54adef1fad13911d5442a066d6556 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Mon, 13 Sep 2021 16:33:15 -0700 Subject: [PATCH 3/6] Skip calling interrupt on windows Signed-off-by: Dhi Aurrahman --- internal/moreos/moreos_func-e_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/moreos/moreos_func-e_test.go b/internal/moreos/moreos_func-e_test.go index 15e3a402..8c088fe0 100644 --- a/internal/moreos/moreos_func-e_test.go +++ b/internal/moreos/moreos_func-e_test.go @@ -60,6 +60,7 @@ func Test_CallSignal(t *testing.T) { name: "interrupt", signal: Interrupt, waitForExiting: true, + skip: runtime.GOOS == OSWindows, }, { name: "SIGTERM", From c891064940d6c489e300e23b0d38389bc1436ca4 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Mon, 13 Sep 2021 20:21:21 -0700 Subject: [PATCH 4/6] Try to enable windows Signed-off-by: Dhi Aurrahman --- internal/moreos/moreos_func-e_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/moreos/moreos_func-e_test.go b/internal/moreos/moreos_func-e_test.go index 8c088fe0..55294fe6 100644 --- a/internal/moreos/moreos_func-e_test.go +++ b/internal/moreos/moreos_func-e_test.go @@ -46,8 +46,8 @@ var ( moreosSrcDir embed.FS ) -// Test_CallSignal tests sending signals to fake func-e. -func Test_CallSignal(t *testing.T) { +// Test_CallSignals tests sending signals to fake func-e. +func Test_CallSignals(t *testing.T) { type testCase struct { name string signal func(*os.Process) error @@ -57,10 +57,9 @@ func Test_CallSignal(t *testing.T) { tests := []testCase{ { - name: "interrupt", + name: "Interrupt", signal: Interrupt, waitForExiting: true, - skip: runtime.GOOS == OSWindows, }, { name: "SIGTERM", @@ -71,7 +70,7 @@ func Test_CallSignal(t *testing.T) { skip: runtime.GOOS == OSWindows, }, { - name: "kill", + name: "Kill", // On Linux, we propagate SIGKILL to the child process as the configured SysProcAttr.Pdeathsig // in proc_linux.go. signal: func(proc *os.Process) error { return proc.Kill() }, @@ -104,6 +103,7 @@ func Test_CallSignal(t *testing.T) { // With an arg so fakeFuncE runs fakeEnvoy as its child and doesn't exit. arg := string(version.LastKnownEnvoy) cmd := exec.Command(fakeFuncE, "run", arg, "-c") + cmd.SysProcAttr = ProcessGroupAttr() // Make sure we have a new process group. cmd.Stdout = stdout stderr, err := cmd.StderrPipe() From 2885ed49cd52ed4e9febbb31fb15aee70b2debae Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Mon, 13 Sep 2021 20:50:30 -0700 Subject: [PATCH 5/6] Add comment Signed-off-by: Dhi Aurrahman --- internal/moreos/moreos_func-e_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/moreos/moreos_func-e_test.go b/internal/moreos/moreos_func-e_test.go index 55294fe6..6c2f1401 100644 --- a/internal/moreos/moreos_func-e_test.go +++ b/internal/moreos/moreos_func-e_test.go @@ -82,7 +82,8 @@ func Test_CallSignals(t *testing.T) { } for _, tc := range tests { - tc := tc + tc := tc // pin! see https://github.com/kyoh86/scopelint for why. + t.Run(tc.name, func(t *testing.T) { if tc.skip { t.Skip() From 78133d1e866e66eeb2b9f254cd19e2e1d8c4f599 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Tue, 14 Sep 2021 01:50:06 -0700 Subject: [PATCH 6/6] Review comments Signed-off-by: Dhi Aurrahman Co-authored-by: Takeshi Yoneda --- internal/moreos/moreos_func-e_test.go | 6 +++++- internal/moreos/testdata/fake_func-e.go | 5 +++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/internal/moreos/moreos_func-e_test.go b/internal/moreos/moreos_func-e_test.go index 6c2f1401..40e4ab2e 100644 --- a/internal/moreos/moreos_func-e_test.go +++ b/internal/moreos/moreos_func-e_test.go @@ -91,7 +91,7 @@ func Test_CallSignals(t *testing.T) { tempDir := t.TempDir() - // Build a fake envoy and pass the ENV hint so that fake func-e uses it + // Build a fake envoy and pass the path via via ENVOY_PATH so that fake func-e uses it. fakeEnvoy := filepath.Join(tempDir, "envoy"+Exe) fakebinary.RequireFakeEnvoy(t, fakeEnvoy) t.Setenv("ENVOY_PATH", fakeEnvoy) @@ -128,6 +128,10 @@ func Test_CallSignals(t *testing.T) { tc.signal(cmd.Process) if tc.waitForExiting { + // When we decide to wait for the fake envoy for exiting, we wait for "exiting" message + // from envoy (see: internal/test/fakebinary/testdata/fake_envoy.go#82) after receiving + // interrupt from func-e (the fake func-e forwards fake envoy's stderr. + // See: internal/moreos/testdata/fake_func-e.go#38). requireScannedWaitFor(t, stderrScanner, "exiting") } diff --git a/internal/moreos/testdata/fake_func-e.go b/internal/moreos/testdata/fake_func-e.go index 7147f055..c6773b18 100644 --- a/internal/moreos/testdata/fake_func-e.go +++ b/internal/moreos/testdata/fake_func-e.go @@ -39,9 +39,11 @@ func main() { // Like envoy.Run. waitCtx, waitCancel := context.WithCancel(context.Background()) - sigCtx, stop := signal.NotifyContext(waitCtx, os.Interrupt, syscall.SIGTERM) defer waitCancel() + sigCtx, stop := signal.NotifyContext(waitCtx, os.Interrupt, syscall.SIGTERM) + defer stop() + moreos.Fprintf(os.Stdout, "starting: %s\n", strings.Join(cmd.Args, " ")) //nolint if err := cmd.Start(); err != nil { moreos.Fprintf(os.Stderr, "error: unable to start Envoy process: %s\n", err) @@ -56,7 +58,6 @@ func main() { // Block until we receive SIGINT or are canceled because Envoy has died. <-sigCtx.Done() - stop() // Simulate handleShutdown like in envoy.Run. _ = moreos.Interrupt(cmd.Process)