Skip to content

Allow configuring signaled shutdown flags#10594

Merged
oranagra merged 15 commits intoredis:unstablefrom
eduardobr:feature/shutdown-sig-savemode
Apr 26, 2022
Merged

Allow configuring signaled shutdown flags#10594
oranagra merged 15 commits intoredis:unstablefrom
eduardobr:feature/shutdown-sig-savemode

Conversation

@eduardobr
Copy link
Contributor

@eduardobr eduardobr commented Apr 17, 2022

The SHUTDOWN command has various flags to change it's default behavior,
but in some cases establishing a connection to redis is complicated and it's easier
for the management software to use signals. however, so far the signals could only
trigger the default shutdown behavior.
Here we introduce the option to control shutdown arguments for SIGTERM and SIGINT.

New config options:
shutdown-on-sigint [nosave | save] [now] [force]
shutdown-on-sigterm [nosave | save] [now] [force]

Implementation:
Support MULTI_ARG_CONFIG on createEnumConfig to support multiple enums to be applied as bit flags.

background

As discussed on #10525

On replicas using RDB, it can be acceptable to not save the latest data on shutdown, especially if we know it will never become a master.
Sometimes the periodic SAVE in background is enough and the blocking SAVE on shutdown undesired.
Also, if guaranteed durability is desired AOF could be used instead.

@eduardobr eduardobr force-pushed the feature/shutdown-sig-savemode branch from 8c2a5c4 to fd8a33d Compare April 18, 2022 12:39
@eduardobr
Copy link
Contributor Author

@oranagra I've ended up updating createEnumConfig to support MULTI_ARG_CONFIG flag.
And for the custom validation to reject NOSAVE + SAVE added a validation function.
So now we can combine the flags as we want, separated by space.

@eduardobr eduardobr changed the title Allow configuring sig shutdown default saving mode Allow configuring signaled shutdown flags Apr 18, 2022
@eduardobr eduardobr force-pushed the feature/shutdown-sig-savemode branch from fd8a33d to 1f1e82e Compare April 18, 2022 12:59
Copy link
Member

@oranagra oranagra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

neat! i had a feeling this is not that complicate to re-use the enum code.

i feel we're missing some tests. maybe one about shutdown, but also some around the config.

I think we may want to expose this to modules too (see #10285), maybe that can be done later.

@oranagra
Copy link
Member

regarding the module configs, i think that all we need to do is add a new REDISMODULE_CONFIG_BITS flag or alike to RM_RegisterEnumConfig.
the rest of the module API can remain the same, i.e. RedisModuleConfigGetEnumFunc still takes an integer.
let me know if you'd like to try handling it in this PR or the next...

@oranagra
Copy link
Member

@eduardobr sorry for hogging your PR, i thought it may be better to judge this whole idea of enum flags in one piece.
@madolson let me know what you think.

Comment on lines +318 to +321
if (!names || values != matches) {
sdsfree(names);
return sdsnew("unknown");
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oranagra Now that you've removed configEnumGetNameOrUnknown, maybe we can panic here instead of returning "unkonwn".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm having second thoughts.. i wonder why it was there in the first place.
maybe to serve the INFO command, but still how come we expected a state in which the integer doesn't have a corresponding enum?

maybe the thought is that panic should be used to protect further data corruption due to a bug, or hidden consecutive bugs that are hard to find without panicking earlier. but for CONFIG GET or INFO, there's no need for that (panic causes data loss).

i'm unsure.

Copy link
Collaborator

@yossigo yossigo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
@oranagra I'd consider adding validation in RM_RegisterEnumConfig to make sure that enum values are unique in both modes, as misusing the API there could end up with unexpected results.

@oranagra
Copy link
Member

@yossigo i thought that letting the user provide overlapping bits is a feature.
maybe we should document and test it...
i.e. you can define

none = 0
apple = 1
kiwi = 2
banana = 4
sweet = 5
all = 7

i think the current code will handle it somewhat correctly.
i.e. config set apple banana, then config get will return sweet
(sorry for the bad example, don't wanna waste time on thinking of a better one)

@oranagra
Copy link
Member

we discussed this in a core-team meeting, and agreed to proceed.
the one thing that we're still uncomfortable with is that multi-arg configs behave differently for CONFIG command, vs the command line argument for redis-server. i.e. config takes one bulk string with spaces in it, while the command line takes an argv array with multiple values. this is ugly, but also causes some complicates for some systems.
a solution we agreed on is to add an option to break a single argument into multiple ones. so both of these will be the same:

redis-server --replicaof localhost 12345
redis-server --replicaof "localhost 12345"

what i'm gonna do next is remove the the module API code i added to this PR, and merge it.
then make another PR with the module API (easier for release notes purposes).
and finally a 3rd PR with the above mentioned command line alternative..

@oranagra oranagra added state:major-decision Requires core team consensus release-notes indication that this issue needs to be mentioned in the release notes labels Apr 26, 2022
@oranagra oranagra merged commit 3a1d142 into redis:unstable Apr 26, 2022
@oranagra oranagra mentioned this pull request Apr 27, 2022
enjoy-binbin added a commit to enjoy-binbin/redis that referenced this pull request Apr 28, 2022
… spaces for MULTI_ARG configs parsing

Currently redis-server looks for arguments that start with `--`,
and anything in between them is considered arguments for the config.
like: `src/redis-server --shutdown-on-sigint nosave force now --port 6380`

MULTI_ARG configs behave differently for CONFIG command, vs the command
line argument for redis-server.
i.e. CONFIG command takes one bulk string with spaces in it, while the
command line takes an argv array with multiple values.

In this PR, in config.c, if `argc > 1` we can take them as is,
and if the config is a `MULTI_ARG` and `argc == 1`, we will split it by spaces.

So both of these will be the same:
```
redis-server --shutdown-on-sigint nosave force now --port 6380
redis-server --shutdown-on-sigint "nosave force now" --port 6380
```

And for MULTI_ARG, now we will check that it has at least one arg,
we can return `wrong number of arguments` before `config->interface.set`.
```
old:
src/redis-server --shutdown-on-sigint
>>> 'shutdown-on-sigint'
argument(s) must be one of the following: default, save, nosave, now, force

new:
src/redis-server --shutdown-on-sigint
>>> 'shutdown-on-sigint'
wrong number of arguments
```

This is mentioned in redis#10594 (comment)

Co-authored-by: Oran Agra <[email protected]>
enjoy-binbin pushed a commit to enjoy-binbin/redis that referenced this pull request Jul 31, 2023
The SHUTDOWN command has various flags to change it's default behavior,
but in some cases establishing a connection to redis is complicated and it's easier
for the management software to use signals. however, so far the signals could only
trigger the default shutdown behavior.
Here we introduce the option to control shutdown arguments for SIGTERM and SIGINT.

New config options:
`shutdown-on-sigint [nosave | save] [now] [force]` 
`shutdown-on-sigterm [nosave | save] [now] [force]`

Implementation:
Support MULTI_ARG_CONFIG on createEnumConfig to support multiple enums to be applied as bit flags.

Co-authored-by: Oran Agra <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

6 participants