Skip to content

Remove dump.rdb on upgrade

username-removed-622111 requested to merge cataphract/omnibus-gitlab:redis_incompat_rdb into master

See https://gitlab.com/gitlab-org/omnibus-gitlab/issues/1401

Here is the request soul-searching involved for this trivial change:

The problem is that dump files from newer versions cannot be loaded in old versions. There is also a backwards compatibility problem with the RESTORE command, as described here (RESTORE doesn't work with dumps of old formats), and which is scheduled to be fixed in 3.2.1, but that is an unrelated problem, triggered through a different code path. Otherwise, it should be safe to use Redis with old dumps:

Redis dump file is 100% backwards compatible. An older dump file format will always work with a newer version of Redis.
https://github.com/sripathikrishnan/redis-rdb-tools/blob/master/docs/RDB_Version_History.textile

or see the comparison that actually triggers the error:

if (rdbver < 1 || rdbver > RDB_VERSION) {
    fclose(fp);
    serverLog(LL_WARNING,"Can't handle RDB format version %d",rdbver);
    errno = EINVAL;
    return C_ERR;
}

(https://github.com/antirez/redis/blob/3.2/src/rdb.c#L1298 )

This problem then can occur only when downgrading a package (assuming gitlab doesn't go back to old Redis versions), and we cannot retroactively fix old packages. All we can do is to make sure that this problem doesn't happen when:

a) downgrading future packages to existing packages and b) downgrading to future packages (from packages even more into the future).

Strategy

One option would be to rename the dump file in newer versions, and rename it every time the format of redis dump changes. This seems simple, but it presents some challenges, the biggest of which is the fact that the name of the file is controlled by a configuration file managed by Chef. Chef runs after Redis has been started after an upgrade, hence it would not be possible to rename the file early enough. This is a problem for both a) and b).

I thought that we could check the compatibility of the dump with the installed Redis just before starting it by calling the redis-check-rdb and removing the file in case of error -- inside the script that starts Redis. This check would add to the starting time of Redis, but it would also have the advantage of removing the file in case it's corrupt, which would otherwise cause Redis to exit. Should the overhead of calling redis-check-rdb every time redis is started not be acceptable, an alternative would be to modify the files/gitlab-ctl-commands/upgrade.rb script and run redis-check-rdb only there, right before starting redis.

(The trouble of checking just the dump version number is that, while it would be easy to check the version of the dump, it's not easy to know the version that Redis expects, as this is a mere define in the source code, used only in a few comparisons and when writing the dump file. Therefore, one would need to maintain a table.)

However, this utility currently has a bug: the version number is hard-coded (!) and it rejects version 7 dumps:

https://github.com/antirez/redis/blob/32f80e2f1bf42d0508f1114a9dddd91c4719eb8e/src/redis-check-rdb.c#L125

This will be fixed only in 3.2.2 (issue: https://github.com/antirez/redis/issues/3353 )

At this point, the only reasonable solution, given the difficulty of determining redis compatibility with the existing dump file, is to either:

  1. tentatively start redis and, if that fails, delete the dump and retry (either on every run or just after upgrading)
  2. unconditionally delete the file after upgrading.

I went with 2).

This solution is, however, like the one involving redis-check-rdb, merely prospective, as it WILL NOT solve a) (downgrading future packages to currently existing ones). I can't see any sensible way to address the problem without changing the post-removal package script or (rather preferably) adding a pre-removal one. There is no pre-removal script and the post-removal script only removes symlinks. When an old package is then installed, redis will be stopped only in the post-install script (writing potentially the new format dump) and started again (failing). Renaming the dump file is not a solution, as it's guaranteed that the config file will only be updated after the Redis restart has been attempted (which is code in current packages, that cannot be changed). The package scripts are defined in the omnbibus fork repository; only postinst delegates to gitlab-ctl upgrade.

Merge request reports