From cac1d76e484e394456a7d38eead3f607ca954bb6 Mon Sep 17 00:00:00 2001 From: Johan Wikman Date: Fri, 8 Feb 2019 12:29:59 +0200 Subject: [PATCH] MXS-2314 Monitor decides whether servers are added to services When the servers of a service are defined by a monitor, then at startup all servers of the monitor should be added to relevant services. Likewise, when a server is added to or removed from a monitor at runtime, those changes should affect services as well. However, whether that should happen or not depends upon the monitor. In the case of the Clustrix monitor this should not happen as it adds and removes servers depending on the runtime state of the Clustrix cluster. --- include/maxscale/monitor.hh | 54 +++++++- server/core/config.cc | 2 +- server/core/config_runtime.cc | 6 +- server/core/internal/monitor.hh | 13 +- server/core/monitor.cc | 118 ++++++++++++------ .../monitor/clustrixmon/clustrixmonitor.cc | 12 ++ .../monitor/clustrixmon/clustrixmonitor.hh | 8 +- 7 files changed, 165 insertions(+), 48 deletions(-) diff --git a/include/maxscale/monitor.hh b/include/maxscale/monitor.hh index 847f38ab8..712a09ab1 100644 --- a/include/maxscale/monitor.hh +++ b/include/maxscale/monitor.hh @@ -337,6 +337,26 @@ protected: */ void detect_handle_state_changes(); + /** + * @brief Called when a server has been added to the monitor. + * + * The default implementation will add the server to associated + * services. + * + * @param server A server. + */ + virtual void server_added(SERVER* server); + + /** + * @brief Called when a server has been removed from the monitor. + * + * The default implementation will remove the server from associated + * services. + * + * @param server A server. + */ + virtual void server_removed(SERVER* server); + /** * Contains monitor base class settings. Since monitors are stopped before a setting change, * the items cannot be modified while a monitor is running. No locking required. @@ -363,6 +383,11 @@ protected: Settings m_settings; +private: + void add_server(SERVER* server); + + void remove_server(SERVER* server); + private: friend class MonitorManager; @@ -395,7 +420,34 @@ private: int launch_command(MXS_MONITORED_SERVER* ptr, EXTERNCMD* cmd); /** - * Populate services with the servers of the monitor. + * @brief Add a server to a monitor. + * + * Add a server to a monitor, provided the server is not currently + * being monitored by any monitor. Before adding the server to the + * monitor, the monitor is stopped if it is running and after the + * addition it is restarted if it was running. + * + * @param monitor A monitor. + * @param server A server. + * + * @return True, if the monitor was added, false otherwise. + */ + static bool add_server(Monitor* mon, SERVER* server); + + /** + * @brief Remove a server from a monitor. + * + * If the server is being monitored by the server, remove it. + * Before removing, the monitor is stopped if it is running and after + * the removal it is restarted if it was running. + * + * @param monitor A monitor. + * @param server A server. + */ + static void remove_server(Monitor* mon, SERVER* server); + + /** + * @brief The monitor should populate associated services. */ virtual void populate_services(); diff --git a/server/core/config.cc b/server/core/config.cc index 2f3d118b8..596cb5093 100644 --- a/server/core/config.cc +++ b/server/core/config.cc @@ -1406,7 +1406,7 @@ std::unordered_set get_dependencies(const std::vectorparameters, CN_CLUSTER)) { - rval.insert(name_to_object(objects, obj, config_get_string(obj->parameters, CN_CLUSTER))); + rval.insert(name_to_object(objects, obj, obj->parameters->get_string(CN_CLUSTER))); } if ((type == CN_MONITOR || type == CN_SERVICE) && config_get_value(obj->parameters, CN_SERVERS)) diff --git a/server/core/config_runtime.cc b/server/core/config_runtime.cc index 150db5b07..aefe0d67f 100644 --- a/server/core/config_runtime.cc +++ b/server/core/config_runtime.cc @@ -165,7 +165,7 @@ bool runtime_link_server(Server* server, const char* target) } else if (monitor) { - if (monitor_add_server(monitor, server)) + if (MonitorManager::add_server(monitor, server)) { monitor_serialize(monitor); rval = true; @@ -213,7 +213,7 @@ bool runtime_unlink_server(Server* server, const char* target) } else if (monitor) { - monitor_remove_server(monitor, server); + MonitorManager::remove_server(monitor, server); monitor_serialize(monitor); rval = true; } @@ -1434,7 +1434,7 @@ bool runtime_destroy_monitor(Monitor* monitor) while (!monitor->m_servers.empty()) { - monitor_remove_server(monitor, monitor->m_servers[0]->server); + MonitorManager::remove_server(monitor, monitor->m_servers[0]->server); } monitor_deactivate(monitor); MXS_NOTICE("Destroyed monitor '%s'", monitor->m_name); diff --git a/server/core/internal/monitor.hh b/server/core/internal/monitor.hh index 0f26c7fac..54121f728 100644 --- a/server/core/internal/monitor.hh +++ b/server/core/internal/monitor.hh @@ -112,6 +112,16 @@ public: * @brief Populate services with the servers of the monitors. */ static void populate_services(); + + static bool add_server(Monitor* mon, SERVER* server) + { + return Monitor::add_server(mon, server); + } + + static void remove_server(Monitor* mon, SERVER* server) + { + Monitor::remove_server(mon, server); + } }; @@ -139,9 +149,6 @@ void monitor_show_all(DCB*); void monitor_list(DCB*); -bool monitor_add_server(Monitor* mon, SERVER* server); -void monitor_remove_server(Monitor* mon, SERVER* server); - void monitor_set_journal_max_age(Monitor* mon, time_t value); /** diff --git a/server/core/monitor.cc b/server/core/monitor.cc index 48138cefc..0b7bdebbc 100644 --- a/server/core/monitor.cc +++ b/server/core/monitor.cc @@ -206,7 +206,7 @@ bool Monitor::configure_base(const MXS_CONFIG_PARAMETER* params) for (auto elem : servers_temp) { // This function checks if server is already monitored. TODO: This should be a config error. - monitor_add_server(this, elem); + Monitor::add_server(this, elem); } /* The previous config values were normal types and were checked by the config manager @@ -350,14 +350,8 @@ void monitor_stop_all() }); } -/** - * Add a server to a monitor. Simply register the server that needs to be - * monitored to the running monitor module. - * - * @param mon The Monitor instance - * @param server The Server to add to the monitoring - */ -bool monitor_add_server(Monitor* mon, SERVER* server) +//static +bool Monitor::add_server(Monitor* mon, SERVER* server) { mxb_assert(mon && server); bool rval = false; @@ -369,8 +363,6 @@ bool monitor_add_server(Monitor* mon, SERVER* server) else { rval = true; - MXS_MONITORED_SERVER* db = new (std::nothrow) MXS_MONITORED_SERVER(server); - MXS_ABORT_IF_NULL(db); monitor_state_t old_state = mon->m_state; @@ -379,12 +371,7 @@ bool monitor_add_server(Monitor* mon, SERVER* server) monitor_stop(mon); } - { - Guard guard(mon->m_lock); - mon->m_servers.push_back(db); - } - - service_add_server(mon, server); + mon->add_server(server); if (old_state == MONITOR_STATE_RUNNING) { @@ -395,6 +382,42 @@ bool monitor_add_server(Monitor* mon, SERVER* server) return rval; } +/** + * @brief Add a server to the monitor. + * + * It is assumed that the monitor is currently not running and that the + * server is not currently being monitored. + * + * @param server A server. + */ +void Monitor::add_server(SERVER* server) +{ + mxb_assert(m_state != MONITOR_STATE_RUNNING); + mxb_assert(!monitor_server_in_use(server)); + + MXS_MONITORED_SERVER* db = new (std::nothrow) MXS_MONITORED_SERVER(server); + MXS_ABORT_IF_NULL(db); + + using Guard = std::unique_lock; + Guard guard(m_lock); + + m_servers.push_back(db); + + guard.unlock(); + + server_added(server); +} + +void Monitor::server_added(SERVER* server) +{ + service_add_server(this, server); +} + +void Monitor::server_removed(SERVER* server) +{ + service_remove_server(this, server); +} + static void monitor_server_free(MXS_MONITORED_SERVER* tofree) { if (tofree) @@ -426,7 +449,8 @@ static void monitor_server_free_all(std::vector& servers) * @param mon The Monitor instance * @param server The Server to remove */ -void monitor_remove_server(Monitor* mon, SERVER* server) +//static +void Monitor::remove_server(Monitor* mon, SERVER* server) { monitor_state_t old_state = mon->m_state; @@ -435,26 +459,7 @@ void monitor_remove_server(Monitor* mon, SERVER* server) monitor_stop(mon); } - MXS_MONITORED_SERVER* ptr = nullptr; - { - Guard guard(mon->m_lock); - for (auto iter = mon->m_servers.begin(); iter != mon->m_servers.end(); ++iter) - { - if ((*iter)->server == server) - { - ptr = *iter; - mon->m_servers.erase(iter); - break; - } - } - } - - if (ptr) - { - monitor_server_free(ptr); - - service_remove_server(mon, server); - } + mon->remove_server(server); if (old_state == MONITOR_STATE_RUNNING) { @@ -462,6 +467,43 @@ void monitor_remove_server(Monitor* mon, SERVER* server) } } +/** + * @brief Remove a server from the monitor. + * + * It is assumed that the monitor is currently not running. + * + * @param server A server + */ +void Monitor::remove_server(SERVER* server) +{ + mxb_assert(m_state != MONITOR_STATE_RUNNING); + + MXS_MONITORED_SERVER* ptr = nullptr; + + using Guard = std::unique_lock; + Guard guard(m_lock); + + for (auto it = m_servers.begin(); it != m_servers.end(); ++it) + { + if ((*it)->server == server) + { + ptr = *it; + m_servers.erase(it); + break; + } + } + + guard.unlock(); + + if (ptr) + { + monitor_server_free(ptr); + + server_removed(server); + } + +} + void Monitor::set_user(const string& user) { m_settings.conn_settings.username = user; diff --git a/server/modules/monitor/clustrixmon/clustrixmonitor.cc b/server/modules/monitor/clustrixmon/clustrixmonitor.cc index d1a9c2708..30dc2e334 100644 --- a/server/modules/monitor/clustrixmon/clustrixmonitor.cc +++ b/server/modules/monitor/clustrixmon/clustrixmonitor.cc @@ -119,6 +119,18 @@ bool ClustrixMonitor::unsoftfail(SERVER* pServer, json_t** ppError) return true; } +void ClustrixMonitor::server_added(SERVER* pServer) +{ + // The servers explicitly added to the Cluster monitor are only used + // as bootstrap servers, so they are not added to any services. +} + +void ClustrixMonitor::server_removed(SERVER* pServer) +{ + // @see server_added(), no action is needed. +} + + void ClustrixMonitor::pre_loop() { make_health_check(); diff --git a/server/modules/monitor/clustrixmon/clustrixmonitor.hh b/server/modules/monitor/clustrixmon/clustrixmonitor.hh index 18dafb06c..9b4500a2c 100644 --- a/server/modules/monitor/clustrixmon/clustrixmonitor.hh +++ b/server/modules/monitor/clustrixmon/clustrixmonitor.hh @@ -64,11 +64,15 @@ public: bool configure(const MXS_CONFIG_PARAMETER* pParams) override; - void populate_services() override; - bool softfail(SERVER* pServer, json_t** ppError); bool unsoftfail(SERVER* pServer, json_t** ppError); +protected: + void populate_services() override; + + void server_added(SERVER* pServer) override; + void server_removed(SERVER* pServer) override; + private: ClustrixMonitor(const std::string& name, const std::string& module);