seastar icon indicating copy to clipboard operation
seastar copied to clipboard

Published 20 hours ago • verified publisher icon scylladb

Make seastar options HUP-updateable

Open xemul opened this issue 1 year ago • 20 comments

Currently all seastar options are immutable after seastar app starts. That's not very convenient as it takes full app restart to pick up new options if needed. Some options, however, allow live-updating because they either just one-to-one map to some simple reactor variable or because live-update can be implemented as a callable.

Seastar can read its options from ini-style seastar.conf file, so what this PR does is makes seastar re-read this file upon receiving HUP signal (*). Options should be marked as live-updateable explicitly by providing a noncopyable function on construction. When config file is re-read all those functions are called with new values arguments.

As an example the --unsafe-bypass-fsync is turned into live-updateable option.

(*) Scylla re-installs HUP handler, so it'll have to call reread_config() by hand.

xemul avatar Apr 14 '23 17:04 xemul

Finally we have it! Thanks for caring

On Fri, Apr 14, 2023 at 10:25 AM Pavel Emelyanov @.***> wrote:

Currently all seastar options are immutable after seastar app starts. That's not very convenient as it takes full app restart to pick up new options if needed. Some options, however, allow live-updating because they either just one-to-one map to some simple reactor variable or because live-update can be implemented as a callable.

Seastar can read its options from ini-style seastar.conf file, so what this PR does is makes seastar re-read this file upon receiving HUP signal (*). Options should be marked as live-updateable explicitly by providing a noncopyable function on construction. When config file is re-read all those functions are called with new values arguments.

As an example the --unsafe-bypass-fsync is turned into live-updateable option.

(*) Scylla re-installs HUP handler, so it'll have to call reread_config() by hand.

You can view, comment on, or merge this pull request online at:

https://github.com/scylladb/seastar/pull/1615 Commit Summary

File Changes

(6 files https://github.com/scylladb/seastar/pull/1615/files)

Patch Links:

  • https://github.com/scylladb/seastar/pull/1615.patch
  • https://github.com/scylladb/seastar/pull/1615.diff

— Reply to this email directly, view it on GitHub https://github.com/scylladb/seastar/pull/1615, or unsubscribe https://github.com/notifications/unsubscribe-auth/AANHUROXFHTEP3A5EB5VC5DXBGCBRANCNFSM6AAAAAAW6V7Z7Q . You are receiving this because you are subscribed to this thread.Message ID: @.***>

dorlaor avatar Apr 14 '23 17:04 dorlaor

upd:

  • make reread_config() public so that app could kick it by hand
  • call updating callback when the option is "back to default"
  • protect against accidental HUP re-entrance
  • default --config-file to be ~/.config/seastar/seastar.conf
  • fail start in case --config-file is specified, but the file is not readable for whatever reason

xemul avatar Apr 26 '23 14:04 xemul

upd:

  • handle restore-default case for nullopt defaults

xemul avatar Apr 26 '23 15:04 xemul

upd (applied @tchaikov suggestions around tilde expansion):

  • check config_file not to be empty
  • abort on unset HOME early and explicitly
  • .replace() tilde with raw home, w/o converting it to std::string

xemul avatar Apr 27 '23 06:04 xemul

@denesb , merge ping

xemul avatar May 02 '23 05:05 xemul

@denesb , merge ping

I'm not a seastar maintainer.

denesb avatar May 02 '23 06:05 denesb

@avikivity , review ping

xemul avatar May 10 '23 13:05 xemul

@avikivity , review ping

xemul avatar May 15 '23 16:05 xemul

@avikivity , review ping

xemul avatar May 23 '23 08:05 xemul

@avikivity , review ping

xemul avatar May 29 '23 05:05 xemul

upd:

  • rebased
  • added sugar for reactor options cross-shard updating
  • made more options live-updateable

@avikivity , your review is very much welcome

xemul avatar Jul 13 '23 10:07 xemul

@avikivity , please review

xemul avatar Jul 27 '23 06:07 xemul

Having Seastar be responsible for reading options was a design mistake IMO. The app should be responsible for options, and of it needs to send some to Seastar that it should do so via app_template::config. Then, the names of the parameters, the format and location of the configuration file, and responsibility for reloading are all up to the application.

So, we need to choose. Do we continue to improve option processing, or do we deprecate them in favor of moving everything to the application?

avikivity avatar Jul 27 '23 07:07 avikivity

I partially agree with that and the "it needs to send some to Seastar that it should do so via app_template::config" was mostly addressed by the 8810950b92eaa7f4115eb19e3294a5c6c1af8cb9 work. The only difference is that the configuration is fed via app_template::seastar_options, not app_template::config. And this PR mostly touches the program_options to make it live-updateable. Hunks to inject HUP handling and the --config-file option are there just because that's the current way program_options are populated.

As far as "the names of the parameters, the format and location of the configuration file, and responsibility for reloading are all up to the application" is concerned. Let's take for example fbb9fcd15a7e15c24b75f85b003723bea29d85e5. If this option was added without seastar support for the option CLI name, then all apps using seastar would need to add the option on their own, which's not very convenient. I think that it's still worth keeping the config and/or CLI parsing in seastar, but maybe isolating it even more in util so that it's fully optional.

xemul avatar Jul 28 '23 07:07 xemul

I though to keep exposing bpo helpers, so a bpo application can continue ask Seastar to add Seastar options to the application bpo parser.

avikivity avatar Aug 07 '23 14:08 avikivity

I think this is problematic, it adds a new configuration file format for the application. For ScyllaDB, it means an extra source of configuration outside scylla.yaml.

avikivity avatar Aug 07 '23 14:08 avikivity

I think this is problematic, it adds a new configuration file format for the application. For ScyllaDB, it means an extra source of configuration outside scylla.yaml.

One cannot tune seastar options via scylla.yaml even now. I do not remember the exact file names and options, but if someone needs to add seastar options to installed-and-running scylla, one needs to find SCYLLA_OPTIONS (?) variable in /etc//scylla. shell script (with variables) and put it there as a CLI option, not as .yaml/.json/.ini entry. So it's pain anyway -- either there are two config files, or the need to teach scylla.yaml to configure seastar options.

xemul avatar Aug 08 '23 06:08 xemul

I think this is problematic, it adds a new configuration file format for the application. For ScyllaDB, it means an extra source of configuration outside scylla.yaml.

One cannot tune seastar options via scylla.yaml even now. I do not remember the exact file names and options, but if someone needs to add seastar options to installed-and-running scylla, one needs to find SCYLLA_OPTIONS (?) variable in /etc//scylla. shell script (with variables) and put it there as a CLI option, not as .yaml/.json/.ini entry. So it's pain anyway -- either there are two config files, or the need to teach scylla.yaml to configure seastar options.

Sure, but isn't it overall nicer to have options in one place? I'm thinking of --blocked-reactor-notify-ms (which might as well be renamed).

avikivity avatar Aug 08 '23 09:08 avikivity

btw, it's a little problematic since the file has to be accessed both via Seastar APIs and via standard APIs (before launch), but that's life.

avikivity avatar Aug 08 '23 09:08 avikivity

Sure, but isn't it overall nicer to have options in one place?

It is, but seastar.conf is the legacy file. I don't know if anyone is using it so presumably we cannot just drop it. OTOH, following the direction of non-framework-ish options maintaining, this PR shouldn't have the part that's responsible for re-reading seastar.conf.

xemul avatar Aug 08 '23 16:08 xemul