From 65ef543f8cc4d470f554f06e50594e85aad0d617 Mon Sep 17 00:00:00 2001
From: Wen Hui <wen.hui.ware@gmail.com>
Date: Thu, 3 Feb 2022 06:20:35 -0500
Subject: [PATCH] Sentinel: return an error if configuration save fails
 (#10151)

When performing `SENTINEL SET`, Sentinel updates the local configuration file. Before this commit, failure to update the file would still result with an `+OK` reply. Now, a `-ERR Failed to save config file` error will be returned.

Co-authored-by: Yossi Gottlieb <yossigo@gmail.com>
---
 src/sentinel.c                             | 36 +++++++++++-----------
 tests/sentinel/tests/03-runtime-reconf.tcl | 12 ++++++++
 2 files changed, 30 insertions(+), 18 deletions(-)

diff --git a/src/sentinel.c b/src/sentinel.c
index f65e29876..60cba2e1b 100644
--- a/src/sentinel.c
+++ b/src/sentinel.c
@@ -2281,6 +2281,16 @@ werr:
     return C_ERR;
 }
 
+/* Call sentinelFlushConfig() produce a success/error reply to the
+ * calling client.
+ */
+static void sentinelFlushConfigAndReply(client *c) {
+    if (sentinelFlushConfig() == C_ERR)
+        addReplyErrorFormat(c,"Failed to save config file");
+    else
+        addReply(c, shared.ok);
+}
+
 /* ====================== hiredis connection handling ======================= */
 
 /* Send the AUTH command with the specified master password if needed.
@@ -3174,8 +3184,7 @@ void sentinelConfigSetCommand(client *c) {
         return;
     }
 
-    sentinelFlushConfig();
-    addReply(c, shared.ok);
+    sentinelFlushConfigAndReply(c);
 
     /* Drop Sentinel connections to initiate a reconnect if needed. */
     if (drop_conns)
@@ -3929,14 +3938,12 @@ NULL
         if (ri == NULL) {
             addReplyError(c,sentinelCheckCreateInstanceErrors(SRI_MASTER));
         } else {
-            sentinelFlushConfig();
+            sentinelFlushConfigAndReply(c);
             sentinelEvent(LL_WARNING,"+monitor",ri,"%@ quorum %d",ri->quorum);
-            addReply(c,shared.ok);
         }
     } else if (!strcasecmp(c->argv[1]->ptr,"flushconfig")) {
         if (c->argc != 2) goto numargserr;
-        sentinelFlushConfig();
-        addReply(c,shared.ok);
+        sentinelFlushConfigAndReply(c);
         return;
     } else if (!strcasecmp(c->argv[1]->ptr,"remove")) {
         /* SENTINEL REMOVE <name> */
@@ -3947,8 +3954,7 @@ NULL
             == NULL) return;
         sentinelEvent(LL_WARNING,"-monitor",ri,"%@");
         dictDelete(sentinel.masters,c->argv[2]->ptr);
-        sentinelFlushConfig();
-        addReply(c,shared.ok);
+        sentinelFlushConfigAndReply(c);
     } else if (!strcasecmp(c->argv[1]->ptr,"ckquorum")) {
         /* SENTINEL CKQUORUM <name> */
         sentinelRedisInstance *ri;
@@ -4348,22 +4354,16 @@ void sentinelSetCommand(client *c) {
             break;
         }
     }
-    if (changes && sentinelFlushConfig() == C_ERR) {
-        addReplyErrorFormat(c,"Failed to save Sentinel new configuration on disk");
-        return;
-    }
-    addReply(c,shared.ok);
+    if (changes) sentinelFlushConfigAndReply(c);
     return;
 
 badfmt: /* Bad format errors */
     addReplyErrorFormat(c,"Invalid argument '%s' for SENTINEL SET '%s'",
         (char*)c->argv[badarg]->ptr,option);
 seterr:
-    if (changes && sentinelFlushConfig() == C_ERR) {
-        addReplyErrorFormat(c,"Failed to save Sentinel new configuration on disk");
-        return;
-    }
-    return;
+    /* TODO: Handle the case of both bad input and save error, possibly handling
+     * SENTINEL SET atomically. */
+    if (changes) sentinelFlushConfig();
 }
 
 /* Our fake PUBLISH command: it is actually useful only to receive hello messages
diff --git a/tests/sentinel/tests/03-runtime-reconf.tcl b/tests/sentinel/tests/03-runtime-reconf.tcl
index ad9284e41..5d6e6b1a5 100644
--- a/tests/sentinel/tests/03-runtime-reconf.tcl
+++ b/tests/sentinel/tests/03-runtime-reconf.tcl
@@ -44,5 +44,17 @@ test "Sentinel Set with other error situations" {
 
    # unknown parameter option
    assert_error "ERR Unknown option or number of arguments for SENTINEL SET 'fakeoption'" {S 0 SENTINEL SET mymaster fakeoption fakevalue}
+
+   # save new config to disk failed
+   set info [S 0 SENTINEL master mymaster]
+   set origin_quorum [dict get $info quorum]
+   set update_quorum [expr $origin_quorum+1]
+   set sentinel_id 0
+   set configfilename [file join "sentinel_$sentinel_id" "sentinel.conf"]
+   exec chmod 000 $configfilename
+
+   catch {[S 0 SENTINEL SET mymaster quorum $update_quorum]} err
+   exec chmod 644 $configfilename
+   assert_equal "ERR Failed to save config file" $err
 }