From 8d6932abb2a16792a1627ac96dc6a1b685a4813d Mon Sep 17 00:00:00 2001 From: Timofey Turenko Date: Wed, 27 Feb 2019 23:57:13 +0200 Subject: [PATCH 1/2] fix compiler warnings in tests --- maxscale-system-test/mariadb_nodes.cpp | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/maxscale-system-test/mariadb_nodes.cpp b/maxscale-system-test/mariadb_nodes.cpp index 5ace71070..e49c5e8d0 100644 --- a/maxscale-system-test/mariadb_nodes.cpp +++ b/maxscale-system-test/mariadb_nodes.cpp @@ -391,12 +391,10 @@ int Mariadb_nodes::cleanup_db_nodes() void Mariadb_nodes::create_users(int node) { - char str[1024]; - char dtr[1024]; + char str[strlen(test_dir) + 17]; // Create users for replication as well as the users that are used by the tests sprintf(str, "%s/create_user.sh", test_dir); - sprintf(dtr, "%s", access_homedir[node]); - copy_to_node(node, str, dtr); + copy_to_node(node, str, access_homedir[node]); ssh_node_f(node, false, "export node_user=\"%s\"; export node_password=\"%s\"; %s/create_user.sh %s", user_name, password, access_homedir[0], socket_cmd[0]); @@ -502,7 +500,7 @@ int Galera_nodes::start_galera() } } - char str[1024]; + char str[strlen(test_dir) + 25]; sprintf(str, "%s/create_user_galera.sh", test_dir); copy_to_node_legacy(str, "~/", 0); @@ -1158,7 +1156,7 @@ int Mariadb_nodes::truncate_mariadb_logs() int Mariadb_nodes::configure_ssl(bool require) { int local_result = 0; - char str[1024]; + char str[strlen(test_dir) + 20]; this->ssl = 1; From 4fd4b726a19f327201c49c06d3e69599cd5bff8b Mon Sep 17 00:00:00 2001 From: Esa Korhonen Date: Wed, 27 Feb 2019 13:24:37 +0200 Subject: [PATCH 2/2] MXS-2325 Only enable events that were enabled on the master The monitor now continuously updates a list of enabled server events. When promoting a new master in failover/switchover, only events that were enabled on the previous master are enabled on the new. This avoids enabling events that may have been disabled on the master yet stayed in the SLAVESIDE_DISABLED- state on the slave. In the case of reset-replication command, events on the new master are only enabled if the monitor had a master when the command was launched. Otherwise all events remain disabled. --- .../mariadbmon/cluster_manipulation.cc | 32 ++-- .../modules/monitor/mariadbmon/mariadbmon.cc | 3 +- .../monitor/mariadbmon/mariadbserver.cc | 146 ++++++++++-------- .../monitor/mariadbmon/mariadbserver.hh | 26 +++- .../monitor/mariadbmon/server_utils.cc | 13 +- .../monitor/mariadbmon/server_utils.hh | 13 +- .../mariadbmon/test/test_cycle_find.cc | 2 +- 7 files changed, 147 insertions(+), 88 deletions(-) diff --git a/server/modules/monitor/mariadbmon/cluster_manipulation.cc b/server/modules/monitor/mariadbmon/cluster_manipulation.cc index 93a033838..b0567ab88 100644 --- a/server/modules/monitor/mariadbmon/cluster_manipulation.cc +++ b/server/modules/monitor/mariadbmon/cluster_manipulation.cc @@ -234,6 +234,9 @@ bool MariaDBMonitor::manual_reset_replication(SERVER* master_server, json_t** er } } + // Also record the previous master, needed for scheduled events. + MariaDBServer* old_master = (m_master && m_master->is_master()) ? m_master : nullptr; + bool rval = false; if (new_master) { @@ -333,11 +336,19 @@ bool MariaDBMonitor::manual_reset_replication(SERVER* master_server, json_t** er if (m_handle_event_scheduler) { - if (!new_master->enable_events(error_out)) + if (old_master) { - error = true; - PRINT_MXS_JSON_ERROR(error_out, "Could not enable events on '%s': %s", - new_master->name(), error_msg.c_str()); + if (!new_master->enable_events(old_master->m_enabled_events, error_out)) + { + error = true; + PRINT_MXS_JSON_ERROR(error_out, "Could not enable events on '%s': %s", + new_master->name(), error_msg.c_str()); + } + } + else + { + MXS_WARNING("No scheduled events were enabled on '%s' because previous master is " + "unknown. Check events manually.", new_master->name()); } } @@ -625,8 +636,8 @@ uint32_t MariaDBMonitor::do_rejoin(const ServerArray& joinable_servers, json_t** { // Assume that server is an old master which was failed over. Even if this is not really // the case, the following is unlikely to do damage. - ServerOperation demotion(joinable, true, /* treat as old master */ - m_handle_event_scheduler, m_demote_sql_file, {} /* unused */); + ServerOperation demotion(joinable, true, /* treat as old master */ + m_handle_event_scheduler, m_demote_sql_file); if (joinable->demote(general, demotion)) { MXS_NOTICE("Directing standalone server '%s' to replicate from '%s'.", name, master_name); @@ -1397,8 +1408,8 @@ unique_ptr MariaDBMonitor::failover_prepare(Log auto time_limit = maxbase::Duration((double)m_failover_timeout); bool promoting_to_master = (demotion_target == m_master); ServerOperation promotion(promotion_target, promoting_to_master, - m_handle_event_scheduler, m_promote_sql_file, - demotion_target->m_slave_status); + m_handle_event_scheduler, m_promote_sql_file, + demotion_target->m_slave_status, demotion_target->m_enabled_events); GeneralOpData general(m_replication_user, m_replication_password, error_out, time_limit); rval.reset(new FailoverParams(promotion, demotion_target, general)); } @@ -1687,9 +1698,10 @@ MariaDBMonitor::switchover_prepare(SERVER* promotion_server, SERVER* demotion_se maxbase::Duration time_limit((double)m_switchover_timeout); bool master_swap = (demotion_target == m_master); ServerOperation promotion(promotion_target, master_swap, m_handle_event_scheduler, - m_promote_sql_file, demotion_target->m_slave_status); + m_promote_sql_file, + demotion_target->m_slave_status, demotion_target->m_enabled_events); ServerOperation demotion(demotion_target, master_swap, m_handle_event_scheduler, - m_demote_sql_file, promotion_target->m_slave_status); + m_demote_sql_file, promotion_target->m_slave_status, {} /* unused */); GeneralOpData general(m_replication_user, m_replication_password, error_out, time_limit); rval.reset(new SwitchoverParams(promotion, demotion, general)); } diff --git a/server/modules/monitor/mariadbmon/mariadbmon.cc b/server/modules/monitor/mariadbmon/mariadbmon.cc index ee190bedf..06bdc24e1 100644 --- a/server/modules/monitor/mariadbmon/mariadbmon.cc +++ b/server/modules/monitor/mariadbmon/mariadbmon.cc @@ -79,7 +79,8 @@ void MariaDBMonitor::reset_server_info() // Next, initialize the data. for (auto mon_server = m_monitor->monitored_servers; mon_server; mon_server = mon_server->next) { - m_servers.push_back(new MariaDBServer(mon_server, m_servers.size(), m_assume_unique_hostnames)); + m_servers.push_back(new MariaDBServer(mon_server, m_servers.size(), + m_assume_unique_hostnames, m_handle_event_scheduler)); } } diff --git a/server/modules/monitor/mariadbmon/mariadbserver.cc b/server/modules/monitor/mariadbmon/mariadbserver.cc index ca38b834b..e946250a8 100644 --- a/server/modules/monitor/mariadbmon/mariadbserver.cc +++ b/server/modules/monitor/mariadbmon/mariadbserver.cc @@ -26,20 +26,12 @@ using maxscale::string_printf; using maxbase::Duration; using maxbase::StopWatch; -class MariaDBServer::EventInfo -{ -public: - std::string database; - std::string name; - std::string definer; - std::string status; -}; - MariaDBServer::MariaDBServer(MXS_MONITORED_SERVER* monitored_server, int config_index, - bool assume_unique_hostnames) + bool assume_unique_hostnames, bool query_events) : m_server_base(monitored_server) , m_config_index(config_index) , m_assume_unique_hostnames(assume_unique_hostnames) + , m_query_events(query_events) { mxb_assert(monitored_server); } @@ -839,6 +831,10 @@ void MariaDBServer::monitor_server() { query_ok = update_gtids(&errmsg); } + if (query_ok && m_query_events) + { + query_ok = update_enabled_events(); + } } else { @@ -1222,22 +1218,24 @@ const SlaveStatus* MariaDBServer::slave_connection_status_host_port(const MariaD return NULL; } -bool MariaDBServer::enable_events(json_t** error_out) +bool MariaDBServer::enable_events(const EventNameSet& event_names, json_t** error_out) { int found_disabled_events = 0; int events_enabled = 0; - // Helper function which enables a slaveside disabled event. - ManipulatorFunc enabler = [this, &found_disabled_events, &events_enabled](const EventInfo& event, - json_t** error_out) { - if (event.status == "SLAVESIDE_DISABLED") + + // Helper function which enables a disabled event if that event name is found in the events-set. + ManipulatorFunc enabler = [this, event_names, &found_disabled_events, &events_enabled]( + const EventInfo& event, json_t** error_out) { + if (event_names.count(event.name) > 0 + && (event.status == "SLAVESIDE_DISABLED" || event.status == "DISABLED")) + { + found_disabled_events++; + if (alter_event(event, "ENABLE", error_out)) { - found_disabled_events++; - if (alter_event(event, "ENABLE", error_out)) - { - events_enabled++; - } + events_enabled++; } - }; + } + }; bool rval = false; if (events_foreach(enabler, error_out)) @@ -1361,8 +1359,7 @@ bool MariaDBServer::events_foreach(ManipulatorFunc& func, json_t** error_out) while (event_info->next_row()) { EventInfo event; - event.database = event_info->get_string(db_name_ind); - event.name = event_info->get_string(event_name_ind); + event.name = event_info->get_string(db_name_ind) + "." + event_info->get_string(event_name_ind); event.definer = event_info->get_string(event_definer_ind); event.status = event_info->get_string(event_status_ind); func(event, error_out); @@ -1382,51 +1379,40 @@ bool MariaDBServer::alter_event(const EventInfo& event, const string& target_sta { bool rval = false; string error_msg; - // First switch to the correct database. - string use_db_query = string_printf("USE %s;", event.database.c_str()); - if (execute_cmd(use_db_query, &error_msg)) + // An ALTER EVENT by default changes the definer (owner) of the event to the monitor user. + // This causes problems if the monitor user does not have privileges to run + // the event contents. Prevent this by setting definer explicitly. + // The definer may be of the form user@host. If host includes %, then it must be quoted. + // For simplicity, quote the host always. + string quoted_definer; + auto loc_at = event.definer.find('@'); + if (loc_at != string::npos) { - // An ALTER EVENT by default changes the definer (owner) of the event to the monitor user. - // This causes problems if the monitor user does not have privileges to run - // the event contents. Prevent this by setting definer explicitly. - // The definer may be of the form user@host. If host includes %, then it must be quoted. - // For simplicity, quote the host always. - string quoted_definer; - auto loc_at = event.definer.find('@'); - if (loc_at != string::npos) - { - auto host_begin = loc_at + 1; - quoted_definer = event.definer.substr(0, loc_at + 1) - + // host_begin may be the null-char if @ was the last char - "'" + event.definer.substr(host_begin, string::npos) + "'"; - } - else - { - // Just the username - quoted_definer = event.definer; - } - string alter_event_query = string_printf("ALTER DEFINER = %s EVENT %s %s;", - quoted_definer.c_str(), - event.name.c_str(), - target_status.c_str()); - if (execute_cmd(alter_event_query, &error_msg)) - { - rval = true; - const char FMT[] = "Event '%s' of database '%s' on server '%s' set to '%s'."; - MXS_NOTICE(FMT, event.name.c_str(), event.database.c_str(), name(), target_status.c_str()); - } - else - { - const char FMT[] = "Could not alter event '%s' of database '%s' on server '%s': %s"; - PRINT_MXS_JSON_ERROR(error_out, FMT, event.name.c_str(), event.database.c_str(), name(), - error_msg.c_str()); - } + auto host_begin = loc_at + 1; + quoted_definer = event.definer.substr(0, loc_at + 1) + + // host_begin may be the null-char if @ was the last char + "'" + event.definer.substr(host_begin, string::npos) + "'"; } else { - const char FMT[] = "Could not switch to database '%s' on '%s': %s Event '%s' not altered."; - PRINT_MXS_JSON_ERROR(error_out, FMT, event.database.c_str(), name(), error_msg.c_str(), - event.name.c_str()); + // Just the username + quoted_definer = event.definer; + } + + string alter_event_query = string_printf("ALTER DEFINER = %s EVENT %s %s;", + quoted_definer.c_str(), + event.name.c_str(), + target_status.c_str()); + if (execute_cmd(alter_event_query, &error_msg)) + { + rval = true; + const char FMT[] = "Event '%s' on server '%s' set to '%s'."; + MXS_NOTICE(FMT, event.name.c_str(), name(), target_status.c_str()); + } + else + { + const char FMT[] = "Could not alter event '%s' on server '%s': %s"; + PRINT_MXS_JSON_ERROR(error_out, FMT, event.name.c_str(), name(), error_msg.c_str()); } return rval; } @@ -1510,7 +1496,7 @@ bool MariaDBServer::promote(GeneralOpData& general, ServerOperation& promotion, if (promotion.handle_events) { // TODO: Add query replying to enable_events - bool events_enabled = enable_events(error_out); + bool events_enabled = enable_events(promotion.events_to_enable, error_out); general.time_remaining -= timer.restart(); if (!events_enabled) { @@ -2119,3 +2105,33 @@ bool MariaDBServer::redirect_existing_slave_conn(GeneralOpData& op, const SlaveS } // 'stop_slave_conn' prints its own errors return success; } + +bool MariaDBServer::update_enabled_events() +{ + string error_msg; + // Get names of all enabled scheduled events on the server. + auto event_info = execute_query("SELECT Event_schema, Event_name FROM information_schema.EVENTS WHERE " + "Status = 'ENABLED';", &error_msg); + if (event_info.get() == NULL) + { + MXS_ERROR("Could not query events of '%s': %s Event handling can be disabled by " + "setting '%s' to false.", + name(), error_msg.c_str(), CN_HANDLE_EVENTS); + return false; + } + + auto db_name_ind = 0; + auto event_name_ind = 1; + + EventNameSet full_names; + full_names.reserve(event_info->get_row_count()); + + while (event_info->next_row()) + { + string full_name = event_info->get_string(db_name_ind) + "." + event_info->get_string(event_name_ind); + full_names.insert(full_name); // Ignore duplicates, they shouldn't exists. + } + + m_enabled_events = std::move(full_names); + return true; +} \ No newline at end of file diff --git a/server/modules/monitor/mariadbmon/mariadbserver.hh b/server/modules/monitor/mariadbmon/mariadbserver.hh index 374d72f76..ff78ec810 100644 --- a/server/modules/monitor/mariadbmon/mariadbserver.hh +++ b/server/modules/monitor/mariadbmon/mariadbserver.hh @@ -72,6 +72,17 @@ struct NodeData class MariaDBServer { public: + MariaDBServer(MXS_MONITORED_SERVER* monitored_server, int config_index, + bool assume_unique_hostnames, bool query_events); + + class EventInfo + { + public: + std::string name; /**< Event name in form */ + std::string definer; /**< Definer of the event */ + std::string status; /**< Status of the event */ + }; + enum class server_type { UNKNOWN, /* Totally unknown. Server has not been connected to yet. */ @@ -135,10 +146,10 @@ public: * 'update_replication_settings' before use. */ ReplicationSettings m_rpl_settings; - bool m_print_update_errormsg = true; /* Should an update error be printed? */ + bool m_query_events; /* Copy of monitor->m_handle_event_scheduler. TODO: move elsewhere */ + EventNameSet m_enabled_events; /* Enabled scheduled events */ - MariaDBServer(MXS_MONITORED_SERVER* monitored_server, int config_index, - bool assume_unique_hostnames = true); + bool m_print_update_errormsg = true; /* Should an update error be printed? */ /** * Print server information to a json object. @@ -329,12 +340,14 @@ public: bool run_sql_from_file(const std::string& path, json_t** error_out); /** - * Enable any "SLAVESIDE_DISABLED" events. Event scheduler is not touched. + * Enable any "SLAVESIDE_DISABLED" or "DISABLED events. Event scheduler is not touched. Only events + * with names matching an element in the event_names set are enabled. * + * @param event_names Names of events that should be enabled * @param error_out Error output * @return True if all SLAVESIDE_DISABLED events were enabled */ - bool enable_events(json_t** error_out); + bool enable_events(const EventNameSet& event_names, json_t** error_out); /** * Disable any "ENABLED" events. Event scheduler is not touched. @@ -516,7 +529,6 @@ public: void set_status(uint64_t bits); private: - class EventInfo; typedef std::function ManipulatorFunc; enum class StopMode @@ -558,4 +570,6 @@ private: bool set_read_only(ReadOnlySetting value, maxbase::Duration time_limit, json_t** error_out); bool merge_slave_conns(GeneralOpData& op, const SlaveStatusArray& conns_to_merge); std::string generate_change_master_cmd(GeneralOpData& op, const SlaveStatus& slave_conn); + + bool update_enabled_events(); }; diff --git a/server/modules/monitor/mariadbmon/server_utils.cc b/server/modules/monitor/mariadbmon/server_utils.cc index b3422cf47..072dcb2d1 100644 --- a/server/modules/monitor/mariadbmon/server_utils.cc +++ b/server/modules/monitor/mariadbmon/server_utils.cc @@ -155,14 +155,21 @@ bool SlaveStatus::should_be_copied(string* ignore_reason_out) const return accepted; } -ServerOperation::ServerOperation(MariaDBServer* target, bool was_is_master, - bool handle_events, const std::string& sql_file, - const SlaveStatusArray& conns_to_copy) +ServerOperation::ServerOperation(MariaDBServer* target, bool was_is_master, bool handle_events, + const std::string& sql_file, const SlaveStatusArray& conns_to_copy, + const EventNameSet& events_to_enable) : target(target) , to_from_master(was_is_master) , handle_events(handle_events) , sql_file(sql_file) , conns_to_copy(conns_to_copy) + , events_to_enable(events_to_enable) +{ +} + +ServerOperation::ServerOperation(MariaDBServer* target, bool was_is_master, bool handle_events, + const std::string& sql_file) + : ServerOperation(target, was_is_master, handle_events, sql_file, {}, {}) { } diff --git a/server/modules/monitor/mariadbmon/server_utils.hh b/server/modules/monitor/mariadbmon/server_utils.hh index 64d9cd24a..3f3c4b39c 100644 --- a/server/modules/monitor/mariadbmon/server_utils.hh +++ b/server/modules/monitor/mariadbmon/server_utils.hh @@ -13,6 +13,7 @@ #pragma once #include "mariadbmon_common.hh" +#include #include #include #include @@ -209,7 +210,8 @@ public: bool should_be_copied(std::string* ignore_reason_out) const; }; -typedef std::vector SlaveStatusArray; +using SlaveStatusArray = std::vector; +using EventNameSet = std::unordered_set; enum class OperationType { @@ -239,8 +241,15 @@ public: const std::string sql_file; // Path to file with SQL commands to run during op const SlaveStatusArray conns_to_copy; // Slave connections the target should copy/merge + const EventNameSet events_to_enable; // Scheduled event names last seen on master. + ServerOperation(MariaDBServer* target, bool was_is_master, bool handle_events, - const std::string& sql_file, const SlaveStatusArray& conns_to_copy); + const std::string& sql_file, const SlaveStatusArray& conns_to_copy, + const EventNameSet& events_to_enable); + + ServerOperation(MariaDBServer* target, bool was_is_master, bool handle_events, + const std::string& sql_file); + }; /** diff --git a/server/modules/monitor/mariadbmon/test/test_cycle_find.cc b/server/modules/monitor/mariadbmon/test/test_cycle_find.cc index e206bb4dd..76b96cbd7 100644 --- a/server/modules/monitor/mariadbmon/test/test_cycle_find.cc +++ b/server/modules/monitor/mariadbmon/test/test_cycle_find.cc @@ -162,7 +162,7 @@ void MariaDBMonitor::Test::init_servers(int count) MXS_MONITORED_SERVER* mon_server = new MXS_MONITORED_SERVER; // Contents mostly undefined mon_server->server = base_server; - MariaDBServer* mariadb_server = new MariaDBServer(mon_server, i - 1, m_use_hostnames); + MariaDBServer* mariadb_server = new MariaDBServer(mon_server, i - 1, m_use_hostnames, false); if (m_use_hostnames) {