Better signal handling for proxy, stern #2715

fisx · 2022-09-20T11:55:28Z

https://wearezeta.atlassian.net/browse/SQPIT-1431

Checklist

Add a new entry in an appropriate subdirectory of changelog.d
Read and follow the PR guidelines

fisx · 2022-09-20T11:58:29Z

Can't reproduce, what am I doing wrong?

When I kill -SIGINT <cannonpid>, I get both files /tmp/x[12], so the handler is called. If I kill -SIGINT <brigpid>, I only get /tmp/x1, but not /tmp/x2. When I don't install the signal handlers, I get the same, so the finally doesn't seem to be skipped if we don't handle signals here.

akshaymankar · 2022-09-20T12:26:50Z

Can you please try SIGTERM? SIGNIT is actually handled correctly by default, as the issue says.

fisx · 2022-09-20T13:04:09Z

Can you please try SIGTERM? SIGNIT is actually handled correctly by default, as the issue says.

Doesn't change anything; the handler still doesn't get caught.

Anyway that's besides the point, no? I'm installing a SIGINT handler that doesn't get called if I send a SIGINT. I must be holding it wrong, somehow.

fisx · 2022-09-20T13:07:08Z

ah, i see: brig is calling runSettingsWithShutdown, and cannon isn't. that function installs its own signal handlers and thus removes the ones i've just installed. so it looks like brig is fine? which services throw the 502 / which services are they talking to at the time. I'm going to check the others.

This reverts commit a175577.

akshaymankar · 2022-09-20T13:11:07Z

ah, i see: brig is calling runSettingsWithShutdown, and cannon isn't. that function installs its own signal handlers and thus removes the ones i've just installed. so it looks like brig is fine? which services throw the 502 / which services are they talking to at the time. I'm going to check the others.

Nice catch! We don't actually have logs from last deployment to prod because they're gone. So, I guess we can either spam staging with a lot of requests and do a deployment and find out or, we could just go through all the services and see which ones don't use runSettingsWithShutdown.

fisx · 2022-09-20T13:27:21Z

or, we could just go through all the services and see which ones don't use runSettingsWithShutdown.

that was easy! there is only proxy who calls runSettings directly. patch coming up!

…t-sigint-handler

fisx · 2022-09-20T13:37:06Z

Not at all confident, but this might resolve it, no? Thanks for finding the issue, @akshaymankar, this was fun!

fisx · 2022-09-20T13:52:54Z

we should also add an hlint rule that forbids us to use runSettings anywhere... :-)

akshaymankar · 2022-09-21T08:02:57Z

services/cannon/src/Cannon/Run.hs

-  void $ installHandler sigTERM (signalHandler (env e) tid) Nothing
-  void $ installHandler sigINT (signalHandler (env e) tid) Nothing
-  runSettings s app `finally` do
+  runSettingsWithShutdown s app 5 `finally` do


I am not sure about the impact of this 5 second wait, so I will run a couple of experiments to see that the draining happens correctly. Earlier we were also not closing the listen socket, so impact of that is another mystery to me.
I am being super careful because this functionality is important and we don't have nice ways of automatically testing it.

Looks like this is closing all the websockets at the same time and then calling the drain script. I suspect this might be due to some cleanup in wai. Given this place is for sure not our problem, I suggest that we revert the change in cannon and rest of the things can be applied.

67946bd (also 0f264d2)

67946bd (also 0f264d2)

no, that's different commits. but the github UI is messing with me, so i'll just leave you to the actually commit history and the "files changed" tab. :)

Also applies hlint again to the whole codebase (excluding tests), as we had some drift between finalising hlint and new PRs being merged without being linted / having CI catch those cases. I also disalbed the pipefail from the script, as that would short-circuit the linter on first issue found. Hopefully that doesn't mess with CI. PS: This will fail CI linters phase until #2715 has been merged.

elland · 2022-09-21T08:14:48Z

After running hlint with the changes from #2718 it found a possible spot where this is also not used, in Federator.Response.

akshaymankar · 2022-09-21T08:41:17Z

I also suspect that the reason for 502s is that we have 5 second grace period for requests which are already running, maybe we can bump it to 30s. Specially in cargohold where the uploads could take some time to finish.

fisx · 2022-09-21T10:20:32Z

After running hlint with the changes from #2718 it found a possible spot where this is also not used, in Federator.Response.

unrelated.

fisx · 2022-09-21T10:21:08Z

I also suspect that the reason for 502s is that we have 5 second grace period for requests which are already running, maybe we can bump it to 30s. Specially in cargohold where the uploads could take some time to finish.

I'll do that 👍

This reverts commit f7bdcdc.

akshaymankar

Looks good. Please update the changelog though!

tools/stern/src/Stern/API.hs

changelog.d/3-bug-fixes/sqpit-1431

Co-authored-by: Akshay Mankar <[email protected]>

* Add new custom hlint rule for runSetting. Also applies hlint again to the whole codebase (excluding tests), as we had some drift between finalising hlint and new PRs being merged without being linted / having CI catch those cases. I also disalbed the pipefail from the script, as that would short-circuit the linter on first issue found. Hopefully that doesn't mess with CI. PS: This will fail CI linters phase until #2715 has been merged. * Removed Federator.Response from runSettings rule.

...

a175577

fisx temporarily deployed to cachix September 20, 2022 11:55 Inactive

zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Sep 20, 2022

Revert "..."

5bf4e52

This reverts commit a175577.

Use utilities in cannon like in all the other services.

f7bdcdc

fisx added 2 commits September 20, 2022 15:27

Also, do it in all the other services...

b15dc5a

Also, stern.

76c1e69

fisx temporarily deployed to cachix September 20, 2022 13:30 Inactive

fisx added 3 commits September 20, 2022 15:33

Changelog

cbf9215

Reference discussion over at ghc's home.

22d2a9c

Merge remote-tracking branch 'origin/develop' into SQPIT-1431-explici…

3364bf7

…t-sigint-handler

fisx temporarily deployed to cachix September 20, 2022 13:35 Inactive

fisx marked this pull request as ready for review September 20, 2022 13:35

fisx changed the title ~~debug some other thign~~ Better signal handling for proxy, stern. Sep 20, 2022

fisx changed the title ~~Better signal handling for proxy, stern.~~ Better signal handling for proxy, stern Sep 20, 2022

fisx requested a review from akshaymankar September 20, 2022 13:36

akshaymankar reviewed Sep 21, 2022

View reviewed changes

elland mentioned this pull request Sep 21, 2022

Add new custom hlint rule for runSetting. #2718

Merged

fisx added 4 commits September 21, 2022 12:24

Revert "Use utilities in cannon like in all the other services."

ba70df1

This reverts commit f7bdcdc.

Write down lessons learned.

e0fc02d

Give runSettingsWithShutdown default shutdown time.

0f264d2

Change default shutdown time from 5 seconds to 30 seconds.

67946bd

fisx temporarily deployed to cachix September 21, 2022 10:31 Inactive

fisx requested a review from akshaymankar September 21, 2022 10:33

akshaymankar approved these changes Sep 22, 2022

View reviewed changes

tools/stern/src/Stern/API.hs Outdated Show resolved Hide resolved

changelog.d/3-bug-fixes/sqpit-1431 Outdated Show resolved Hide resolved

Update tools/stern/src/Stern/API.hs

23d6941

Co-authored-by: Akshay Mankar <[email protected]>

fisx temporarily deployed to cachix September 22, 2022 08:58 Inactive

Update changelog.d/3-bug-fixes/sqpit-1431

f50598c

Co-authored-by: Akshay Mankar <[email protected]>

fisx temporarily deployed to cachix September 22, 2022 08:59 Inactive

...

62ff92e

fisx temporarily deployed to cachix September 22, 2022 19:25 Inactive

fisx merged commit b83b671 into develop Sep 23, 2022

fisx deleted the SQPIT-1431-explicit-sigint-handler branch September 23, 2022 07:48

zebot mentioned this pull request Sep 27, 2022

Release 2022-09-27 - (expected chart version 4.24.0) #2728

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better signal handling for proxy, stern #2715

Better signal handling for proxy, stern #2715

fisx commented Sep 20, 2022 •

edited

Loading

fisx commented Sep 20, 2022

akshaymankar commented Sep 20, 2022 •

edited

Loading

fisx commented Sep 20, 2022

fisx commented Sep 20, 2022

akshaymankar commented Sep 20, 2022

fisx commented Sep 20, 2022

fisx commented Sep 20, 2022

fisx commented Sep 20, 2022

akshaymankar Sep 21, 2022

akshaymankar Sep 21, 2022 •

edited

Loading

fisx Sep 21, 2022

fisx Sep 21, 2022

elland commented Sep 21, 2022

akshaymankar commented Sep 21, 2022

fisx commented Sep 21, 2022

fisx commented Sep 21, 2022

akshaymankar left a comment

Better signal handling for proxy, stern #2715

Better signal handling for proxy, stern #2715

Conversation

fisx commented Sep 20, 2022 • edited Loading

Checklist

fisx commented Sep 20, 2022

akshaymankar commented Sep 20, 2022 • edited Loading

fisx commented Sep 20, 2022

fisx commented Sep 20, 2022

akshaymankar commented Sep 20, 2022

fisx commented Sep 20, 2022

fisx commented Sep 20, 2022

fisx commented Sep 20, 2022

akshaymankar Sep 21, 2022

Choose a reason for hiding this comment

akshaymankar Sep 21, 2022 • edited Loading

Choose a reason for hiding this comment

fisx Sep 21, 2022

Choose a reason for hiding this comment

fisx Sep 21, 2022

Choose a reason for hiding this comment

elland commented Sep 21, 2022

akshaymankar commented Sep 21, 2022

fisx commented Sep 21, 2022

fisx commented Sep 21, 2022

akshaymankar left a comment

Choose a reason for hiding this comment

fisx commented Sep 20, 2022 •

edited

Loading

akshaymankar commented Sep 20, 2022 •

edited

Loading

akshaymankar Sep 21, 2022 •

edited

Loading