Fix stats_updates_frequency router option in ClusterSets

joaosigma · joaosigma · commit f5ef1bdb523d · 2023-06-02T14:59:39.000+01:00
WL#15601 added support for stats_updates_frequency in Clusters and
ReplicaSets, but inadvertently changed how the default value of the
option is used in ClusterSets, when the metadata version is older than
2.2. This is due to how the Router treats the default value of the
option.

This patch fixes this by making sure that, when the metadata has a
version older than 2.2, the default value of stats_updates_frequency is
0 rather than null.

Change-Id: Ia07f9c94d6432a97930ff094194e549c62f930ef
diff --git a/modules/adminapi/cluster_set/cluster_set_impl.cc b/modules/adminapi/cluster_set/cluster_set_impl.cc
@@ -1915,26 +1915,24 @@ void Cluster_set_impl::record_in_metadata(
   // delete unrelated records if there are any
   metadata->cleanup_for_cluster(seed_cluster_id);
 
-  Cluster_set_id csid = metadata->create_cluster_set_record(
-      this, seed_cluster_id, shcore::make_dict());
-
-  m_id = csid;
+  m_id = metadata->create_cluster_set_record(this, seed_cluster_id,
+                                             shcore::make_dict());
 
   // Record the ClusterSet SSL mode in the Metadata
   metadata->update_cluster_set_attribute(
-      csid, k_cluster_set_attribute_ssl_mode,
+      m_id, k_cluster_set_attribute_ssl_mode,
       shcore::Value(to_string(options.ssl_mode)));
 
   metadata->update_cluster_set_attribute(
-      csid, k_cluster_set_attribute_member_auth_type,
+      m_id, k_cluster_set_attribute_member_auth_type,
       shcore::Value(to_string(auth_type)));
 
   metadata->update_cluster_set_attribute(
-      csid, k_cluster_set_attribute_cert_issuer,
+      m_id, k_cluster_set_attribute_cert_issuer,
       shcore::Value(member_auth_cert_issuer));
 
   metadata->update_cluster_set_attribute(
-      csid, k_cluster_attribute_replication_allowed_host,
+      m_id, k_cluster_attribute_replication_allowed_host,
       options.replication_allowed_host.empty()
           ? shcore::Value("%")
           : shcore::Value(options.replication_allowed_host));
@@ -1946,9 +1944,16 @@ void Cluster_set_impl::record_in_metadata(
   metadata->migrate_routers_to_clusterset(seed_cluster_id, get_id());
 
   // Set and store the default Routing Options
-  for (const auto &i : k_default_clusterset_router_options)
-    metadata->set_global_routing_option(Cluster_type::REPLICATED_CLUSTER, csid,
-                                        i.first, i.second);
+  for (const auto &[name, value] : k_default_clusterset_router_options) {
+    if ((name == k_router_option_stats_updates_frequency) &&
+        (metadata->installed_version() < mysqlshdk::utils::Version(2, 2))) {
+      metadata->set_global_routing_option(Cluster_type::REPLICATED_CLUSTER,
+                                          m_id, name, shcore::Value(0));
+    } else {
+      metadata->set_global_routing_option(Cluster_type::REPLICATED_CLUSTER,
+                                          m_id, name, value);
+    }
+  }
 
   // Migrate the Read Only Targets setting from Cluster to ClusterSet
   log_info(
diff --git a/modules/adminapi/common/metadata_management_mysql.cc b/modules/adminapi/common/metadata_management_mysql.cc
@@ -119,6 +119,20 @@ bool schema_exists(const std::shared_ptr<Instance> &group_server,
 
   return row ? true : false;
 }
+
+void adjust_data(const Instance &group_server, const Version &version) {
+  // the option "stats_updates_frequency" should be upgraded to "null", if
+  // previous value is 0
+  if (version >= Version(2, 2, 0))
+    group_server.execute(
+        "UPDATE mysql_innodb_cluster_metadata.clustersets SET router_options = "
+        "JSON_SET(router_options, '$.stats_updates_frequency', CAST(NULL as "
+        "JSON)) WHERE "
+        "(NULLIF(router_options->>'$.stats_updates_frequency','null') IS NOT "
+        "NULL) AND (CAST(router_options->>'$.stats_updates_frequency' AS "
+        "SIGNED INTEGER) = 0)");
+}
+
 }  // namespace
 
 namespace scripts {
@@ -797,10 +811,8 @@ void do_upgrade_schema(const std::shared_ptr<Instance> &group_server,
 
   auto console = mysqlsh::current_console();
 
-  std::string instance_data = group_server->descr();
-  auto path = get_upgrade_path(installed_ver);
   if (!dry_run) {
-    console->print_info("Upgrading metadata at '" + instance_data +
+    console->print_info("Upgrading metadata at '" + group_server->descr() +
                         "' from version " + installed_ver.get_base() +
                         " to version " + last_ver.get_base() + ".");
   }
@@ -824,114 +836,117 @@ void do_upgrade_schema(const std::shared_ptr<Instance> &group_server,
                            kMetadataSchemaPreviousName));
   }
 
+  auto path = get_upgrade_path(installed_ver);
   size_t total = path.size();
+
   if (path.size() > 1) {
     console->print_info(shcore::str_format(
         "Upgrade %s require %zu steps", (dry_run ? "would" : "will"), total));
   }
 
-  if (!dry_run) {
-    mysqlshdk::utils::nullable<upgrade::Stage> stage;
-    Version actual_version = installed_ver;
+  if (dry_run) return;
 
-    std::vector<std::string> tables;
-    Instance_list locked_instances;
-    try {
-      // Acquires the upgrade locks
-      locked_instances = upgrade::get_locks(group_server);
+  mysqlshdk::utils::nullable<upgrade::Stage> stage;
+  Version actual_version = installed_ver;
 
-      console->print_info("Creating backup of the metadata schema...");
+  std::vector<std::string> tables;
+  Instance_list locked_instances;
+  try {
+    // Acquires the upgrade locks
+    locked_instances = upgrade::get_locks(group_server);
 
-      backup(installed_ver, kMetadataSchemaName, kMetadataSchemaBackupName,
-             group_server, "creating backup of the metadata");
+    console->print_info("Creating backup of the metadata schema...");
 
-      DBUG_EXECUTE_IF("dba_FAIL_metadata_upgrade_at_BACKING_UP_METADATA", {
-        throw std::logic_error(
-            "Debug emulation of failed metadata upgrade "
-            "BACKING_UP_METADATA.");
-      });
+    backup(installed_ver, kMetadataSchemaName, kMetadataSchemaBackupName,
+           group_server, "creating backup of the metadata");
 
-      // Sets the upgrading version
-      DBUG_EXECUTE_IF("dba_limit_lock_wait_timeout",
-                      { group_server->execute("SET lock_wait_timeout=1"); });
+    DBUG_EXECUTE_IF("dba_FAIL_metadata_upgrade_at_BACKING_UP_METADATA", {
+      throw std::logic_error(
+          "Debug emulation of failed metadata upgrade BACKING_UP_METADATA.");
+    });
 
-      stage = upgrade::save_stage(group_server,
-                                  upgrade::Stage::SETTING_UPGRADE_VERSION);
-      actual_version = set_schema_version(group_server, kUpgradingVersion);
+    // Sets the upgrading version
+    DBUG_EXECUTE_IF("dba_limit_lock_wait_timeout",
+                    { group_server->execute("SET lock_wait_timeout=1"); });
 
-      stage = upgrade::save_stage(group_server, upgrade::Stage::UPGRADING);
+    stage = upgrade::save_stage(group_server,
+                                upgrade::Stage::SETTING_UPGRADE_VERSION);
+    actual_version = set_schema_version(group_server, kUpgradingVersion);
 
-      DBUG_EXECUTE_IF("dba_CRASH_metadata_upgrade_at_UPGRADING", {
-        // On a CRASH the cleanup logic would not be executed letting the
-        // metadata in UPGRADING stage, however the locks would be released
-        upgrade::release_locks(locked_instances);
-        return;
-      });
+    stage = upgrade::save_stage(group_server, upgrade::Stage::UPGRADING);
 
-      group_server->execute("SET FOREIGN_KEY_CHECKS=0");
+    DBUG_EXECUTE_IF("dba_CRASH_metadata_upgrade_at_UPGRADING", {
+      // On a CRASH the cleanup logic would not be executed letting the
+      // metadata in UPGRADING stage, however the locks would be released
+      upgrade::release_locks(locked_instances);
+      return;
+    });
 
-      size_t count = 1;
-      for (const auto &step : path) {
-        console->print_info(shcore::str_format(
-            "Step %zu of %zu: upgrading from %s to %s...", count++, total,
-            step->source_version.get_base().c_str(),
-            step->target_version.get_base().c_str()));
+    group_server->execute("SET FOREIGN_KEY_CHECKS=0");
 
-        // Inter step backup so upgrade can perform the required upgrade
-        // operations
-        backup(step->source_version, kMetadataSchemaName,
-               kMetadataSchemaPreviousName, group_server,
-               "creating step backup of the metadata");
+    size_t count = 1;
+    for (const auto &step : path) {
+      console->print_info(shcore::str_format(
+          "Step %zu of %zu: upgrading from %s to %s...", count++, total,
+          step->source_version.get_base().c_str(),
+          step->target_version.get_base().c_str()));
 
-        step->function(group_server);
+      // Inter step backup so upgrade can perform the required upgrade
+      // operations
+      backup(step->source_version, kMetadataSchemaName,
+             kMetadataSchemaPreviousName, group_server,
+             "creating step backup of the metadata");
 
-        // Removing temporary step backup
-        group_server->execute(std::string("DROP SCHEMA ") +
-                              kMetadataSchemaPreviousName);
-      }
+      step->function(group_server);
 
-      stage = upgrade::save_stage(group_server, upgrade::Stage::DONE);
+      // Removing temporary step backup
+      group_server->execute(std::string("DROP SCHEMA ") +
+                            kMetadataSchemaPreviousName);
+    }
 
-      upgrade::drop_metadata_backups(group_server, false);
+    stage = upgrade::save_stage(group_server, upgrade::Stage::DONE);
 
-      actual_version = set_schema_version(group_server, last_ver);
+    upgrade::drop_metadata_backups(group_server, false);
 
-      group_server->execute("SET FOREIGN_KEY_CHECKS=1");
+    actual_version = set_schema_version(group_server, last_ver);
 
-      upgrade::release_locks(locked_instances);
+    group_server->execute("SET FOREIGN_KEY_CHECKS=1");
 
-      installed_ver = installed_version(group_server);
-      console->print_info(
-          shcore::str_format("Upgrade process successfully finished, metadata "
-                             "schema is now on version %s",
-                             installed_ver.get_base().c_str()));
-    } catch (const std::exception &e) {
-      console->print_error(e.what());
-
-      // Only if the error happened after the upgrade started the data is
-      // restored and the backup deleted
-      upgrade::cleanup(
-          group_server,
-          upgrade::compute_failed_upgrade_stage(
-              stage, actual_version,
-              schema_exists(group_server, kMetadataSchemaBackupName)));
-
-      // Finally enables foreign key checks
-      group_server->execute("SET FOREIGN_KEY_CHECKS=1");
+    adjust_data(*group_server, actual_version);
 
-      upgrade::release_locks(locked_instances);
+    upgrade::release_locks(locked_instances);
 
-      // Get the version after the restore to notify the final version
-      installed_ver = installed_version(group_server);
+    installed_ver = installed_version(group_server);
+    console->print_info(
+        shcore::str_format("Upgrade process successfully finished, metadata "
+                           "schema is now on version %s",
+                           installed_ver.get_base().c_str()));
+  } catch (const std::exception &e) {
+    console->print_error(e.what());
 
-      if (stage == upgrade::Stage::UPGRADING) {
-        throw shcore::Exception::runtime_error(
-            "Upgrade process failed, metadata has been restored to version " +
-            installed_ver.get_base() + ".");
-      } else {
-        throw shcore::Exception::runtime_error(
-            "Upgrade process failed, metadata was not modified.");
-      }
+    // Only if the error happened after the upgrade started the data is
+    // restored and the backup deleted
+    upgrade::cleanup(
+        group_server,
+        upgrade::compute_failed_upgrade_stage(
+            stage, actual_version,
+            schema_exists(group_server, kMetadataSchemaBackupName)));
+
+    // Finally enables foreign key checks
+    group_server->execute("SET FOREIGN_KEY_CHECKS=1");
+
+    upgrade::release_locks(locked_instances);
+
+    // Get the version after the restore to notify the final version
+    installed_ver = installed_version(group_server);
+
+    if (stage == upgrade::Stage::UPGRADING) {
+      throw shcore::Exception::runtime_error(
+          "Upgrade process failed, metadata has been restored to version " +
+          installed_ver.get_base() + ".");
+    } else {
+      throw shcore::Exception::runtime_error(
+          "Upgrade process failed, metadata was not modified.");
     }
   }
 }
diff --git a/modules/adminapi/common/metadata_storage.cc b/modules/adminapi/common/metadata_storage.cc
@@ -2556,6 +2556,13 @@ Router_options_metadata MetadataStorage::get_routing_options(
       option_defaults = {k_default_clusterset_router_options.begin(),
                          k_default_clusterset_router_options.end()};
 
+      if (installed_version() < mysqlshdk::utils::Version(2, 2)) {
+        assert(option_defaults.find(k_router_option_stats_updates_frequency) !=
+               option_defaults.end());
+        option_defaults[k_router_option_stats_updates_frequency] =
+            shcore::Value(0);
+      }
+
       query =
           "SELECT concat(r.address, '::', r.router_name) AS router_label" +
           router_opt_select_items("r.options", k_clusterset_router_options) +
@@ -2887,7 +2894,15 @@ void MetadataStorage::set_global_routing_option(Cluster_type type,
   shcore::Value value_copy = value;
   std::string option_name = option;
 
-  if (value.type == shcore::Null) value_copy = option_defaults->at(option);
+  if (value.type == shcore::Null) {
+    if ((type == Cluster_type::REPLICATED_CLUSTER) &&
+        (option == k_router_option_stats_updates_frequency) &&
+        (installed_version() < mysqlshdk::utils::Version(2, 2))) {
+      value_copy = shcore::Value(0);
+    } else {
+      value_copy = option_defaults->at(option);
+    }
+  }
 
   if (type == Cluster_type::REPLICATED_CLUSTER) {
     if (value_copy.type == shcore::Null) {
diff --git a/modules/adminapi/common/router.cc b/modules/adminapi/common/router.cc
@@ -211,6 +211,22 @@ shcore::Dictionary_t router_options(MetadataStorage *md, Cluster_type type,
   auto router_options = shcore::make_dict();
   auto romd = md->get_routing_options(type, id);
 
+  if (Cluster_set_id cs_id;
+      (type == Cluster_type::GROUP_REPLICATION) &&
+      md->check_cluster_set(nullptr, nullptr, nullptr, &cs_id)) {
+    // if the Cluster belongs to a ClusterSet, we need to replace the options in
+    // the Cluster with the options in the ClusterSet
+    auto cs_romd =
+        md->get_routing_options(Cluster_type::REPLICATED_CLUSTER, cs_id);
+
+    for (const auto &[option, value] : cs_romd.global) {
+      if (option == "tags") continue;
+
+      auto it = romd.global.find(option);
+      if (it != romd.global.end()) it->second = value;
+    }
+  }
+
   const auto get_options_dict =
       [](const std::map<std::string, shcore::Value> &entry) {
         auto ret = shcore::make_dict();
diff --git a/modules/adminapi/common/router.h b/modules/adminapi/common/router.h
@@ -26,6 +26,7 @@
 
 #include <array>
 #include <string>
+#include <string_view>
 #include <utility>
 
 #include "modules/adminapi/common/base_cluster_impl.h"
@@ -71,10 +72,9 @@ inline constexpr std::array<decltype(k_router_option_target_cluster), 6>
 extern const std::map<std::string, shcore::Value>
     k_default_clusterset_router_options;
 
-inline constexpr std::array<decltype(k_router_option_target_cluster), 3>
-    k_cluster_router_options = {k_router_option_tags,
-                                k_router_option_read_only_targets,
-                                k_router_option_stats_updates_frequency};
+inline constexpr std::array<std::string_view, 3> k_cluster_router_options = {
+    k_router_option_tags, k_router_option_read_only_targets,
+    k_router_option_stats_updates_frequency};
 
 extern const std::map<std::string, shcore::Value>
     k_default_cluster_router_options;
diff --git a/unittest/scripts/auto/js_adminapi_clusterset/scripts/cs_router_management.js b/unittest/scripts/auto/js_adminapi_clusterset/scripts/cs_router_management.js
diff --git a/unittest/scripts/auto/js_adminapi_clusterset/scripts/metadata_schema_utils.inc b/unittest/scripts/auto/js_adminapi_clusterset/scripts/metadata_schema_utils.inc
