Restore signals, add pre-stop sleep #88

xakep666 · 2022-11-08T09:20:19Z

Changes in pull-request:

Stop notification on signal channel after signal received. This recovers default signal behaviour (if no other notifiers registered) and allows to close application via Ctrl-C faster if any shutdown routine hangs.
Add new option for hook to sleep before actual execution. This needed for correct termination in k8s. See https://blog.palark.com/graceful-shutdown-in-kubernetes-is-not-always-trivial/ for detailed description.

This allows to restore default signal behaviour. It's useful to faster termination when i.e. user pressed Ctrl-C, shutdown routine hangs, user presses Ctrl-C again and application terminates without waiting.

…ion in k8s.

skovtunenko · 2022-12-19T13:29:10Z

@xakep666 , I will review the given article in some time, thank you

BMalaichik · 2022-12-19T16:03:38Z

terminator.go

@@ -140,6 +141,10 @@ func (t *Terminator) waitShutdown(appCtx context.Context) {
 			go func(f Hook) {
 				defer runWg.Done()

+				if f.preStop > 0 {


time.Sleep will be called for every registered hook order despite the initial intention to wait once before executing all hooks

Is it a good idea to introduce global pre-stop sleep instead of per-hook one to resolve this?

IMO yes, what might be the point of having that for each hook? What are the use-cases ?

BMalaichik · 2022-12-19T16:06:13Z

terminator_test.go

@@ -277,6 +279,39 @@ func TestTerminator_Wait(t *testing.T) {
 	}
 }

+func TestTerminator_restores_signals(t *testing.T) {


this test is more like a integration one, introducing a dedicated env variable to simulate the behavior.
To test the actual library end-to-end a separate integration test suite can be added, that can spawn a simple app with graterm configured and webserver waiting for requests

This timing-specific test relying on the scheduler are tricky to implement right, consider mocking time adding assertions that Sleep method was called

This test actually is not timing-specific because it tests signal default handler restoration. I'll try to test without sleeps between signals.

it's scheduler timing-specific because you rely on time.Sleep which is not a good practice, tests are likely to be flaky especially on lower CPU resources machines.

I removed sleeps before sending SIGINT's locally. Test doesn't fail. So it shouldn't be flaky at all because we're waiting for process termination and checking for specific flags.

@xakep666 now it looks better, thank you. What about having a separate integration test suite in a separate file ? So we could leverage integration testing with this approach for other cases as well and not mix it with unit tests

It's good idea but Suite from testify doesn't allow parallel runs so maybe just move this test to separate file. Probably with specific build tag.

BMalaichik · 2022-12-19T16:06:45Z

terminator_test.go

+		return
+	}
+
+	cmd := exec.Command(os.Args[0], "-test.run=TestTerminator_restores_signal")


it's not a good approach to introduce a recursion in tests, pls see the comment above

Subprocess-based testing is a widely used technique
You can find a lot of articles mentions it. I.e. https://about.sourcegraph.com/blog/go/advanced-testing-in-go

@BMalaichik thank you for review! I implemented "preStop" because k8s' one relies on "sleep" availability inside container. But sometimes container doesn't contain it. I. e. container built from scratch with app only.

Hm, sleep is provided as a part of coreutils distribution package, having a size <3 MBs. I believe if the docker container is packed as minimalistic as possible for some reason graterm not likely to be used there as a dependency either :)

"Distroless" images from Google doesn't contain coreutils.

$ docker run --rm -it gcr.io/distroless/base sleep 1 docker: Error response from daemon: failed to create shim task: OCI runtime create failed: runc create failed: unable to start container process: exec: "sleep": executable file not found in $PATH: unknown.

Thinking generally about this approach I believe it makes sense when you do integration testing of your CLI application, so it's easy an interesting approach to bind the test case to the CLI program execution, as in the given article they do in Hashicorp

For this particular case testing a package providing a utility structure it's an overkill IMO

BMalaichik · 2022-12-19T16:15:10Z

terminator.go

@@ -55,6 +55,7 @@ func withSignals(ctx context.Context, chSignals chan os.Signal, sig ...os.Signal

 	// function invoke cancel once a signal arrived OR parent context is done:
 	go func() {
+		defer signal.Stop(chSignals)


BMalaichik · 2022-12-19T16:24:17Z

Generally SIGKILL + SIGTERM combination handling would be sufficient to gracefully handle the shutdown.
If you need to wait for some period to ensure the Pod is removed from the Endpoints you can register a Pod preStop hook with sleep 5; command, waiting before sending the SIGTERM command.
As per official documentation

Anyway, If there is a specific need to wait for some time before starting the shutdown this can be achieved in with registering a 1st order hook to wait for some time
to make the api in a k8s style a PreStop hook can be defined, however IMO for this minimalistic lib it's not needed.

xakep666 · 2022-12-19T19:32:53Z

@BMalaichik thank you for review! I implemented "preStop" because k8s' one relies on "sleep" availability inside container. But sometimes container doesn't contain it. I. e. container built from scratch with app only.

BMalaichik · 2022-12-19T20:19:13Z

@xakep666 Have you considered as an alternative simply registering a first 0 order hook with time.Sleep(X seconds) ?
I believe it will do the job you need

xakep666 · 2022-12-19T20:28:12Z

@BMalaichik this should work as expected. But IMHO it looks weird when user should specify same value twice (inside hook and as hook timeout) and add it to global termination timeout.

BMalaichik · 2022-12-19T20:44:44Z

@BMalaichik this should work as expected. But IMHO it looks weird when user should specify same value twice (inside hook and as hook timeout) and add it to global termination timeout.

I propose to internally use it as a high-priority hook with the time limit > then the specified duration, to avoid scheduling uncertainties and random rejections, smth like

terminator.WithOrder(-1).Register(timePeriod * 1.5, func (_) { time.Sleep(timePeriod) })

This way you re-use already tested code not introducing additional complexity.

Also I think would be good to set it as a constructor argument instead of a Hook API, @xakep666 WDYT ?
I think we can add this if @skovtunenko has no objections

xakep666 · 2022-12-19T20:46:45Z

@BMalaichik "twice" because

t.Register(time.Second, func(context.Context) { time.Sleep(time.Second) })

My initial idea was about decoupling pre-stop pause from global termination timeout. But maybe you're right and it's better not to do this. So for simplicity I can add method that registers hook with sleep always as first one.

UPD: sent this before your last comment. Actually we found the same approach :)

BMalaichik · 2022-12-19T20:51:28Z

@BMalaichik "twice" because
t.Register(time.Second, func(context.Context) { time.Sleep(time.Second) })
My initial idea was about decoupling pre-stop pause from global termination timeout. But maybe you're right and it's better not to do this. So for simplicity I can add method that registers hook with sleep always as first one.

yes, Constructor API doesn't accept extra params so it would be easier to add a method like BeforeAll or RegisterBeforeAll (needs to think more about the naming) that would bind internally to the list of before all hooks with same priority, e.g. -1
PreStop is not a good name because the whole idea of the terminator is about stop/termination, so I think classic hook naming might be used

xakep666 · 2022-12-20T05:19:49Z

@BMalaichik how about adding just one function:

func Wait(ctx context.Context) { <-ctx.Done() }

and use it like

terminator.WithOrder(0).Register(3*time.Second, Wait)

This approach requires actually no changes in terminator and user don't have to specify sleep period twice.

Also maybe it will be good to add alias like

terminator.WithOrder(0).RegisterWait(3*time.Second)

And one more question: is it ok to force-push changes after discussion?

BMalaichik · 2022-12-20T08:35:27Z

@BMalaichik how about adding just one function:
func Wait(ctx context.Context) { <-ctx.Done() }
and use it like
terminator.WithOrder(0).Register(3*time.Second, Wait)
This approach requires actually no changes in terminator and user don't have to specify sleep period twice.

Also maybe it will be good to add alias like
terminator.WithOrder(0).RegisterWait(3*time.Second)
And one more question: is it ok to force-push changes after discussion?

it's up to the @skovtunenko , IMO whatever works for you, you can force push or open new fresh PR

After sleeping on our discussion I think adding new API methods to the lib is redundant, you can do a number of workaround using lib primitives, e.g. the last provided example by you OR even having a hook with time.Sleep seems not bad.

What might be the way to improve is to add a support for general beforeAll hook(s), standard way of intercepting the component lifecycle.
Let's wait what @skovtunenko replies

xakep666 · 2023-01-18T08:53:27Z

@skovtunenko @BMalaichik gentle bump

xakep666 added 2 commits November 8, 2022 12:54

Stop notification on signal channel after signal received

e002652

This allows to restore default signal behaviour. It's useful to faster termination when i.e. user pressed Ctrl-C, shutdown routine hangs, user presses Ctrl-C again and application terminates without waiting.

Add WithPreStopSleep option to Hook for correct graceful terminat…

3a15de9

…ion in k8s.

BMalaichik reviewed Dec 19, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore signals, add pre-stop sleep #88

Restore signals, add pre-stop sleep #88

xakep666 commented Nov 8, 2022

skovtunenko commented Dec 19, 2022

BMalaichik Dec 19, 2022 •

edited

Loading

xakep666 Dec 19, 2022

BMalaichik Dec 19, 2022 •

edited

Loading

BMalaichik Dec 19, 2022

xakep666 Dec 19, 2022

BMalaichik Dec 20, 2022

xakep666 Dec 20, 2022

BMalaichik Dec 20, 2022

xakep666 Dec 20, 2022 •

edited

Loading

BMalaichik Dec 19, 2022

xakep666 Dec 19, 2022

BMalaichik Dec 19, 2022

xakep666 Dec 20, 2022

BMalaichik Dec 20, 2022

BMalaichik Dec 19, 2022

BMalaichik commented Dec 19, 2022

xakep666 commented Dec 19, 2022 •

edited

Loading

BMalaichik commented Dec 19, 2022

xakep666 commented Dec 19, 2022

BMalaichik commented Dec 19, 2022 •

edited

Loading

xakep666 commented Dec 19, 2022 •

edited

Loading

BMalaichik commented Dec 19, 2022

xakep666 commented Dec 20, 2022 •

edited

Loading

BMalaichik commented Dec 20, 2022

xakep666 commented Jan 18, 2023

Restore signals, add pre-stop sleep #88

Are you sure you want to change the base?

Restore signals, add pre-stop sleep #88

Conversation

xakep666 commented Nov 8, 2022

skovtunenko commented Dec 19, 2022

BMalaichik Dec 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BMalaichik Dec 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xakep666 Dec 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BMalaichik commented Dec 19, 2022

xakep666 commented Dec 19, 2022 • edited Loading

BMalaichik commented Dec 19, 2022

xakep666 commented Dec 19, 2022

BMalaichik commented Dec 19, 2022 • edited Loading

xakep666 commented Dec 19, 2022 • edited Loading

BMalaichik commented Dec 19, 2022

xakep666 commented Dec 20, 2022 • edited Loading

BMalaichik commented Dec 20, 2022

xakep666 commented Jan 18, 2023

BMalaichik Dec 19, 2022 •

edited

Loading

BMalaichik Dec 19, 2022 •

edited

Loading

xakep666 Dec 20, 2022 •

edited

Loading

xakep666 commented Dec 19, 2022 •

edited

Loading

BMalaichik commented Dec 19, 2022 •

edited

Loading

xakep666 commented Dec 19, 2022 •

edited

Loading

xakep666 commented Dec 20, 2022 •

edited

Loading