From 5077ad62df902475effe81c3463a1e2598fbd5f9 Mon Sep 17 00:00:00 2001 From: Esa Korhonen Date: Tue, 7 May 2019 18:47:25 +0300 Subject: [PATCH] Move monitor runtime modification functions to MonitorManager Requires moving some general configuration checking functions to config.cc. --- server/core/config.cc | 50 ++++++++ server/core/config_runtime.cc | 168 +++---------------------- server/core/internal/config.hh | 8 ++ server/core/internal/monitormanager.hh | 63 +++++----- server/core/monitormanager.cc | 127 ++++++++++++++++++- 5 files changed, 235 insertions(+), 181 deletions(-) diff --git a/server/core/config.cc b/server/core/config.cc index 32db7d9e3..9b1c38205 100644 --- a/server/core/config.cc +++ b/server/core/config.cc @@ -5432,3 +5432,53 @@ int64_t config_enum_to_value(const std::string& value, const MXS_ENUM_VALUE* val return MXS_UNKNOWN_ENUM_VALUE; } + +bool validate_param(const MXS_MODULE_PARAM* basic, const MXS_MODULE_PARAM* module, + const string& key, const string& value, string* error_out) +{ + bool success = false; + string error_msg; + if (!param_is_known(basic, module, key.c_str())) + { + error_msg = mxb::string_printf("Unknown parameter: %s", key.c_str()); + } + else if (!value[0]) + { + error_msg = mxb::string_printf("Empty value for parameter: %s", key.c_str()); + } + else if (!param_is_valid(basic, module, key.c_str(), value.c_str())) + { + error_msg = mxb::string_printf("Invalid parameter value for '%s': %s", key.c_str(), value.c_str()); + } + else + { + success = true; + } + + if (!success) + { + *error_out = error_msg; + } + return success; +} + +bool param_is_known(const MXS_MODULE_PARAM* basic, const MXS_MODULE_PARAM* module, const char* key) +{ + std::unordered_set names; + + for (auto param : {basic, module}) + { + for (int i = 0; param[i].name; i++) + { + names.insert(param[i].name); + } + } + + return names.count(key); +} + +bool param_is_valid(const MXS_MODULE_PARAM* basic, const MXS_MODULE_PARAM* module, + const char* key, const char* value) +{ + return config_param_is_valid(basic, key, value, NULL) || config_param_is_valid(module, key, value, NULL); +} diff --git a/server/core/config_runtime.cc b/server/core/config_runtime.cc index a022a7676..4860d37f7 100644 --- a/server/core/config_runtime.cc +++ b/server/core/config_runtime.cc @@ -161,66 +161,6 @@ 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 (MonitorManager::server_is_monitored(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 (MonitorManager::server_is_monitored(server) != mon) - { - config_runtime_error("Server '%s' is not monitored by '%s'.", server->name(), mon->name()); - } - else - { - MonitorStop stop(mon); - - // Construct the new server list - auto params = mon->parameters; - auto names = config_break_list_string(params.get_string(CN_SERVERS)); - names.erase(std::remove(names.begin(), names.end(), server->name())); - std::string servers = mxb::join(names, ","); - params.set(CN_SERVERS, servers); - - if (MonitorManager::reconfigure_monitor(mon, params)) - { - rval = MonitorManager::monitor_serialize(mon); - } - } - - return rval; -} - bool runtime_link_server(Server* server, const char* target) { std::lock_guard guard(crt_lock); @@ -255,14 +195,14 @@ bool runtime_link_server(Server* server, const char* target) } else if (monitor) { - if (runtime_add_server(monitor, server)) + std::string error_msg; + if (MonitorManager::add_server_to_monitor(monitor, server, &error_msg)) { - MonitorManager::monitor_serialize(monitor); rval = true; } else { - config_runtime_error("Server '%s' is already monitored", server->name()); + config_runtime_error("%s", error_msg.c_str()); } } @@ -303,9 +243,14 @@ bool runtime_unlink_server(Server* server, const char* target) } else if (monitor) { - if ((rval = runtime_remove_server(monitor, server))) + std::string error_msg; + if (MonitorManager::remove_server_from_monitor(monitor, server, &error_msg)) { - MonitorManager::monitor_serialize(monitor); + rval = true; + } + else + { + config_runtime_error("%s", error_msg.c_str()); } } @@ -542,32 +487,6 @@ static inline bool is_valid_integer(const char* value) return strtol(value, &endptr, 10) >= 0 && *value && *endptr == '\0'; } -bool param_is_known(const MXS_MODULE_PARAM* basic, - const MXS_MODULE_PARAM* module, - const char* key) -{ - std::unordered_set names; - - for (auto param : {basic, module}) - { - for (int i = 0; param[i].name; i++) - { - names.insert(param[i].name); - } - } - - return names.count(key); -} - -bool param_is_valid(const MXS_MODULE_PARAM* basic, - const MXS_MODULE_PARAM* module, - const char* key, - const char* value) -{ - return config_param_is_valid(basic, key, value, NULL) - || config_param_is_valid(module, key, value, NULL); -} - bool runtime_alter_server(Server* server, const char* key, const char* value) { if (!value[0]) @@ -706,25 +625,12 @@ bool validate_param(const MXS_MODULE_PARAM* basic, const char* key, const char* value) { - bool rval = false; - - if (!param_is_known(basic, module, key)) + std::string error; + bool rval = validate_param(basic, module, key, value, &error); + if (!rval) { - config_runtime_error("Unknown parameter: %s", key); + config_runtime_error("%s", error.c_str()); } - else if (!value[0]) - { - config_runtime_error("Empty value for parameter: %s", key); - } - else if (!param_is_valid(basic, module, key, value)) - { - config_runtime_error("Invalid parameter value for '%s': %s", key, value); - } - else - { - rval = true; - } - return rval; } @@ -745,39 +651,18 @@ bool validate_param(const MXS_MODULE_PARAM* basic, return rval; } -bool do_alter_monitor(Monitor* monitor, const char* key, const char* value) +bool runtime_alter_monitor(Monitor* monitor, const char* key, const char* value) { - mxb_assert(monitor->state() == MONITOR_STATE_STOPPED); - const MXS_MODULE* mod = get_module(monitor->m_module.c_str(), MODULE_MONITOR); - - if (!validate_param(config_monitor_params, mod->parameters, key, value)) - { - return false; - } - - MXS_CONFIG_PARAMETER modified = monitor->parameters; - modified.set(key, value); - - bool success = MonitorManager::reconfigure_monitor(monitor, modified); - + std::string error_msg; + bool success = MonitorManager::alter_monitor(monitor, key, value, &error_msg); if (success) { MXS_NOTICE("Updated monitor '%s': %s=%s", monitor->name(), key, value); } - - return success; -} - -bool runtime_alter_monitor(Monitor* monitor, const char* key, const char* value) -{ - MonitorStop stop(monitor); - bool success = do_alter_monitor(monitor, key, value); - - if (success) + else { - MonitorManager::monitor_serialize(monitor); + config_runtime_error("%s", error_msg.c_str()); } - return success; } @@ -2627,20 +2512,7 @@ bool runtime_alter_monitor_from_json(Monitor* monitor, json_t* new_json) && validate_param(config_monitor_params, mod->parameters, ¶ms) && server_relationship_to_parameter(new_json, ¶ms)) { - bool restart = (monitor->state() != MONITOR_STATE_STOPPED); - MonitorManager::stop_monitor(monitor); - auto old_params = monitor->parameters; - - if (MonitorManager::reconfigure_monitor(monitor, params)) - { - MonitorManager::monitor_serialize(monitor); - success = true; - } - - if (restart) - { - MonitorManager::start_monitor(monitor); - } + success = MonitorManager::reconfigure_monitor(monitor, params); } return success; diff --git a/server/core/internal/config.hh b/server/core/internal/config.hh index de2f8b25e..daefa2fd0 100644 --- a/server/core/internal/config.hh +++ b/server/core/internal/config.hh @@ -250,3 +250,11 @@ constexpr int64_t MXS_UNKNOWN_ENUM_VALUE {-1}; * @return The enum value or MXS_UNKNOWN_ENUM_VALUE on unknown value */ int64_t config_enum_to_value(const std::string& key, const MXS_ENUM_VALUE* values); + +bool validate_param(const MXS_MODULE_PARAM* basic, const MXS_MODULE_PARAM* module, + const std::string& key, const std::string& value, std::string* error_out); + +bool param_is_known(const MXS_MODULE_PARAM* basic, const MXS_MODULE_PARAM* module, const char* key); + +bool param_is_valid(const MXS_MODULE_PARAM* basic, const MXS_MODULE_PARAM* module, + const char* key, const char* value); diff --git a/server/core/internal/monitormanager.hh b/server/core/internal/monitormanager.hh index ad3c7b0ca..e64da210c 100644 --- a/server/core/internal/monitormanager.hh +++ b/server/core/internal/monitormanager.hh @@ -116,17 +116,47 @@ public: static bool monitor_serialize(const mxs::Monitor* monitor); /** - * Attempt to reconfigure a monitor - * - * If the configuration fails, the old parameters are restored. + * Attempt to reconfigure a monitor. If the reconfiguration fails, the old parameters are restored. + * Should be only called from the admin thread. * * @param monitor Monitor to reconfigure * @param parameters New parameters to apply - * * @return True if reconfiguration was successful */ static bool reconfigure_monitor(mxs::Monitor* monitor, const MXS_CONFIG_PARAMETER& parameters); + /** + * Change one parameter in the monitor during runtime. Should only be called from the admin thread. + * + * @param monitor Monitor to reconfigure + * @param key Setting to change + * @param value New value + * @param error_out Error output + * @return True if reconfiguration was successful + */ + static bool alter_monitor(mxs::Monitor* monitor, const std::string& key, const std::string& value, + std::string* error_out); + + /** + * Add server to monitor during runtime. Should only be called from the admin thread. + * + * @param mon Target monitor + * @param server Server to add + * @param error_out Error output + * @return True on success + */ + static bool add_server_to_monitor(mxs::Monitor* mon, SERVER* server, std::string* error_out); + + /** + * Remove a server from a monitor during runtime. Should only be called from the admin thread. + * + * @param mon Target monitor + * @param server Server to remove + * @param error_out Error output + * @return True on success + */ + static bool remove_server_from_monitor(mxs::Monitor* mon, SERVER* server, std::string* error_out); + /** * @brief Convert monitor to JSON * @@ -164,28 +194,3 @@ public: */ static void debug_wait_one_tick(); }; - -// RAII helper class for temprarily stopping monitors -class MonitorStop -{ -public: - MonitorStop(mxs::Monitor* monitor) - : m_monitor(monitor->state() == MONITOR_STATE_RUNNING ? monitor : nullptr) - { - if (m_monitor) - { - MonitorManager::stop_monitor(m_monitor); - } - } - - ~MonitorStop() - { - if (m_monitor) - { - MonitorManager::start_monitor(m_monitor); - } - } - -private: - mxs::Monitor* m_monitor; -}; diff --git a/server/core/monitormanager.cc b/server/core/monitormanager.cc index b8ac508e5..b358a226d 100644 --- a/server/core/monitormanager.cc +++ b/server/core/monitormanager.cc @@ -13,6 +13,7 @@ #include +#include #include #include #include @@ -27,6 +28,7 @@ using maxscale::Monitor; using maxscale::MonitorServer; using Guard = std::lock_guard; using std::string; +using mxb::string_printf; namespace { @@ -85,6 +87,9 @@ private: }; ThisUnit this_unit; + +const char RECONFIG_FAILED[] = "Monitor reconfiguration failed when %s. Check log for more details."; + } Monitor* MonitorManager::create_monitor(const string& name, const string& module, @@ -403,22 +408,57 @@ bool MonitorManager::monitor_serialize(const Monitor* monitor) return rval; } -// static bool MonitorManager::reconfigure_monitor(mxs::Monitor* monitor, const MXS_CONFIG_PARAMETER& parameters) { mxb_assert(Monitor::is_admin_thread()); // Backup monitor parameters in case configure fails. auto orig = monitor->parameters; - monitor->parameters.clear(); + // Stop/start monitor if it's currently running. If monitor was stopped already, this is likely + // managed by the caller. + bool stopstart = (monitor->state() == MONITOR_STATE_RUNNING); + if (stopstart) + { + monitor->stop(); + } - bool success = monitor->configure(¶meters); + bool success = false; + if (monitor->configure(¶meters)) + { + // Serialization must also succeed. + success = MonitorManager::monitor_serialize(monitor); + } if (!success) { + // Try to restore old values, it should work. MXB_AT_DEBUG(bool check = ) monitor->configure(&orig); mxb_assert(check); } + if (stopstart && !monitor->start()) + { + MXB_ERROR("Reconfiguration of monitor '%s' failed because monitor did not start.", monitor->name()); + } + return success; +} + +bool MonitorManager::alter_monitor(mxs::Monitor* monitor, const std::string& key, const std::string& value, + std::string* error_out) +{ + const MXS_MODULE* mod = get_module(monitor->m_module.c_str(), MODULE_MONITOR); + if (!validate_param(config_monitor_params, mod->parameters, key, value, error_out)) + { + return false; + } + + MXS_CONFIG_PARAMETER modified = monitor->parameters; + modified.set(key, value); + + bool success = MonitorManager::reconfigure_monitor(monitor, modified); + if (!success) + { + *error_out = string_printf(RECONFIG_FAILED, "changing a parameter"); + } return success; } @@ -509,4 +549,83 @@ bool MonitorManager::clear_server_status(SERVER* srv, int bit, string* errmsg_ou written = true; } return written; -} \ No newline at end of file +} + +bool MonitorManager::add_server_to_monitor(mxs::Monitor* mon, SERVER* server, std::string* error_out) +{ + mxb_assert(Monitor::is_admin_thread()); + bool success = false; + string server_monitor = Monitor::get_server_monitor(server); + if (!server_monitor.empty()) + { + // Error, server is already monitored. + string error = string_printf("Server '%s' is already monitored by '%s', ", + server->name(), server_monitor.c_str()); + error += (server_monitor == mon->name()) ? "cannot add again to the same monitor." : + "cannot add to another monitor."; + *error_out = error; + } + else + { + // To keep monitor modifications straightforward, all changes should go through the same + // reconfigure-function. As the function accepts key-value combinations (so that they are easily + // serialized), construct the value here. + MXS_CONFIG_PARAMETER modified_params = mon->parameters; + string serverlist = modified_params.get_string(CN_SERVERS); + if (serverlist.empty()) + { + // Unusual. + serverlist += server->name(); + } + else + { + serverlist += string(", ") + server->name(); + } + modified_params.set(CN_SERVERS, serverlist); + success = reconfigure_monitor(mon, modified_params); + if (!success) + { + *error_out = string_printf(RECONFIG_FAILED, "adding a server"); + } + } + return success; +} + +bool MonitorManager::remove_server_from_monitor(mxs::Monitor* mon, SERVER* server, std::string* error_out) +{ + mxb_assert(Monitor::is_admin_thread()); + bool success = false; + string server_monitor = Monitor::get_server_monitor(server); + if (server_monitor != mon->name()) + { + // Error, server is not monitored by given monitor. + string error; + if (server_monitor.empty()) + { + error = string_printf("Server '%s' is not monitored by any monitor, ", server->name()); + } + else + { + error = string_printf("Server '%s' is monitored by '%s', ", + server->name(), server_monitor.c_str()); + } + error += string_printf("cannot remove it from '%s'.", mon->name()); + *error_out = error; + } + else + { + // Construct the new server list + auto params = mon->parameters; + auto names = config_break_list_string(params.get_string(CN_SERVERS)); + names.erase(std::remove(names.begin(), names.end(), server->name())); + std::string servers = mxb::join(names, ","); + params.set(CN_SERVERS, servers); + success = MonitorManager::reconfigure_monitor(mon, params); + if (!success) + { + *error_out = string_printf(RECONFIG_FAILED, "removing a server"); + } + } + + return success; +}