Add fakebinary for acommodating testing the child process management #371

dio · 2021-09-11T08:44:20Z

This adds the internal/test/fakebinary package allowing us to test running a fake func-e process that spawns a fake envoy. This allows us to isolate the process control angle and can lead to quicker diagnosis of platform-specific behaviors, as well as show potential for future process abstraction. For example: to check the behavior of func-e when we send SIGKILL to it (in Linux, we need to make sure the child dies and receive whatever signal we have configured in SysProcAttr.Pdeathsig).

We changed the setting for SysProcAttr.Pdeathsig to SIGKILL since when anyone sends SIGKILL on Linux to func-e, we want the spawned envoy to receive SIGKILL as well. Reference: #371 (comment).

This also abstracts away the process for building any fake binary that optionally uses some of func-e internal packages. For example, in this change, we can isolate the build for fake func-e to use internal/moreos package (to check on moreos.ProcessGroupAttr()) only and no more than that what it requires.

Signed-off-by: Dhi Aurrahman dio@rockybars.com

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

codefromthecrypt

Thanks for narrowing the scope and giving another stab. The end of the test looks good.

I have a more preferred approach to isolation of the incidental binary, ex so that it doesn't end up depending on anything but moreos. I also prefer to be more complete in how we launch processes as there are a couple key utilities used by envoy.Run. I made a more comprehensive sketch here, which will replace most of the code except the bottom of the test you wrote. https://github.com/tetratelabs/func-e/compare/fake-deux

Note: It took a while for me to figure out why things were moving around. While your description says the test case desired in text, and that's good, we should try to make notes either in the description about other structural change and why or self-PR comments. This helps explain the incidental change and why, in this case around needing to get a relative path to the module (the reason is that the fake source is different than fake envoy as it is importing a fully-qualified go import). When change is explained, it can help others understand the why vs have them guessing or figuring it out from scratch in a review out of timezone.

internal/moreos/test/moreos_linux_test.go

internal/test/fake_binary.go

internal/test/testdata/fake_func-e/fake_func-e.go

internal/test/func-e.go

dio · 2021-09-12T23:49:34Z

@codefromthecrypt do you want me to continue this or you to create a PR from your branch? Sorry, it is not clear since as you said: "I made a more comprehensive sketch here, which will replace most of the code except the bottom of the test you wrote." So I feel like I'll just copy-and-paste yours and add that tiny bit of change which I'm not sure why I want to do that 😅.

codefromthecrypt

main thing I meant was that retain almost all "moreos_linux_test.go" except the setup which is different per the other branch. Feel free to continue that branch and raise a PR out of it or force push this one and delete the other.

It would be nice to not increment notify watchers with new issue numbers as much as we have been, so I prefer to not raise yet another PR on the same topic. IOTW I prefer to rework this PR vs drop another PR and create another one on the same topic. That's why I didn't raise a PR on the other branch.

Signed-off-by: Dhi Aurrahman <dio@rockybars.com> Co-authored-by: Adrian Cole <adrian@tetrate.io>

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

internal/moreos/moreos_func-e_linux_test.go

dio · 2021-09-13T02:58:58Z

@codefromthecrypt when you have time, please take a look. Mostly I migrated everything you have done to this branch, so mostly you have seen all of these before 😅.

internal/moreos/moreos_func-e_test.go

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

codefromthecrypt

Looking good. some minor concerns when addressed, please merge!

A follow-up would be to matrix this test and show the behavior on TERM vs KILL. That can be a follow-up PR actually.

internal/moreos/moreos_func-e_linux_test.go

codefromthecrypt · 2021-09-13T05:27:38Z

internal/moreos/moreos_func-e_linux_test.go

+
+// TestProcessGroupAttr_Kill tests sending SIGKILL to fake func-e and makes sure fake envoy receives
+// SIGTERM (as defined in Pdeathsig).
+func TestProcessGroupAttr_Kill(t *testing.T) {


I prefer to use a condition on runtime.GOOS vs _linux because myself and others might refactor something and it will break compilation only on CI.

So, basically handle this in a t.Skip() instead of build condition please? We can do this because there are no linux-specific code called here. When doing the skip, do note why it won't work in darwin or windows in case we can follow-up.

internal/moreos/moreos_func-e_linux_test.go

codefromthecrypt · 2021-09-13T05:41:03Z

@mathetake if you have some other notes of interest, feel free to review also. Otherwise, with things already said LGTM!

internal/moreos/moreos_func-e_test.go

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

internal/moreos/moreos_func-e_test.go

mathetake · 2021-09-13T06:38:40Z

internal/moreos/testdata/fake_func-e.go

@@ -0,0 +1,73 @@
+package main


nit I would like to name this to fake_func_e.go following the Go convention 😄

I have a mixed feeling about this since func-e is a name. I will defer this to @codefromthecrypt 🙏🏽.

internal/moreos/moreos_func-e_test.go

Signed-off-by: Dhi Aurrahman <dio@rockybars.com> Co-authored-by: Takeshi Yoneda <takeshi@tetrate.io>

This is a follow-up for #371 (review) 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 <dio@rockybars.com>

This is a follow-up for #371 (review) to test a set of possible signals (`SIGINT`, `SIGTERM`, `SIGKILL`) to terminate `func-e`. A minor change on checking the stderr: this PR uses a scanner instead of peeking 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 a scanner to `io.ReadCloser` returned by `cmd.StderrPipe()` instead. Signed-off-by: Dhi Aurrahman <dio@rockybars.com> Co-authored-by: Takeshi Yoneda <takeshi@tetrate.io>

dio added 6 commits September 11, 2021 01:03

Test sending SIGKILL to func-e

3c034e4

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

License

4f0dd5e

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Test only for linux

c512d41

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Add more comment

c8366a8

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Fix format

8a0a37a

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Fix comment

5de58eb

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

dio mentioned this pull request Sep 11, 2021

Ensure the child process receives SIGTERM when parent dies on Linux #370

Closed

dio added 2 commits September 11, 2021 02:01

Add more comments

e4cf00b

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Fix comments

ee11cb2

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

dio changed the title ~~Test sending SIGKILL to func-e~~ Add test sending SIGKILL to func-e on Linux Sep 11, 2021

dio changed the title ~~Add test sending SIGKILL to func-e on Linux~~ Add test for sending SIGKILL to func-e on Linux Sep 11, 2021

dio added 8 commits September 11, 2021 02:39

Fix comments in fake_func-e.go

9d49d37

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Modify TestProcessGroupAttr_Kill comment

9307900

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Fix

13f0074

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Ensure both processes are killed

f83baa4

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Use eventually when checking the SIGKILLed process

e82285e

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Clarify the use of os-specific attrs when spawning child process

910ed12

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Fix requireProcessError semantic

2f9f767

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Rename

735f613

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

codefromthecrypt reviewed Sep 12, 2021

View reviewed changes

internal/moreos/test/moreos_linux_test.go Outdated Show resolved Hide resolved

internal/test/fake_binary.go Outdated Show resolved Hide resolved

internal/test/testdata/fake_func-e/fake_func-e.go Outdated Show resolved Hide resolved

codefromthecrypt reviewed Sep 12, 2021

View reviewed changes

internal/test/func-e.go Outdated Show resolved Hide resolved

codefromthecrypt reviewed Sep 13, 2021

View reviewed changes

Transfer from Adrian's branch

538054f

Signed-off-by: Dhi Aurrahman <dio@rockybars.com> Co-authored-by: Adrian Cole <adrian@tetrate.io>

dio changed the title ~~Add test for sending SIGKILL to func-e on Linux~~ Add fakebinary for acommodating testing the child process management Sep 13, 2021

dio added 2 commits September 12, 2021 19:24

Remove unused

7ff6f0e

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Add ENVOY_SIGTERM check

9e74fcc

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

dio commented Sep 13, 2021

View reviewed changes

internal/moreos/moreos_func-e_linux_test.go Outdated Show resolved Hide resolved

dio commented Sep 13, 2021

View reviewed changes

internal/moreos/moreos_func-e_linux_test.go Outdated Show resolved Hide resolved

dio requested a review from codefromthecrypt September 13, 2021 02:39

dio marked this pull request as ready for review September 13, 2021 02:39

codefromthecrypt reviewed Sep 13, 2021

View reviewed changes

internal/moreos/moreos_func-e_test.go Outdated Show resolved Hide resolved

Rename Test_X

5fac78e

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

codefromthecrypt approved these changes Sep 13, 2021

View reviewed changes

codefromthecrypt reviewed Sep 13, 2021

View reviewed changes

internal/moreos/moreos_func-e_test.go Show resolved Hide resolved

dio added 2 commits September 12, 2021 23:08

Review comments

1e9c003

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Add TODO

77d37d2

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

dio requested a review from mathetake September 13, 2021 06:12

Comment

40748ad

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

mathetake approved these changes Sep 13, 2021

View reviewed changes

Fix

7f3ff54

Signed-off-by: Dhi Aurrahman <dio@rockybars.com> Co-authored-by: Takeshi Yoneda <takeshi@tetrate.io>

dio requested a review from mathetake September 13, 2021 08:25

mathetake approved these changes Sep 13, 2021

View reviewed changes

dio merged commit 20554ef into master Sep 13, 2021

dio deleted the kill-func-e branch September 13, 2021 08:44

dio mentioned this pull request Sep 13, 2021

Add tests calling terminating signals to func-e #372

Merged

dio mentioned this pull request Sep 14, 2021

Incorrect use of Pdeathsig for killing child on linux. #173

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fakebinary for acommodating testing the child process management #371

Add fakebinary for acommodating testing the child process management #371

dio commented Sep 11, 2021 •

edited

Loading

codefromthecrypt left a comment

dio commented Sep 12, 2021 •

edited

Loading

codefromthecrypt left a comment

dio commented Sep 13, 2021

codefromthecrypt left a comment

codefromthecrypt Sep 13, 2021

codefromthecrypt commented Sep 13, 2021

mathetake Sep 13, 2021 •

edited

Loading

dio Sep 13, 2021

Add fakebinary for acommodating testing the child process management #371

Add fakebinary for acommodating testing the child process management #371

Conversation

dio commented Sep 11, 2021 • edited Loading

codefromthecrypt left a comment

Choose a reason for hiding this comment

dio commented Sep 12, 2021 • edited Loading

codefromthecrypt left a comment

Choose a reason for hiding this comment

dio commented Sep 13, 2021

codefromthecrypt left a comment

Choose a reason for hiding this comment

codefromthecrypt Sep 13, 2021

Choose a reason for hiding this comment

codefromthecrypt commented Sep 13, 2021

mathetake Sep 13, 2021 • edited Loading

Choose a reason for hiding this comment

dio Sep 13, 2021

Choose a reason for hiding this comment

dio commented Sep 11, 2021 •

edited

Loading

dio commented Sep 12, 2021 •

edited

Loading

mathetake Sep 13, 2021 •

edited

Loading