diff --git a/include/maxscale/monitor.hh b/include/maxscale/monitor.hh index 39460cbce..e97e61409 100644 --- a/include/maxscale/monitor.hh +++ b/include/maxscale/monitor.hh @@ -193,7 +193,6 @@ class Monitor public: Monitor(const std::string& name, const std::string& module); virtual ~Monitor(); - virtual bool configure(const MXS_CONFIG_PARAMETER* params) = 0; static const int STATUS_FLAG_NOCHECK = 0; static const int STATUS_FLAG_CHECK = -1; @@ -201,14 +200,13 @@ public: virtual monitor_state_t state() const = 0; /** - * Starts the monitor. If the monitor requires polling of the servers, it should create - * a separate monitoring thread. - * - * @param params Parameters for this monitor + * Configure the monitor. Called by monitor creation and altering code. Any inheriting classes + * should override this with their own configuration processing function. The overriding function + * should first call the configure() of its immediate base class, similar to constructors. * * @return True, if the monitor could be started, false otherwise. */ - virtual bool start(const MXS_CONFIG_PARAMETER* params) = 0; + virtual bool configure(const MXS_CONFIG_PARAMETER* params); /** * Stops the monitor. @@ -393,21 +391,19 @@ protected: Settings m_settings; -private: - void add_server(SERVER* server); - - void remove_server(SERVER* server); - private: friend class MonitorManager; + void add_server(SERVER* server); + void remove_server(SERVER* server); + /** - * Configure base class. Called by monitor creation code. + * Starts the monitor. If the monitor requires polling of the servers, it should create + * a separate monitoring thread. * - * @param params Config parameters - * @return True on success + * @return True, if the monitor could be started, false otherwise. */ - bool configure_base(const MXS_CONFIG_PARAMETER* params); + virtual bool start() = 0; /** * Launch a script @@ -653,7 +649,6 @@ public: * * - Calls @c has_sufficient_permissions(), if it has not been done earlier. * - Updates the 'script' and 'events' configuration paramameters. - * - Calls @c configure(). * - Starts the monitor thread. * * - Once the monitor thread starts, it will @@ -668,11 +663,9 @@ public: * - Sleep until time for next @c tick(). * - Call @c post_loop(). * - * @param param The parameters of the monitor. - * * @return True, if the monitor started, false otherwise. */ - bool start(const MXS_CONFIG_PARAMETER* params) final; + bool start() final; /** * @brief Write diagnostics @@ -747,7 +740,7 @@ protected: * * @note If false is returned, then the monitor will not be started. */ - virtual bool configure(const MXS_CONFIG_PARAMETER* pParams); + bool configure(const MXS_CONFIG_PARAMETER* pParams) override; /** * @brief Check whether the monitor has sufficient rights diff --git a/maxutils/maxbase/include/maxbase/worker.hh b/maxutils/maxbase/include/maxbase/worker.hh index 56e7311fe..f37ba4120 100644 --- a/maxutils/maxbase/include/maxbase/worker.hh +++ b/maxutils/maxbase/include/maxbase/worker.hh @@ -423,7 +423,7 @@ public: * * @return True if the thread could be started, false otherwise. */ - bool start(); + virtual bool start(); /** * Waits for the worker to finish. diff --git a/server/core/config_runtime.cc b/server/core/config_runtime.cc index e08f0246e..fdc33ba75 100644 --- a/server/core/config_runtime.cc +++ b/server/core/config_runtime.cc @@ -131,6 +131,78 @@ static std::pair load_defaults(const char* name, return {rval, params}; } +bool runtime_add_server(Monitor* mon, Server* server) +{ + using std::string; + mxb_assert(mon && server); + bool rval = false; + + if (monitor_server_in_use(server)) + { + MXS_ERROR("Server '%s' is already monitored.", server->name()); + } + else + { + // To keep monitor modifications straightforward, all changes should go through the same + // alter_monitor function. As the function accepts key-value combinations (so that they are easily + // serialized), construct the value here. + string serverlist = mon->parameters.get_string(CN_SERVERS); + if (serverlist.empty()) + { + // Unusual. + serverlist += server->name(); + } + else + { + serverlist += string(", ") + server->name(); + } + rval = runtime_alter_monitor(mon, CN_SERVERS, serverlist.c_str()); + } + return rval; +} + +bool runtime_remove_server(Monitor* mon, Server* server) +{ + using std::string; + mxb_assert(mon && server); + bool rval = false; + + if (monitor_server_in_use(server) != mon) + { + MXS_ERROR("Server '%s' is not monitored by '%s'.", server->name(), mon->m_name); + } + else + { + // Construct the new list. The removed value could be anywhere. + string serverlist = mon->parameters.get_string(CN_SERVERS); + auto names = config_break_list_string(serverlist); + bool found = false; + for (auto iter = names.begin(); iter != names.end(); ++iter) + { + if (*iter == server->name()) + { + found = true; + names.erase(iter); + break; + } + } + + if (found) + { + // Rebuild the string. + string new_list; + string separator; + for (auto name : names) + { + new_list += separator + name; + separator = ", "; + } + rval = runtime_alter_monitor(mon, CN_SERVERS, new_list.c_str()); + } + } + return rval; +} + bool runtime_link_server(Server* server, const char* target) { std::lock_guard guard(crt_lock); @@ -165,7 +237,7 @@ bool runtime_link_server(Server* server, const char* target) } else if (monitor) { - if (MonitorManager::add_server(monitor, server)) + if (runtime_add_server(monitor, server)) { monitor_serialize(monitor); rval = true; @@ -213,7 +285,7 @@ bool runtime_unlink_server(Server* server, const char* target) } else if (monitor) { - MonitorManager::remove_server(monitor, server); + runtime_remove_server(monitor, server); monitor_serialize(monitor); rval = true; } @@ -597,86 +669,24 @@ bool do_alter_monitor(Monitor* monitor, const char* key, const char* value) { return false; } - else if (strcmp(key, "servers") == 0) - { - config_runtime_error("Parameter '%s' cannot be altered via this method", key); - return false; - } - std::lock_guard guard(crt_lock); - monitor->parameters.set(key, value); - bool success = true; - if (strcmp(key, CN_USER) == 0) - { - monitor->set_user(value); - } - else if (strcmp(key, CN_PASSWORD) == 0) - { - monitor->set_password(value); - } - else if (strcmp(key, CN_MONITOR_INTERVAL) == 0) - { - if (auto ival = get_positive_int(value)) - { - monitor->set_interval(ival); - } - } - else if (strcmp(key, CN_BACKEND_CONNECT_TIMEOUT) == 0) - { - if (auto ival = get_positive_int(value)) - { - monitor->set_network_timeout(MONITOR_CONNECT_TIMEOUT, ival, CN_BACKEND_CONNECT_TIMEOUT); - } - } - else if (strcmp(key, CN_BACKEND_WRITE_TIMEOUT) == 0) - { - if (auto ival = get_positive_int(value)) - { - monitor->set_network_timeout(MONITOR_WRITE_TIMEOUT, ival, CN_BACKEND_WRITE_TIMEOUT); - } - } - else if (strcmp(key, CN_BACKEND_READ_TIMEOUT) == 0) - { - if (auto ival = get_positive_int(value)) - { - monitor->set_network_timeout(MONITOR_READ_TIMEOUT, ival, CN_BACKEND_READ_TIMEOUT); - } - } - else if (strcmp(key, CN_BACKEND_CONNECT_ATTEMPTS) == 0) - { - if (auto ival = get_positive_int(value)) - { - monitor->set_network_timeout(MONITOR_CONNECT_ATTEMPTS, ival, CN_BACKEND_CONNECT_ATTEMPTS); - } - } - else if (strcmp(key, CN_JOURNAL_MAX_AGE) == 0) - { - if (auto ival = get_positive_int(value)) - { - monitor->monitor_set_journal_max_age(ival); - } - } - else if (strcmp(key, CN_SCRIPT_TIMEOUT) == 0) - { - if (auto ival = get_positive_int(value)) - { - monitor->set_script_timeout(ival); - } - } - else if (strcmp(key, CN_DISK_SPACE_THRESHOLD) == 0) - { - success = monitor->set_disk_space_threshold(value); - } - else - { - // This should be a module specific parameter - mxb_assert(config_param_is_valid(mod->parameters, key, value, NULL)); - } + // Backup monitor parameters in case configure fails. + MXS_CONFIG_PARAMETER originals = monitor->parameters; + MXS_CONFIG_PARAMETER modified = monitor->parameters; + modified.set(key, value); + bool success = monitor->configure(&modified); if (success) { MXS_NOTICE("Updated monitor '%s': %s=%s", monitor->m_name, key, value); } + else + { + // Configure failed, restore original configs. This should not fail. + // TODO: add a flag to monitor which prevents startup if config is wrong. + MXB_AT_DEBUG(bool check =) monitor->configure(&originals); + mxb_assert(check); + } return success; } @@ -686,7 +696,7 @@ bool runtime_alter_monitor(Monitor* monitor, const char* key, const char* value) bool was_running = (monitor->state() == MONITOR_STATE_RUNNING); if (was_running) { - monitor_stop(monitor); + MonitorManager::monitor_stop(monitor); } bool success = do_alter_monitor(monitor, key, value); if (success) @@ -695,7 +705,7 @@ bool runtime_alter_monitor(Monitor* monitor, const char* key, const char* value) } if (was_running) { - MonitorManager::monitor_start(monitor, &monitor->parameters); + MonitorManager::monitor_start(monitor); } return success; } @@ -1422,7 +1432,7 @@ bool runtime_destroy_monitor(Monitor* monitor) if (rval) { - monitor_stop(monitor); + MonitorManager::monitor_stop(monitor); while (!monitor->m_servers.empty()) { @@ -2258,7 +2268,7 @@ Monitor* runtime_create_monitor_from_json(json_t* json) } else { - MonitorManager::monitor_start(rval, &rval->parameters); + MonitorManager::monitor_start(rval); } } } @@ -2392,14 +2402,15 @@ bool service_to_filter_relations(Service* service, json_t* old_json, json_t* new bool runtime_alter_monitor_from_json(Monitor* monitor, json_t* new_json) { - bool rval = false; + bool success = false; + std::unique_ptr old_json(monitor_to_json(monitor, "")); mxb_assert(old_json.get()); if (is_valid_resource_body(new_json) && object_to_server_relations(monitor->m_name, old_json.get(), new_json)) { - rval = true; + success = true; bool changed = false; json_t* parameters = mxs_json_pointer(new_json, MXS_JSON_PTR_PARAMETERS); json_t* old_parameters = mxs_json_pointer(old_json.get(), MXS_JSON_PTR_PARAMETERS); @@ -2409,7 +2420,7 @@ bool runtime_alter_monitor_from_json(Monitor* monitor, json_t* new_json) if (parameters) { bool restart = (monitor->state() != MONITOR_STATE_STOPPED); - monitor_stop(monitor); + MonitorManager::monitor_stop(monitor); const char* key; json_t* value; @@ -2428,24 +2439,24 @@ bool runtime_alter_monitor_from_json(Monitor* monitor, json_t* new_json) } else { - rval = false; + success = false; break; } } - if (rval && changed) + if (success && changed) { monitor_serialize(monitor); } if (restart) { - MonitorManager::monitor_start(monitor, &monitor->parameters); + MonitorManager::monitor_start(monitor); } } } - return rval; + return success; } bool runtime_alter_monitor_relationships_from_json(Monitor* monitor, json_t* json) diff --git a/server/core/internal/monitor.hh b/server/core/internal/monitor.hh index 54121f728..0f8eb19f8 100644 --- a/server/core/internal/monitor.hh +++ b/server/core/internal/monitor.hh @@ -106,27 +106,32 @@ public: */ static void destroy_all_monitors(); - static void monitor_start(Monitor*, const MXS_CONFIG_PARAMETER*); + static void monitor_start(Monitor*); /** * @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 bool add_server(Monitor* mon, SERVER* server); + static void remove_server(Monitor* mon, SERVER* server) { Monitor::remove_server(mon, server); } + + /** + * Stop a given monitor + * + * @param monitor The monitor to stop + */ + static void monitor_stop(Monitor*); }; -void monitor_stop(Monitor*); + /** * @brief Mark monitor as deactivated diff --git a/server/core/monitor.cc b/server/core/monitor.cc index b454e7d54..13bb5d715 100644 --- a/server/core/monitor.cc +++ b/server/core/monitor.cc @@ -167,7 +167,7 @@ Monitor* MonitorManager::create_monitor(const string& name, const string& module return NULL; } - if (mon->configure_base(params)) // TODO: Move derived class configure() here + if (mon->configure(params)) { this_unit.insert_front(mon); } @@ -198,7 +198,7 @@ void Monitor::stop() } } -bool Monitor::configure_base(const MXS_CONFIG_PARAMETER* params) +bool Monitor::configure(const MXS_CONFIG_PARAMETER* params) { m_settings.conn_settings.read_timeout = params->get_integer(CN_BACKEND_READ_TIMEOUT); m_settings.conn_settings.write_timeout = params->get_integer(CN_BACKEND_WRITE_TIMEOUT); @@ -214,11 +214,19 @@ bool Monitor::configure_base(const MXS_CONFIG_PARAMETER* params) m_settings.conn_settings.password = params->get_string(CN_PASSWORD); // The monitor serverlist has already been checked to be valid. Empty value is ok too. + // First, remove all servers. + auto servers_temp = params->get_server_list(CN_SERVERS); + while (!m_servers.empty()) + { + SERVER* remove = m_servers.front()->server; + remove_server(remove); + } + 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); + add_server(elem); } /* The previous config values were normal types and were checked by the config manager @@ -258,12 +266,7 @@ void MonitorManager::destroy_all_monitors() } } -/** - * Start an individual monitor that has previously been stopped. - * - * @param monitor The Monitor that should be started - */ -void MonitorManager::monitor_start(Monitor* monitor, const MXS_CONFIG_PARAMETER* params) +void MonitorManager::monitor_start(Monitor* monitor) { mxb_assert(monitor); @@ -272,13 +275,18 @@ void MonitorManager::monitor_start(Monitor* monitor, const MXS_CONFIG_PARAMETER* // Only start the monitor if it's stopped. if (monitor->state() == MONITOR_STATE_STOPPED) { - if (!monitor->start(params)) + if (!monitor->start()) { MXS_ERROR("Failed to start monitor '%s'.", monitor->m_name); } } } +bool MonitorManager::add_server(Monitor* mon, SERVER* server) +{ + return false; +} + void MonitorManager::populate_services() { this_unit.foreach_monitor([](Monitor* pMonitor) -> bool { @@ -295,18 +303,13 @@ void monitor_start_all() this_unit.foreach_monitor([](Monitor* monitor) { if (monitor->m_active) { - MonitorManager::monitor_start(monitor, &monitor->parameters); + MonitorManager::monitor_start(monitor); } return true; }); } -/** - * Stop a given monitor - * - * @param monitor The monitor to stop - */ -void monitor_stop(Monitor* monitor) +void MonitorManager::monitor_stop(Monitor* monitor) { mxb_assert(monitor); @@ -334,7 +337,7 @@ void monitor_stop_all() this_unit.foreach_monitor([](Monitor* monitor) { if (monitor->m_active) { - monitor_stop(monitor); + MonitorManager::monitor_stop(monitor); } return true; }); @@ -358,14 +361,14 @@ bool Monitor::add_server(Monitor* mon, SERVER* server) if (old_state == MONITOR_STATE_RUNNING) { - monitor_stop(mon); + MonitorManager::monitor_stop(mon); } mon->add_server(server); if (old_state == MONITOR_STATE_RUNNING) { - MonitorManager::monitor_start(mon, &mon->parameters); + MonitorManager::monitor_start(mon); } } @@ -446,14 +449,14 @@ void Monitor::remove_server(Monitor* mon, SERVER* server) if (old_state == MONITOR_STATE_RUNNING) { - monitor_stop(mon); + MonitorManager::monitor_stop(mon); } mon->remove_server(server); if (old_state == MONITOR_STATE_RUNNING) { - MonitorManager::monitor_start(mon, &mon->parameters); + MonitorManager::monitor_start(mon); } } @@ -2498,7 +2501,7 @@ json_t* MonitorWorker::diagnostics_json() const return json_object(); } -bool MonitorWorker::start(const MXS_CONFIG_PARAMETER* pParams) +bool MonitorWorker::start() { // This should only be called by monitor_start(). NULL worker is allowed since the main worker may // not exist during program start/stop. @@ -2530,27 +2533,23 @@ bool MonitorWorker::start(const MXS_CONFIG_PARAMETER* pParams) if (m_checked) { m_master = NULL; - - if (configure(pParams)) + m_loop_called = get_time_ms() - m_settings.interval; // Next tick should happen immediately. + if (!Worker::start()) { - m_loop_called = get_time_ms() - m_settings.interval; // Next tick should happen immediately. - if (!Worker::start()) - { - MXS_ERROR("Failed to start worker for monitor '%s'.", m_name); - } - else - { - // Ok, so the thread started. Let's wait until we can be certain the - // state has been updated. - m_semaphore.wait(); + MXS_ERROR("Failed to start worker for monitor '%s'.", m_name); + } + else + { + // Ok, so the thread started. Let's wait until we can be certain the + // state has been updated. + m_semaphore.wait(); - started = m_thread_running.load(std::memory_order_acquire); - if (!started) - { - // Ok, so the initialization failed and the thread will exit. - // We need to wait on it so that the thread resources will not leak. - Worker::join(); - } + started = m_thread_running.load(std::memory_order_acquire); + if (!started) + { + // Ok, so the initialization failed and the thread will exit. + // We need to wait on it so that the thread resources will not leak. + Worker::join(); } } } @@ -2719,7 +2718,7 @@ void MonitorWorker::update_disk_space_status(MXS_MONITORED_SERVER* pMs) bool MonitorWorker::configure(const MXS_CONFIG_PARAMETER* pParams) { - return true; + return Monitor::configure(pParams); } bool MonitorWorker::has_sufficient_permissions() diff --git a/server/core/resource.cc b/server/core/resource.cc index 5a43d7892..e8393cfcc 100644 --- a/server/core/resource.cc +++ b/server/core/resource.cc @@ -262,7 +262,7 @@ HttpResponse cb_stop_monitor(const HttpRequest& request) Monitor* monitor = monitor_find(request.uri_part(1).c_str()); if (monitor) { - monitor_stop(monitor); + MonitorManager::monitor_stop(monitor); } return HttpResponse(MHD_HTTP_NO_CONTENT); } @@ -272,7 +272,7 @@ HttpResponse cb_start_monitor(const HttpRequest& request) Monitor* monitor = monitor_find(request.uri_part(1).c_str()); if (monitor) { - MonitorManager::monitor_start(monitor, &monitor->parameters); + MonitorManager::monitor_start(monitor); } return HttpResponse(MHD_HTTP_NO_CONTENT); } diff --git a/server/modules/monitor/clustrixmon/clustrixmonitor.cc b/server/modules/monitor/clustrixmon/clustrixmonitor.cc index 3c6866e78..fb4b43566 100644 --- a/server/modules/monitor/clustrixmon/clustrixmonitor.cc +++ b/server/modules/monitor/clustrixmon/clustrixmonitor.cc @@ -55,6 +55,11 @@ ClustrixMonitor* ClustrixMonitor::create(const string& name, const string& modul bool ClustrixMonitor::configure(const MXS_CONFIG_PARAMETER* pParams) { + if (!MonitorWorker::configure(pParams)) + { + return false; + } + m_health_urls.clear(); m_nodes.clear(); diff --git a/server/modules/monitor/csmon/csmon.cc b/server/modules/monitor/csmon/csmon.cc index e2bcfe957..3ea0b120e 100644 --- a/server/modules/monitor/csmon/csmon.cc +++ b/server/modules/monitor/csmon/csmon.cc @@ -119,6 +119,11 @@ void CsMonitor::update_server_status(MXS_MONITORED_SERVER* srv) bool CsMonitor::configure(const MXS_CONFIG_PARAMETER* pParams) { + if (MonitorWorkerSimple::configure(pParams)) + { + return false; + } + m_primary = pParams->get_server("primary"); return true; } diff --git a/server/modules/monitor/galeramon/galeramon.cc b/server/modules/monitor/galeramon/galeramon.cc index dc18dbe92..da40e11c7 100644 --- a/server/modules/monitor/galeramon/galeramon.cc +++ b/server/modules/monitor/galeramon/galeramon.cc @@ -104,6 +104,11 @@ json_t* GaleraMonitor::diagnostics_json() const bool GaleraMonitor::configure(const MXS_CONFIG_PARAMETER* params) { + if (!MonitorWorkerSimple::configure(params)) + { + return false; + } + m_disableMasterFailback = params->get_bool("disable_master_failback"); m_availableWhenDonor = params->get_bool("available_when_donor"); m_disableMasterRoleSetting = params->get_bool("disable_master_role_setting"); diff --git a/server/modules/monitor/mariadbmon/mariadbmon.cc b/server/modules/monitor/mariadbmon/mariadbmon.cc index 5656da983..4fa98ac6e 100644 --- a/server/modules/monitor/mariadbmon/mariadbmon.cc +++ b/server/modules/monitor/mariadbmon/mariadbmon.cc @@ -187,6 +187,11 @@ MariaDBMonitor* MariaDBMonitor::create(const string& name, const string& module) */ bool MariaDBMonitor::configure(const MXS_CONFIG_PARAMETER* params) { + if (!MonitorWorker::configure(params)) + { + return false; + } + m_detect_stale_master = params->get_bool("detect_stale_master"); m_detect_stale_slave = params->get_bool("detect_stale_slave"); m_ignore_external_masters = params->get_bool("ignore_external_masters"); diff --git a/server/modules/monitor/mariadbmon/mariadbmon.hh b/server/modules/monitor/mariadbmon/mariadbmon.hh index 057dcbea3..a50b51b44 100644 --- a/server/modules/monitor/mariadbmon/mariadbmon.hh +++ b/server/modules/monitor/mariadbmon/mariadbmon.hh @@ -238,7 +238,7 @@ private: // Base methods MariaDBMonitor(const std::string& name, const std::string& module); - bool configure(const MXS_CONFIG_PARAMETER* params); + bool configure(const MXS_CONFIG_PARAMETER* params) override; bool set_replication_credentials(const MXS_CONFIG_PARAMETER* params); void reset_server_info(); void clear_server_info(); diff --git a/server/modules/routing/debugcli/debugcmd.cc b/server/modules/routing/debugcli/debugcmd.cc index 15599411f..63e8a7fc4 100644 --- a/server/modules/routing/debugcli/debugcmd.cc +++ b/server/modules/routing/debugcli/debugcmd.cc @@ -2716,7 +2716,7 @@ static void show_qc_all(DCB* dcb) */ static void shutdown_monitor(DCB* dcb, Monitor* monitor) { - monitor_stop(monitor); + MonitorManager::monitor_stop(monitor); } /** @@ -2727,7 +2727,7 @@ static void shutdown_monitor(DCB* dcb, Monitor* monitor) */ static void restart_monitor(DCB* dcb, Monitor* monitor) { - MonitorManager::monitor_start(monitor, &monitor->parameters); + MonitorManager::monitor_start(monitor); } /** diff --git a/server/modules/routing/maxinfo/maxinfo_exec.cc b/server/modules/routing/maxinfo/maxinfo_exec.cc index 30d8659e0..c51e3606e 100644 --- a/server/modules/routing/maxinfo/maxinfo_exec.cc +++ b/server/modules/routing/maxinfo/maxinfo_exec.cc @@ -506,7 +506,7 @@ void exec_shutdown_monitor(DCB* dcb, MAXINFO_TREE* tree) Monitor* monitor = monitor_find(tree->value); if (monitor) { - monitor_stop(monitor); + MonitorManager::monitor_stop(monitor); maxinfo_send_ok(dcb); } else @@ -615,7 +615,7 @@ void exec_restart_monitor(DCB* dcb, MAXINFO_TREE* tree) Monitor* monitor = monitor_find(tree->value); if (monitor) { - MonitorManager::monitor_start(monitor, &monitor->parameters); + MonitorManager::monitor_start(monitor); maxinfo_send_ok(dcb); } else