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.

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;
+                        if (!new_master->enable_events(old_master->m_enabled_events, error_out))
-                        PRINT_MXS_JSON_ERROR(error_out, "Could not enable events on '%s': %s",
+                        {
-                                             new_master->name(), error_msg.c_str());
+                            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 */
+                ServerOperation demotion(joinable, true,  /* treat as old master */
-                                         m_handle_event_scheduler, m_demote_sql_file, {} /* unused */);
+                                         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::FailoverParams> 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,
+                                      m_handle_event_scheduler, m_promote_sql_file,
-                                         demotion_target->m_slave_status);
+                                      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));
     }

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));
     }
 }
 

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,
+    // Helper function which enables a disabled event if that event name is found in the events-set.
-                                                                              json_t** error_out) {
+    ManipulatorFunc enabler = [this, event_names, &found_disabled_events, &events_enabled](
-            if (event.status == "SLAVESIDE_DISABLED")
+            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++;
+                events_enabled++;
-                if (alter_event(event, "ENABLE", error_out))
-                {
-                    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(db_name_ind) + "." + event_info->get_string(event_name_ind);
-        event.name = 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.
+    // An ALTER EVENT by default changes the definer (owner) of the event to the monitor user.
-    string use_db_query = string_printf("USE %s;", event.database.c_str());
+    // This causes problems if the monitor user does not have privileges to run
-    if (execute_cmd(use_db_query, &error_msg))
+    // 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.
+        auto host_begin = loc_at + 1;
-        // This causes problems if the monitor user does not have privileges to run
+        quoted_definer = event.definer.substr(0, loc_at + 1)
-        // the event contents. Prevent this by setting definer explicitly.
+            +   // host_begin may be the null-char if @ was the last char
-        // The definer may be of the form user@host. If host includes %, then it must be quoted.
+            "'" + event.definer.substr(host_begin, string::npos) + "'";
-        // 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());
-        }
     }
     else
     {
-        const char FMT[] = "Could not switch to database '%s' on '%s': %s Event '%s' not altered.";
+        // Just the username
-        PRINT_MXS_JSON_ERROR(error_out, FMT, event.database.c_str(), name(), error_msg.c_str(),
+        quoted_definer = event.definer;
-                             event.name.c_str());
+    }
+
+    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;
+}

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 <database.name> 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 m_print_update_errormsg = true;    /* Should an update error be printed? */
-                  bool assume_unique_hostnames = true);
 
     /**
      * 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<void (const EventInfo&, json_t** error_out)> 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();
 };

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,
+ServerOperation::ServerOperation(MariaDBServer* target, bool was_is_master, bool handle_events,
-                                 bool handle_events, const std::string& sql_file,
+                                 const std::string& sql_file, const SlaveStatusArray& conns_to_copy,
-                                 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, {}, {})
 {
 }
 

server/modules/monitor/mariadbmon/server_utils.hh

@@ -13,6 +13,7 @@
 #pragma once
 #include "mariadbmon_common.hh"
 
+#include <unordered_set>
 #include <string>
 #include <vector>
 #include <maxbase/stopwatch.hh>
@@ -209,7 +210,8 @@ public:
     bool                      should_be_copied(std::string* ignore_reason_out) const;
 };
 
-typedef std::vector<SlaveStatus> SlaveStatusArray;
+using SlaveStatusArray = std::vector<SlaveStatus>;
+using EventNameSet = std::unordered_set<std::string>;
 
 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);
+
 };
 
 /**

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)
         {