BUG#34614769 clusterset.status() reports repl channel issues when its own failover is ongoing

Currently, shell retrieves the state of the replication channel
"clusterset_replication", and if it contains an error, it reports it as
such. This means that, if the channel is connecting (and the error is
related to that), it will never show the state as connecting but always
as in error. This is misleading because errors during a connection
attempt are acceptable.

This patch addresses this by acknowledging that the channel is
connecting and ignoring the error until the channel state updates to
either ON or OFF. This means that, in ClusterSet.status(), the
"clusterSetReplicationStatus" field in a cluster can return "CONNECTING"
(in which case the "globalStatus" is now "OK"), and in
ReplicaSet.status(), the "status" field of an instance can now show
"CONNECTING".

Change-Id: Ief5d96f14ce6342a29f78916f316f99c9188116d

Author: joaosigma

modules/adminapi/cluster_set/cluster_set_impl.cc

@@ -550,53 +550,47 @@ Cluster_channel_status Cluster_set_impl::get_replication_channel_status(
     const Cluster_impl &cluster) const {
   // Get the ClusterSet member metadata
   auto cluster_primary = cluster.get_cluster_server();
-
-  if (cluster_primary) {
-    mysqlshdk::mysql::Replication_channel channel;
-
-    if (mysqlshdk::mysql::get_channel_status(
-            *cluster_primary, k_clusterset_async_channel_name, &channel)) {
-      if (channel.status() != mysqlshdk::mysql::Replication_channel::ON) {
-        log_info("Channel '%s' at %s not ON: %s",
-                 k_clusterset_async_channel_name,
-                 cluster_primary->descr().c_str(),
-                 mysqlshdk::mysql::format_status(channel, true).c_str());
-      } else {
-        if (cluster.is_primary_cluster()) {
-          log_info("Unexpected channel '%s' at %s: %s",
-                   k_clusterset_async_channel_name,
-                   cluster_primary->descr().c_str(),
-                   mysqlshdk::mysql::format_status(channel, true).c_str());
-        }
-      }
-
-      switch (channel.status()) {
-        case mysqlshdk::mysql::Replication_channel::CONNECTING:
-        case mysqlshdk::mysql::Replication_channel::ON: {
-          auto primary = get_primary_master();
-          auto primary_cluster = get_primary_cluster();
-
-          if (primary && primary_cluster &&
-              primary_cluster->cluster_availability() ==
-                  Cluster_availability::ONLINE) {
-            if (channel.source_uuid != primary->get_uuid())
-              return Cluster_channel_status::MISCONFIGURED;
-          }
-          return Cluster_channel_status::OK;
-          break;
-        }
-
-        case mysqlshdk::mysql::Replication_channel::OFF:
-        case mysqlshdk::mysql::Replication_channel::RECEIVER_OFF:
-        case mysqlshdk::mysql::Replication_channel::APPLIER_OFF:
-          return Cluster_channel_status::STOPPED;
-        case mysqlshdk::mysql::Replication_channel::CONNECTION_ERROR:
-        case mysqlshdk::mysql::Replication_channel::APPLIER_ERROR:
-          return Cluster_channel_status::ERROR;
-      }
-    } else {
-      return Cluster_channel_status::MISSING;
+  if (!cluster_primary) return Cluster_channel_status::UNKNOWN;
+
+  mysqlshdk::mysql::Replication_channel channel;
+  if (!mysqlshdk::mysql::get_channel_status(
+          *cluster_primary, k_clusterset_async_channel_name, &channel))
+    return Cluster_channel_status::MISSING;
+
+  if (channel.status() != mysqlshdk::mysql::Replication_channel::ON) {
+    log_info("Channel '%s' at %s not ON: %s", k_clusterset_async_channel_name,
+             cluster_primary->descr().c_str(),
+             mysqlshdk::mysql::format_status(channel, true).c_str());
+  } else if (cluster.is_primary_cluster()) {
+    log_info("Unexpected channel '%s' at %s: %s",
+             k_clusterset_async_channel_name, cluster_primary->descr().c_str(),
+             mysqlshdk::mysql::format_status(channel, true).c_str());
+  }
+
+  switch (channel.status()) {
+    case mysqlshdk::mysql::Replication_channel::CONNECTING:
+      return Cluster_channel_status::CONNECTING;
+
+    case mysqlshdk::mysql::Replication_channel::ON: {
+      auto primary = get_primary_master();
+      auto primary_cluster = get_primary_cluster();
+
+      if (primary && primary_cluster &&
+          (primary_cluster->cluster_availability() ==
+           Cluster_availability::ONLINE) &&
+          (channel.source_uuid != primary->get_uuid()))
+        return Cluster_channel_status::MISCONFIGURED;
+
+      return Cluster_channel_status::OK;
     }
+
+    case mysqlshdk::mysql::Replication_channel::OFF:
+    case mysqlshdk::mysql::Replication_channel::RECEIVER_OFF:
+    case mysqlshdk::mysql::Replication_channel::APPLIER_OFF:
+      return Cluster_channel_status::STOPPED;
+    case mysqlshdk::mysql::Replication_channel::CONNECTION_ERROR:
+    case mysqlshdk::mysql::Replication_channel::APPLIER_ERROR:
+      return Cluster_channel_status::ERROR;
   }
 
   return Cluster_channel_status::UNKNOWN;
@@ -676,6 +670,7 @@ Cluster_global_status Cluster_set_impl::get_cluster_global_status(
 
     switch (get_replication_channel_status(*cluster)) {
       case Cluster_channel_status::OK:
+      case Cluster_channel_status::CONNECTING:
       case Cluster_channel_status::MISCONFIGURED:
       case Cluster_channel_status::ERROR:
         // unexpected at primary cluster
@@ -705,6 +700,7 @@ Cluster_global_status Cluster_set_impl::get_cluster_global_status(
     default:
       switch (get_replication_channel_status(*cluster)) {
         case Cluster_channel_status::OK:
+        case Cluster_channel_status::CONNECTING:
           ret = Cluster_global_status::OK;
           break;
         case Cluster_channel_status::STOPPED:

modules/adminapi/cluster_set/status.cc

@@ -40,11 +40,11 @@ shcore::Array_t cluster_diagnostics(
     shcore::Array_t cluster_errors) {
   using mysqlshdk::mysql::Replication_channel;
 
-  auto append_error = [&cluster_errors](const std::string &msg) {
+  auto append_error = [&cluster_errors](std::string msg) {
     if (!cluster_errors) {
       cluster_errors = shcore::make_array();
     }
-    cluster_errors->push_back(shcore::Value(msg));
+    cluster_errors->push_back(shcore::Value(std::move(msg)));
   };
 
   // GTID_EXECUTED consistency handled elsewhere
@@ -53,30 +53,34 @@ shcore::Array_t cluster_diagnostics(
     if (cluster->is_primary_cluster()) {
       if (!channel.host.empty() ||
           channel.status() != Replication_channel::Status::OFF) {
-        append_error("WARNING: Unexpected replication channel '" +
-                     channel.channel_name + "' at Primary Cluster");
+        append_error(shcore::str_format(
+            "WARNING: Unexpected replication channel '%s' at Primary Cluster",
+            channel.channel_name.c_str()));
       }
     } else {
       if (channel.host.empty()) {
         append_error(
             "WARNING: Replication channel from the Primary Cluster is missing");
       } else {
-        if (channel.status() != Replication_channel::Status::ON) {
+        auto channel_status = channel.status();
+        if ((channel_status != Replication_channel::Status::CONNECTING) &&
+            (channel_status != Replication_channel::Status::ON)) {
           append_error(
               "WARNING: Replication from the Primary Cluster not in expected "
               "state");
-        }
-
-        auto primary = primary_cluster->get_primary_master();
 
-        if (primary_cluster->cluster_availability() ==
-                Cluster_availability::ONLINE &&
-            channel.source_uuid != primary->get_uuid()) {
-          append_error(shcore::str_format(
-              "WARNING: Replicating from wrong source. Expected %s (%s) but is "
-              "%s:%i (%s)",
-              primary->descr().c_str(), primary->get_uuid().c_str(),
-              channel.host.c_str(), channel.port, channel.source_uuid.c_str()));
+          auto primary = primary_cluster->get_primary_master();
+
+          if (primary_cluster->cluster_availability() ==
+                  Cluster_availability::ONLINE &&
+              channel.source_uuid != primary->get_uuid()) {
+            append_error(shcore::str_format(
+                "WARNING: Replicating from wrong source. Expected %s (%s) but "
+                "is %s:%i (%s)",
+                primary->descr().c_str(), primary->get_uuid().c_str(),
+                channel.host.c_str(), channel.port,
+                channel.source_uuid.c_str()));
+          }
         }
       }
     }

modules/adminapi/common/cluster_types.cc

@@ -176,6 +176,8 @@ std::string to_string(Cluster_channel_status status) {
   switch (status) {
     case Cluster_channel_status::OK:
       return "OK";
+    case Cluster_channel_status::CONNECTING:
+      return "CONNECTING";
     case Cluster_channel_status::STOPPED:
       return "STOPPED";
     case Cluster_channel_status::ERROR:

modules/adminapi/common/cluster_types.h

@@ -102,6 +102,7 @@ Cluster_status to_cluster_status(const std::string &s);
 enum class Cluster_global_status {
   OK,  // If it's a Primary, it must have any of the OK_* status. If a Replica,
        // any of the OK_* status plus the Replication Channel status must be OK
+       // or CONNECTING
   OK_NOT_REPLICATING,  // Replica Cluster with any of the OK_* status and
                        // Replication Channel status STOPPED or ERROR
   OK_NOT_CONSISTENT,   // Replica Cluster with any of the OK_* status and
@@ -120,15 +121,16 @@ enum class Cluster_global_status {
 std::string to_string(Cluster_global_status status);
 
 enum class Cluster_channel_status {
-  OK,       // Replication channel up and running
-  STOPPED,  // Replication channel stopped gracefully. Either both IO and SQL
-            // threads or just one of them.
-  ERROR,    // Replication channel stopped due to a replication error (e.g.
-            // conflicting GTID-set)
-  MISCONFIGURED,  // Channel exists but is replicating from the wrong place
-  MISSING,        // Channel doesn't exist
+  OK,          // Replication channel up and running.
+  CONNECTING,  // Replication channel is connecting.
+  STOPPED,     // Replication channel stopped gracefully. Either both IO and SQL
+               // threads or just one of them.
+  ERROR,       // Replication channel stopped due to a replication error (e.g.
+               // conflicting GTID-set).
+  MISCONFIGURED,  // Channel exists but is replicating from the wrong place.
+  MISSING,        // Channel doesn't exist.
   UNKNOWN  // Shell cannot connect to the Replica Cluster to obtain information
-           // about the replication channel and others
+           // about the replication channel and others.
 };
 
 std::string to_string(Cluster_channel_status status);

modules/adminapi/common/dba_errors.h

@@ -173,6 +173,7 @@
 #define SHERR_DBA_CLUSTER_NOT_FENCED 51617
 #define SHERR_DBA_CLUSTER_NOT_CONFIGURED_TRANSACTION_SIZE_LIMIT 51618
 #define SHERR_DBA_CLUSTER_OFFLINE 51619
+#define SHERR_DBA_PRIMARY_CLUSTER_STILL_CONNECTING 51620
 
 // Read-Replica errors
 #define SHERR_DBA_READ_REPLICA_SETUP_ERROR 51700

modules/adminapi/common/global_topology.cc

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2019, 2022, Oracle and/or its affiliates.
+ * Copyright (c) 2019, 2023, Oracle and/or its affiliates.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License, version 2.0,
@@ -52,6 +52,8 @@ std::string to_string(topology::Node_status status) {
       return "INCONSISTENT";
     case topology::Node_status::ONLINE:
       return "ONLINE";
+    case topology::Node_status::CONNECTING:
+      return "CONNECTING";
   }
   throw std::logic_error("internal error");
 }
@@ -212,50 +214,46 @@ Node_status Server::status() const {
 
   if (server.status() != Instance_status::OK) return Node_status::UNREACHABLE;
 
-  if (!master_instance_uuid.empty()) {
-    // SECONDARY checks
-    if (!server.master_channel || !master_node_ptr) return Node_status::ERROR;
-
-    // check that the actual source is the expected source
-    if (server.master_channel->info.source_uuid !=
-        master_node_ptr->get_primary_member()->uuid) {
-      log_warning(
-          "Instance %s is expected to have source %s (%s), but is %s:%i (%s)",
-          label.c_str(), master_node_ptr->label.c_str(),
-          master_node_ptr->get_primary_member()->uuid.c_str(),
-          server.master_channel->info.host.c_str(),
-          server.master_channel->info.port,
-          server.master_channel->info.source_uuid.c_str());
-      return Node_status::ERROR;
-    }
-
-    if (server.master_channel->info.status() ==
-            mysqlshdk::mysql::Replication_channel::OFF ||
-        server.master_channel->info.status() ==
-            mysqlshdk::mysql::Replication_channel::APPLIER_OFF ||
-        server.master_channel->info.status() ==
-            mysqlshdk::mysql::Replication_channel::RECEIVER_OFF)
-      return Node_status::OFFLINE;
-
-    if (server.master_channel->info.status() !=
-            mysqlshdk::mysql::Replication_channel::CONNECTING &&
-        server.master_channel->info.status() !=
-            mysqlshdk::mysql::Replication_channel::ON)
-      return Node_status::ERROR;
-
-    // check for GTID set inconsistencies
-    if (errant_transaction_count.value_or(0) > 0)
-      return Node_status::INCONSISTENT;
-
-    if (!server.is_fenced()) return Node_status::ERROR;
-  } else {
+  if (master_instance_uuid.empty()) {
     // PRIMARY checks
     if (server.is_fenced()) return Node_status::ERROR;
-
-    if (server.master_channel) return Node_status::ERROR;
+    return (server.master_channel ? Node_status::ERROR : Node_status::ONLINE);
+  }
+
+  // SECONDARY checks
+  if (!server.master_channel || !master_node_ptr) return Node_status::ERROR;
+
+  // check that the actual source is the expected source
+  if (server.master_channel->info.source_uuid !=
+      master_node_ptr->get_primary_member()->uuid) {
+    log_warning(
+        "Instance %s is expected to have source %s (%s), but is %s:%i (%s)",
+        label.c_str(), master_node_ptr->label.c_str(),
+        master_node_ptr->get_primary_member()->uuid.c_str(),
+        server.master_channel->info.host.c_str(),
+        server.master_channel->info.port,
+        server.master_channel->info.source_uuid.c_str());
+    return Node_status::ERROR;
+  }
+
+  switch (auto status = server.master_channel->info.status(); status) {
+    case mysqlshdk::mysql::Replication_channel::ON:
+      break;
+    case mysqlshdk::mysql::Replication_channel::OFF:
+    case mysqlshdk::mysql::Replication_channel::APPLIER_OFF:
+    case mysqlshdk::mysql::Replication_channel::RECEIVER_OFF:
+      return Node_status::OFFLINE;
+    case mysqlshdk::mysql::Replication_channel::CONNECTING:
+      return Node_status::CONNECTING;
+    default:
+      return Node_status::ERROR;
   }
 
-  return Node_status::ONLINE;
+  // replication channel status is ON, check for GTID set inconsistencies
+  if (errant_transaction_count.value_or(0) > 0)
+    return Node_status::INCONSISTENT;
+
+  return (server.is_fenced() ? Node_status::ONLINE : Node_status::ERROR);
 }
 
 Node_role Server::role() const {

modules/adminapi/common/global_topology.h

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2019, 2022, Oracle and/or its affiliates.
+ * Copyright (c) 2019, 2023, Oracle and/or its affiliates.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License, version 2.0,
@@ -50,6 +50,7 @@ class Node;
 
 enum class Node_status {
   ONLINE,
+  CONNECTING,
   ERROR,         // Replication related error
   INCONSISTENT,  // GTID inconsistencies detected
   INVALIDATED,   // Instance was invalidated in a failover

modules/adminapi/common/global_topology_check.cc

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2019, 2022, Oracle and/or its affiliates.
+ * Copyright (c) 2019, 2023, Oracle and/or its affiliates.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License, version 2.0,
@@ -22,6 +22,7 @@
  */
 
 #include "modules/adminapi/common/global_topology_check.h"
+
 #include "modules/adminapi/common/dba_errors.h"
 #include "mysqlshdk/include/shellcore/console.h"
 
@@ -65,6 +66,7 @@ void validate_node_status(const topology::Node *node) {
               " is inconsistent with the rest of the replicaset.",
           SHERR_DBA_ASYNC_MEMBER_INCONSISTENT);
 
+    case topology::Node_status::CONNECTING:
     case topology::Node_status::ONLINE:
       break;
   }
@@ -82,25 +84,21 @@ void validate_star_topology_consistent(
 
 void validate_global_topology_active_cluster_available(
     const topology::Global_topology &topology) {
-  auto console = current_console();
-
+  assert(topology.is_single_active());
   if (topology.is_single_active()) {
     const topology::Node *master_node = topology.get_primary_master_node();
 
     validate_node_status(master_node);
-  } else {
-    assert(0);
   }
 }
 
 void validate_global_topology_consistent(
     const topology::Global_topology &topology) {
   validate_global_topology_active_cluster_available(topology);
 
+  assert(topology.type() == Global_topology_type::SINGLE_PRIMARY_TREE);
   if (topology.type() == Global_topology_type::SINGLE_PRIMARY_TREE) {
     validate_star_topology_consistent(topology);
-  } else {
-    assert(0);
   }
 
   //   // check the cluster that will be our master