Eliminate signal monitoring shutdown lag #2807

tobim · 2022-12-18T11:31:50Z

This replaces the signal monitor with an adapter approach that translates POSIX signals into CAF messages.
The feature is intentionally limited to SIGINT and SIGTERM and should always lead to termination. Other signals are not supported in order to avert the temptation of using signals for poor mans IPC.

To the reviewer:

verify that client commands exit normally upon completion.
verify that client commands can be interrupted with a single CTRL-C.
verify that commands that start a node attempt a graceful shutdown with CTRL-C but can be terminated immediately with a second CTRL-C.
verify that client commands with the -N option exit regularly upon completion.

dominiklohmann

The current approach does not exit gracefully for me.

libvast/src/system/signal_reflector.cpp

libvast/src/system/pivot_command.cpp

libvast/include/vast/system/signal_reflector.hpp

libvast/src/system/signal_reflector.cpp

This replaces the signal monitor with an adapter approach that translates POSIX signals into CAF messages. The feature is intentionally limited to SIGINT and SIGTERM and should always lead to termination. Other signals are not supported in order to avert the temptation of using signals for poor mans IPC.

Instead of sending a signal to the process we use the `pthread_cancel` function. This works because the `sigwait` is a cancelation point.

With the faster termination thanks to the new signal handling logic the symptoms of the not quite working blocking import become much more pronounced. This is especially problematic for the integration tests which rely on blocking import a lot. In order to preserve the blocking semantic the flag now implies a flush to disk of in memory data. This ensures that active partitions are written to disk and added catalog before the command returns.

tobim · 2022-12-21T07:33:41Z

macOS CI fails because it doesn't have jthread, but homebrew libc++ should have it. I'm not sure what's wrong here.
Could it be that homebrew clang uses apples libc++ by default?

dominiklohmann · 2022-12-21T08:39:44Z

macOS CI fails because it doesn't have jthread, but homebrew libc++ should have it. I'm not sure what's wrong here.
Could it be that homebrew clang uses apples libc++ by default?

Homebrew libc++ doesn't have it, so you'll have to use std::thread instead and join manually.

dominiklohmann

I still sometimes see the 30s delay in the integration tests:

2022-12-21 10:38:28 DEBUG    stopping server fixture: [PosixPath('/Users/dominiklohmann/Desktop/vast/master/build/bin/vast'), '--bare-mode', '--config=/Users/dominiklohmann/Desktop/vast/master/plugins/web/integration/vast.yaml', '-e', ':42024', 'stop']
2022-12-21 10:38:59 INFO     ran all 5 steps successfully

For the remainder, I left some questions, but I think this is mostly ready to go. Works nicely locally.

vast/vast.cpp

libvast/src/system/import_command.cpp

libvast/src/system/index.cpp

Co-authored-by: Dominik Lohmann <mail@dominiklohmann.de>

dominiklohmann · 2022-12-21T13:07:58Z

I still sometimes see the 30s delay in the integration tests:

2022-12-21 10:38:28 DEBUG    stopping server fixture: [PosixPath('/Users/dominiklohmann/Desktop/vast/master/build/bin/vast'), '--bare-mode', '--config=/Users/dominiklohmann/Desktop/vast/master/plugins/web/integration/vast.yaml', '-e', ':42024', 'stop']
2022-12-21 10:38:59 INFO     ran all 5 steps successfully

We identified this to be a separate bug specific to the web plugin, where the web server command does not shut down properly. This must not necessarily be fixed by this PR.

dominiklohmann

Approving modulo the ongoing open discussions.

dominiklohmann · 2022-12-21T16:43:41Z

I enabled auto-merge here since this was really annoying over the past few weeks and shouldn't be blocking further work while most of everyone is on vacation.

This fixes a bug that was only surfaced by #2807, so it's not really worth a separate changelog entry.

tobim added the performance Improvements or regressions of performance label Dec 18, 2022

tobim requested a review from a team December 18, 2022 11:31

dominiklohmann requested changes Dec 18, 2022

View reviewed changes

tobim force-pushed the story/sc-39891/eliminate-signal-monitor-shutdown-lag branch from 9ce7c27 to b89cb1f Compare December 21, 2022 00:35

tobim added 13 commits December 21, 2022 06:29

Drop a few unused includes

58c76b0

Move termination handler registration to spawn node

ae8ad5b

Handle termination signal in remaining client commands

9399e58

Use connect_to_node directly for the flush command

2569130

Improve robustness of active partition shutdown

9d87fd8

Fix rendering of caf::exit_reason

aeec45e

Avoid a data race during shutdown

235e674

Use native thread cancellation on shutdown

63efb54

Instead of sending a signal to the process we use the `pthread_cancel` function. This works because the `sigwait` is a cancelation point.

Remove obsolete invocations of flush

9caa35b

Remove an unused variable

47806de

Clean up includes in vast.cpp

ebda73a

tobim force-pushed the story/sc-39891/eliminate-signal-monitor-shutdown-lag branch from cdd7cd0 to ebda73a Compare December 21, 2022 06:44

tobim requested a review from dominiklohmann December 21, 2022 08:25

Use std::thread instead of std::jthread

1a6f24a

dominiklohmann reviewed Dec 21, 2022

View reviewed changes

vast/vast.cpp Outdated Show resolved Hide resolved

libvast/src/system/import_command.cpp Show resolved Hide resolved

libvast/src/system/index.cpp Show resolved Hide resolved

libvast/src/system/index.cpp Show resolved Hide resolved

Demote signal log message to DEBUG

89297c0

Co-authored-by: Dominik Lohmann <mail@dominiklohmann.de>

dominiklohmann approved these changes Dec 21, 2022

View reviewed changes

tobim added 3 commits December 21, 2022 14:38

Document changed semantics of spawn_node

d0e94f5

Improve logging for partition persist errors

9ffdb29

Add a changelog entry

db9eb67

dominiklohmann enabled auto-merge December 21, 2022 16:43

tobim added 2 commits December 22, 2022 07:56

Workaround an ASAN bug that manifests with GCC

bab8545

Add the CI configuration to CMake presets

d881d56

dominiklohmann merged commit f889fe5 into master Dec 23, 2022

dominiklohmann deleted the story/sc-39891/eliminate-signal-monitor-shutdown-lag branch December 23, 2022 06:08

dominiklohmann added a commit that referenced this pull request Jan 17, 2023

Add PR number to existing changelog entry

3ba5a61

This fixes a bug that was only surfaced by #2807, so it's not really worth a separate changelog entry.

Dakostu pushed a commit that referenced this pull request Jan 26, 2023

Add PR number to existing changelog entry

6aacf50

This fixes a bug that was only surfaced by #2807, so it's not really worth a separate changelog entry.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eliminate signal monitoring shutdown lag #2807

Eliminate signal monitoring shutdown lag #2807

tobim commented Dec 18, 2022 •

edited by dominiklohmann

Loading

dominiklohmann left a comment

tobim commented Dec 21, 2022

dominiklohmann commented Dec 21, 2022

dominiklohmann left a comment

dominiklohmann commented Dec 21, 2022

dominiklohmann left a comment

dominiklohmann commented Dec 21, 2022

Eliminate signal monitoring shutdown lag #2807

Eliminate signal monitoring shutdown lag #2807

Conversation

tobim commented Dec 18, 2022 • edited by dominiklohmann Loading

dominiklohmann left a comment

Choose a reason for hiding this comment

tobim commented Dec 21, 2022

dominiklohmann commented Dec 21, 2022

dominiklohmann left a comment

Choose a reason for hiding this comment

dominiklohmann commented Dec 21, 2022

dominiklohmann left a comment

Choose a reason for hiding this comment

dominiklohmann commented Dec 21, 2022

tobim commented Dec 18, 2022 •

edited by dominiklohmann

Loading