Add 'extended-redis-compatibility' config (valkey-io#306)

New config 'extended-redis-compatibility' (yes/no) default no * When yes: * Use "Redis" in the following error replies: - `-LOADING Redis is loading the dataset in memory` - `-BUSY Redis is busy`... - `-MISCONF Redis is configured to`... * Use `=== REDIS BUG REPORT` in the crash log delimiters (START and END). * The HELLO command returns `"server" => "redis"` and `"version" => "7.2.4"` (our Redis OSS compatibility version). * The INFO field for mode is called `"redis_mode"`. * When no: * Use "Valkey" instead of "Redis" in the mentioned errors and crash log delimiters. * The HELLO command returns `"server" => "valkey"` and the Valkey version for `"version"`. * The INFO field for mode is called `"server_mode"`. * Documentation added in valkey.conf: > Valkey is largely compatible with Redis OSS, apart from a few cases where > Redis OSS compatibility mode makes Valkey pretend to be Redis. Enable this > only if you have problems with tools or clients. This is a temporary > configuration added in Valkey 8.0 and is scheduled to have no effect in Valkey > 9.0 and be completely removed in Valkey 10.0. * A test case for the config is added. It is designed to fail if the config is not deprecated (has no effect) in Valkey 9 and deleted in Valkey 10. * Other test cases are adjusted to work regardless of this config. Fixes valkey-io#274 Fixes valkey-io#61 --------- Signed-off-by: Viktor Söderqvist <[email protected]>
Shivshankar-Reddy · Apr 18, 2024 · 9e2b783 · 9e2b783
1 parent b6dbc81
commit 9e2b783
Show file tree

Hide file tree

Showing 11 changed files with 114 additions and 37 deletions.
diff --git a/src/config.c b/src/config.c
@@ -2540,6 +2540,12 @@ static int updateAofAutoGCEnabled(const char **err) {
     return 1;
 }
 
+static int updateExtendedRedisCompat(const char **err) {
+    UNUSED(err);
+    createSharedObjectsWithCompat();
+    return 1;
+}
+
 static int updateSighandlerEnabled(const char **err) {
     UNUSED(err);
     if (server.crashlog_enabled)
@@ -3110,6 +3116,7 @@ standardConfig static_configs[] = {
     createBoolConfig("latency-tracking", NULL, MODIFIABLE_CONFIG, server.latency_tracking_enabled, 1, NULL, NULL),
     createBoolConfig("aof-disable-auto-gc", NULL, MODIFIABLE_CONFIG | HIDDEN_CONFIG, server.aof_disable_auto_gc, 0, NULL, updateAofAutoGCEnabled),
     createBoolConfig("replica-ignore-disk-write-errors", NULL, MODIFIABLE_CONFIG, server.repl_ignore_disk_write_error, 0, NULL, NULL),
+    createBoolConfig("extended-redis-compatibility", NULL, MODIFIABLE_CONFIG, server.extended_redis_compat, 0, NULL, updateExtendedRedisCompat),
 
     /* String Configs */
     createStringConfig("aclfile", NULL, IMMUTABLE_CONFIG, ALLOW_EMPTY_STRING, server.acl_filename, "", NULL, NULL),

diff --git a/src/debug.c b/src/debug.c
@@ -1157,8 +1157,9 @@ void _serverPanic(const char *file, int line, const char *msg, ...) {
 int bugReportStart(void) {
     pthread_mutex_lock(&bug_report_start_mutex);
     if (bug_report_start == 0) {
-        serverLogRaw(LL_WARNING|LL_RAW,
-        "\n\n=== REDIS BUG REPORT START: Cut & paste starting from here ===\n");
+        serverLog(LL_WARNING|LL_RAW,
+                  "\n\n=== %s BUG REPORT START: Cut & paste starting from here ===\n",
+                  server.extended_redis_compat ? "REDIS" : "VALKEY");
         bug_report_start = 1;
         pthread_mutex_unlock(&bug_report_start_mutex);
         return 1;
@@ -2379,14 +2380,14 @@ void printCrashReport(void) {
 void bugReportEnd(int killViaSignal, int sig) {
     struct sigaction act;
 
-    serverLogRawFromHandler(LL_WARNING|LL_RAW,
-"\n=== REDIS BUG REPORT END. Make sure to include from START to END. ===\n\n"
-"       Please report the crash by opening an issue on github:\n\n"
-"           http://github.com/valkey-io/valkey/issues\n\n"
-"  If a module was involved, please open in the module's repo instead.\n\n"
-"  Suspect RAM error? Use valkey-server --test-memory to verify it.\n\n"
-"  Some other issues could be detected by valkey-server --check-system\n"
-);
+    serverLogFromHandler(LL_WARNING|LL_RAW,
+                         "\n=== %s BUG REPORT END. Make sure to include from START to END. ===\n\n"
+                         "       Please report the crash by opening an issue on github:\n\n"
+                         "           https://github.com/valkey-io/valkey/issues\n\n"
+                         "  If a module was involved, please open in the module's repo instead.\n\n"
+                         "  Suspect RAM error? Use valkey-server --test-memory to verify it.\n\n"
+                         "  Some other issues could be detected by valkey-server --check-system\n",
+                         server.extended_redis_compat ? "REDIS" : "VALKEY");
 
     /* free(messages); Don't call free() with possibly corrupted memory. */
     if (server.daemonize && server.supervised == 0 && server.pidfile) unlink(server.pidfile);

diff --git a/src/networking.c b/src/networking.c
@@ -3646,10 +3646,10 @@ void helloCommand(client *c) {
     addReplyMapLen(c,6 + !server.sentinel_mode);
 
     addReplyBulkCString(c,"server");
-    addReplyBulkCString(c,"redis");
+    addReplyBulkCString(c, server.extended_redis_compat ? "redis" : SERVER_NAME);
 
     addReplyBulkCString(c,"version");
-    addReplyBulkCString(c,VALKEY_VERSION);
+    addReplyBulkCString(c, server.extended_redis_compat ? REDIS_VERSION : VALKEY_VERSION);
 
     addReplyBulkCString(c,"proto");
     addReplyLongLong(c,c->resp);

diff --git a/src/script.c b/src/script.c
@@ -164,14 +164,17 @@ int scriptPrepareForRun(scriptRunCtx *run_ctx, client *engine_client, client *ca
             int deny_write_type = writeCommandsDeniedByDiskError();
             if (deny_write_type != DISK_ERROR_TYPE_NONE && !obey_client) {
                 if (deny_write_type == DISK_ERROR_TYPE_RDB)
-                    addReplyError(caller, "-MISCONF Redis is configured to save RDB snapshots, "
+                    addReplyErrorFormat(caller, "-MISCONF %s is configured to save RDB snapshots, "
                                      "but it's currently unable to persist to disk. "
-                                     "Writable scripts are blocked. Use 'no-writes' flag for read only scripts.");
+                                     "Writable scripts are blocked. Use 'no-writes' flag for read only scripts.",
+                                     server.extended_redis_compat ? "Redis" : SERVER_TITLE);
                 else
-                    addReplyErrorFormat(caller, "-MISCONF Redis is configured to persist data to AOF, "
+                    addReplyErrorFormat(caller, "-MISCONF %s is configured to persist data to AOF, "
                                            "but it's currently unable to persist to disk. "
                                            "Writable scripts are blocked. Use 'no-writes' flag for read only scripts. "
-                                           "AOF error: %s", strerror(server.aof_last_write_errno));
+                                           "AOF error: %s",
+                                        server.extended_redis_compat ? "Redis" : SERVER_TITLE,
+                                        strerror(server.aof_last_write_errno));
                 return C_ERR;
             }
 

diff --git a/src/server.c b/src/server.c
@@ -1841,6 +1841,32 @@ void afterSleep(struct aeEventLoop *eventLoop) {
 
 /* =========================== Server initialization ======================== */
 
+/* These shared strings depend on the extended-redis-compatibility config and is
+ * called when the config changes. When the config is phased out, these
+ * initializations can be moved back inside createSharedObjects() below. */
+void createSharedObjectsWithCompat(void) {
+    const char *name = server.extended_redis_compat ? "Redis" : SERVER_TITLE;
+    if (shared.loadingerr) decrRefCount(shared.loadingerr);
+    shared.loadingerr = createObject(OBJ_STRING, sdscatfmt(sdsempty(),
+        "-LOADING %s is loading the dataset in memory\r\n", name));
+    if (shared.slowevalerr) decrRefCount(shared.slowevalerr);
+    shared.slowevalerr = createObject(OBJ_STRING, sdscatfmt(sdsempty(),
+        "-BUSY %s is busy running a script. You can only call SCRIPT KILL or SHUTDOWN NOSAVE.\r\n", name));
+    if (shared.slowscripterr) decrRefCount(shared.slowscripterr);
+    shared.slowscripterr = createObject(OBJ_STRING, sdscatfmt(sdsempty(),
+        "-BUSY %s is busy running a script. You can only call FUNCTION KILL or SHUTDOWN NOSAVE.\r\n", name));
+    if (shared.slowmoduleerr) decrRefCount(shared.slowmoduleerr);
+    shared.slowmoduleerr = createObject(OBJ_STRING, sdscatfmt(sdsempty(),
+        "-BUSY %s is busy running a module command.\r\n", name));
+    if (shared.bgsaveerr) decrRefCount(shared.bgsaveerr);
+    shared.bgsaveerr = createObject(OBJ_STRING, sdscatfmt(sdsempty(),
+        "-MISCONF %s 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 %s logs for details about the RDB error.\r\n", name, name));
+}
+
 void createSharedObjects(void) {
     int j;
 
@@ -1870,18 +1896,9 @@ void createSharedObjects(void) {
         "-ERR index out of range\r\n"));
     shared.noscripterr = createObject(OBJ_STRING,sdsnew(
         "-NOSCRIPT No matching script. Please use EVAL.\r\n"));
-    shared.loadingerr = createObject(OBJ_STRING,sdsnew(
-        "-LOADING Redis is loading the dataset in memory\r\n"));
-    shared.slowevalerr = createObject(OBJ_STRING,sdsnew(
-        "-BUSY Redis is busy running a script. You can only call SCRIPT KILL or SHUTDOWN NOSAVE.\r\n"));
-    shared.slowscripterr = createObject(OBJ_STRING,sdsnew(
-        "-BUSY Redis is busy running a script. You can only call FUNCTION KILL or SHUTDOWN NOSAVE.\r\n"));
-    shared.slowmoduleerr = createObject(OBJ_STRING,sdsnew(
-        "-BUSY Redis is busy running a module command.\r\n"));
+    createSharedObjectsWithCompat();
     shared.masterdownerr = createObject(OBJ_STRING,sdsnew(
         "-MASTERDOWN Link with MASTER is down and replica-serve-stale-data is set to 'no'.\r\n"));
-    shared.bgsaveerr = createObject(OBJ_STRING,sdsnew(
-        "-MISCONF Redis 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 Redis logs for details about the RDB error.\r\n"));
     shared.roslaveerr = createObject(OBJ_STRING,sdsnew(
         "-READONLY You can't write against a read only replica.\r\n"));
     shared.noautherr = createObject(OBJ_STRING,sdsnew(
@@ -2084,6 +2101,7 @@ void initServerConfig(void) {
     server.migrate_cached_sockets = dictCreate(&migrateCacheDictType);
     server.next_client_id = 1; /* Client IDs, start from 1 .*/
     server.page_size = sysconf(_SC_PAGESIZE);
+    server.extended_redis_compat = 0;
     server.pause_cron = 0;
     server.dict_resizing = 1;
 
@@ -5590,7 +5608,8 @@ sds genRedisInfoString(dict *section_dict, int all_sections, int everything) {
             "redis_git_sha1:%s\r\n", serverGitSHA1(),
             "redis_git_dirty:%i\r\n", strtol(serverGitDirty(),NULL,10) > 0,
             "redis_build_id:%s\r\n", serverBuildIdString(),
-            "redis_mode:%s\r\n", mode,
+            "%s_mode:", (server.extended_redis_compat ? "redis" : "server"),
+            "%s\r\n", mode,
             "os:%s", name.sysname,
             " %s", name.release,
             " %s\r\n", name.machine,

diff --git a/src/server.h b/src/server.h
@@ -1760,6 +1760,7 @@ struct valkeyServer {
     int set_proc_title;             /* True if change proc title */
     char *proc_title_template;      /* Process title template format */
     clientBufferLimitsConfig client_obuf_limits[CLIENT_TYPE_OBUF_COUNT];
+    int extended_redis_compat;      /* True if extended Redis OSS compatibility is enabled */
     int pause_cron;                 /* Don't run cron tasks (debug) */
     int dict_resizing;              /* Whether to allow main dict and expired dict to be resized (debug) */
     int latency_tracking_enabled;   /* 1 if extended latency tracking is enabled, 0 otherwise. */
@@ -3244,6 +3245,7 @@ void initConfigValues(void);
 void removeConfig(sds name);
 sds getConfigDebugInfo(void);
 int allowProtectedAction(int config, client *c);
+void createSharedObjectsWithCompat(void);
 void initServerClientMemUsageBuckets(void);
 void freeServerClientMemUsageBuckets(void);
 

diff --git a/tests/instances.tcl b/tests/instances.tcl
@@ -173,7 +173,7 @@ proc spawn_instance {type base_port count {conf {}} {base_conf_file ""}} {
 }
 
 proc log_crashes {} {
-    set start_pattern {*REDIS BUG REPORT START*}
+    set start_pattern {*BUG REPORT START*}
     set logs [glob */log.txt]
     foreach log $logs {
         set fd [open $log]

diff --git a/tests/support/util.tcl b/tests/support/util.tcl
@@ -37,7 +37,7 @@ proc crashlog_from_file {filename} {
     set logall 0
     set result {}
     foreach line $lines {
-        if {[string match {*REDIS BUG REPORT START*} $line]} {
+        if {[string match {*BUG REPORT START*} $line]} {
             set logall 1
         }
         if {[regexp {^\[\d+\]\s+\d+\s+\w+\s+\d{2}:\d{2}:\d{2} \#} $line]} {

diff --git a/tests/unit/moduleapi/crash.tcl b/tests/unit/moduleapi/crash.tcl
@@ -9,7 +9,7 @@ if {!$::valgrind} {
         r module load $testmodule assert
         test {Test module crash when info crashes with an assertion } {
             catch {r 0 info infocrash}
-            set res [wait_for_log_messages 0 {"*=== REDIS BUG REPORT START: Cut & paste starting from here ===*"} 0 10 1000]
+            set res [wait_for_log_messages 0 {"*=== * BUG REPORT START: Cut & paste starting from here ===*"} 0 10 1000]
             set loglines [lindex $res 1]
 
             set res [wait_for_log_messages 0 {"*ASSERTION FAILED*"} $loglines 10 1000]
@@ -18,24 +18,24 @@ if {!$::valgrind} {
             set res [wait_for_log_messages 0 {"*RECURSIVE ASSERTION FAILED*"} $loglines 10 1000]
             set loglines [lindex $res 1]
 
-            wait_for_log_messages 0 {"*=== REDIS BUG REPORT END. Make sure to include from START to END. ===*"} $loglines 10 1000
-            assert_equal 1 [count_log_message 0 "=== REDIS BUG REPORT END. Make sure to include from START to END. ==="]
+            wait_for_log_messages 0 {"*=== * REPORT END. Make sure to include from START to END. ===*"} $loglines 10 1000
+            assert_equal 1 [count_log_message 0 "=== .* BUG REPORT END. Make sure to include from START to END. ==="]
             assert_equal 2 [count_log_message 0 "ASSERTION FAILED"]
             if {$backtrace_supported} {
                 # Make sure the crash trace is printed twice. There will be 3 instances of,
                 # assertCrash 1 in the first stack trace and 2 in the second.
                 assert_equal 3 [count_log_message 0 "assertCrash"]
             }
             assert_equal 1 [count_log_message 0 "RECURSIVE ASSERTION FAILED"]
-            assert_equal 1 [count_log_message 0 "=== REDIS BUG REPORT START: Cut & paste starting from here ==="]
+            assert_equal 1 [count_log_message 0 "=== .* BUG REPORT START: Cut & paste starting from here ==="]
         }
     }
 
     start_server {tags {"modules"}} {
         r module load $testmodule segfault
         test {Test module crash when info crashes with a segfault} {
             catch {r 0 info infocrash}
-            set res [wait_for_log_messages 0 {"*=== REDIS BUG REPORT START: Cut & paste starting from here ===*"} 0 10 1000]
+            set res [wait_for_log_messages 0 {"*=== * BUG REPORT START: Cut & paste starting from here ===*"} 0 10 1000]
             set loglines [lindex $res 1]
 
             if {$backtrace_supported} {
@@ -48,16 +48,16 @@ if {!$::valgrind} {
                 set loglines [lindex $res 1]
             }
 
-            wait_for_log_messages 0 {"*=== REDIS BUG REPORT END. Make sure to include from START to END. ===*"} $loglines 10 1000
-            assert_equal 1 [count_log_message 0 "=== REDIS BUG REPORT END. Make sure to include from START to END. ==="]
+            wait_for_log_messages 0 {"*=== * BUG REPORT END. Make sure to include from START to END. ===*"} $loglines 10 1000
+            assert_equal 1 [count_log_message 0 "=== .* BUG REPORT END. Make sure to include from START to END. ==="]
             assert_equal 1 [count_log_message 0 "Crashed running signal handler. Providing reduced version of recursive crash report"]
             if {$backtrace_supported} {
                 assert_equal 2 [count_log_message 0 "Crashed running the instruction at"]
                 # Make sure the crash trace is printed twice. There will be 3 instances of 
                 # modulesCollectInfo, 1 in the first stack trace and 2 in the second.
                 assert_equal 3 [count_log_message 0 "modulesCollectInfo"]
             }
-            assert_equal 1 [count_log_message 0 "=== REDIS BUG REPORT START: Cut & paste starting from here ==="]
+            assert_equal 1 [count_log_message 0 "=== .* BUG REPORT START: Cut & paste starting from here ==="]
         }
     }
 }
diff --git a/tests/unit/other.tcl b/tests/unit/other.tcl
@@ -353,6 +353,42 @@ start_server {tags {"other"}} {
         assert_error {*unknown command*} {r CONFIG|GET GET_XX}
         assert_error {*unknown subcommand*} {r CONFIG GET_XX}
     }
+
+    test "Extended Redis Compatibility config" {
+        # This config is added in Valkey 8.0, shall be deprecated and have no
+        # effect in 9.x and be deleted in 10.0.
+        set hello [r hello 3]
+        set version [dict get $hello version]
+        if {[string match "10.*" $version]} {
+            # Check that the config doesn't exist anymore.
+            assert_error "ERR Unknown*" {r config set extended-redis-compatibility yes}
+            error "We shall also delete this test case"
+        } elseif {[string match "9.*" $version]} {
+            # This config is scheduled for removal. In 9.x it should still
+            # exists but have no effect.
+            r config set extended-redis-compatibility yes
+            set hello [r hello 3]
+            assert_equal valkey [dict get $hello server]
+            assert_equal $version [dict get $hello version]
+            r config set extended-redis-compatibility no
+        } elseif {[string match "8.*" $version] || ($version eq "255.255.255")} {
+            # In 8.x, the config shall work and affect HELLO server and version.
+            r config set extended-redis-compatibility yes
+            set hello [r hello 3]
+            assert_equal "redis" [dict get $hello server]
+            assert_match "7.2.*" [dict get $hello version]
+            set info [r info server]
+            assert_match "*redis_mode:*" $info
+            assert_no_match "*server_mode:*" $info
+            r config set extended-redis-compatibility no
+            set hello [r hello 3]
+            assert_equal "valkey" [dict get $hello server]
+            assert_equal $version [dict get $hello version]
+            set info [r info server]
+            assert_no_match "*redis_mode:*" $info
+            assert_match "*server_mode:*" $info
+        }
+    }
 }
 
 start_server {tags {"other external:skip"}} {

diff --git a/valkey.conf b/valkey.conf
@@ -413,6 +413,15 @@ proc-title-template "{title} {listen-addr} {server-mode}"
 # is derived from the environment variables.
 locale-collate ""
 
+# Valkey is largely compatible with Redis OSS, apart from a few cases where
+# Valkey identifies itself itself as "Valkey" rather than "Redis". Extended
+# Redis OSS compatibility mode makes Valkey pretend to be Redis. Enable this
+# only if you have problems with tools or clients. This is a temporary
+# configuration added in Valkey 8.0 and is scheduled to have no effect in Valkey
+# 9.0 and be completely removed in Valkey 10.0.
+#
+# extended-redis-compatibility no
+
 ################################ SNAPSHOTTING  ################################
 
 # Save the DB to disk.