BUG#35795129 BUG#35782142 BUG#29455103 BUG#29949127

Misc cosmetic fixes and code cleanups

BUG#35795129 Incorrect help for Cluster.describe

The helper text for Cluster.describe() mentions that the output includes
a field named 'version' with the version of each member. That's
incorrect and there's no such field in described().

This patch removes the incorrect information from the helper text.

BUG#35782142 Shell user, api, and cmdline docs missing info on resource naming

The API documentation and helper lacks information about naming
limitations for the Cluster/ReplicaSet/ClusterSet name and instance labels.

This patch extends the API documentation and helper with all the
naming limitations.

BUG#29455103 IMPROVE MESSAGES FOR CONFIGUREINSTANCE WHEN INSTANCE IS VALID

dba.configureInstance() prints two similar messages when an instance is
already ready to be used in a Cluster. For example:

The instance 'localhost:3300' is valid for InnoDB cluster usage.
The instance 'localhost:3300' is already ready for InnoDB cluster usage.

The second message is redundant so this patch removes it while improving
the first one.

BUG#29949127 MENTION CURRENT SERVER VERSION WHEN ADD_INSTANCE() FAILS DUE < 8.017

When addInstance() finds that only clone can be used and the target
instance does not support it because of its version, the command prints
an error indicating clone must be used and that it is available since
8.0.17, however, it does not include the target instance version.

To make the message more clear, this patch includes the target instance
version in the error message.

Change-Id: Id0fdac5699e9f0a0697ebe780fd6107d4513826c

Author: miguelaraujo

modules/adminapi/common/cluster_types.cc

@@ -200,13 +200,13 @@ std::string to_display_string(Cluster_type type, Display_form form) {
     case Cluster_type::GROUP_REPLICATION:
       switch (form) {
         case Display_form::THING_FULL:
-          return "InnoDB cluster";
+          return "InnoDB Cluster";
 
         case Display_form::A_THING_FULL:
-          return "an InnoDB cluster";
+          return "an InnoDB Cluster";
 
         case Display_form::THINGS_FULL:
-          return "InnoDB clusters";
+          return "InnoDB Clusters";
 
         case Display_form::THING:
           return "cluster";

modules/adminapi/common/gtid_validations.cc

@@ -31,7 +31,7 @@
 #include "mysqlshdk/libs/mysql/clone.h"
 #include "mysqlshdk/libs/mysql/gtid_utils.h"
 #include "mysqlshdk/libs/mysql/replication.h"
-#include "mysqlshdk/libs/textui/textui.h"
+#include "mysqlshdk/libs/utils/version.h"
 
 namespace mysqlsh {
 namespace dba {
@@ -139,18 +139,20 @@ void validate_incremental_recovery(Member_op_action op_action,
   }
 }
 
-Prompt_type validate_auto_recovery(Cluster_type cluster_type,
-                                   Member_op_action op_action,
-                                   bool recovery_possible, bool recovery_safe,
-                                   bool clone_supported, bool gtid_set_diverged,
-                                   bool interactive, bool clone_disabled) {
+Prompt_type validate_auto_recovery(
+    Cluster_type cluster_type, Member_op_action op_action,
+    bool recovery_possible, bool recovery_safe, bool clone_supported,
+    bool gtid_set_diverged, bool interactive, bool clone_disabled,
+    mysqlshdk::utils::Version target_instance_version) {
   auto console = current_console();
 
   Prompt_type prompt = None;
 
   std::string clone_available_since =
       "Built-in clone support is available starting with MySQL 8.0.17 and is "
-      "the recommended method for provisioning instances.";
+      "the recommended method for provisioning instances. Instance is running "
+      "MySQL " +
+      target_instance_version.get_full() + ".";
 
   std::string target_clone_or_provisioned_add =
       "The target instance must be either cloned or fully provisioned before "
@@ -510,7 +512,8 @@ Member_recovery_method validate_instance_recovery(
   } else {
     Prompt_type prompt = validate_auto_recovery(
         cluster_type, op_action, recovery_possible, recovery_safe,
-        clone_supported, gtid_set_diverged, interactive, clone_disabled);
+        clone_supported, gtid_set_diverged, interactive, clone_disabled,
+        target_instance.get_version());
 
     console->print_info();
 

modules/adminapi/common/validations.cc

@@ -29,6 +29,7 @@
 #include "modules/adminapi/common/common.h"
 #include "modules/adminapi/common/dba_errors.h"
 #include "modules/adminapi/common/instance_validations.h"
+#include "modules/adminapi/common/topology_executor.h"
 #include "modules/adminapi/dba/check_instance.h"
 #include "mysqlshdk/include/shellcore/console.h"
 #include "mysqlshdk/libs/mysql/replication.h"
@@ -49,11 +50,11 @@ void ensure_gr_instance_configuration_valid(
         "Validating instance configuration at " +
         target_instance->get_connection_options().uri_endpoint() + "...");
 
-    Check_instance check(target_instance->get_connection_options(), "", true,
-                         skip_check_tables_pk);
-    check.prepare();
-    result = check.execute();
-    check.finish();
+    result =
+        Topology_executor<Check_instance>{
+            target_instance->get_connection_options(), "", true,
+            skip_check_tables_pk}
+            .run();
 
     if (!result || result.as_map()->at("status").get_string() == "ok") {
       console->print_info("Instance configuration is suitable.");

modules/adminapi/dba/check_instance.cc

@@ -25,26 +25,17 @@
 
 #include <memory>
 
-#include "adminapi/common/common.h"
 #include "modules/adminapi/common/accounts.h"
+#include "modules/adminapi/common/common.h"
 #include "modules/adminapi/common/instance_validations.h"
-#include "modules/adminapi/common/provision.h"
 #include "modules/adminapi/common/server_features.h"
-#include "modules/adminapi/common/sql.h"
 #include "modules/adminapi/common/validations.h"
 #include "mysqlshdk/include/shellcore/console.h"
-#include "mysqlshdk/libs/config/config_file_handler.h"
-#include "mysqlshdk/libs/config/config_server_handler.h"
-#include "mysqlshdk/libs/db/mysql/session.h"
 #include "mysqlshdk/libs/mysql/clone.h"
-#include "mysqlshdk/libs/mysql/replication.h"
 #include "mysqlshdk/libs/utils/utils_file.h"
-#include "mysqlshdk/libs/utils/utils_general.h"
 #include "mysqlshdk/libs/utils/utils_net.h"
-#include "mysqlshdk/libs/utils/utils_string.h"
 
-namespace mysqlsh {
-namespace dba {
+namespace mysqlsh::dba {
 
 // Constructor receives command parameters, ref to ProvisioningInterface and
 // ref to IConsole. It should also perform basic validation of
@@ -58,8 +49,6 @@ Check_instance::Check_instance(
       m_silent(silent),
       m_skip_check_tables_pk{skip_check_tables_pk} {}
 
-Check_instance::~Check_instance() = default;
-
 void Check_instance::check_instance_address() {
   // Sanity check for the instance address
   if (is_sandbox(*m_target_instance, nullptr)) {
@@ -158,17 +147,46 @@ bool Check_instance::check_configuration() {
     return false;
   }
 
-  if (!m_silent)
+  if (!m_silent) {
     console->print_info(
         "Instance configuration is compatible with InnoDB cluster");
+  }
+
   return true;
 }
 
-/*
- * Validates the parameter and performs other validations regarding
- * the command execution
- */
-void Check_instance::prepare() {
+void Check_instance::prepare_config_object() {
+  bool use_cfg_handler = false;
+  // if the configuration file was provided and exists, we add it to the
+  // config object.
+  if (!m_mycnf_path.empty()) {
+    if (shcore::is_file(m_mycnf_path)) {
+      use_cfg_handler = true;
+    } else {
+      mysqlsh::current_console()->print_error(
+          "Configuration file " + m_mycnf_path +
+          " doesn't exist. The verification of the file will be skipped.");
+
+      // Clear it up so we don't print any odd log message
+      m_mycnf_path = "";
+    }
+  }
+  // Add server configuration handler depending on SET PERSIST support.
+  // NOTE: Add server handler first to set it has the default handler.
+  m_cfg = mysqlsh::dba::create_server_config(
+      m_target_instance.get(), mysqlshdk::config::k_dft_cfg_server_handler,
+      true);
+
+  // Add configuration handle to update option file (if provided) and not to
+  // be skipped
+  if (use_cfg_handler) {
+    mysqlsh::dba::add_config_file_handler(
+        m_cfg.get(), mysqlshdk::config::k_dft_cfg_file_handler,
+        m_target_instance->get_uuid(), m_mycnf_path, m_mycnf_path);
+  }
+}
+
+shcore::Value Check_instance::do_run() {
   auto console = mysqlsh::current_console();
 
   // Establish a session to the target instance
@@ -184,12 +202,12 @@ void Check_instance::prepare() {
     if (!local_target) {
       console->print_info("Validating MySQL instance at " +
                           m_target_instance->descr() +
-                          " for use in an InnoDB cluster...");
+                          " for use in an InnoDB Cluster...");
     } else {
       console->print_info(
           "Validating local MySQL instance listening at port " +
           std::to_string(m_target_instance->get_canonical_port()) +
-          " for use in an InnoDB cluster...");
+          " for use in an InnoDB Cluster...");
     }
   }
 
@@ -242,57 +260,11 @@ void Check_instance::prepare() {
   if (m_is_valid && !m_silent) {
     console->print_info();
     console->print_info("The instance '" + target +
-                        "' is valid to be used in an InnoDB cluster.");
+                        "' is valid for InnoDB Cluster usage.");
   }
-}
 
-/*
- * Executes the API command.
- *
- * This is a purely check command, no changes are made, thus execute()
- * is a no-op.
- */
-shcore::Value Check_instance::execute() {
   mysqlsh::current_console()->print_info();
-  return m_ret_val;
-}
-
-void Check_instance::rollback() {
-  // nothing to rollback
-}
-
-void Check_instance::finish() {}
-
-void Check_instance::prepare_config_object() {
-  bool use_cfg_handler = false;
-  // if the configuration file was provided and exists, we add it to the
-  // config object.
-  if (!m_mycnf_path.empty()) {
-    if (shcore::is_file(m_mycnf_path)) {
-      use_cfg_handler = true;
-    } else {
-      mysqlsh::current_console()->print_error(
-          "Configuration file " + m_mycnf_path +
-          " doesn't exist. The verification of the file will be skipped.");
 
-      // Clear it up so we don't print any odd log message
-      m_mycnf_path = "";
-    }
-  }
-  // Add server configuration handler depending on SET PERSIST support.
-  // NOTE: Add server handler first to set it has the default handler.
-  m_cfg = mysqlsh::dba::create_server_config(
-      m_target_instance.get(), mysqlshdk::config::k_dft_cfg_server_handler,
-      true);
-
-  // Add configuration handle to update option file (if provided) and not to
-  // be skipped
-  if (use_cfg_handler) {
-    mysqlsh::dba::add_config_file_handler(
-        m_cfg.get(), mysqlshdk::config::k_dft_cfg_file_handler,
-        m_target_instance->get_uuid(), m_mycnf_path, m_mycnf_path);
-  }
+  return m_ret_val;
 }
-
-}  // namespace dba
-}  // namespace mysqlsh
+}  // namespace mysqlsh::dba

modules/adminapi/dba/check_instance.h

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2017, 2022, Oracle and/or its affiliates.
+ * Copyright (c) 2017, 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,
@@ -24,30 +24,30 @@
 #ifndef MODULES_ADMINAPI_DBA_CHECK_INSTANCE_H_
 #define MODULES_ADMINAPI_DBA_CHECK_INSTANCE_H_
 
-#include <memory>
-#include <optional>
-#include <string>
-
 #include "modules/adminapi/common/instance_pool.h"
-#include "modules/adminapi/common/provisioning_interface.h"
-#include "modules/adminapi/dba/configure_instance.h"
-#include "modules/command_interface.h"
-#include "mysqlshdk/include/scripting/types_cpp.h"
+#include "mysqlshdk/libs/config/config.h"
 
-namespace mysqlsh {
-namespace dba {
+namespace mysqlsh::dba {
 
-class Check_instance : public Command_interface {
+class Check_instance {
  public:
-  Check_instance(const mysqlshdk::db::Connection_options &instance_cnx_opts,
-                 const std::string &verify_mycnf_path, bool silent = false,
-                 bool skip_check_tables_pk = false);
-  ~Check_instance();
+  Check_instance() = delete;
+
+  explicit Check_instance(
+      const mysqlshdk::db::Connection_options &instance_cnx_opts,
+      const std::string &verify_mycnf_path, bool silent = false,
+      bool skip_check_tables_pk = false);
+
+  Check_instance(const Check_instance &) = delete;
+  Check_instance(Check_instance &&) = delete;
+  Check_instance &operator=(const Check_instance &) = delete;
+  Check_instance &operator=(Check_instance &&) = delete;
+
+  ~Check_instance() = default;
 
-  void prepare() override;
-  shcore::Value execute() override;
-  void rollback() override;
-  void finish() override;
+ protected:
+  shcore::Value do_run();
+  static constexpr bool supports_undo() noexcept { return false; }
 
  private:
   void check_instance_address();
@@ -79,7 +79,6 @@ class Check_instance : public Command_interface {
   std::unique_ptr<mysqlshdk::config::Config> m_cfg;
 };
 
-}  // namespace dba
-}  // namespace mysqlsh
+}  // namespace mysqlsh::dba
 
 #endif  // MODULES_ADMINAPI_DBA_CHECK_INSTANCE_H_