Bug#33989165 MySQL Router should not disconnect client connections upon adding a member

Andrzej Religa · Andrzej Religa · commit 6da82182dccd · 2023-09-21T11:46:40.000+02:00
When the user adds a node to the cluster (cluster.addInstance()) there is a
short period of time, when this node is present in the GR metadata but still not
present in the cluster metadata. This breaks Router assumptions, as the function
that calculates quorum assumes this will never happen. This assumption is wrong
and it is not satisfied in this case. The Router treat this scenario as no
quorum (even thought the new node is ONLINE) and  drops all the existing
connections. It brings the service back once the new node appears in the cluster
metadata.

This patch fixes this behavior. The GR quorum no longer depends on cluster
metadata, only on what GR is reporting. Once the quorum decision is made,
and there is a quorum, the Router uses the nodes that are present in
both GR and cluster metadata as a valid destinations.

Change-Id: I1831acc553678d453bca63b6c0a7dd8f705c9bf0
diff --git a/router/src/metadata_cache/src/cluster_metadata_gr.cc b/router/src/metadata_cache/src/cluster_metadata_gr.cc
@@ -418,7 +418,8 @@ void GRClusterMetadata::update_cluster_status(
                                                       // with recovering nodes
                                                       // (cornercase)
           log_warning(
-              "quorum for cluster '%s' consists only of recovering nodes!",
+              "quorum for cluster '%s' consists only of recovering nodes or "
+              "nodes that are not defined in the metadata!",
               cluster.name.c_str());
           found_quorum = true;  // no point in futher search
           break;
@@ -469,31 +470,29 @@ GRClusterStatus GRClusterMetadata::check_cluster_status(
     std::vector<metadata_cache::ManagedInstance> &instances,
     const std::map<std::string, GroupReplicationMember> &member_status,
     bool &metadata_gr_discrepancy) const noexcept {
-  // In ideal world, the best way to write this function would be to completely
-  // ignore nodes in `instances` and operate on information from `member_status`
-  // only. However, there is one problem: the host:port information contained
-  // there may not be accurate (localhost vs external addressing issues), and we
-  // are forced to use the host:port from `instances` instead. This leads to
-  // nasty corner-cases if inconsistencies exist between the two sets, however.
-
-  // Therefore, this code will work well only under one assumption:
-  // All nodes in `member_status` are present in `instances`. This assumption
-  // should hold unless a user "manually" adds new nodes to the cluster
-  // without adding them to metadata (and the user is not allowed to do that).
-
-  // Detect violation of above assumption (alarm if there's a node in
-  // `member_status` not present in `instances`). It's O(n*m), but the CPU time
-  // is negligible while keeping code simple.
-
   using metadata_cache::ServerMode;
   using metadata_cache::ServerRole;
   using GR_State = GroupReplicationMember::State;
   using GR_Role = GroupReplicationMember::Role;
 
   metadata_gr_discrepancy = false;
-  auto number_of_all_members = member_status.size();
+  const auto number_of_all_members = member_status.size();
+  size_t number_of_online_members = 0, number_of_recovering_members = 0;
+  // Check if the GR has a quorum. Warn about nodes that are present in the GR
+  // and missing in the cluster metadata.
   for (const auto &status_node : member_status) {
     using MI = metadata_cache::ManagedInstance;
+
+    switch (status_node.second.state) {
+      case GR_State::Online:
+        number_of_online_members++;
+        break;
+      case GR_State::Recovering:
+        number_of_recovering_members++;
+        break;
+      default:;
+    }
+
     auto found = std::find_if(instances.begin(), instances.end(),
                               [&status_node](const MI &metadata_node) {
                                 return status_node.first ==
@@ -505,40 +504,22 @@ GRClusterStatus GRClusterMetadata::check_cluster_status(
             node_in_metadata, EventStateTracker::EventId::GRNodeInMetadata,
             status_node.first);
     if (!node_in_metadata) {
-      if (status_node.second.state == GR_State::Recovering) {
-        const auto log_level =
-            node_in_metadata_changed ? LogLevel::kInfo : LogLevel::kDebug;
-        log_custom(log_level,
-                   "GR member %s:%d (%s) Recovering, missing in the metadata, "
-                   "ignoring",
-                   status_node.second.host.c_str(), status_node.second.port,
-                   status_node.first.c_str());
-        // if the node is Recovering and it is missing in the metadata it can't
-        // increase the pool used for quorum calculations. This is for example
-        // important when we have single node cluster and we add another node
-        // with cloning. While cloning the new node will be present in the GR
-        // tables but missing in the metadata.
-        --number_of_all_members;
-      } else {
-        const auto log_level =
-            node_in_metadata_changed ? LogLevel::kWarning : LogLevel::kDebug;
-        log_custom(
-            log_level, "GR member %s:%d (%s) %s, missing in the metadata",
-            status_node.second.host.c_str(), status_node.second.port,
-            status_node.first.c_str(), to_string(status_node.second.state));
-      }
+      const auto log_level =
+          node_in_metadata_changed ? LogLevel::kWarning : LogLevel::kDebug;
+      log_custom(log_level,
+                 "GR member %s:%d (%s), state: '%s' missing in the metadata, "
+                 "ignoring",
+                 status_node.second.host.c_str(), status_node.second.port,
+                 status_node.first.c_str(),
+                 to_string(status_node.second.state));
 
-      // we want to set this in both cases as it increases the metadata refresh
-      // rate
+      // increases the metadata refresh rate
       metadata_gr_discrepancy = true;
     }
   }
 
-  // we do two things here:
-  // 1. for all `instances`, set .mode according to corresponding .status found
+  // for all `instances`, set .mode according to corresponding .status found
   // in `member_status`
-  // 2. count nodes which are part of quorum (online/recovering nodes)
-  unsigned int quorum_count = 0;
   bool have_primary_instance = false;
   bool have_secondary_instance = false;
   for (auto &member : instances) {
@@ -556,13 +537,11 @@ GRClusterStatus GRClusterMetadata::check_cluster_status(
               have_primary_instance = true;
               member.role = ServerRole::Primary;
               member.mode = ServerMode::ReadWrite;
-              quorum_count++;
               break;
             case GR_Role::Secondary:
               have_secondary_instance = true;
               member.role = ServerRole::Secondary;
               member.mode = ServerMode::ReadOnly;
-              quorum_count++;
               break;
           }
           break;
@@ -571,10 +550,6 @@ GRClusterStatus GRClusterMetadata::check_cluster_status(
         case GR_State::Offline:  // online node with disabled GR maps to this
         case GR_State::Error:
         case GR_State::Other:
-          // This could be done with a fallthrough but latest gcc (7.1)
-          // generates a warning for that and there is no sane and portable way
-          // to suppress it.
-          if (GR_State::Recovering == status->second.state) quorum_count++;
           member.role = ServerRole::Unavailable;
           member.mode = ServerMode::Unavailable;
           break;
@@ -593,10 +568,13 @@ GRClusterStatus GRClusterMetadata::check_cluster_status(
     }
   }
 
-  // quorum_count is based on nodes from `instances` instead of `member_status`.
-  // This is okay, because all nodes in `member_status` are present in
-  // `instances` (our assumption described at the top)
-  bool have_quorum = (quorum_count > number_of_all_members / 2);
+  // 1 node:  [ 1 ONL ] -> OK
+  // 1 node:  [ 1 REC ] -> OK (not possible, checked in a later step )
+  // 2 nodes: [ 1 ONL, 1 REC ] -> OK
+  // 2 nodes: [ 1 ONL, 1 OFF ] -> NOT OK
+  // 3 nodes: [ 1 ONL, 1 REC, 1 OFF ] -> OK
+  bool have_quorum = (number_of_online_members + number_of_recovering_members) >
+                     number_of_all_members / 2;
 
   // if we don't have quorum, we don't allow any access. Some configurations
   // might allow RO access in this case, but we don't support it at the momemnt
diff --git a/router/src/router/src/cluster_metadata.cc b/router/src/router/src/cluster_metadata.cc
@@ -531,24 +531,26 @@ bool check_group_replication_online(MySQLSession *mysql) {
 
 bool check_group_has_quorum(MySQLSession *mysql) {
   std::string q =
-      "SELECT SUM(IF(member_state = 'ONLINE', 1, 0)) as num_onlines, COUNT(*) "
-      "as num_total"
-      " FROM performance_schema.replication_group_members";
+      "SELECT SUM(IF(member_state = 'ONLINE', 1, 0)) as num_onlines, "
+      "SUM(IF(member_state = 'RECOVERING', 1, 0)) as num_recovering, "
+      "COUNT(*) as num_total "
+      "FROM performance_schema.replication_group_members";
 
   std::unique_ptr<MySQLSession::ResultRow> result(
       mysql->query_one(q));  // throws MySQLSession::Error
   if (result) {
-    if (result->size() != 2) {
+    if (result->size() != 3) {
       throw std::out_of_range(
           "Invalid number of values returned from "
           "performance_schema.replication_group_members: "
-          "expected 2 got " +
+          "expected 3 got " +
           std::to_string(result->size()));
     }
     int online = strtoi_checked((*result)[0]);
-    int all = strtoi_checked((*result)[1]);
-    // log_info("%d members online out of %d", online, all);
-    if (online >= all / 2 + 1) return true;
+    int recovering = strtoi_checked((*result)[1]);
+    int all = strtoi_checked((*result)[2]);
+
+    if ((online + recovering) > all / 2) return true;
     return false;
   }
 
diff --git a/router/tests/component/data/local_modules/common_statements.js b/router/tests/component/data/local_modules/common_statements.js
@@ -93,6 +93,7 @@ var defaults = {
 
   gr_member_state: "ONLINE",
   gr_members_all: 3,
+  gr_members_recovering: 0,
   gr_members_online: 3,
 };
 
@@ -400,13 +401,17 @@ function get_response(stmt_key, options) {
     case "router_select_members_count":
       return {
         "stmt":
-            "SELECT SUM(IF(member_state = 'ONLINE', 1, 0)) as num_onlines, COUNT(*) as num_total FROM performance_schema.replication_group_members",
+            "SELECT SUM(IF(member_state = 'ONLINE', 1, 0)) as num_onlines, SUM(IF(member_state = 'RECOVERING', 1, 0)) as num_recovering, COUNT(*) as num_total FROM performance_schema.replication_group_members",
         "result": {
           "columns": [
             {"type": "LONGLONG", "name": "num_onlines"},
+            {"type": "LONGLONG", "name": "num_recovering"},
             {"type": "LONGLONG", "name": "num_total"}
           ],
-          "rows": [[options.gr_members_online, options.gr_members_all]]
+          "rows": [[
+            options.gr_members_online, , options.gr_members_recovering,
+            options.gr_members_all
+          ]]
         }
       };
     case "router_select_replication_group_name":
diff --git a/router/tests/component/data/local_modules/gr_memberships.js b/router/tests/component/data/local_modules/gr_memberships.js
@@ -39,10 +39,10 @@ exports.single_host_cluster_nodes = function(host, classic_port, uuid) {
 exports.gr_members = function(host, port_and_state) {
   return port_and_state.map(function(current_value) {
     return [
-      current_value[0],  // use port as uuid
+      current_value[0],  // uuid
       host,
-      current_value[0],  // port
-      current_value[1]   // member_state
+      current_value[1],  // port
+      current_value[2]   // member_state
     ];
   });
 };
@@ -56,11 +56,11 @@ exports.members = function(id_host_and_port) {
 exports.cluster_nodes = function(host, cluster_instances) {
   return cluster_instances.map(function(current_value) {
     return [
-      current_value[0],  // classic port as uuid
+      current_value[0],  // uuid
       host,
-      current_value[0],  // classic port
-      current_value[1],  // x port
-      current_value[2],  // attributes
+      current_value[1],  // classic port
+      current_value[2],  // x port
+      current_value[3],  // attributes
     ];
   });
 };
diff --git a/router/tests/component/data/metadata_3_secondaries_primary_failover.js b/router/tests/component/data/metadata_3_secondaries_primary_failover.js
@@ -17,8 +17,8 @@ var gr_node_host = "127.0.0.1";
 var nodes = function(host, port_and_state) {
   return port_and_state.map(function(current_value) {
     return [
-      current_value[0], host, current_value[0], current_value[1],
-      current_value[2]
+      current_value[0], host, current_value[1], current_value[2],
+      current_value[3]
     ];
   });
 };
diff --git a/router/tests/component/data/metadata_3_secondaries_primary_failover_v2_gr.js b/router/tests/component/data/metadata_3_secondaries_primary_failover_v2_gr.js
@@ -17,8 +17,8 @@ var gr_node_host = "127.0.0.1";
 var nodes = function(host, port_and_state) {
   return port_and_state.map(function(current_value) {
     return [
-      current_value[0], host, current_value[0], current_value[1],
-      current_value[2]
+      current_value[0], host, current_value[1], current_value[2],
+      current_value[3]
     ];
   });
 };
diff --git a/router/tests/component/data/metadata_dynamic_nodes_v2_ar.js b/router/tests/component/data/metadata_dynamic_nodes_v2_ar.js
@@ -53,8 +53,8 @@ if (mysqld.global.cluster_type === undefined) {
 var nodes = function(host, port_and_state) {
   return port_and_state.map(function(current_value) {
     return [
-      current_value[0], host, current_value[0], current_value[1],
-      current_value[2]
+      current_value[0], host, current_value[1], current_value[2],
+      current_value[3]
     ];
   });
 };
diff --git a/router/tests/component/data/metadata_dynamic_nodes_v2_gr.js b/router/tests/component/data/metadata_dynamic_nodes_v2_gr.js
@@ -73,6 +73,12 @@ const online_gr_nodes = members
                             })
                             .length;
 
+const recovering_gr_nodes = members
+                                .filter(function(memb, indx) {
+                                  return (memb[3] === "RECOVERING");
+                                })
+                                .length;
+
 const member_state = members[mysqld.global.gr_pos] ?
     members[mysqld.global.gr_pos][3] :
     undefined;
@@ -82,6 +88,7 @@ var options = {
   gr_member_state: member_state,
   gr_members_all: members.length,
   gr_members_online: online_gr_nodes,
+  gr_members_recovering: recovering_gr_nodes,
   innodb_cluster_instances: gr_memberships.cluster_nodes(
       mysqld.global.gr_node_host, mysqld.global.cluster_nodes),
   gr_id: mysqld.global.gr_id,
diff --git a/router/tests/component/data/metadata_dynamic_nodes_version_update.js b/router/tests/component/data/metadata_dynamic_nodes_version_update.js
@@ -70,8 +70,8 @@ if (mysqld.global.transaction_count === undefined) {
 var nodes = function(host, port_and_state) {
   return port_and_state.map(function(current_value) {
     return [
-      current_value[0], host, current_value[0], current_value[1],
-      current_value[2]
+      current_value[0], host, current_value[1], current_value[2],
+      current_value[3]
     ];
   });
 };
diff --git a/router/tests/component/data/metadata_dynamic_nodes_version_update_v2_ar.js b/router/tests/component/data/metadata_dynamic_nodes_version_update_v2_ar.js
@@ -89,8 +89,8 @@ var nodes = function(host, port_and_state) {
     return [
       current_value[0],
       host,
-      current_value[0],
       current_value[1],
+      current_value[2],
     ];
   });
 };
diff --git a/router/tests/component/data/metadata_dynamic_nodes_version_update_v2_gr.js b/router/tests/component/data/metadata_dynamic_nodes_version_update_v2_gr.js
@@ -92,8 +92,8 @@ if (mysqld.global.bootstrap_target_type === undefined) {
 var nodes = function(host, port_and_state) {
   return port_and_state.map(function(current_value) {
     return [
-      current_value[0], host, current_value[0], current_value[1],
-      current_value[2]
+      current_value[0], host, current_value[1], current_value[2],
+      current_value[3]
     ];
   });
 };
diff --git a/router/tests/component/data/metadata_http_auth_backend.js b/router/tests/component/data/metadata_http_auth_backend.js
@@ -30,8 +30,8 @@ if (mysqld.global.error_on_md_query === undefined) {
 var nodes = function(host, port_and_state) {
   return port_and_state.map(function(current_value) {
     return [
-      current_value[0], host, current_value[0], current_value[1],
-      current_value[2]
+      current_value[0], host, current_value[1], current_value[2],
+      current_value[3]
     ];
   });
 };
diff --git a/router/tests/component/data/rest_api_enable.js b/router/tests/component/data/rest_api_enable.js
@@ -40,8 +40,8 @@ if (mysqld.global.gr_nodes === undefined &&
 var nodes = function(host, port_and_state) {
   return port_and_state.map(function(current_value) {
     return [
-      current_value[0], host, current_value[0], current_value[1],
-      current_value[2]
+      current_value[0], host, current_value[1], current_value[2],
+      current_value[3]
     ];
   });
 };
diff --git a/router/tests/component/test_bootstrap.cc b/router/tests/component/test_bootstrap.cc
@@ -121,7 +121,7 @@ TEST_F(RouterBootstrapTest, bootstrap_and_run_from_symlinked_dir) {
   launch_mysql_server_mock(runtime_json_stmts, server_port, EXIT_SUCCESS, false,
                            http_port);
   set_mock_metadata(http_port, "cluster-specific-id", {GRNode{server_port}}, 0,
-                    {ClusterNode{server_port, server_x_port}});
+                    {ClusterNode{server_port, "uuid-1", server_x_port}});
 
   SCOPED_TRACE("// launch router with bootstrapped config");
   launch_router({"-c", bootstrap_dir.name() + "/mysqlrouter.conf"});
@@ -1714,8 +1714,9 @@ TEST_P(ConfUseGrNotificationParamTest, ConfUseGrNotificationParam) {
   // launch mock server that is our metadata server
   launch_mysql_server_mock(runtime_json_stmts, server_port, EXIT_SUCCESS, false,
                            http_port);
-  set_mock_metadata(http_port, "cluster-specific-id", {GRNode{server_port}}, 0,
-                    {ClusterNode{server_port, server_x_port}});
+  set_mock_metadata(http_port, "cluster-specific-id",
+                    {GRNode{server_port, "uuid-1"}}, 0,
+                    {ClusterNode{server_port, "uuid-1", server_x_port}});
 
   // check that the Router accepts the config file
   auto &router2 = launch_router({"-c", conf_file});
diff --git a/router/tests/component/test_gr_notifications.cc b/router/tests/component/test_gr_notifications.cc
@@ -151,8 +151,11 @@ class GrNotificationsTest : public RouterComponentTest {
     gr_id_.reset(new JsonValue(gr_id.c_str(), gr_id.length(), allocator));
 
     gr_nodes_.reset(new JsonValue(rapidjson::kArrayType));
-    for (auto &gr_node : gr_node_ports) {
+    for (auto [i, gr_node] : stdx::views::enumerate(gr_node_ports)) {
       JsonValue node(rapidjson::kArrayType);
+      const std::string uuid = "uuid-" + std::to_string(i + 1);
+      node.PushBack(JsonValue(uuid.c_str(), uuid.length(), allocator),
+                    allocator);
       node.PushBack(JsonValue(static_cast<int>(gr_node)), allocator);
       node.PushBack(JsonValue("ONLINE", strlen("ONLINE"), allocator),
                     allocator);
@@ -164,6 +167,9 @@ class GrNotificationsTest : public RouterComponentTest {
         cluster_node_ports.empty() ? gr_node_ports : cluster_node_ports;
     for (auto [i, cluster_node] : stdx::views::enumerate(cluster_nodes)) {
       JsonValue node(rapidjson::kArrayType);
+      const std::string uuid = "uuid-" + std::to_string(i + 1);
+      node.PushBack(JsonValue(uuid.c_str(), uuid.length(), allocator),
+                    allocator);
       node.PushBack(JsonValue(static_cast<int>(cluster_node)), allocator);
       node.PushBack(JsonValue(static_cast<int>(gr_node_xports[i])), allocator);
       node.PushBack(JsonValue("{}", strlen("{}"), allocator), allocator);
diff --git a/router/tests/component/test_metadata_ttl.cc b/router/tests/component/test_metadata_ttl.cc
diff --git a/router/tests/helpers/mock_server_testutils.cc b/router/tests/helpers/mock_server_testutils.cc
diff --git a/router/tests/helpers/mock_server_testutils.h b/router/tests/helpers/mock_server_testutils.h
diff --git a/router/tests/helpers/router_component_test.h b/router/tests/helpers/router_component_test.h
