aof rewrite and rdb save counters in info #10178

yoav-steinberg · 2022-01-25T09:56:12Z

Add aof_rewrites and rdb_snapshots counters to info. This is useful to figure out if a rewrite or snapshot happened since last check. This was part of the (ongoing) effort to provide a safe backup solution for multipart-aof backups.

Note, it was eventually decided not to include a backup script in the redis repository and instead document how to safely backup AOF files, see: redis/redis-doc#1794

Original top-comment before backup script was removed from this PR:

AOF backup script defined in #10063.

In Redis 7 we introduced multi-part AOFs (#9788) based on a manifest file that indicates which files are part of Redis's AOF persistence. Our updated documentation about backup tells the user to simply copy the directory where all the AOF files (and manifest) reside.

This is wrong and bad advice. The reason is that during the copy files in the directory might change. During a rewrite we might end up copying an old manifest file and new base and increment files. We'll end up with a useless backup.

So this PR adds a backups script that performs the following:

Verify the server isn't performing a rewrite.
Create hard links to files in the directory.
Verify no rewrite started or happened since (1). If it did delete the hard links and go back to (1).
Copy/gzip the hard links.
Delete the hard links.

Other changes:
Include stat counters for aof rewrites and bgsaves:
Useful in general but also practical if we want to make sure a certain operation was performed without any aof rewrite in the middle.

fix docs aof backup doc PR accordingly.
Decide if we want to use a lock file, a mem flag or just use the existing auto-aof-rewrite-percentage (and fail backup if rewrite is in progress) config to disable rewrites.
If we create a new way to abort and disable rewrites then decide if we want it to be a generic way to to abort all forks coupled with auto-aof-rewrite-percentage or a specific command for rewrites.

oranagra

@yossigo my bash is not as fluent as my tcl 8-)
please review too.

utils/aof_backup.sh

oranagra · 2022-02-01T14:26:03Z

utils/aof_backup.sh

+while true; do
+    aof_rewrites=$(get_info_field aof_rewrites)
+    if [ $(get_info_field "aof_rewrite_in_progress") -eq 1 ]; then
+      echo "Redis is performing an AOF rewrite, waiting..."


this could be very verbose, maybe we should print just the first one? or add a silent mode?

I prefer not to complicate the script with "first time checks". Users can easily remove this or change this. I don't think it's that bad to have your terminal filled with these if you have 1800 seconds of a rewrite.

i do, but we can leave it for Yossi to rule

I agree with @oranagra, no need for a silent mode just a flag to say we've printed this message so let's not print it again as nothing changed.

utils/aof_backup.sh

Co-authored-by: Oran Agra <oran@redislabs.com>

chenyang8094

Some questions:

Do we need to support AOF backups when redis not running （When we don't specify -h -p option）
The current implementation is to directly establish a soft link to appenddir, do we need to deal with the case where multiple redis share the same dir (they have the same appenddir), I mean we should strictly follow the manifest file instructions to backup AOF
Do we need to be compatible with the macos ? for example, this code (readlink /proc/$pid/cwd) does not work in macos, but AFAIK probably many people will test this script on macos

oranagra · 2022-02-08T06:46:43Z

btw, can someone check if all this ln and /proc magic works on FreeBSD / MacOS?
@devnexen maybe you can take a look

Ohh @chenyang8094 i missed your above edit.
can you please suggest a readlink alternative that works?

devnexen · 2022-02-08T07:01:52Z

I would not rely on procfs for one does not exist on macOs and is optional on FreeBSD there is the procstat command line e.g. procstat pargs <pid>

yoav-steinberg · 2022-02-08T08:02:57Z

@chenyang8094 thanks for the comments,

Do we need to support AOF backups when redis not running （When we don't specify -h -p option）

I think the aim should be to provide a relatively simple script the user can modify or just read to understand how to safely backup their AOF. The goal I had in mind wasn't to provide a full blown backup solution (we don't even do this for rdb). In that respect I think we need to show how to backup a running server, and if the user needs to backup a downed server they can figure it out from the script.

The current implementation is to directly establish a soft link to appenddir, do we need to deal with the case where multiple redis share the same dir (they have the same appenddir), I mean we should strictly follow the manifest file instructions to backup AOF.

Right, I'll try to fix this. Thanks.

Do we need to be compatible with the macos ? for example, this code (readlink /proc/$pid/cwd) does not work in macos, but AFAIK probably many people will test this script on macos

I think this isn't critical, again, this is only an example. But if there's a good way to support both (trying to avoid adding os checks), I'm for it.

oranagra · 2022-02-08T08:15:06Z

i'm not sure about treating this script just as an example. @yossigo please share your opinion.
i may be ok about ignoring a down server, but i rather make the effort to support popular Unixes (depends on the compexity).

regarding multiple servers using the same folder, one option is to parse the manifest file, the other option is to use the prefix / pattern matching. the difference is that it'll include history files, but i suppose that's ok.

yoav-steinberg · 2022-02-08T08:19:21Z

regarding multiple servers using the same folder, one option is to parse the manifest file, the other option is to use the prefix / pattern matching. the difference is that it'll include history files, but i suppose that's ok.

In any case I'll need the prefix in order to find the correct manifest file. I also don't want the complexity of parsing the file. I'll just backup based on appendfilename.* prefix.

chenyang8094 · 2022-02-08T08:42:42Z

@yoav-steinberg If we distribute the script with the redis source code, I think it will not be an example, and many users will use it, like the redis-check-aof tool. So I think we need to take it more seriously.

- use `lsof` instead or procfs to find server's working dir. - use no support for `-p` relative dir in `mktemp` - no support for file change warning suppression in `tar`.

chenyang8094

.

chenyang8094 · 2022-02-08T10:57:49Z

utils/aof_backup.sh

+    exit 1
+fi
+
+if [ $(get_config appendonly) != yes ]; then


If redis once enabled AOF (and created many AOF files) but disable it later, i think we need to support this scenario.

yossigo

Added a few specific comments.

I think there are bigger issues around this script, as @chenyang8094 mentioned: we expect the server to be up, but what if it's down? or restarting? or changing configuration? Not sure we can cover all cases.

I think we should also consider other options where Redis is instructed to hold back a rewrite because a backup is in progress.

yossigo · 2022-02-08T19:27:33Z

utils/aof_backup.sh

+pid=$(get_info_field process_id)
+appenddirname=$(get_config appenddirname)
+appendfilename=$(get_config appendfilename).
+working_dir=$(lsof -a -p $pid -d cwd -Fn | grep "^n" | sed "s/n\//\//")


Why not get_config dir? I wouldn't assume lsof is always available.

redis dir is relative to the the working directory. I need the working directory to know how to treat all other locations.
Couldn't find any good way to find the working directory on bsd/macos other than lsof. I can check the os and use procf fs on linux. Or verify lsof exists.

yossigo · 2022-02-08T19:28:14Z

utils/aof_backup.sh

+appendfilename=$(get_config appendfilename).
+working_dir=$(lsof -a -p $pid -d cwd -Fn | grep "^n" | sed "s/n\//\//")
+appenddir_path=$working_dir/$appenddirname
+if [ ! -d $appenddir_path ]; then


Suggested change

if [ ! -d $appenddir_path ]; then

if [ ! -d "$appenddir_path" ]; then

Better practice.

This repeats in many places, I did not comment everywhere.

yossigo · 2022-02-08T19:29:02Z

utils/aof_backup.sh

+working_dir=$(lsof -a -p $pid -d cwd -Fn | grep "^n" | sed "s/n\//\//")
+appenddir_path=$working_dir/$appenddirname
+if [ ! -d $appenddir_path ]; then
+    echo "Couldn't find $appenddir_path. redis-server must be run locally. Aborting."


This could also fail due to permissions.

yossigo · 2022-02-08T19:29:18Z

utils/aof_backup.sh

+appenddirname=$(get_config appenddirname)
+appendfilename=$(get_config appendfilename).
+working_dir=$(lsof -a -p $pid -d cwd -Fn | grep "^n" | sed "s/n\//\//")
+appenddir_path=$working_dir/$appenddirname


Suggested change

appenddir_path=$working_dir/$appenddirname

appenddir_path="$working_dir/$appenddirname"

yossigo · 2022-02-08T19:31:26Z

utils/aof_backup.sh

+while true; do
+    aof_rewrites=$(get_info_field aof_rewrites)
+    if [ $(get_info_field "aof_rewrite_in_progress") -eq 1 ]; then
+      echo "Redis is performing an AOF rewrite, waiting..."


I agree with @oranagra, no need for a silent mode just a flag to say we've printed this message so let's not print it again as nothing changed.

yossigo · 2022-02-08T19:33:59Z

utils/aof_backup.sh

+    backup_path=$tmp_dir/$appenddirname
+    mkdir $backup_path
+
+    ln $appenddir_path/$appendfilename* $backup_path


We should probably add an earlier check to make sure we're not crossing filesystem boundaries here, because links won't work in that case.

IIUC we made sure to create the temp folder inside dir, so shouldn't that be covered?

I think it's covered now except if someone mounts another fs under dir which is a case I'm willing to ignore.

yoav-steinberg · 2022-02-09T07:38:14Z

I think there are bigger issues around this script, as @chenyang8094 mentioned: we expect the server to be up, but what if it's down? or restarting? or changing configuration? Not sure we can cover all cases.

I think we should also consider other options where Redis is instructed to hold back a rewrite because a backup is in progress.

@oranagra @yossigo I'm getting the feeling I'm slowly being nudged to create a full blow backup solution for AOF in bash. I originally proposed we drop official support for AOF backups because it's pretty sketchy whether this makes sense given Redis's RDB feature. I'd like us to have a talk about what's the aim of this script... Once we pass a certain level of complexity I think we should start thinking of some export feature in Redis and not a script.

yoav-steinberg · 2022-02-10T08:45:54Z

Had a talk with @yossigo & @oranagra relating to above concerns:

The script is complex and requires redis-cli and other utilities in order to run.
We'll need to maintain the script in the future and we'll probably need to fix stuff like OS support or other usage configurations we haven't thought of.
We'd like a solution that's easy to document and explain to users and will also work if the server is down or restarts during the process.

@yossigo Suggested we create a mechanism for disabling/aborting the rewrite process and persisting this setting between restarts: create an appendonly.lock file in the appendonlydir. Redis will never perform rewrites if this file is present. We'll add a command REWRITEAOF_LOCK ON|OFF which creates the lock file aborts any running aof rewrite (on ON) and deletes the lock file on OFF.

After the server is protected against doing rewrites we can safely create hard links/copy the dir or whatever we want. Then we can delete the lock file or call the command with OFF.

We'll document this mechanism and it's logic so users can create their own backup scripts/logic instead of having to maintain a script that handles all edge cases.

chenyang8094 · 2022-02-10T09:39:33Z

I don't think it's a good suggestion to create appendonly.lock files for the following reasons:

AOFRW has to perform a file system call to check if the file exists before executing
Once we or the user forget to delete the appendonly.lock file, we will have an increasing AOF
If the user accidentally copies and backs up appendonly.lock together, when a redis uses this backup in the future, it will not be able to execute AOFRW.

oranagra · 2022-02-10T11:21:21Z

maybe the lock file should be in dir and not in appendonlydir? (will reduce the chance someone copies it).
or maybe we can relax the concern about server restart, and just keep this flag in memory.

yoav-steinberg · 2022-02-10T13:30:13Z

maybe the lock file should be in dir and not in appendonlydir? (will reduce the chance someone copies it).

I'm afraid we need it in appendonlydir to make sure it doesn't conflict with any other Redis instance. It'll need to be under <dir>/<appendonlydir>/<appendonlyname>.lock. I think this is the only way to make sure it won't conflict with other Redises.

or maybe we can relax the concern about server restart, and just keep this flag in memory.

We wanted to make sure we have a solution for these cases. So I'm not sure we want to back down from that now. What we can do is print a big warning if Redis starts with this in the directory, or even fail to start in such a case.

yossigo · 2022-02-10T17:41:08Z

I think keeping the flag in memory might be enough. A server that restarts and immediately rewrites and completes the rewrite before creating links of all files is possible, but very unlikely..

chenyang8094 · 2022-02-11T00:01:31Z

I think we can keep the changes in stat_aof_rewrites and stat_rdb_saves in this PR, they do work in reality and give us the opportunity to safely back up multi part aof.

Also, if we don't plan to maintain and improve this backup script, we shouldn't release it in the redis source code, instead, we can put it in the redis documentation as a suggestion or an example for users to refer to (the only uncertainty is that how many people will actually see it).

Another suggestion is that we abandon the use of scripts and instead directly implement the C code tool of redis-backup-aof (similar to redis-check-aof), so that we can abandon the dependence on tools such as redis-cli, so it can be distributed in redis code and be an AOF tool out of the box.

p.s. If we want to temporarily disable AOFRW, we can set auto-aof-rewrite-percentage to 0, which will disable auto-triggered AOFRW (I believe users don't intentionally trigger AOFRW manually when backing up). And If we only want to know whether AOFRW occurred during the backup process, we also can detect whether the content of the manifest file has changed (for example, create a manifest MD5 when starting the backup)

yoav-steinberg · 2022-02-14T07:10:39Z

I think we can keep the changes in stat_aof_rewrites and stat_rdb_saves in this PR, they do work in reality and give us the opportunity to safely back up multi part aof.

👍 I'll change this PR to only include the stats and update the top comment.

Also, if we don't plan to maintain and improve this backup script, we shouldn't release it in the redis source code, instead, we can put it in the redis documentation as a suggestion or an example for users to refer to (the only uncertainty is that how many people will actually see it).

👍 I agree. So given the intention to make the mechanism simpler by adding a block rewrite mechanism, we'll just document this and forget about the script.

Another suggestion is that we abandon the use of scripts and instead directly implement the C code tool of redis-backup-aof (similar to redis-check-aof), so that we can abandon the dependence on tools such as redis-cli, so it can be distributed in redis code and be an AOF tool out of the box.

I think this is too complex and will hide the internals of the mechanism which we want (advanced) users to be able to understand and customize for their own needs.

p.s. If we want to temporarily disable AOFRW, we can set auto-aof-rewrite-percentage to 0, which will disable auto-triggered AOFRW (I believe users don't intentionally trigger AOFRW manually when backing up). And If we only want to know whether AOFRW occurred during the backup process, we also can detect whether the content of the manifest file has changed (for example, create a manifest MD5 when starting the backup)

This is a decent option, but bear in mind that changing auto-aof-rewrite-percentage to 0 won't abort a running rewrite. So adding a mechanism for this will make performing backups simpler (no need to check if there's a rewrite after disabling rewrites).

oranagra

@redis/core-team please approve two new counters in INFO stats.

Add aof_rewrites and rdb_snapshots counters to info. This is useful to figure our if a rewrite or snapshot happened since last check. This was part of the (ongoing) effort to provide a safe backup solution for multipart-aof backups.

enjoy-binbin · 2022-06-07T05:11:45Z

src/rdb.c

@@ -1471,6 +1471,7 @@ int rdbSaveBackground(int req, char *filename, rdbSaveInfo *rsi) {
    pid_t childpid;

    if (hasActiveChildProcess()) return C_ERR;
+    server.stat_rdb_saves++;


i notice we only do server.stat_rdb_saves++ in rdbSaveBackground
and did not do it in SAVE command, (or in FLUSHALL it may trigger rdbsave) (or other)

I think it should be counted in the save command?
or maybe we should put it near server.lastsave?

i agree that we should increment it in the SAVE command too, but i'm not sure about the others.

i think there's value (or at least it's very different) setting it before we fork, rather than at completion (in which case we need to decide if we count failures too).

i don't think the implicit save of FLUSHALL should count, arguably it's the same as deleting the old persistence file, rather than saving an empty one.

i don't think we should count the empty rdb we save when creating a an empty base for a multipart AOF when starting up empty.

yes, the one in SAVE command is my first thinking, and then I looked at the others (mention in case we forget).
as long as we agree with the stat field meaning, just a random look
i will make a PR with the SAVE one

Currently, we only increment stat_rdb_saves in rdbSaveBackground, we should also increment it in the SAVE command. The stat counter was introduced in redis#10178

Currently, we only increment stat_rdb_saves in rdbSaveBackground, we should also increment it in the SAVE command. We concluded there's no need to increment when: 1. saving a base file for an AOF 2. when saving an empty rdb file to delete an old one 3. when saving to sockets (not creating a persistence / snapshot file) The stat counter was introduced in #10178 * fix a wrong comment in startSaving

Currently, we only increment stat_rdb_saves in rdbSaveBackground, we should also increment it in the SAVE command. We concluded there's no need to increment when: 1. saving a base file for an AOF 2. when saving an empty rdb file to delete an old one 3. when saving to sockets (not creating a persistence / snapshot file) The stat counter was introduced in redis#10178 * fix a wrong comment in startSaving

Stats counters for aof rewrites and bgsaves

cd18bd1

yoav-steinberg added the state:needs-doc-pr requires a PR to redis-doc repository label Jan 25, 2022

oranagra added this to Backlog in 7.0 via automation Jan 25, 2022

oranagra moved this from Backlog to In progress in 7.0 Jan 25, 2022

yoav-steinberg changed the title ~~Stat counters for aof rewrites and bgsaves~~ AOF backup script Jan 25, 2022

yoav-steinberg marked this pull request as draft January 25, 2022 10:15

yoav-steinberg added 3 commits January 27, 2022 13:42

AOF backup script for multipart AOF

f08a6c7

cleanup and docs

fcf82de

cleanup

947904d

yoav-steinberg marked this pull request as ready for review January 27, 2022 11:51

yoav-steinberg added 2 commits January 27, 2022 13:53

cleanup

614c30d

Comment related to tar warning supression

1a42813

yoav-steinberg mentioned this pull request Jan 28, 2022

fix multipart aof backup doc redis/redis-doc#1760

Merged

oranagra linked an issue Jan 31, 2022 that may be closed by this pull request

backing up multi-prart-aof #10063

Closed

oranagra added the 7.0-must-have label Jan 31, 2022

oranagra reviewed Feb 1, 2022

View reviewed changes

oranagra changed the title ~~AOF backup script~~ Safe multi-part AOF backup script Feb 1, 2022

yoav-steinberg added the release-notes indication that this issue needs to be mentioned in the release notes label Feb 1, 2022

oranagra moved this from In progress to In Review in 7.0 Feb 3, 2022

yoav-steinberg and others added 2 commits February 3, 2022 17:23

Update utils/aof_backup.sh

28e7de7

Co-authored-by: Oran Agra <oran@redislabs.com>

cr fix

5830213

yoav-steinberg requested a review from yossigo February 5, 2022 21:18

chenyang8094 reviewed Feb 8, 2022

View reviewed changes

CR: use appendfilename in case multiple Redises using same aof dir.

53f656b

CR: macos/bsd support.

7887c47

- use `lsof` instead or procfs to find server's working dir. - use no support for `-p` relative dir in `mktemp` - no support for file change warning suppression in `tar`.

chenyang8094 suggested changes Feb 8, 2022

View reviewed changes

yossigo reviewed Feb 8, 2022

View reviewed changes

yoav-steinberg added 2 commits February 14, 2022 12:11

Merge branch 'unstable' into aof_rewrites_bgsaves_counters

86eed7a

Remove backup script in favor of simpler documentaion based solution.

afedb5e

yoav-steinberg changed the title ~~Safe multi-part AOF backup script~~ aof rewrite and rdb save counters in info Feb 14, 2022

oranagra approved these changes Feb 16, 2022

View reviewed changes

oranagra moved this from In Review to Awaits merge in 7.0 Feb 16, 2022

yossigo approved these changes Feb 16, 2022

View reviewed changes

itamarhaber approved these changes Feb 17, 2022

View reviewed changes

oranagra merged commit 56fa48f into redis:unstable Feb 17, 2022

oranagra added the state:major-decision Requires core team consensus label Feb 17, 2022

oranagra moved this from Awaits merge to Unreleased in 7.0 Feb 17, 2022

oranagra mentioned this pull request Feb 28, 2022

Release 7.0 RC2 #10355

Merged

oranagra moved this from Unreleased to Done in 7.0 Mar 1, 2022

enjoy-binbin reviewed Jun 7, 2022

View reviewed changes

enjoy-binbin mentioned this pull request Jun 7, 2022

Increment the stat_rdb_saves counter in SAVE command #10827

Merged

	if [ ! -d $appenddir_path ]; then
	if [ ! -d "$appenddir_path" ]; then

	appenddir_path=$working_dir/$appenddirname
	appenddir_path="$working_dir/$appenddirname"

aof rewrite and rdb save counters in info #10178

aof rewrite and rdb save counters in info #10178

Conversation

yoav-steinberg commented Jan 25, 2022 • edited Loading

oranagra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yoav-steinberg Feb 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenyang8094 left a comment • edited Loading

Choose a reason for hiding this comment

oranagra commented Feb 8, 2022 • edited Loading

devnexen commented Feb 8, 2022

yoav-steinberg commented Feb 8, 2022

oranagra commented Feb 8, 2022

yoav-steinberg commented Feb 8, 2022

chenyang8094 commented Feb 8, 2022

chenyang8094 left a comment • edited Loading

Choose a reason for hiding this comment

chenyang8094 Feb 8, 2022 • edited Loading

Choose a reason for hiding this comment

yossigo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yoav-steinberg Feb 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yoav-steinberg commented Feb 9, 2022

yoav-steinberg commented Feb 10, 2022

chenyang8094 commented Feb 10, 2022 • edited Loading

oranagra commented Feb 10, 2022

yoav-steinberg commented Feb 10, 2022

yossigo commented Feb 10, 2022

chenyang8094 commented Feb 11, 2022 • edited Loading

yoav-steinberg commented Feb 14, 2022

oranagra left a comment

Choose a reason for hiding this comment

enjoy-binbin Jun 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yoav-steinberg commented Jan 25, 2022 •

edited

Loading

yoav-steinberg Feb 3, 2022 •

edited

Loading

chenyang8094 left a comment •

edited

Loading

oranagra commented Feb 8, 2022 •

edited

Loading

chenyang8094 left a comment •

edited

Loading

chenyang8094 Feb 8, 2022 •

edited

Loading

yoav-steinberg Feb 9, 2022 •

edited

Loading

chenyang8094 commented Feb 10, 2022 •

edited

Loading

chenyang8094 commented Feb 11, 2022 •

edited

Loading

enjoy-binbin Jun 7, 2022 •

edited

Loading