Adapt redis-check-aof tool for Multi Part Aof #10061

chenyang8094 · 2022-01-06T04:53:13Z

issue link : #9788 (comment)

Modifications of this PR:

Support the verification of Multi Part AOF, while still maintaining support for the old-style AOF/RDB-preamble. redis-check-aof will automatically choose which mode to use according to the incoming file format.

Usage: redis-check-aof [--fix|--truncate-to-timestamp $timestamp] <AOF/manifest>
Refactor part of the code to make it easier to understand
Currently only supports truncate (--fix or --truncate-to-timestamp) the last AOF file (may be BASE or INCR)

The reasons for 3 above:

for --fix: Only the last AOF may be truncated, this is guaranteed by redis
for --truncate-to-timestamp: Normally, we only have BASE + INCR files at most, and BASE cannot be truncated(It only contains a timestamp annotation at the beginning of the file), so only INCR can be truncated. If we have a BASE+INCR1+INCR2 file (meaning we have an interrupted AOFRW), Only INCR2 files can be truncated at this time. If we still insist on truncate INCR1, we need to manually delete INCR2 and update the manifest file, then re-run redis-check-aof
If we want to support truncate any file, we need to add very complicated code to support the atomic modification of multiple file deletion and update manifest, I think this is unnecessary

chenyang8094 · 2022-01-06T04:54:19Z

PAT @yoav-steinberg

oranagra · 2022-01-06T07:42:16Z

@chenyang8094 i didn't review the code yet, but i'd like to respond to the top comment.

i think redis-check-aof should still support checking old aof files (preamble or non-preamble files without manifest). i think someone may be using a new redis-check-aof with on an AOF file from an old server. (if this complicates the code too much, maybe we can drop that, but i'd like to try supporting it).
i think your reasoning above about not truncating or fixing INCR files other than the last one, are acceptable. but i think we need to print a message to a user trying to use --truncate-to-timestamp, telling him what to do (edit the manifest file manually). i.e. if we find the timestamp in a non-last INCR file, we should tell him in which file we found it, and instruct him to edit the manifest and re-run.

yoav-steinberg · 2022-01-06T07:44:16Z

Before I review here are some questions:
1.

If we want to support truncate any file, we need to add very complicated code to support the atomic modification of multiple file deletion and update manifest, I think this is unnecessary

I don't think the tool needs to be atomic. If we have BASE+INCR1+INCR2 and the specified timestamp is in the middle of INCR1 then we need to delete to truncate INCR1, delete INCR2 and update the manifest. I think it's OK not to be atomic here. The user should always backup the directory before running this tool.

Don't we still support a non RDB BASE? If so then the tool can also truncate the base file in such a case.
Is the tool still backwards compatible with the old (non multi-part) AOF file?

oranagra · 2022-01-06T07:57:03Z

Don't we still support a non RDB BASE? If so then the tool can also truncate the base file in such a case.

Ahe AOF file produced by rewrite won't have timestamps.

And note that corruptions are only expected at the last INCR file (due to fsync or bad mount options)

chenyang8094 · 2022-01-06T07:58:17Z

i think redis-check-aof should still support checking old aof files (preamble or non-preamble files without manifest). i think someone may be using a new redis-check-aof with on an AOF file from an old server. (if this complicates the code too much, maybe we can drop that, but i'd like to try supporting it).

Unless we add special parameters, I can't think of any effective way to identify whether this file is an old AOF or a manifest. It is not reliable to rely on the file name alone.

i think your reasoning above about not truncating or fixing INCR files other than the last one, are acceptable. but i think we need to print a message to a user trying to use --truncate-to-timestamp, telling him what to do (edit the manifest file manually). i.e. if we find the timestamp in a non-last INCR file, we should tell him in which file we found it, and instruct him to edit the manifest and re-run.

I thought, in this case, the user will see the following message:

Failed to truncate AOF appendonly.aof.1.incr.aof to timestamp 1628217471 because it is not the last file.
If you insist, please delete all files after this file according to the manifest file and delete the corresponding records in manifest file manually. Then re-run redis-check-aof.

oranagra · 2022-01-06T08:07:10Z

regarding the detection of an old AOF file, or a manifest i'm not sure it's that bad to rely on the file name (we do control the name of the manifest file, and it's unlikely that users will name their old AOF files with a "manifest" suffix).
however, i think it's not that complicated to detect automatically by the content.
either we try to match the first word we know must be on the first line of the manifest, or even add some new unique header to our manifest files (it's not too late).

regarding the truncation message, that's great.
two suggestions for improvements:

we can take this opportunity to mention the offset we found in that file (so user can use other tools to manually truncate it, rather than go though the slow AOF file processing again)
do we really have to instruct them to delete the files? isn't it enough to just modify the manifest?

chenyang8094 · 2022-01-06T08:07:42Z

I don't think the tool needs to be atomic. If we have BASE+INCR1+INCR2 and the specified timestamp is in the middle of INCR1 then we need to delete to truncate INCR1, delete INCR2 and update the manifest. I think it's OK not to be atomic here. The user should always backup the directory before running this tool.

We only recommend that users back up but it is not mandatory. Without the guarantee of atomicity, I don't want to destroy the original data. In addition, if it crashes due to some reason in the middle, and we have no chance to print out enough log, may cause the manifest and AOF to be incompatible, the user will not know whether the problem is caused by the redis-check-aof tool itself or the original data error. This will be confusing.

Don't we still support a non RDB BASE? If so then the tool can also truncate the base file in such a case.

Of course we can (if BASE is the last file).

Is the tool still backwards compatible with the old (non multi-part) AOF file?

No, we are discussing this.

chenyang8094 · 2022-01-06T08:11:06Z

The manifest must start with file ， like：
file appendonly.aof.1.incr.aof seq 1 type i

So, we can do like this:

if (file start  with REDIS) {
   check rdb file
} else if (file start with file) {
  check multi part aof
} else {
  check old aof
}

do we really have to instruct them to delete the files? isn't it enough to just modify the manifest?

Yes, only modifying the manifest is enough, but a better suggestion is to delete the files together, otherwise these files will never be seen by redis and will not be recycled by the GC mechanism.

oranagra · 2022-01-06T08:21:13Z

ok, so i think we can agree on:

no need to automatically handle truncation of non-last files (for both atomicity and complexity concerns)
detect old aof files vs manifest files like you just described.
keep the message about delete (or move) files when you manually remove them from the manifest.
let's add the offset inside the file where the truncation is needed in the log message.

yoav-steinberg · 2022-01-06T09:11:39Z

For the record I think a recovery tool shouldn't worry about atomicity. For two main reasons:

When dealing with production issues that require manual fixes to get back up online we always perform non-atomic operations. We don't have the luxury to stop and consider what happens if we have another failure (causing atomicity issues) while performing the recovery process. This is simply in order to fix things and get back online as soon as possible because developing an atomic custom recovery plan during a crisis is never practical.
To mitigate the risk in 1. we backup our data or any configuration before performing a risky operation. Sometimes the cost of this is large (copying a 500GB AOF file is expensive) but there's no practical way around it.
I think this tool should in general disregard atomicity concerns but should definitely warn the user to backup the files before applying any fixes to them.

I'm also fine with not editing the manifest file at this point to keep the tool simple (in 99% of the cases we won't need to truncate a non last INCR file), but I don't think atomicity has anything to do with it.

chenyang8094 · 2022-01-06T10:10:17Z

@yoav-steinberg Aside from atomicity, but it does bring a lot of complexity. Thinking from another perspective, if the user really wants to truncate the non-last INCR file, we print a friendly message and ask him to manually delete these files (this time he may realize that these files should be backed up first). I think this is a safe practice, and deleting files is a very easy thing for user, and the probability of this happening is very small.

chenyang8094 · 2022-01-06T10:11:20Z

@oranagra 1，2，3，4 all done.

src/aof.c

src/redis-check-aof.c

src/aof.c

src/redis-check-aof.c

oranagra

there's quite a lot of refactoring here, which is good since the old code was out of shape.
maybe though some of this refactoring could have been left aside for another chance (like the process function which i'm not sure why you had to refactor).

additionally, if we already do so much work here, let's try to further improve.
specifically i feel some doc comments are still missing.

src/redis-check-aof.c

tests/integration/aof.tcl

oranagra

LGTM.

src/redis-check-aof.c

Modifications of this PR: 1. Support the verification of `Multi Part AOF`, while still maintaining support for the old-style `AOF/RDB-preamble`. `redis-check-aof` will automatically choose which mode to use according to the incoming file format. `Usage: redis-check-aof [--fix|--truncate-to-timestamp $timestamp] <AOF/manifest>` 2. Refactor part of the code to make it easier to understand 3. Currently only supports truncate (`--fix` or `--truncate-to-timestamp`) the last AOF file (may be `BASE` or `INCR`) The reasons for 3 above: - for `--fix`: Only the last AOF may be truncated, this is guaranteed by redis - for `--truncate-to-timestamp`: Normally, we only have `BASE` + `INCR` files at most, and `BASE` cannot be truncated(It only contains a timestamp annotation at the beginning of the file), so only `INCR` can be truncated. If we have a `BASE+INCR1+INCR2` file (meaning we have an interrupted AOFRW), Only `INCR2` files can be truncated at this time. If we still insist on truncate `INCR1`, we need to manually delete `INCR2` and update the manifest file, then re-run `redis-check-aof` - If we want to support truncate any file, we need to add very complicated code to support the atomic modification of multiple file deletion and update manifest, I think this is unnecessary

oranagra · 2022-02-20T20:51:01Z

@chenyang8094 please take a look at this failure
https://github.com/redis/redis/runs/5266360953?check_suite_focus=true#step:5:1732

child process exited abnormally
    while executing
"exec src/redis-check-aof --truncate-to-timestamp 1628217470 $aof_manifest_file"
    ("uplevel" body line 23)
    invoked from within
"uplevel 1 $code"
    (procedure "test" line 51)
    invoked from within
"test {Truncate AOF to specific timestamp} {

oranagra · 2022-02-21T14:54:47Z

another similar error:
https://github.com/redis/redis/runs/5273856527?check_suite_focus=true#step:5:1456

[err]: Short read: Utility should confirm the AOF is not valid in tests/integration/aof.tcl
Expected 'Start checking Multi Part AOF
Start to check INCR files.
Cannot open file: appendonly.aof.1.incr.aof, aborting...
child process exited abnormally' to match '*not valid*' (context: type eval line 5 cmd {assert_match "*not valid*" $result} proc ::test)

i don't understand how we could get "aborting...", if we didn't pass the --fix argument.

chenyang8094 · 2022-02-22T02:36:42Z

@oranagra GOT, I have added more logs (printf the errno and AOF abs path) in my branch and re-run the workflow, but so far it has not reappeared.

chenyang8094 · 2022-02-22T03:40:45Z

finally reappeared.

It looks like we are getting a wrong path, is there a bug in the dirname function?

[exception]: Executing test client: Start checking Multi Part AOF
Start to check INCR files.
Cannot open file ./tests/tmp/server.aof.2565.11/appendonlydir/appendonly.aof.manifest/appendonly.aof.1.incr.aof: Not a directory, aborting...
child process exited abnormally.
Start checking Multi Part AOF

chenyang8094 · 2022-02-22T06:14:06Z

I add this log:

and finally reproduced it (https://github.com/chenyang8094/redis/runs/5283813036?check_suite_focus=true):

So it's dirname that is returning us the wrong path, I'm not sure that dirname will have some kind of exception under SANITIZER=address, at least I haven't found a similar answer yet.

Copy a glibc implementation:

char *dirname(char *path) {
    static const char dot[] = ".";
    char *last_slash;
    /* Find last '/'.  */
    last_slash = path != NULL ? strrchr(path, '/') : NULL;
    if (last_slash != NULL && last_slash != path && last_slash[1] == '\0') {
        /* Determine whether all remaining characters are slashes.  */
        char *runp;
        for (runp = last_slash; runp != path; --runp)
            if (runp[-1] != '/') break;
        /* The '/' is the last character, we have to look further.  */
        if (runp != path) last_slash = __memrchr(path, '/', runp - path);
    }
    if (last_slash != NULL) {
        /* Determine whether all remaining characters are slashes.  */
        char *runp;
        for (runp = last_slash; runp != path; --runp)
            if (runp[-1] != '/') break;
        /* Terminate the path.  */
        if (runp == path) {
            /* The last slash is the first character in the string.  We have to
               return "/".  As a special case we have to return "//" if there
               are exactly two slashes at the beginning of the string.  See
               XBD 4.10 Path Name Resolution for more information.  */
            if (last_slash == path + 1)
                ++last_slash;
            else
                last_slash = path + 1;
        } else
            last_slash = runp;
        last_slash[0] = '\0';
    } else
        /* This assignment is ill-designed but the XPG specs require to
           return a string containing "." in any case no directory part is
           found and so a static and constant string is required.  */
        path = (char *)dot;
    return path;
}

chenyang8094 · 2022-02-22T06:51:12Z

@yossigo @oranagra Do you have any relevant insights on this?

chenyang8094 · 2022-02-22T07:41:00Z

finally found the bug:

    char temp_filepath[PATH_MAX + 1];
    memcpy(temp_filepath, filepath, strlen(filepath));
    dirpath = dirname(temp_filepath);

Since temp_filepath is allocated on the stack and not initialized, and we did not copy '\0' when executing memcpy, so when a lot of '\' are left on the stack, dirname may get a wrong result.

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

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

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

chenyang8094 closed this Jan 6, 2022

chenyang8094 reopened this Jan 6, 2022

Adapt redis-check-aof to multi-part-aof

5cd1d04

yoav-steinberg reviewed Jan 10, 2022

View reviewed changes

chenyang8094 added 3 commits January 10, 2022 21:42

Add error check for strtol

a9a28a6

Update log message

63b5a8f

Merge branch 'unstable' into adapt-check-aof-for-multi-aof

6aa2c38

chenyang8094 requested a review from yoav-steinberg January 11, 2022 00:09

yoav-steinberg reviewed Jan 11, 2022

View reviewed changes

src/aof.c Outdated Show resolved Hide resolved

src/redis-check-aof.c Outdated Show resolved Hide resolved

Update comments

485c111

chenyang8094 requested a review from yoav-steinberg January 11, 2022 07:26

oranagra reviewed Jan 20, 2022

View reviewed changes

chenyang8094 and others added 3 commits January 25, 2022 17:20

Delete the residual code related to aof rewrite buf

3941b14

Merge branch 'redis:unstable' into unstable

8e7bb20

Merge branch 'redis:unstable' into unstable

d09ebcb

Merge branch 'redis:unstable' into unstable

5a635c9

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

chenyang8094 and others added 2 commits February 8, 2022 08:41

Merge branch 'redis:unstable' into unstable

8115a34

Merge branch 'unstable' into adapt-check-aof-for-multi-aof

7c92872

oranagra added the release-notes indication that this issue needs to be mentioned in the release notes label Feb 14, 2022

chenyang8094 added 5 commits February 16, 2022 15:38

Add comments and some update

05daf8e

Refact aofLoadManifestFromDisk

708080e

Add comments

4e8cc18

Fix assert error

7569c2f

Add tests

a42981b

chenyang8094 requested a review from oranagra February 16, 2022 09:55

Some update

9517b9a

oranagra approved these changes Feb 16, 2022

View reviewed changes

src/redis-check-aof.c Outdated Show resolved Hide resolved

src/redis-check-aof.c Outdated Show resolved Hide resolved

Fix comments repetition

c3c84b0

oranagra approved these changes Feb 17, 2022

View reviewed changes

oranagra merged commit a50aa29 into redis:unstable Feb 17, 2022

oranagra moved this from In Review to Unreleased in 7.0 Feb 17, 2022

chenyang8094 mentioned this pull request Feb 22, 2022

Fix path copy bug and add more logs. #10324

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adapt redis-check-aof tool for Multi Part Aof #10061

Adapt redis-check-aof tool for Multi Part Aof #10061

chenyang8094 commented Jan 6, 2022 •

edited

Loading

chenyang8094 commented Jan 6, 2022

oranagra commented Jan 6, 2022

yoav-steinberg commented Jan 6, 2022

oranagra commented Jan 6, 2022

chenyang8094 commented Jan 6, 2022

oranagra commented Jan 6, 2022

chenyang8094 commented Jan 6, 2022

chenyang8094 commented Jan 6, 2022 •

edited

Loading

oranagra commented Jan 6, 2022

yoav-steinberg commented Jan 6, 2022

chenyang8094 commented Jan 6, 2022 •

edited

Loading

chenyang8094 commented Jan 6, 2022

oranagra left a comment

oranagra left a comment

oranagra commented Feb 20, 2022 •

edited

Loading

oranagra commented Feb 21, 2022

chenyang8094 commented Feb 22, 2022 •

edited

Loading

chenyang8094 commented Feb 22, 2022 •

edited

Loading

chenyang8094 commented Feb 22, 2022 •

edited

Loading

chenyang8094 commented Feb 22, 2022

chenyang8094 commented Feb 22, 2022 •

edited

Loading

Adapt redis-check-aof tool for Multi Part Aof #10061

Adapt redis-check-aof tool for Multi Part Aof #10061

Conversation

chenyang8094 commented Jan 6, 2022 • edited Loading

chenyang8094 commented Jan 6, 2022

oranagra commented Jan 6, 2022

yoav-steinberg commented Jan 6, 2022

oranagra commented Jan 6, 2022

chenyang8094 commented Jan 6, 2022

oranagra commented Jan 6, 2022

chenyang8094 commented Jan 6, 2022

chenyang8094 commented Jan 6, 2022 • edited Loading

oranagra commented Jan 6, 2022

yoav-steinberg commented Jan 6, 2022

chenyang8094 commented Jan 6, 2022 • edited Loading

chenyang8094 commented Jan 6, 2022

oranagra left a comment

Choose a reason for hiding this comment

oranagra left a comment

Choose a reason for hiding this comment

oranagra commented Feb 20, 2022 • edited Loading

oranagra commented Feb 21, 2022

chenyang8094 commented Feb 22, 2022 • edited Loading

chenyang8094 commented Feb 22, 2022 • edited Loading

chenyang8094 commented Feb 22, 2022 • edited Loading

chenyang8094 commented Feb 22, 2022

chenyang8094 commented Feb 22, 2022 • edited Loading

chenyang8094 commented Jan 6, 2022 •

edited

Loading

chenyang8094 commented Jan 6, 2022 •

edited

Loading

chenyang8094 commented Jan 6, 2022 •

edited

Loading

oranagra commented Feb 20, 2022 •

edited

Loading

chenyang8094 commented Feb 22, 2022 •

edited

Loading

chenyang8094 commented Feb 22, 2022 •

edited

Loading

chenyang8094 commented Feb 22, 2022 •

edited

Loading

chenyang8094 commented Feb 22, 2022 •

edited

Loading