Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle SIGINT in segcache #74

Closed
masih opened this issue Feb 15, 2023 · 6 comments · Fixed by #79
Closed

Handle SIGINT in segcache #74

masih opened this issue Feb 15, 2023 · 6 comments · Fixed by #79

Comments

@masih
Copy link

masih commented Feb 15, 2023

Currently, a containerised segcache process does not handle SIGNINT. This means to stop the container one has to force-kill it.

It would be great if the code is changed to gracefully handle SIGINT and stops the segcache server. This needs SIGNIT signal handling at Rust level for Server.Process.

@hderms
Copy link
Contributor

hderms commented Feb 15, 2023

will look into this

@thinkingfish
Copy link
Member

thinkingfish commented Feb 16, 2023

I think we should aim for a solution that will handle more signals than SIGINT in the diff, which probably means using signal-hook based solutions. Here are considerations for saying that:

  1. Pelikan (legacy) and Twemcache both use SIGHUP to perform (nocopytruncate) log rotation, here's the code in Pelikan's C implementation as a reference;
  2. Memcached recognizes SIGHUP though it doesn't really do anything with it. It uses SIGUSR1 to perform graceful shutdown (typically when data is persisted on PMEM or extstore) instead of immediate shutdown.
  3. Redis uses SIGUSR1 to kill the background save thread that's writes to the AOFs (code).

SIGINT & SIGTERM are generally supported by all existing cache backend, so we should treat that as the baseline to match. ctrlc does support both, but given the above considerations, we probably need a general solution sooner rather than later.

@masih
Copy link
Author

masih commented Feb 16, 2023

Thank you folks for your quick response on this issue. It would be fantastic if a simple handling of server termination via SIGINT is not blocked by a more sophisticated multi signal handling solution.

@hderms
Copy link
Contributor

hderms commented Feb 16, 2023

w.r.t log rotation: @brayniac https://github.com/pelikan-io/rustcommon/blob/main/ringlog/src/outputs.rs#L80 it seems like ringlog automatically rotates files, but only if the size has been exceeded. If we wanted to force rotation (assuming we want to eventually emulate Pelikan legacy SIGHUP handling) would we want to modify Ringlog to have a forced rotation (i.e. regardless of whether size is exceeded)?

SIGHUP:

If we ever wanted to have SIGHUP force log rotation, a la Pelikan-legacy, it seems like we'd probably want to hold off on handling that signal via graceful shutdown

SIGUSR1:

SIGUSR1 seems like something worth handling similar to Redis once we have a persistence

SIGINT

We should use our current graceful shutdown

SIGTERM

We should use our current graceful shutdown

@thinkingfish since there isn't any persistence right now, then I'm assuming we don't want to do anything with SIGUSR1. Which then it seems like we have a few options currently:

  1. Handle SIGINT/SIGTERM/SIGHUP all through graceful shutdown
    a. violates potential desire to have SIGHUP do log rotation. If we changed SIGHUP to do log rotation in the future, that would presumably be considered a breaking change.
    b. this would be easy to do with ctrl-c by using the termination feature
  2. Handle only SIGINT through graceful shutdown
    a. we can do this using ctrl-c crate but not activating the termination feature
  3. Handle SIGINT and SIGTERM through graceful shutdown, but do nothing with SIGHUP until we have persistence
    a. we would have to use the signal-hook crate (or something equivalent) so we can specify which signals we want to do what with

@hderms
Copy link
Contributor

hderms commented Feb 18, 2023

ok I have set up my PR https://github.com/pelikan-io/pelikan/pull/79/files/b0296bcf6882bf388175ee6153a138dde96fd4de..fb1513df7a0309651cf57d6d3d00772bad5ec8f8 to use signal-hook instead in the commits I linked and it isn't much more complex than ctrlc.

I'm not doing anything specific with SIGHUP at the moment as forced log rotation seems like a reasonable iterative enhancement and it wasn't clear to me if it was necessary with the Ringlog style automatic rotation (or even if it's currently exposed to the user at all as something they can initiate)

all it does currently is handle SIGINT/SIGTERM/SIGHUP on *nix style operating systems by prompting shutdown of listeners/workers over the admin channel

This solution will not work for Windows but Windows portability is not considered a priority for Pelikan

@masih
Copy link
Author

masih commented Feb 20, 2023

Thank you all

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants