BUG#35358531 ClusterSet.createReplicaCluster proposing non usable clone provisioning

By default, all commands that allow clone-based recovery will pick a
clone donor if not explicitly set using the 'cloneDonor' option.
However, the validations regarding donor and recipient are only
performed when the cloneDonor option is used.

This patch fixes that by ensuring the usage of clone is valid for
recovery and fail soon avoiding any execution of steps and possible
revert of operations.

Other than that, this patch also fixes a bug in rejoinInstance() for
Read-Replicas on which the cloneDonor option was being ignored so a
donor would be picked automatically.

Change-Id: I9f8983c0a2f2a78744de9c3a04cde0873875a811

Author: miguelaraujo

modules/adminapi/cluster/add_replica_instance.cc

@@ -377,13 +377,14 @@ void Add_replica_instance::do_run() {
         m_options.clone_options.clone_donor.has_value()) {
       std::string donor = *m_options.clone_options.clone_donor;
 
-      // Check if the donor is valid
-      m_cluster_impl->ensure_compatible_clone_donor(donor, m_target_instance);
-
       m_donor_instance =
           Scoped_instance(m_cluster_impl->connect_target_instance(donor));
     }
 
+    // Check if the donor is valid
+    m_cluster_impl->ensure_compatible_clone_donor(*m_donor_instance,
+                                                  *m_target_instance);
+
     console->print_info("* Checking transaction state of the instance...");
     m_options.clone_options.recovery_method = validate_instance_recovery();
   }

modules/adminapi/cluster/cluster_impl.cc

@@ -2432,6 +2432,43 @@ void Cluster_impl::refresh_connections() {
   }
 }
 
+void Cluster_impl::ensure_compatible_clone_donor(
+    const mysqlshdk::mysql::IInstance &donor,
+    const mysqlshdk::mysql::IInstance &recipient) {
+  /*
+   * A donor is compatible if:
+   *
+   *   - It's an ONLINE member of the Cluster
+   *   - The target (recipient) and donor instances support clone (version
+   *     >= 8.0.17)
+   *   - It has the same version of the recipient
+   *   - It has the same operating system as the recipient
+   */
+
+  // Check if the donor belongs to the PRIMARY Cluster (MD)
+  std::string donor_address = donor.get_canonical_address();
+  try {
+    get_metadata_storage()->get_instance_by_address(donor_address);
+  } catch (const shcore::Exception &e) {
+    if (e.code() == SHERR_DBA_MEMBER_METADATA_MISSING) {
+      throw shcore::Exception("Instance " + donor_address +
+                                  " does not belong to the Primary Cluster",
+                              SHERR_DBA_BADARG_INSTANCE_NOT_IN_CLUSTER);
+    }
+    throw;
+  }
+
+  auto target_status = mysqlshdk::gr::get_member_state(donor);
+
+  if (target_status != mysqlshdk::gr::Member_state::ONLINE) {
+    throw shcore::Exception("Instance " + donor_address +
+                                " is not an ONLINE member of the Cluster.",
+                            SHERR_DBA_BADARG_INSTANCE_NOT_ONLINE);
+  }
+
+  Base_cluster_impl::ensure_compatible_clone_donor(donor, recipient);
+}
+
 void Cluster_impl::setup_admin_account(const std::string &username,
                                        const std::string &host,
                                        const Setup_account_options &options) {

modules/adminapi/cluster/cluster_impl.h

@@ -584,6 +584,10 @@ class Cluster_impl final : public Base_cluster_impl,
 
   void refresh_connections();
 
+  void ensure_compatible_clone_donor(
+      const mysqlshdk::mysql::IInstance &donor,
+      const mysqlshdk::mysql::IInstance &recipient) override;
+
   // Lock methods
 
   [[nodiscard]] mysqlshdk::mysql::Lock_scoped get_lock_shared(

modules/adminapi/cluster/rejoin_replica_instance.cc

@@ -127,45 +127,19 @@ bool Rejoin_replica_instance::check_rejoinable() {
 }
 
 Member_recovery_method Rejoin_replica_instance::validate_instance_recovery() {
-  // When replicationSources is set to 'primary' or 'secondary' the main source
-  // is the primary member of the cluster
-  if (m_replication_sources.source_type == Source_type::PRIMARY ||
-      m_replication_sources.source_type == Source_type::SECONDARY) {
-    m_clone_donor_instance = m_cluster_impl->get_cluster_server();
-  } else {
-    // When replicationSources is set to a list of instances the main source
-    // must be the first member of the list (the one with highest weight)
-    auto first_element = *(m_replication_sources.replication_sources.begin());
-
-    std::string source_str = first_element.to_string();
-
-    std::shared_ptr<Instance> source_instance;
-
-    try {
-      source_instance =
-          m_cluster_impl->get_session_to_cluster_instance(source_str);
-    } catch (const shcore::Exception &e) {
-      throw shcore::Exception(
-          "Source is not reachable",
-          SHERR_DBA_READ_REPLICA_INVALID_SOURCE_LIST_UNREACHABLE);
-    }
-
-    m_clone_donor_instance = source_instance;
-  }
-
   auto check_recoverable =
       [=](const mysqlshdk::mysql::IInstance &tgt_instance) {
         // Get the gtid state in regards to the donor
         mysqlshdk::mysql::Replica_gtid_state state =
-            check_replica_group_gtid_state(*m_clone_donor_instance,
-                                           tgt_instance, nullptr, nullptr);
+            check_replica_group_gtid_state(*m_donor_instance, tgt_instance,
+                                           nullptr, nullptr);
 
         return (state != mysqlshdk::mysql::Replica_gtid_state::IRRECOVERABLE);
       };
 
   return mysqlsh::dba::validate_instance_recovery(
       Cluster_type::GROUP_REPLICATION, Member_op_action::ADD_INSTANCE,
-      *m_clone_donor_instance, *m_target_instance, check_recoverable,
+      *m_donor_instance, *m_target_instance, check_recoverable,
       m_options.clone_options.recovery_method.get_safe(),
       m_cluster_impl->get_gtid_set_is_complete(), m_options.interactive());
 }
@@ -188,23 +162,57 @@ void Rejoin_replica_instance::validate_replication_channels() {
   }
 }
 
+std::shared_ptr<Instance>
+Rejoin_replica_instance::get_default_donor_instance() {
+  // When replicationSources is set to 'primary' or 'secondary' the main source
+  // is the primary member of the cluster
+  if (m_replication_sources.source_type == Source_type::PRIMARY ||
+      m_replication_sources.source_type == Source_type::SECONDARY) {
+    return m_cluster_impl->get_cluster_server();
+  } else {
+    // When replicationSources is set to a list of instances the main source
+    // must be the first member of the list (the one with highest weight)
+    assert(!m_replication_sources.replication_sources.empty());
+    auto first_element = *(m_replication_sources.replication_sources.begin());
+
+    std::string source_str = first_element.to_string();
+
+    std::shared_ptr<Instance> source_instance;
+
+    try {
+      source_instance =
+          m_cluster_impl->get_session_to_cluster_instance(source_str);
+    } catch (const shcore::Exception &e) {
+      throw shcore::Exception(
+          "Source is not reachable",
+          SHERR_DBA_READ_REPLICA_INVALID_SOURCE_LIST_UNREACHABLE);
+    }
+
+    return source_instance;
+  }
+}
+
 void Rejoin_replica_instance::prepare() {
   auto console = current_console();
 
   // Validate the Clone options.
   m_options.clone_options.check_option_values(m_target_instance->get_version());
 
-  // If the cloneDonor option us used, validate if the selected donor is
-  // valid and set it as the donor instance
+  // Get the main donor and source instance
+  // By default, the instance is:
+  //
+  //   - The primary member when 'replicationSources' is set to "primary" or
+  //   "secondary"
+  //   - The first member of the 'replicationSources' list
+  m_donor_instance = get_default_donor_instance();
+
+  // If the cloneDonor option us used set it as the donor instance
   if (*m_options.clone_options.recovery_method ==
       Member_recovery_method::CLONE) {
     if (m_options.clone_options.clone_donor.has_value()) {
       std::string donor = *m_options.clone_options.clone_donor;
 
-      // Check if the donor is valid
-      m_cluster_impl->ensure_compatible_clone_donor(donor, m_target_instance);
-
-      m_clone_donor_instance =
+      m_donor_instance =
           Scoped_instance(m_cluster_impl->connect_target_instance(donor));
     }
   }
@@ -220,6 +228,10 @@ void Rejoin_replica_instance::prepare() {
 
   console->print_info("* Checking transaction state of the instance...");
   m_options.clone_options.recovery_method = validate_instance_recovery();
+
+  // Check if the donor is valid
+  m_cluster_impl->ensure_compatible_clone_donor(*m_donor_instance,
+                                                *m_target_instance);
 }
 
 void Rejoin_replica_instance::do_run() {
@@ -257,9 +269,9 @@ void Rejoin_replica_instance::do_run() {
   if (*m_options.clone_options.recovery_method ==
       Member_recovery_method::CLONE) {
     m_cluster_impl->handle_clone_provisioning(
-        m_target_instance, m_clone_donor_instance, ar_options,
-        repl_account_host, "", "", m_options.get_recovery_progress(),
-        m_options.timeout, m_options.dry_run);
+        m_target_instance, m_donor_instance, ar_options, repl_account_host, "",
+        "", m_options.get_recovery_progress(), m_options.timeout,
+        m_options.dry_run);
 
     // Clone will copy all tables, including the replication settings stored
     // in mysql.slave_master_info. MySQL will start replication by default

modules/adminapi/cluster/rejoin_replica_instance.h

@@ -58,6 +58,7 @@ class Rejoin_replica_instance {
   void prepare();
   Member_recovery_method validate_instance_recovery();
   void validate_replication_channels();
+  std::shared_ptr<Instance> get_default_donor_instance();
 
  protected:
   shcore::Scoped_callback_list m_undo_list;
@@ -66,7 +67,7 @@ class Rejoin_replica_instance {
  private:
   Cluster_impl *m_cluster_impl = nullptr;
   std::shared_ptr<mysqlsh::dba::Instance> m_target_instance;
-  std::shared_ptr<mysqlsh::dba::Instance> m_clone_donor_instance;
+  std::shared_ptr<mysqlsh::dba::Instance> m_donor_instance;
   Rejoin_instance_options m_options;
   std::string m_account_host;
   std::string m_target_read_replica_address;

modules/adminapi/cluster_set/create_replica_cluster.cc

@@ -430,13 +430,11 @@ shcore::Value Create_replica_cluster::execute() {
     // Handle clone provisioning
     if (*m_options.clone_options.recovery_method ==
         Member_recovery_method::CLONE) {
-      // Ensure cloneDonor is valid and pick a valid donor if the option is not
-      // used. By default, the donor must be the PRIMARY member of the PRIMARY
-      // Cluster.
+      // Pick a donor if the option is not used. By default, the donor must be
+      // the PRIMARY member of the PRIMARY Cluster.
       std::string donor;
       if (m_options.clone_options.clone_donor.has_value()) {
         donor = *m_options.clone_options.clone_donor;
-        m_cluster_set->ensure_compatible_clone_donor(donor, m_target_instance);
       } else {
         // Pick the primary instance of the primary cluster as donor
         donor = m_primary_instance->get_canonical_address();
@@ -445,6 +443,13 @@ shcore::Value Create_replica_cluster::execute() {
       const auto donor_instance =
           Scoped_instance(m_cluster_set->connect_target_instance(donor));
 
+      // Ensure the donor is valid:
+      //   - It's an ONLINE member of the Primary Cluster
+      //   - It has the same version of the recipient
+      //   - It has the same operating system as the recipient
+      m_cluster_set->get_primary_cluster()->ensure_compatible_clone_donor(
+          donor_instance, *m_target_instance);
+
       m_cluster_set->handle_clone_provisioning(
           m_target_instance, donor_instance, ar_options, repl_account_host,
           m_cluster_set->query_clusterset_auth_cert_issuer(),

modules/adminapi/common/base_cluster_impl.cc

@@ -30,11 +30,11 @@
 #include <string>
 #include <vector>
 
-#include "adminapi/cluster/cluster_impl.h"
-#include "adminapi/common/cluster_types.h"
-#include "adminapi/common/common.h"
+#include "modules/adminapi/cluster/cluster_impl.h"
 #include "modules/adminapi/common/accounts.h"
 #include "modules/adminapi/common/async_topology.h"
+#include "modules/adminapi/common/cluster_types.h"
+#include "modules/adminapi/common/common.h"
 #include "modules/adminapi/common/dba_errors.h"
 #include "modules/adminapi/common/errors.h"
 #include "modules/adminapi/common/instance_monitoring.h"
@@ -43,11 +43,13 @@
 #include "modules/adminapi/common/metadata_storage.h"
 #include "modules/adminapi/common/preconditions.h"
 #include "modules/adminapi/common/router.h"
+#include "modules/adminapi/common/server_features.h"
 #include "modules/adminapi/common/setup_account.h"
 #include "modules/adminapi/common/sql.h"
 #include "modules/mod_shell.h"
 #include "modules/mysqlxtest_utils.h"
 #include "mysqlshdk/include/shellcore/console.h"
+#include "mysqlshdk/include/shellcore/utils_help.h"
 #include "mysqlshdk/libs/mysql/async_replication.h"
 #include "mysqlshdk/libs/mysql/group_replication.h"
 #include "mysqlshdk/libs/mysql/replication.h"
@@ -56,7 +58,7 @@
 #include "mysqlshdk/libs/utils/utils_general.h"
 #include "mysqlshdk/libs/utils/utils_mysql_parsing.h"
 #include "mysqlshdk/libs/utils/utils_net.h"
-#include "shellcore/utils_help.h"
+#include "mysqlshdk/libs/utils/version.h"
 
 namespace mysqlsh {
 namespace dba {
@@ -577,62 +579,67 @@ void Base_cluster_impl::handle_clone_provisioning(
 }
 
 void Base_cluster_impl::ensure_compatible_clone_donor(
-    const std::string &donor_def,
-    const std::shared_ptr<mysqlsh::dba::Instance> &recipient) {
+    const mysqlshdk::mysql::IInstance &donor,
+    const mysqlshdk::mysql::IInstance &recipient) {
   /*
    * A donor is compatible if:
    *
-   *   - It's an ONLINE member of the Cluster
+   *   - Supports clone
    *   - It has the same version of the recipient
    *   - It has the same operating system as the recipient
    */
+  std::string target_address = donor.get_canonical_address();
 
-  const auto target = Scoped_instance(connect_target_instance(donor_def));
-
-  // Check if the target belongs to the PRIMARY Cluster (MD)
-  std::string target_address = target->get_canonical_address();
-  try {
-    get_metadata_storage()->get_instance_by_address(target_address);
-  } catch (const shcore::Exception &e) {
-    if (e.code() == SHERR_DBA_MEMBER_METADATA_MISSING) {
-      throw shcore::Exception("Instance " + target_address +
-                                  " does not belong to the Primary Cluster",
-                              SHERR_DBA_BADARG_INSTANCE_NOT_IN_CLUSTER);
-    }
-    throw;
+  // Check if the instance support clone
+  if (!supports_mysql_clone(donor.get_version())) {
+    throw shcore::Exception(
+        "Instance " + target_address + " does not support MySQL Clone.",
+        SHERR_DBA_CLONE_NO_SUPPORT);
   }
 
-  auto target_status = mysqlshdk::gr::get_member_state(*target);
+  auto donor_version = donor.get_version();
+  auto recipient_version = recipient.get_version();
 
-  if (target_status != mysqlshdk::gr::Member_state::ONLINE) {
-    throw shcore::Exception("Instance " + target_address +
-                                " is not an ONLINE member of the Cluster.",
-                            SHERR_DBA_BADARG_INSTANCE_NOT_ONLINE);
-  }
+  DBUG_EXECUTE_IF("dba_clone_version_check_fail",
+                  { donor_version = mysqlshdk::utils::Version(8, 0, 17); });
 
   // Check if the instance has the same version of the recipient
-  if (target->get_version() != recipient->get_version()) {
+  if (donor_version != recipient_version) {
     throw shcore::Exception("Instance " + target_address +
                                 " cannot be a donor because it has a different "
-                                "version than the recipient.",
+                                "version (" +
+                                donor_version.get_full() +
+                                ") than the recipient (" +
+                                recipient_version.get_full() + ").",
                             SHERR_DBA_CLONE_DIFF_VERSION);
   }
 
   // Check if the instance has the same OS the recipient
-  if (target->get_version_compile_os() != recipient->get_version_compile_os()) {
+  auto donor_version_compile_os = donor.get_version_compile_os();
+  auto recipient_version_compile_os = recipient.get_version_compile_os();
+
+  if (donor_version_compile_os != recipient_version_compile_os) {
     throw shcore::Exception("Instance " + target_address +
                                 " cannot be a donor because it has a different "
-                                "Operating System than the recipient.",
+                                "Operating System (" +
+                                donor_version_compile_os +
+                                ") than the recipient (" +
+                                recipient_version_compile_os + ").",
                             SHERR_DBA_CLONE_DIFF_OS);
   }
 
   // Check if the instance is running on the same platform of the recipient
-  if (target->get_version_compile_machine() !=
-      recipient->get_version_compile_machine()) {
+  auto donor_version_compile_machine = donor.get_version_compile_machine();
+  auto recipient_version_compile_machine =
+      recipient.get_version_compile_machine();
+
+  if (donor_version_compile_machine != recipient_version_compile_machine) {
     throw shcore::Exception(
         "Instance " + target_address +
             " cannot be a donor because it is running on a different "
-            "platform than the recipient.",
+            "platform (" +
+            donor_version_compile_machine + ") than the recipient (" +
+            recipient_version_compile_machine + ").",
         SHERR_DBA_CLONE_DIFF_PLATFORM);
   }
 }