BUG#26870329: CLUSTER.REJOININSTANCE() SHOULD ENSURE MEMBER IS NOT ALREADY IN GROUP

The rejoinInstance operation will attempt to rejoin an instance even
if the member being rejoined is already part of the group.

This patch adds a new check that validates if the instance being added
is missing from the group, in which case it is rejoined. Otherwise a
runtime error is thrown.
Interactive documentation was updated.

New tests were added and existing tests were updated.

Author: Nelson Goncalves

modules/adminapi/mod_dba_cluster.cc

@@ -411,6 +411,9 @@ REGISTER_HELP(CLUSTER_REJOININSTANCE_THROWS5, "@throws ArgumentError if the "\
 REGISTER_HELP(CLUSTER_REJOININSTANCE_THROWS6, "@throws RuntimeError if the SSL "\
                                               "mode specified is not compatible "\
                                               "with the one used in the cluster.");
+REGISTER_HELP(CLUSTER_REJOININSTANCE_THROWS7, "@throws RuntimeError if the "\
+                                              "instance is an active member "\
+                                              "of the ReplicaSet.");
 
 REGISTER_HELP(CLUSTER_REJOININSTANCE_RETURNS, "@returns nothing.");
 
@@ -493,6 +496,7 @@ REGISTER_HELP(CLUSTER_REJOININSTANCE_DETAIL25, "The ipWhitelist format is a "\
 * $(CLUSTER_REJOININSTANCE_THROWS4)
 * $(CLUSTER_REJOININSTANCE_THROWS5)
 * $(CLUSTER_REJOININSTANCE_THROWS6)
+* $(CLUSTER_REJOININSTANCE_THROWS7)
 *
 * $(CLUSTER_REJOININSTANCE_RETURNS)
 *

modules/adminapi/mod_dba_common.cc

@@ -1075,7 +1075,7 @@ std::string get_gr_replicaset_group_name(
  *
  * @param metadata metadata object which represents the session to the metadata
  *                 storage, i.e. the current instance session on this case
- * @param rd_id the ReplicaSet id
+ * @param rs_id the ReplicaSet id
  * @return a boolean value indicating whether the replicaSet
  * 'group_replication_group_name' is the same as the one registered in the
  * corresponding replicaset in the Metadata
@@ -1158,5 +1158,39 @@ bool validate_super_read_only(
   return super_read_only;
 }
 
+/*
+ * Checks if an instance can be rejoined to a given ReplicaSet.
+ *
+ * @param session Object which represents the session to the instance
+ * @param metadata Metadata object which represents the session to the metadata
+ * storage
+ * @param rs_id The ReplicaSet id
+ *
+ * @return a boolean value which is true if the instance can be rejoined to the
+ * ReplicaSet and false otherwise.
+ */
+bool validate_instance_rejoinable(
+    mysqlsh::mysql::ClassicSession *instance_session,
+    const std::shared_ptr<MetadataStorage> &metadata,
+    uint64_t rs_id) {
+  std::string instance_uuid;
+  // get server_uuid from the instance that we're trying to rejoin
+  get_server_variable(instance_session->connection(), "server_uuid",
+                      instance_uuid);
+  // get the list of missing instances
+  std::vector<MissingInstanceInfo> missing_instances_list =
+      get_unavailable_instances(metadata, rs_id);
+
+  // Unless the instance is missing, we cannot add it back to the cluster
+  auto it =
+      std::find_if(missing_instances_list.begin(), missing_instances_list.end(),
+                   [&instance_uuid](MissingInstanceInfo &info) {
+                     return instance_uuid == info.id;
+                   });
+  // if the instance is not on the list of missing instances, then it cannot
+  // be rejoined
+  return it != std::end(missing_instances_list);
+}
+
 }  // namespace dba
 }  // namespace mysqlsh

modules/adminapi/mod_dba_common.h

@@ -17,12 +17,13 @@
  * 02110-1301  USA
  */
 
-#ifndef _MODULES_ADMINAPI_MOD_DBA_COMMON_
-#define _MODULES_ADMINAPI_MOD_DBA_COMMON_
+#ifndef MODULES_ADMINAPI_MOD_DBA_COMMON_H_
+#define MODULES_ADMINAPI_MOD_DBA_COMMON_H_
 
 #include <locale>
 #include <string>
 #include <vector>
+#include <memory>
 
 #include "modules/adminapi/mod_dba_provisioning_interface.h"
 #include "modules/mod_mysql_session.h"
@@ -165,6 +166,9 @@ bool SHCORE_PUBLIC validate_replicaset_group_name(
     mysqlsh::mysql::ClassicSession *session, uint64_t rs_id);
 bool validate_super_read_only(
     mysqlsh::mysql::ClassicSession *session, bool clear_read_only);
-}
-}
-#endif
+bool validate_instance_rejoinable(
+    mysqlsh::mysql::ClassicSession *instance_session,
+    const std::shared_ptr<MetadataStorage> &metadata, uint64_t rs_id);
+}  // namespace dba
+}  // namespace mysqlsh
+#endif  // MODULES_ADMINAPI_MOD_DBA_COMMON_H_

modules/adminapi/mod_dba_replicaset.cc

@@ -725,6 +725,33 @@ shcore::Value ReplicaSet::rejoin_instance(const shcore::Argument_list &args) {
     }
   }
 
+  // Verify if the instance being added is MISSING, otherwise throw an error
+  // Bug#26870329
+  {
+    // get server_uuid from the instance that we're trying to rejoin
+    classic = dynamic_cast<mysqlsh::mysql::ClassicSession*>(session.get());
+    if (!validate_instance_rejoinable(classic, _metadata_storage, _id)) {
+      // instance not missing, so throw an error
+      std::string get_state_query =
+          "SELECT MEMBER_STATE FROM "
+          "performance_schema.replication_group_members as m JOIN "
+          "performance_schema.replication_group_member_stats as s "
+          "on m.MEMBER_ID = s.MEMBER_ID "
+          "AND m.MEMBER_ID = @@server_uuid";
+      auto result = classic->execute_sql(get_state_query);
+      auto row = result->fetch_one();
+      std::string member_state = row->get_value(0).as_string();
+      std::string nice_error_msg = "Cannot rejoin instance '" +
+                                   instance_address + "' with state '" +
+                                   member_state +
+                                   "' since it "
+                                   "is an active member of the ReplicaSet '" +
+                                   get_member("name").as_string() + "'.";
+      session->close(shcore::Argument_list());
+      throw std::runtime_error(nice_error_msg);
+    }
+  }
+
   // Get @@group_replication_local_address
   std::string seed_instance_xcom_address;
   get_server_variable(classic->connection(), "group_replication_local_address",

unittest/modules/adminapi/mod_dba_common_t.cc

@@ -1038,7 +1038,7 @@ TEST_F(Dba_common_test, get_unavailable_instances_001) {
     EXPECT_TRUE(unavailable_instances_list.empty());
   } catch (const shcore::Exception &e) {
     SCOPED_TRACE(e.what());
-    SCOPED_TRACE("Unexpected failure at get_instances_md");
+    SCOPED_TRACE("Unexpected failure at get_unavailable_instances_001");
     ADD_FAILURE();
   }
 
@@ -1109,7 +1109,7 @@ TEST_F(Dba_common_test, get_unavailable_instances_002) {
     EXPECT_TRUE(unavailable_instances_list.empty());
   } catch (const shcore::Exception &e) {
     SCOPED_TRACE(e.what());
-    SCOPED_TRACE("Unexpected failure at get_instances_md");
+    SCOPED_TRACE("Unexpected failure at get_unavailable_instances_002");
     ADD_FAILURE();
   }
 
@@ -1245,6 +1245,205 @@ TEST_F(Dba_common_test, validate_replicaset_group_name_002) {
   stop_server_mock(_mysql_sandbox_nport1);
   stop_server_mock(_mysql_sandbox_nport2);
 }
+
+TEST_F(Dba_common_test, validate_instance_rejoinable_01) {
+  // There are missing instances and the instance we are checking belongs to
+  // that list
+
+  // get_instances_gr():
+  //
+  // member_id
+  // ------------------------------------
+  // 851f0e89-5730-11e7-9e4f-b86b230042b9
+  // 8a8ae9ce-5730-11e7-a437-b86b230042b9
+
+  std::vector<tests::Fake_result_data> queries;
+
+  std::vector<std::vector<std::string>> values;
+  values = {{"8fcb92c9-5730-11e7-aa60-b86b230042b9"},
+            {"851f0e89-5730-11e7-9e4f-b86b230042b9"}};
+
+  add_ps_gr_group_members_query(&queries, values);
+
+  // get_instances_md():
+  //
+  // member_id
+  // ------------------------------------
+  // 851f0e89-5730-11e7-9e4f-b86b230042b9
+  // 8a8ae9ce-5730-11e7-a437-b86b230042b9
+  // 8fcb92c9-5730-11e7-aa60-b86b230042b9
+
+  values = {{"851f0e89-5730-11e7-9e4f-b86b230042b9"},
+            {"8a8ae9ce-5730-11e7-a437-b86b230042b9"},
+            {"8fcb92c9-5730-11e7-aa60-b86b230042b9"}};
+
+  add_md_group_members_query(&queries, values);
+
+  values = {{"8a8ae9ce-5730-11e7-a437-b86b230042b9", "localhost:3320",
+             "localhost:3320"}};
+
+  add_md_group_members_full_query(
+      &queries, "8a8ae9ce-5730-11e7-a437-b86b230042b9", values);
+
+  START_SERVER_MOCK(_mysql_sandbox_nport1, queries);
+
+  auto md_session = create_dev_session(_mysql_sandbox_nport1);
+
+  std::shared_ptr<mysqlsh::dba::MetadataStorage> metadata;
+  metadata.reset(new mysqlsh::dba::MetadataStorage(md_session));
+
+  std::vector<tests::Fake_result_data> instance_queries;
+  add_get_server_variable_query(&instance_queries, "server_uuid",
+                                tests::Type::String,
+                                "8a8ae9ce-5730-11e7-a437-b86b230042b9");
+  START_SERVER_MOCK(_mysql_sandbox_nport2, instance_queries);
+  auto instance_session = create_session(_mysql_sandbox_nport2);
+  try {
+    bool is_rejoinable(
+        validate_instance_rejoinable(instance_session.get(), metadata, 1));
+
+    EXPECT_TRUE(is_rejoinable);
+  } catch (const shcore::Exception &e) {
+    SCOPED_TRACE(e.what());
+    SCOPED_TRACE("Unexpected failure at validate_instance_rejoinable_01");
+    ADD_FAILURE();
+  }
+
+  md_session->close(shcore::Argument_list());
+  stop_server_mock(_mysql_sandbox_nport1);
+  instance_session->close(shcore::Argument_list());
+  stop_server_mock(_mysql_sandbox_nport2);
+}
+
+TEST_F(Dba_common_test, validate_instance_rejoinable_02) {
+  // There are missing instances but the instance we are checking does not
+  // belong to that list.
+
+  // get_instances_gr():
+  //
+  // member_id
+  // ------------------------------------
+  // 851f0e89-5730-11e7-9e4f-b86b230042b9
+  // 8a8ae9ce-5730-11e7-a437-b86b230042b9
+
+  std::vector<tests::Fake_result_data> queries;
+
+  std::vector<std::vector<std::string>> values;
+  values = {{"8fcb92c9-5730-11e7-aa60-b86b230042b9"},
+            {"851f0e89-5730-11e7-9e4f-b86b230042b9"}};
+
+  add_ps_gr_group_members_query(&queries, values);
+
+  // get_instances_md():
+  //
+  // member_id
+  // ------------------------------------
+  // 851f0e89-5730-11e7-9e4f-b86b230042b9
+  // 8a8ae9ce-5730-11e7-a437-b86b230042b9
+  // 8fcb92c9-5730-11e7-aa60-b86b230042b9
+
+  values = {{"851f0e89-5730-11e7-9e4f-b86b230042b9"},
+            {"8a8ae9ce-5730-11e7-a437-b86b230042b9"},
+            {"8fcb92c9-5730-11e7-aa60-b86b230042b9"}};
+
+  add_md_group_members_query(&queries, values);
+
+  values = {{"8a8ae9ce-5730-11e7-a437-b86b230042b9", "localhost:3320",
+             "localhost:3320"}};
+
+  add_md_group_members_full_query(
+      &queries, "8a8ae9ce-5730-11e7-a437-b86b230042b9", values);
+
+  START_SERVER_MOCK(_mysql_sandbox_nport1, queries);
+
+  auto md_session = create_dev_session(_mysql_sandbox_nport1);
+
+  std::shared_ptr<mysqlsh::dba::MetadataStorage> metadata;
+  metadata.reset(new mysqlsh::dba::MetadataStorage(md_session));
+
+  std::vector<tests::Fake_result_data> instance_queries;
+  add_get_server_variable_query(&instance_queries, "server_uuid",
+                                tests::Type::String,
+                                "8a8ae9ce-5730-11e7-a437-b86b230042b1");
+  START_SERVER_MOCK(_mysql_sandbox_nport2, instance_queries);
+  auto instance_session = create_session(_mysql_sandbox_nport2);
+  try {
+    bool is_rejoinable(
+        validate_instance_rejoinable(instance_session.get(), metadata, 1));
+
+    EXPECT_FALSE(is_rejoinable);
+  } catch (const shcore::Exception &e) {
+    SCOPED_TRACE(e.what());
+    SCOPED_TRACE("Unexpected failure at validate_instance_rejoinable_02");
+    ADD_FAILURE();
+  }
+
+  md_session->close(shcore::Argument_list());
+  stop_server_mock(_mysql_sandbox_nport1);
+  instance_session->close(shcore::Argument_list());
+  stop_server_mock(_mysql_sandbox_nport2);
+}
+
+TEST_F(Dba_common_test, validate_instance_rejoinable_03) {
+  // There are no missing instances
+
+  // get_instances_gr():
+  //
+  // member_id
+  // ------------------------------------
+  // 851f0e89-5730-11e7-9e4f-b86b230042b9
+  // 8a8ae9ce-5730-11e7-a437-b86b230042b9
+  // 8fcb92c9-5730-11e7-aa60-b86b230042b9
+
+  std::vector<tests::Fake_result_data> queries;
+
+  std::vector<std::vector<std::string>> values;
+  values = {{"851f0e89-5730-11e7-9e4f-b86b230042b9"},
+            {"8a8ae9ce-5730-11e7-a437-b86b230042b9"},
+            {"8fcb92c9-5730-11e7-aa60-b86b230042b9"}};
+
+  add_ps_gr_group_members_query(&queries, values);
+
+  // get_instances_md():
+  //
+  // member_id
+  // ------------------------------------
+  // 851f0e89-5730-11e7-9e4f-b86b230042b9
+  // 8a8ae9ce-5730-11e7-a437-b86b230042b9
+  // 8fcb92c9-5730-11e7-aa60-b86b230042b9
+
+  add_md_group_members_query(&queries, values);
+
+  START_SERVER_MOCK(_mysql_sandbox_nport1, queries);
+
+  auto md_session = create_dev_session(_mysql_sandbox_nport1);
+
+  std::shared_ptr<mysqlsh::dba::MetadataStorage> metadata;
+  metadata.reset(new mysqlsh::dba::MetadataStorage(md_session));
+
+  std::vector<tests::Fake_result_data> instance_queries;
+  add_get_server_variable_query(&instance_queries, "server_uuid",
+                                tests::Type::String,
+                                "8a8ae9ce-5730-11e7-a437-b86b230042b9");
+  START_SERVER_MOCK(_mysql_sandbox_nport2, instance_queries);
+  auto instance_session = create_session(_mysql_sandbox_nport2);
+  try {
+    bool is_rejoinable(
+        validate_instance_rejoinable(instance_session.get(), metadata, 1));
+
+    EXPECT_FALSE(is_rejoinable);
+  } catch (const shcore::Exception &e) {
+    SCOPED_TRACE(e.what());
+    SCOPED_TRACE("Unexpected failure at validate_instance_rejoinable_03");
+    ADD_FAILURE();
+  }
+
+  md_session->close(shcore::Argument_list());
+  stop_server_mock(_mysql_sandbox_nport1);
+  instance_session->close(shcore::Argument_list());
+  stop_server_mock(_mysql_sandbox_nport2);
+}
+
 }  // namespace tests
 
 TEST(mod_dba_common, validate_label) {

unittest/scripts/js_devapi/scripts/dba_cluster_rejoin_instance.js

@@ -75,7 +75,8 @@ wait_slave_state(cluster, uri2, "ONLINE");
 //@<OUT> Cluster status after rejoin
 cluster.status();
 
-// Dissolve the cluser and remove the created accounts
+//@ Cannot rejoin an instance that is already in the group (not missing) Bug#26870329
+cluster.rejoinInstance({dbUser: 'foo', host: 'localhost', port:__mysql_sandbox_port2}, {password: 'bar'});
 
 //@ Dissolve cluster
 cluster.dissolve({force: true})

unittest/scripts/js_devapi/validation/dba_cluster_help.js

@@ -386,6 +386,7 @@ EXCEPTIONS
                  allowed: "AUTO", "DISABLED", "REQUIRED".
   RuntimeError: if the SSL mode specified is not compatible with the one used
                 in the cluster.
+  RuntimeError: if the instance is an active member of the ReplicaSet.
 
 RETURNS
 

unittest/scripts/js_devapi/validation/dba_cluster_rejoin_instance.js

@@ -84,6 +84,9 @@
     }
 }
 
+//@ Cannot rejoin an instance that is already in the group (not missing) Bug#26870329
+||Cluster.rejoinInstance: Cannot rejoin instance '<<<localhost>>>:<<<__mysql_sandbox_port2>>>' with state 'ONLINE' since it is an active member of the ReplicaSet 'default'.
+
 //@ Dissolve cluster
 ||
 

unittest/scripts/py_devapi/validation/dba_cluster_help.py

@@ -388,6 +388,7 @@
                  allowed: "AUTO", "DISABLED", "REQUIRED".
   RuntimeError: if the SSL mode specified is not compatible with the one used
                 in the cluster.
+  RuntimeError: if the instance is an active member of the ReplicaSet.
 
 RETURNS