From 82b4338ecaa76124bf5488c936cc0d277c4480f8 Mon Sep 17 00:00:00 2001 From: Esa Korhonen Date: Fri, 26 Apr 2019 12:18:26 +0300 Subject: [PATCH] Remove MonitorManager calls from Monitor functions Also adds admin thread checks to MonitorManager functions and combines anonymous namespaces. --- include/maxscale/monitor.hh | 40 ++++--- server/core/config.cc | 1 + server/core/config_runtime.cc | 1 + server/core/gateway.cc | 1 + server/core/internal/monitor.hh | 28 ----- server/core/internal/monitormanager.hh | 32 +++++- server/core/modulecmd.cc | 1 + server/core/monitor.cc | 103 +++++++++--------- server/core/monitormanager.cc | 32 ++++-- server/core/resource.cc | 1 + server/core/server.cc | 1 + server/core/test/test_modulecmd.cc | 1 + server/modules/routing/cli/debugcmd.cc | 1 + .../modules/routing/maxinfo/maxinfo_exec.cc | 1 + .../modules/routing/maxinfo/maxinfo_http.cc | 1 + 15 files changed, 138 insertions(+), 107 deletions(-) diff --git a/include/maxscale/monitor.hh b/include/maxscale/monitor.hh index 5043648a0..8584c8775 100644 --- a/include/maxscale/monitor.hh +++ b/include/maxscale/monitor.hh @@ -37,7 +37,6 @@ class Monitor; struct DCB; struct json_t; struct EXTERNCMD; -class MonitorManager; /** * @verbatim @@ -302,6 +301,13 @@ public: static std::string get_server_monitor(const SERVER* server); + /** + * Is the current thread either the main thread or the runtime admin thread? + * + * @return True if running in an admin thread + */ + static bool is_admin_thread(); + /* * Convert a monitor event (enum) to string. * @@ -323,11 +329,29 @@ public: */ virtual bool configure(const MXS_CONFIG_PARAMETER* params); + /** + * Starts the monitor. If the monitor requires polling of the servers, it should create + * a separate monitoring thread. + * + * @return True, if the monitor could be started, false otherwise. + */ + virtual bool start() = 0; + /** * Stops the monitor. */ void stop(); + /** + * @brief The monitor should populate associated services. + */ + virtual void populate_services(); + + /** + * Deactivate the monitor. Stops the monitor and removes all servers. + */ + void deactivate(); + /** * Write diagnostic information to a DCB. * @@ -531,19 +555,10 @@ protected: Settings m_settings; private: - friend class ::MonitorManager; bool add_server(SERVER* server); void remove_all_servers(); - /** - * Starts the monitor. If the monitor requires polling of the servers, it should create - * a separate monitoring thread. - * - * @return True, if the monitor could be started, false otherwise. - */ - virtual bool start() = 0; - /** * Launch a script * @@ -564,11 +579,6 @@ private: */ int launch_command(MonitorServer* ptr, EXTERNCMD* cmd); - /** - * @brief The monitor should populate associated services. - */ - virtual void populate_services(); - FILE* open_data_file(Monitor* monitor, char* path); int get_data_file_path(char* path) const; diff --git a/server/core/config.cc b/server/core/config.cc index f3c443efb..68691c104 100644 --- a/server/core/config.cc +++ b/server/core/config.cc @@ -63,6 +63,7 @@ #include "internal/filter.hh" #include "internal/modules.hh" #include "internal/monitor.hh" +#include "internal/monitormanager.hh" #include "internal/server.hh" #include "internal/service.hh" diff --git a/server/core/config_runtime.cc b/server/core/config_runtime.cc index 3755b10f8..c9f819107 100644 --- a/server/core/config_runtime.cc +++ b/server/core/config_runtime.cc @@ -38,6 +38,7 @@ #include "internal/filter.hh" #include "internal/modules.hh" #include "internal/monitor.hh" +#include "internal/monitormanager.hh" #include "internal/query_classifier.hh" #include "internal/server.hh" diff --git a/server/core/gateway.cc b/server/core/gateway.cc index d0c4ab065..6f54a23ca 100644 --- a/server/core/gateway.cc +++ b/server/core/gateway.cc @@ -69,6 +69,7 @@ #include "internal/maxscale.hh" #include "internal/modules.hh" #include "internal/monitor.hh" +#include "internal/monitormanager.hh" #include "internal/poll.hh" #include "internal/service.hh" diff --git a/server/core/internal/monitor.hh b/server/core/internal/monitor.hh index 66624d706..93637af2c 100644 --- a/server/core/internal/monitor.hh +++ b/server/core/internal/monitor.hh @@ -17,9 +17,6 @@ */ #include -#include -#include "externcmd.hh" -#include "monitormanager.hh" #define MON_ARG_MAX 8192 @@ -49,28 +46,3 @@ static const MXS_ENUM_VALUE mxs_monitor_event_enum_values[] = {"new_donor", NEW_DONOR_EVENT}, {NULL} }; - -// 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/internal/monitormanager.hh b/server/core/internal/monitormanager.hh index 07fcc5734..d1c2e418d 100644 --- a/server/core/internal/monitormanager.hh +++ b/server/core/internal/monitormanager.hh @@ -14,6 +14,8 @@ #include +class ResultSet; + /** * This class contains internal monitor management functions that should not be exposed in the public * monitor class. It's a friend of MXS_MONITOR. @@ -65,7 +67,10 @@ public: static mxs::Monitor* find_monitor(const char* name); /** - * @brief Populate services with the servers of the monitors. + * Populate services with the servers of the monitors. Should be called at the end of configuration file + * processing to ensure that services are notified of the servers a monitor has. During runtime, the + * normal add/remove server functions do the notifying. TODO: If a service is created at runtime, is + * it properly populated? */ static void populate_services(); @@ -139,3 +144,28 @@ 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; +}; \ No newline at end of file diff --git a/server/core/modulecmd.cc b/server/core/modulecmd.cc index 4710e0f5d..e8a9e1945 100644 --- a/server/core/modulecmd.cc +++ b/server/core/modulecmd.cc @@ -22,6 +22,7 @@ #include "internal/filter.hh" #include "internal/modules.hh" #include "internal/monitor.hh" +#include "internal/monitormanager.hh" #include "internal/server.hh" /** Size of the error buffer */ diff --git a/server/core/monitor.cc b/server/core/monitor.cc index 08202ff11..4186f3839 100644 --- a/server/core/monitor.cc +++ b/server/core/monitor.cc @@ -87,17 +87,6 @@ const char CN_SCRIPT_TIMEOUT[] = "script_timeout"; namespace { -/** - * Is the current thread either the main thread or the runtime admin thread? - * - * @return True if running in an admin thread - */ -bool is_admin_thread() -{ - mxb::Worker* current = mxb::Worker::get_current(); - return (current == nullptr || current == mxs_rworker_get(MXS_RWORKER_MAIN)); -} - class ThisUnit { public: @@ -112,7 +101,7 @@ public: */ bool claim_server(const string& server, const string& new_owner, string* existing_owner) { - mxb_assert(is_admin_thread()); + mxb_assert(Monitor::is_admin_thread()); bool claim_success = false; auto iter = m_server_owners.find(server); if (iter != m_server_owners.end()) @@ -135,7 +124,7 @@ public: */ void release_server(const string& server) { - mxb_assert(is_admin_thread()); + mxb_assert(Monitor::is_admin_thread()); auto iter = m_server_owners.find(server); mxb_assert(iter != m_server_owners.end()); m_server_owners.erase(iter); @@ -144,7 +133,7 @@ public: string claimed_by(const string& server) { - mxb_assert(is_admin_thread()); + mxb_assert(Monitor::is_admin_thread()); string rval; auto iter = m_server_owners.find(server); if (iter != m_server_owners.end()) @@ -435,6 +424,36 @@ json_t* monitor_parameters_to_json(const Monitor* monitor) return rval; } +bool check_disk_space_exhausted(MonitorServer* pMs, + const std::string& path, + const maxscale::disk::SizesAndName& san, + int32_t max_percentage) +{ + bool disk_space_exhausted = false; + + int32_t used_percentage = ((san.total() - san.available()) / (double)san.total()) * 100; + + if (used_percentage >= max_percentage) + { + MXS_ERROR("Disk space on %s at %s is exhausted; %d%% of the the disk " + "mounted on the path %s has been used, and the limit it %d%%.", + pMs->server->name(), + pMs->server->address, + used_percentage, + path.c_str(), + max_percentage); + disk_space_exhausted = true; + } + + return disk_space_exhausted; +} + +const char ERR_CANNOT_MODIFY[] = + "The server is monitored, so only the maintenance status can be " + "set/cleared manually. Status was not modified."; +const char WRN_REQUEST_OVERWRITTEN[] = + "Previous maintenance request was not yet read by the monitor and was overwritten."; + } json_t* monitor_json_data(const Monitor* monitor, const char* host) @@ -1277,6 +1296,12 @@ string Monitor::get_server_monitor(const SERVER* server) return this_unit.claimed_by(server->name()); } +bool Monitor::is_admin_thread() +{ + mxb::Worker* current = mxb::Worker::get_current(); + return (current == nullptr || current == mxs_rworker_get(MXS_RWORKER_MAIN)); +} + /** * Log an error about the failure to connect to a backend server and why it happened. * @@ -1719,16 +1744,6 @@ bool Monitor::set_disk_space_threshold(const string& dst_setting) return rv; } -namespace -{ - -const char ERR_CANNOT_MODIFY[] = - "The server is monitored, so only the maintenance status can be " - "set/cleared manually. Status was not modified."; -const char WRN_REQUEST_OVERWRITTEN[] = - "Previous maintenance request was not yet read by the monitor and was overwritten."; -} - bool Monitor::set_server_status(SERVER* srv, int bit, string* errmsg_out) { MonitorServer* msrv = get_monitored_server(srv); @@ -1860,6 +1875,15 @@ void Monitor::populate_services() } } +void Monitor::deactivate() +{ + if (state() == MONITOR_STATE_RUNNING) + { + stop(); + } + remove_all_servers(); +} + bool Monitor::check_disk_space_this_tick() { bool should_update_disk_space = false; @@ -1989,34 +2013,6 @@ bool MonitorServer::can_update_disk_space_status() const return ok_to_check_disk_space && (!monitor_limits.empty() || server->have_disk_space_limits()); } -namespace -{ - -bool check_disk_space_exhausted(MonitorServer* pMs, - const std::string& path, - const maxscale::disk::SizesAndName& san, - int32_t max_percentage) -{ - bool disk_space_exhausted = false; - - int32_t used_percentage = ((san.total() - san.available()) / (double)san.total()) * 100; - - if (used_percentage >= max_percentage) - { - MXS_ERROR("Disk space on %s at %s is exhausted; %d%% of the the disk " - "mounted on the path %s has been used, and the limit it %d%%.", - pMs->server->name(), - pMs->server->address, - used_percentage, - path.c_str(), - max_percentage); - disk_space_exhausted = true; - } - - return disk_space_exhausted; -} -} - void MonitorServer::update_disk_space_status() { auto pMs = this; // TODO: Clean @@ -2327,7 +2323,6 @@ bool MonitorWorker::immediate_tick_required() const { return false; } -} MonitorServer::MonitorServer(SERVER* server, const SERVER::DiskSpaceLimits& monitor_limits) : server(server) @@ -2342,3 +2337,5 @@ MonitorServer::~MonitorServer() mysql_close(con); } } + +} diff --git a/server/core/monitormanager.cc b/server/core/monitormanager.cc index ab7f70046..fbc3acad4 100644 --- a/server/core/monitormanager.cc +++ b/server/core/monitormanager.cc @@ -15,10 +15,13 @@ #include #include +#include #include "internal/config.hh" #include "internal/monitor.hh" +#include "internal/monitormanager.hh" #include "internal/modules.hh" +#include "internal/externcmd.hh" using maxscale::Monitor; using maxscale::MonitorServer; @@ -88,6 +91,7 @@ ThisUnit this_unit; Monitor* MonitorManager::create_monitor(const string& name, const string& module, MXS_CONFIG_PARAMETER* params) { + mxb_assert(Monitor::is_admin_thread()); MXS_MONITOR_API* api = (MXS_MONITOR_API*)load_module(module.c_str(), MODULE_MONITOR); if (!api) { @@ -117,6 +121,7 @@ Monitor* MonitorManager::create_monitor(const string& name, const string& module void MonitorManager::debug_wait_one_tick() { + mxb_assert(Monitor::is_admin_thread()); using namespace std::chrono; std::map ticks; @@ -148,6 +153,8 @@ void MonitorManager::debug_wait_one_tick() void MonitorManager::destroy_all_monitors() { + mxb_assert(Monitor::is_admin_thread()); + auto monitors = this_unit.clear(); for (auto monitor : monitors) { @@ -158,9 +165,7 @@ void MonitorManager::destroy_all_monitors() void MonitorManager::start_monitor(Monitor* monitor) { - mxb_assert(monitor); - - Guard guard(monitor->m_lock); + mxb_assert(Monitor::is_admin_thread()); // Only start the monitor if it's stopped. if (monitor->state() == MONITOR_STATE_STOPPED) @@ -174,6 +179,7 @@ void MonitorManager::start_monitor(Monitor* monitor) void MonitorManager::populate_services() { + mxb_assert(Monitor::is_admin_thread()); this_unit.foreach_monitor([](Monitor* pMonitor) -> bool { pMonitor->populate_services(); return true; @@ -185,6 +191,7 @@ void MonitorManager::populate_services() */ void MonitorManager::start_all_monitors() { + mxb_assert(Monitor::is_admin_thread()); this_unit.foreach_monitor([](Monitor* monitor) { MonitorManager::start_monitor(monitor); return true; @@ -193,9 +200,7 @@ void MonitorManager::start_all_monitors() void MonitorManager::stop_monitor(Monitor* monitor) { - mxb_assert(monitor); - - Guard guard(monitor->m_lock); + mxb_assert(Monitor::is_admin_thread()); /** Only stop the monitor if it is running */ if (monitor->state() == MONITOR_STATE_RUNNING) @@ -206,11 +211,10 @@ void MonitorManager::stop_monitor(Monitor* monitor) void MonitorManager::deactivate_monitor(Monitor* monitor) { + mxb_assert(Monitor::is_admin_thread()); // This cannot be done with configure(), since other, module-specific config settings may depend on the - // "servers"-setting of the base monitor. Directly manipulate monitor field for now, later use a dtor - // to cleanly "deactivate" inherited objects. - stop_monitor(monitor); - monitor->remove_all_servers(); + // "servers"-setting of the base monitor. + monitor->deactivate(); this_unit.move_to_deactivated_list(monitor); } @@ -219,6 +223,7 @@ void MonitorManager::deactivate_monitor(Monitor* monitor) */ void MonitorManager::stop_all_monitors() { + mxb_assert(Monitor::is_admin_thread()); this_unit.foreach_monitor([](Monitor* monitor) { MonitorManager::stop_monitor(monitor); return true; @@ -232,6 +237,7 @@ void MonitorManager::stop_all_monitors() */ void MonitorManager::show_all_monitors(DCB* dcb) { + mxb_assert(Monitor::is_admin_thread()); this_unit.foreach_monitor([dcb](Monitor* monitor) { monitor_show(dcb, monitor); return true; @@ -245,6 +251,7 @@ void MonitorManager::show_all_monitors(DCB* dcb) */ void MonitorManager::monitor_show(DCB* dcb, Monitor* monitor) { + mxb_assert(Monitor::is_admin_thread()); monitor->show(dcb); } @@ -255,6 +262,7 @@ void MonitorManager::monitor_show(DCB* dcb, Monitor* monitor) */ void MonitorManager::monitor_list(DCB* dcb) { + mxb_assert(Monitor::is_admin_thread()); dcb_printf(dcb, "---------------------+---------------------\n"); dcb_printf(dcb, "%-20s | Status\n", "Monitor"); dcb_printf(dcb, "---------------------+---------------------\n"); @@ -294,6 +302,7 @@ Monitor* MonitorManager::find_monitor(const char* name) */ std::unique_ptr MonitorManager::monitor_get_list() { + mxb_assert(Monitor::is_admin_thread()); std::unique_ptr set = ResultSet::create({"Monitor", "Status"}); this_unit.foreach_monitor([&set](Monitor* ptr) { const char* state = ptr->state() == MONITOR_STATE_RUNNING ? "Running" : "Stopped"; @@ -317,6 +326,7 @@ Monitor* MonitorManager::server_is_monitored(const SERVER* server) bool MonitorManager::create_monitor_config(const Monitor* monitor, const char* filename) { + mxb_assert(Monitor::is_admin_thread()); int file = open(filename, O_EXCL | O_CREAT | O_WRONLY, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH); if (file == -1) { @@ -350,6 +360,7 @@ bool MonitorManager::create_monitor_config(const Monitor* monitor, const char* f bool MonitorManager::monitor_serialize(const Monitor* monitor) { + mxb_assert(Monitor::is_admin_thread()); bool rval = false; char filename[PATH_MAX]; snprintf(filename, @@ -393,6 +404,7 @@ bool MonitorManager::monitor_serialize(const Monitor* monitor) // 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(); diff --git a/server/core/resource.cc b/server/core/resource.cc index e1a5baa0b..ca67a4d9c 100644 --- a/server/core/resource.cc +++ b/server/core/resource.cc @@ -31,6 +31,7 @@ #include "internal/httpresponse.hh" #include "internal/modules.hh" #include "internal/monitor.hh" +#include "internal/monitormanager.hh" #include "internal/query_classifier.hh" #include "internal/server.hh" #include "internal/service.hh" diff --git a/server/core/server.cc b/server/core/server.cc index b35af0b22..8cd2a3285 100644 --- a/server/core/server.cc +++ b/server/core/server.cc @@ -45,6 +45,7 @@ #include #include "internal/monitor.hh" +#include "internal/monitormanager.hh" #include "internal/poll.hh" #include "internal/config.hh" #include "internal/service.hh" diff --git a/server/core/test/test_modulecmd.cc b/server/core/test/test_modulecmd.cc index 3f687c33e..712b339aa 100644 --- a/server/core/test/test_modulecmd.cc +++ b/server/core/test/test_modulecmd.cc @@ -24,6 +24,7 @@ #include #include "../internal/monitor.hh" +#include "../internal/monitormanager.hh" #define TEST(a, b) do {if (!(a)) {printf("%s:%d " b "\n", __FILE__, __LINE__); return 1;}} while (false) diff --git a/server/modules/routing/cli/debugcmd.cc b/server/modules/routing/cli/debugcmd.cc index 713e2ce9f..d26cf90c4 100644 --- a/server/modules/routing/cli/debugcmd.cc +++ b/server/modules/routing/cli/debugcmd.cc @@ -60,6 +60,7 @@ #include "../../../core/internal/maxscale.hh" #include "../../../core/internal/modules.hh" #include "../../../core/internal/monitor.hh" +#include "../../../core/internal/monitormanager.hh" #include "../../../core/internal/poll.hh" #include "../../../core/internal/session.hh" #include "../../../core/internal/server.hh" diff --git a/server/modules/routing/maxinfo/maxinfo_exec.cc b/server/modules/routing/maxinfo/maxinfo_exec.cc index 0fbdbc2d0..b15bc0fd8 100644 --- a/server/modules/routing/maxinfo/maxinfo_exec.cc +++ b/server/modules/routing/maxinfo/maxinfo_exec.cc @@ -47,6 +47,7 @@ #include "../../../core/internal/maxscale.hh" #include "../../../core/internal/modules.hh" #include "../../../core/internal/monitor.hh" +#include "../../../core/internal/monitormanager.hh" #include "../../../core/internal/poll.hh" #include "../../../core/internal/session.hh" #include "../../../core/internal/server.hh" diff --git a/server/modules/routing/maxinfo/maxinfo_http.cc b/server/modules/routing/maxinfo/maxinfo_http.cc index c078e907f..691e830b8 100644 --- a/server/modules/routing/maxinfo/maxinfo_http.cc +++ b/server/modules/routing/maxinfo/maxinfo_http.cc @@ -21,6 +21,7 @@ #include "../../../core/internal/poll.hh" #include "../../../core/internal/monitor.hh" +#include "../../../core/internal/monitormanager.hh" #include "../../../core/internal/server.hh" #include "../../../core/internal/service.hh" #include "../../../core/internal/modules.hh"