Introduce bgsave cancel

[ranshid]

trafficstars

In some cases bgsave child process can run for a long time exhausting system resources. Although it is possible to kill the bgsave child process from the system shell, sometimes it is not possible allowing OS level access.

This PR adds a new subcommand to the BGSAVE command. When user will issue BGSAVE CANCEL It will do one of the 2:

1. In case a bgsave child process is currently running, the child process would be immediately killed thus terminating any save/replication full sync process.
2. In case a bgsave child process is SCHEDULED to run, the scheduled execution will be cancelled.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 83.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 70.40%. Comparing base (5f0ccf1) to head (55a1541). Report is 223 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/rdb.c	83.33%	2 Missing :warning:

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable     #757      +/-   ##
============================================
+ Coverage     70.13%   70.40%   +0.27%     
============================================
  Files           111      112       +1     
  Lines         60300    61476    +1176     
============================================
+ Hits          42292    43283     +991     
- Misses        18008    18193     +185     

Files with missing lines	Coverage Δ
src/commands.def	100.00% <ø> (ø)
src/rdb.c	76.47% <83.33%> (+0.48%)	:arrow_up:

... and 35 files with indirect coverage changes

[enjoy-binbin]

It occurred to me that maybe we also want to cancel server.rdb_bgsave_scheduled?

Noted it will also cancel the replication child process.

Also does AOFRW need the same cancel? seem to be the same case.

[ranshid]

It occurred to me that maybe we also want to cancel server.rdb_bgsave_scheduled?

I also thought about that during the implementation. I think the main goal was to kill the current running bgsave. Do you think this way it is currently implemented is confusing?

[enjoy-binbin]

Looks like it could be a complement to SCHEDULE. I am ok with both.

Another question here, kill the current running bgsave, does it include the disk-based replication or the diskless replication intentional or unintentional?

[ranshid]

Looks like it could be a complement to SCHEDULE. I am ok with both.

Another question here, kill the current running bgsave, does it include the disk-based replication or the diskless replication intentional or unintentional?

Yes it should kill both brands

[ranshid]

I think I can add the following logic:

if only bgsave is running kill it and ALSO cancel scheduled bgsave if no bgsave is running BUT there is a scheduled bgsave - unschedule the scheduled BGSAVE and return O.K if no bgsave is running OR scheduled - return an error @enjoy-binbin WDYT?

[enjoy-binbin]

look good to me. That's also what i thought. Let's also make sure we won't kill the replication fork? Since it is a bgsave command, i think we should let replication out or at least add a new arg for it.

[ranshid]

look good to me. That's also what i thought. Let's also make sure we won't kill the replication fork? Since it is a bgsave command, i think we should let replication out or at least add a new arg for it.

In AWS we encountered many cases of replication save which is taking too long and yield the system to a bad state. Sometimes the user would prefer to kill it and flush data then retry it to make the system back in good state. I do not mind adding explicit parameter to target save or replication, but I think killing the replication save is also needed

[PingXie]

couple of comments

1. Fun time again on naming :) have we considered other options? bgsave abort, bgsave stop, bgsave kill, etc? cancel seems to indicate some sort of gracefulness but based on the implementation it is just a process kill. So I was wondering if abort would be a better name?
2. I think @enjoy-binbin has a good point. It might not be how AWS uses it but if the goal is to "stop the bleeding" quickly, I think it makes sense in general to cancel the scheduled one too.
3. Can you document exactly what this action entails as part of the PR description at the top?

Since we are adding a new (sub)command, I consider this is a major decision so adding @valkey-io/core-team too

[ranshid]

couple of comments

1. Fun time again on naming :) have we considered other options? bgsave abort, bgsave stop, bgsave kill, etc? cancel seems to indicate some sort of gracefulness but based on the implementation it is just a process kill. So I was wondering if abort would be a better name?

I thought about other naming as well. IMO the kill/stop/abort is better in case we are not cancelling the scheduled save. In case we do cancell the scheduled save maybe cancel still has a good rational? I can also make separate sub-commands for each like: bgsave cancel - will cancel scheduled save bgsave kill/abort - will only stop a running bgsave

2. I think @enjoy-binbin has a good point. It might not be how AWS uses it but if the goal is to "stop the bleeding" quickly, I think it makes sense in general to cancel the scheduled one too.

I agree. we are in full consensus here. will add it ASAP

3. Can you document exactly what this action entails as part of the PR description at the top?

Definitely.

Since we are adding a new (sub)command, I consider this is a major decision so adding @valkey-io/core-team too

[ranshid]

@enjoy-binbin @PingXie when you have the time please take a look at the last update. Since we cannot have both arguments and subcommands the previous change actually broke 'bgsave schedule' :) I had to change the format of bgsave so basically this is now a breaking change IMO

[madolson]

@ranshid What is the value of having this be a subcommand? I feel like the previous Redis folks went overzealous about subcommands and made everything a subcommand. Given this is already an administration command, do we think should have the ability to restrict usage to cancel independently of starting a save?

[zuiderkwast]

In which way is killing the thread non-graceful? Does it leave any junk in the file system or is it visible to the user in any way?

KILL is exposing the fact that it runs in a process, which is not necessary to iexpose in the name IMO. Besides, having both KILL and CANCEL seems excessive to me and can be confusing users. Is there use cases for both?

In which way is it a breaking change to change BGSAVE from an argument to a subcommand? Isn't it just a different way to express the same syntax?

I don't see the point of making this a completely separate command for cancelling. BGSAVE is already an admin command. BGSAVE SCEDULE looks like a subcommand or parameter. It seems logical to me to add more related operations on the form BGSAVE [ SCEDULE | CANCEL | ... ]. It doesn't matter to me if it's subcommands or just arguments.

[PingXie]

In which way is it a breaking change to change BGSAVE from an argument to a subcommand?

We don't currently allow anything else but schedule to appear after bgsave so if someone is expecting the server to fail bgsave cancel they would see it succeeding instead. It is a behavior change but I also don't consider it "breaking".

It seems logical to me to add more related operations on the form BGSAVE [ SCEDULE | CANCEL | ... ]

+1. And also agreed that we don't need two. cancel works at the right abstraction level to me.

do we think should have the ability to restrict usage to cancel independently of starting a save?

I think cancel should remain an admin command.

[hwware]

BGSAVE [ SCEDULE | CANCEL | ... ]

I prefer this format: BGSAVE [ SCEDULE | CANCEL | KILL], and we need describe the difference between CANCEL and KILL CANCEL represents to cancel a scheduled job , and KILL means to shutdown an on going process.

[hwware]

@ranshid I just find one issue when I kill the child process by kill -9 child-pid command.

I run the memtier for heavy write commands, and execute "bgsave" on server. When server message: Background saving started by pid XXX, i manually kill the child-pid, then the memtier report the following error:

handle error response: -MISCONF Valkey is configured to save RDB snapshots, but it's currently unable to persist to disk. Commands that may modify the data set are disabled, because this instance is configured to report errors during writes if RDB snapshotting fails (stop-writes-on-bgsave-error option). Please check the Valkey logs for details about the RDB error.

I am not sure if you have the same issue when you do the test?

Note: bgsave kill call kill(server.child_pid, SIGUSR1);

[ranshid]

@ranshid What is the value of having this be a subcommand?

So having this as an argument does not really provide any kind of friendly interface. There is no way I could find to provide command argument description which states that 2 arguments cannot coexist. I have no special favor for subcommands, but this was the only way I could think of in order to provide strict way to state that you can either provide no argument (BGSAVE) or BGSAVE SCHEDULE OR BGSAVE CANCEL I would have also preferred to keep 'schedule' as an argument, since it makes more sense to me. but there is no way to mix arguments and sub-commands

In which way is killing the thread non-graceful? Does it leave any junk in the file system or is it visible to the user in any way?

We already have cases were the thread is killed and the filesystem cleanup is done as part of the signal handling. I also check this as part of the test.

KILL is exposing the fact that it runs in a process, which is not necessary to iexpose in the name IMO. Besides, having both KILL and CANCEL seems excessive to me and can be confusing users. Is there use cases for both?

Sure. I can change the kill and only provide the cancel option.

In which way is it a breaking change to change BGSAVE from an argument to a subcommand?

I think that the breaking is only once schedule is provided as a subcommand instead of a command argument is breaking the previous docs right? for example this is how it looks today:

> command docs

bgsave
summary
Asynchronously saves the database(s) to disk.
since
1.0.0
group
server
complexity
O(1)
history
3.2.2
Added the `SCHEDULE` option.
arguments
name
schedule
type
pure-token
display_text
schedule
token
SCHEDULE
since
3.2.2
flags
optional

and after this change it will be:

bgsave
summary
Asynchronously saves the database(s) to disk.
since
1.0.0
group
server
complexity
O(1)
history
3.2.2
Added the `SCHEDULE` option.
8.0.0
Refactor SCHEDULE as a subcommand
subcommands
bgsave|schedule
summary
Asynchronously saves the database(s) to disk. When an AOF rewrite is in progress and schedule the background save to r>
since
8.0.0
group
server
complexity
O(1)
bgsave|cancel
summary
Unschedule all scheduled bgsave requests.
since
8.0.0
group
server
complexity
O(1)

It seems logical to me to add more related operations on the form BGSAVE [ SCEDULE | CANCEL | ... ]. It doesn't matter to me if it's subcommands or just arguments.

As I pointed before. It can be implemented as arguments and we can enforce the correct arguments were supplied (like argc <=2 ) I just think that there is no current way to correctly describe that in command docs. If all of you feel that subcommand is not the correct way to go. I will change it back.

[ranshid]

@ranshid I just find one issue when I kill the child process by kill -9 child-pid command.

I run the memtier for heavy write commands, and execute "bgsave" on server. When server message: Background saving started by pid XXX, i manually kill the child-pid, then the memtier report the following error:

handle error response: -MISCONF Valkey is configured to save RDB snapshots, but it's currently unable to persist to disk. Commands that may modify the data set are disabled, because this instance is configured to report errors during writes if RDB snapshotting fails (stop-writes-on-bgsave-error option). Please check the Valkey logs for details about the RDB error.

I am not sure if you have the same issue when you do the test?

Note: bgsave kill call kill(server.child_pid, SIGUSR1);

@hwware you are killing the bgsave process with SIGKILL which is uncatchable. In this case we kill the process in more manageable (as you correctly pointed out) via SIGUSR1 which does not have the same issue.

[zuiderkwast]

Ah, I didn't think about COMMAND DOCS. When you say docs, I think about the rendered docs on web page and man page. Anyway, COMMAND DOCS is based on the same JSON files.

There is no way I could find to provide command argument description which states that 2 arguments cannot coexist.

In the JSON arguments, make it a "oneof" block with "optional": true, no?

I'm fine with subcommand. I'm just think args should be possible too.

[ranshid]

Ah, I didn't think about COMMAND DOCS. When you say docs, I think about the rendered docs on web page and man page. Anyway, COMMAND DOCS is based on the same JSON files.

There is no way I could find to provide command argument description which states that 2 arguments cannot coexist.

In the JSON arguments, make it a "oneof" block with "optional": true, no?

I'm fine with subcommand. I'm just think args should be possible too.

Let me try that. I think in arguments every top level argument must have a name and only the argument values can be enumerated, but I am far from being an expert. let me circle back on it

[zuiderkwast]

An arg with type "pure token" has no value. There's a README for the JSON format now.

[ranshid]

An arg with type "pure token" has no value. There's a README for the JSON format now.

TIL

[ranshid]

Thank you @zuiderkwast for the insights about the args :) I now only added the cancel option and updated the bgsave.json. Since now I think it is no longer a breaking change I will also remove the tag.

[ranshid]

I just noticed, the formulation in replies and log messages should be "background saving" (instead of "background bgsave") to match the existing messages.

I marked some occurrences but there are a few more.

Yeh I did use save initially. let me search and change it back

[madolson]

@valkey-io/core-team Please vote on adding this new argument to cancel on ongoing save. This is useful for admins.

[zuiderkwast]

@ranshid Do you want to prepare a doc PR, or at least create some issue for it so it's not forgotten?

[ranshid]

@ranshid Do you want to prepare a doc PR, or at least create some issue for it so it's not forgotten?

@zuiderkwast Sure will create a doc PR

[ranshid]

@ranshid Do you want to prepare a doc PR, or at least create some issue for it so it's not forgotten?

@zuiderkwast Sure will create a doc PR

@zuiderkwast - https://github.com/valkey-io/valkey-doc/pull/182