diff --git a/include/maxscale/monitor.hh b/include/maxscale/monitor.hh index 28c85141b..3c35dff8a 100644 --- a/include/maxscale/monitor.hh +++ b/include/maxscale/monitor.hh @@ -280,9 +280,6 @@ public: Monitor(const std::string& name, const std::string& module); virtual ~Monitor(); - static const int STATUS_FLAG_NOCHECK = 0; - static const int STATUS_FLAG_CHECK = -1; - /** * Ping or connect to a database. If connection does not exist or ping fails, a new connection * is created. This will always leave a valid database handle in @c *ppCon, allowing the user @@ -329,6 +326,15 @@ public: */ virtual bool configure(const MXS_CONFIG_PARAMETER* params); + /** + * Get text-form settings. + * + * @return Monitor configuration parameters + */ + const MXS_CONFIG_PARAMETER& parameters() const; + + int64_t ticks() const; + /** * Starts the monitor. If the monitor requires polling of the servers, it should create * a separate monitoring thread. @@ -416,14 +422,7 @@ public: mutable std::mutex m_lock; - /** Set when admin requests a maintenance status change. */ - int check_status_flag = STATUS_FLAG_NOCHECK; - - uint64_t m_ticks {0}; /**< Number of performed monitoring intervals */ - uint8_t m_journal_hash[SHA_DIGEST_LENGTH]; /**< SHA1 hash of the latest written journal */ - - MXS_CONFIG_PARAMETER parameters; /**< Configuration parameters */ - std::vector m_servers; /**< Monitored servers */ + std::vector m_servers; /**< Monitored servers */ protected: /** @@ -529,6 +528,8 @@ protected: */ bool check_disk_space_this_tick(); + bool server_status_request_waiting() const; + /** * 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. @@ -552,7 +553,10 @@ protected: MonitorServer::ConnectionSettings conn_settings; }; - Settings m_settings; + const Settings& settings() const; + + /**< Number of monitor ticks ran. Derived classes should increment this whenever completing a tick. */ + std::atomic_int64_t m_ticks {0}; private: @@ -582,7 +586,12 @@ private: FILE* open_data_file(Monitor* monitor, char* path); int get_data_file_path(char* path) const; - mxb::StopWatch m_disk_space_checked; /**< When was disk space checked the last time */ + mxb::StopWatch m_disk_space_checked; /**< When was disk space checked the last time */ + std::atomic_bool m_status_change_pending {false}; /**< Set when admin requests a status change. */ + uint8_t m_journal_hash[SHA_DIGEST_LENGTH]; /**< SHA1 hash of the latest written journal */ + + MXS_CONFIG_PARAMETER m_parameters; /**< Configuration parameters in text form */ + Settings m_settings; /**< Base class settings */ }; /** diff --git a/server/core/config_runtime.cc b/server/core/config_runtime.cc index 4860d37f7..9561e8722 100644 --- a/server/core/config_runtime.cc +++ b/server/core/config_runtime.cc @@ -2505,7 +2505,7 @@ bool runtime_alter_monitor_from_json(Monitor* monitor, json_t* new_json) mxb_assert(old_json.get()); const MXS_MODULE* mod = get_module(monitor->m_module.c_str(), MODULE_MONITOR); - auto params = monitor->parameters; + auto params = monitor->parameters(); params.set_multiple(extract_parameters(new_json)); if (is_valid_resource_body(new_json) diff --git a/server/core/monitor.cc b/server/core/monitor.cc index 5984032b4..e35c6e0dd 100644 --- a/server/core/monitor.cc +++ b/server/core/monitor.cc @@ -16,6 +16,7 @@ */ #include +#include #include #include #include @@ -30,7 +31,6 @@ #include #include -#include #include #include #include @@ -416,7 +416,8 @@ json_t* monitor_parameters_to_json(const Monitor* monitor) { json_t* rval = json_object(); const MXS_MODULE* mod = get_module(monitor->m_module.c_str(), MODULE_MONITOR); - config_add_module_params_json(&monitor->parameters, + auto mon_config = monitor->parameters(); + config_add_module_params_json(&mon_config, {CN_TYPE, CN_MODULE, CN_SERVERS}, config_monitor_params, mod->parameters, @@ -468,7 +469,7 @@ json_t* monitor_json_data(const Monitor* monitor, const char* host) json_object_set_new(attr, CN_MODULE, json_string(monitor->m_module.c_str())); json_object_set_new(attr, CN_STATE, json_string(monitor_state_to_string(monitor->state()))); - json_object_set_new(attr, CN_TICKS, json_integer(monitor->m_ticks)); + json_object_set_new(attr, CN_TICKS, json_integer(monitor->ticks())); /** Monitor parameters */ json_object_set_new(attr, CN_PARAMETERS, monitor_parameters_to_json(monitor)); @@ -578,13 +579,28 @@ bool Monitor::configure(const MXS_CONFIG_PARAMETER* params) if (!error) { // Store module name into parameter storage. - parameters.set(CN_MODULE, m_module); + m_parameters.set(CN_MODULE, m_module); // Add all config settings to text-mode storage. Needed for serialization. - parameters.set_multiple(*params); + m_parameters.set_multiple(*params); } return !error; } +const MXS_CONFIG_PARAMETER& Monitor::parameters() const +{ + return m_parameters; +} + +const Monitor::Settings& Monitor::settings() const +{ + return m_settings; +} + +int64_t Monitor::ticks() const +{ + return m_ticks.load(std::memory_order_acquire); +} + Monitor::~Monitor() { for (auto server : m_servers) @@ -653,7 +669,7 @@ void Monitor::show(DCB* dcb) { dcb_printf(dcb, "Name: %s\n", name()); dcb_printf(dcb, "State: %s\n", monitor_state_to_string(state())); - dcb_printf(dcb, "Times monitored: %lu\n", m_ticks); + dcb_printf(dcb, "Times monitored: %lu\n", ticks()); dcb_printf(dcb, "Sampling interval: %lu milliseconds\n", m_settings.interval); dcb_printf(dcb, "Connect Timeout: %i seconds\n", m_settings.conn_settings.connect_timeout); dcb_printf(dcb, "Read Timeout: %i seconds\n", m_settings.conn_settings.read_timeout); @@ -1356,19 +1372,17 @@ void Monitor::check_maintenance_requests() { /* In theory, the admin may be modifying the server maintenance status during this function. The overall * maintenance flag should be read-written atomically to prevent missing a value. */ - int flags_changed = atomic_exchange_int(&check_status_flag, Monitor::STATUS_FLAG_NOCHECK); - if (flags_changed != Monitor::STATUS_FLAG_NOCHECK) + bool was_pending = m_status_change_pending.exchange(false, std::memory_order_acq_rel); + if (was_pending) { for (auto ptr : m_servers) { - // The only server status bit the admin may change is the [Maintenance] bit. - int admin_msg = atomic_exchange_int(&ptr->status_request, - MonitorServer::NO_CHANGE); + // The admin can only modify the [Maintenance] and [Drain] bits. + int admin_msg = atomic_exchange_int(&ptr->status_request, MonitorServer::NO_CHANGE); switch (admin_msg) { case MonitorServer::MAINT_ON: - // TODO: Change to writing MONITORED_SERVER->pending status instead once cleanup done. ptr->server->set_status(SERVER_MAINT); break; @@ -1692,13 +1706,13 @@ std::vector Monitor::get_monitored_serverlist(const string& key, { std::vector monitored_array; // Check that value exists. - if (!parameters.contains(key)) + if (!m_parameters.contains(key)) { return monitored_array; } string name_error; - auto servers = parameters.get_server_list(key, &name_error); + auto servers = m_parameters.get_server_list(key, &name_error); if (!servers.empty()) { // All servers in the array must be monitored by the given monitor. @@ -1793,7 +1807,7 @@ bool Monitor::set_server_status(SERVER* srv, int bit, string* errmsg_out) MXS_WARNING(WRN_REQUEST_OVERWRITTEN); } // Also set a flag so the next loop happens sooner. - atomic_store_int(&this->check_status_flag, Monitor::STATUS_FLAG_CHECK); + m_status_change_pending.store(true, std::memory_order_release); } } else @@ -1851,7 +1865,7 @@ bool Monitor::clear_server_status(SERVER* srv, int bit, string* errmsg_out) MXS_WARNING(WRN_REQUEST_OVERWRITTEN); } // Also set a flag so the next loop happens sooner. - atomic_store_int(&this->check_status_flag, Monitor::STATUS_FLAG_CHECK); + m_status_change_pending.store(true, std::memory_order_release); } } else @@ -1898,6 +1912,11 @@ bool Monitor::check_disk_space_this_tick() return should_update_disk_space; } +bool Monitor::server_status_request_waiting() const +{ + return m_status_change_pending.load(std::memory_order_acquire); +} + MonitorWorker::MonitorWorker(const string& name, const string& module) : Monitor(name, module) , m_thread_running(false) @@ -1973,7 +1992,7 @@ bool MonitorWorker::start() bool started = false; if (m_checked) { - m_loop_called = get_time_ms() - m_settings.interval; // Next tick should happen immediately. + m_loop_called = get_time_ms() - settings().interval; // Next tick should happen immediately. if (!Worker::start()) { MXS_ERROR("Failed to start worker for monitor '%s'.", name()); @@ -2168,7 +2187,7 @@ void MonitorWorkerSimple::tick() pMs->mon_prev_status = pMs->server->status; pMs->pending_status = pMs->server->status; - mxs_connect_result_t rval = pMs->ping_or_connect(m_settings.conn_settings); + mxs_connect_result_t rval = pMs->ping_or_connect(settings().conn_settings); if (connection_is_ok(rval)) { @@ -2289,9 +2308,9 @@ bool MonitorWorker::call_run_one_tick(Worker::Call::action_t action) { int64_t now = get_time_ms(); // Enough time has passed, - if ((now - m_loop_called > m_settings.interval) - // or maintenance flag is set, - || atomic_load_int(&this->check_status_flag) == Monitor::STATUS_FLAG_CHECK + if ((now - m_loop_called > settings().interval) + // or a server status change request is waiting, + || server_status_request_waiting() // or a monitor-specific condition is met. || immediate_tick_required()) { @@ -2300,7 +2319,7 @@ bool MonitorWorker::call_run_one_tick(Worker::Call::action_t action) now = get_time_ms(); } - int64_t ms_to_next_call = m_settings.interval - (now - m_loop_called); + int64_t ms_to_next_call = settings().interval - (now - m_loop_called); // ms_to_next_call will be negative, if the run_one_tick() call took // longer than one monitor interval. int64_t delay = ((ms_to_next_call <= 0) || (ms_to_next_call >= base_interval_ms)) ? @@ -2314,7 +2333,7 @@ bool MonitorWorker::call_run_one_tick(Worker::Call::action_t action) void MonitorWorker::run_one_tick() { tick(); - mxb::atomic::add(&m_ticks, 1, mxb::atomic::RELAXED); + m_ticks.fetch_add(1, std::memory_order_acq_rel); } bool MonitorWorker::immediate_tick_required() const diff --git a/server/core/monitormanager.cc b/server/core/monitormanager.cc index b358a226d..a9d6bb847 100644 --- a/server/core/monitormanager.cc +++ b/server/core/monitormanager.cc @@ -127,34 +127,32 @@ void MonitorManager::debug_wait_one_tick() { mxb_assert(Monitor::is_admin_thread()); using namespace std::chrono; - std::map ticks; + std::map ticks; // Get tick values for all monitors this_unit.foreach_monitor([&ticks](Monitor* mon) { - ticks[mon] = mxb::atomic::load(&mon->m_ticks); - return true; - }); + ticks[mon] = mon->ticks(); + return true; + }); // Wait for all running monitors to advance at least one tick. this_unit.foreach_monitor([&ticks](Monitor* mon) { - if (mon->state() == MONITOR_STATE_RUNNING) - { - auto start = steady_clock::now(); - // A monitor may have been added in between the two foreach-calls (not - // if config changes are - // serialized). Check if entry exists. - if (ticks.count(mon) > 0) - { - auto tick = ticks[mon]; - while (mxb::atomic::load(&mon->m_ticks) == tick - && (steady_clock::now() - start < seconds(60))) - { - std::this_thread::sleep_for(milliseconds(100)); - } - } - } - return true; - }); + if (mon->state() == MONITOR_STATE_RUNNING) + { + auto start = steady_clock::now(); + // A monitor may have been added in between the two foreach-calls (not if config changes are + // serialized). Check if entry exists. + if (ticks.count(mon) > 0) + { + auto tick = ticks[mon]; + while (mon->ticks() == tick && (steady_clock::now() - start < seconds(60))) + { + std::this_thread::sleep_for(milliseconds(100)); + } + } + } + return true; + }); } void MonitorManager::destroy_all_monitors() @@ -351,7 +349,7 @@ bool MonitorManager::create_monitor_config(const Monitor* monitor, const char* f const MXS_MODULE* mod = get_module(monitor->m_module.c_str(), NULL); mxb_assert(mod); - string config = generate_config_string(monitor->m_name, monitor->parameters, + string config = generate_config_string(monitor->m_name, monitor->parameters(), config_monitor_params, mod->parameters); if (dprintf(file, "%s", config.c_str()) == -1) @@ -412,7 +410,7 @@ bool MonitorManager::reconfigure_monitor(mxs::Monitor* monitor, const MXS_CONFIG { mxb_assert(Monitor::is_admin_thread()); // Backup monitor parameters in case configure fails. - auto orig = monitor->parameters; + auto orig = monitor->parameters(); // 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); @@ -451,7 +449,7 @@ bool MonitorManager::alter_monitor(mxs::Monitor* monitor, const std::string& key return false; } - MXS_CONFIG_PARAMETER modified = monitor->parameters; + MXS_CONFIG_PARAMETER modified = monitor->parameters(); modified.set(key, value); bool success = MonitorManager::reconfigure_monitor(monitor, modified); @@ -570,7 +568,7 @@ bool MonitorManager::add_server_to_monitor(mxs::Monitor* mon, SERVER* server, st // 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; + MXS_CONFIG_PARAMETER modified_params = mon->parameters(); string serverlist = modified_params.get_string(CN_SERVERS); if (serverlist.empty()) { @@ -615,7 +613,7 @@ bool MonitorManager::remove_server_from_monitor(mxs::Monitor* mon, SERVER* serve else { // Construct the new server list - auto params = mon->parameters; + 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, ","); diff --git a/server/modules/monitor/clustrixmon/clustrixmonitor.cc b/server/modules/monitor/clustrixmon/clustrixmonitor.cc index 5c399ff00..3679ff303 100644 --- a/server/modules/monitor/clustrixmon/clustrixmonitor.cc +++ b/server/modules/monitor/clustrixmon/clustrixmonitor.cc @@ -375,7 +375,7 @@ bool ClustrixMonitor::choose_dynamic_hub(Clustrix::Softfailed softfailed, std::s { ClustrixNode& node = kv.second; - if (node.can_be_used_as_hub(name(), m_settings.conn_settings, softfailed)) + if (node.can_be_used_as_hub(name(), settings().conn_settings, softfailed)) { m_pHub_con = node.release_connection(); m_pHub_server = node.server(); @@ -398,7 +398,7 @@ bool ClustrixMonitor::choose_bootstrap_hub(Clustrix::Softfailed softfailed, std: { if (ips_checked.find(pMs->server->address) == ips_checked.end()) { - if (Clustrix::ping_or_connect_to_hub(name(), m_settings.conn_settings, softfailed, *pMs)) + if (Clustrix::ping_or_connect_to_hub(name(), settings().conn_settings, softfailed, *pMs)) { m_pHub_con = pMs->con; m_pHub_server = pMs->server; @@ -433,8 +433,8 @@ bool ClustrixMonitor::refresh_using_persisted_nodes(std::set& ips_checke if (rv == SQLITE_OK) { - const std::string& username = m_settings.conn_settings.username; - const std::string& password = m_settings.conn_settings.password; + const std::string& username = settings().conn_settings.username; + const std::string& password = settings().conn_settings.password; char* zPassword = decrypt_password(password.c_str()); auto it = nodes.begin(); @@ -806,7 +806,7 @@ void ClustrixMonitor::check_hub(Clustrix::Softfailed softfailed) mxb_assert(m_pHub_con); mxb_assert(m_pHub_server); - if (!Clustrix::ping_or_connect_to_hub(name(), m_settings.conn_settings, softfailed, + if (!Clustrix::ping_or_connect_to_hub(name(), settings().conn_settings, softfailed, *m_pHub_server, &m_pHub_con)) { mysql_close(m_pHub_con); @@ -998,7 +998,7 @@ void ClustrixMonitor::initiate_delayed_http_check() { mxb_assert(m_delayed_http_check_id == 0); - long max_delay_ms = m_settings.interval / 10; + long max_delay_ms = settings().interval / 10; long ms = m_http.wait_no_more_than(); diff --git a/server/modules/monitor/mariadbmon/mariadbmon.cc b/server/modules/monitor/mariadbmon/mariadbmon.cc index 9e5ae7626..c2c83da0e 100644 --- a/server/modules/monitor/mariadbmon/mariadbmon.cc +++ b/server/modules/monitor/mariadbmon/mariadbmon.cc @@ -182,8 +182,8 @@ bool MariaDBMonitor::set_replication_credentials(const MXS_CONFIG_PARAMETER* par else { // Ok, neither is set. Use monitor credentials. - repl_user = m_settings.conn_settings.username; - repl_pw = m_settings.conn_settings.password; + repl_user = settings().conn_settings.username; + repl_pw = settings().conn_settings.password; } } @@ -395,7 +395,7 @@ void MariaDBMonitor::tick() // Query all servers for their status. bool should_update_disk_space = check_disk_space_this_tick(); - const auto& conn_settings = m_settings.conn_settings; + const auto& conn_settings = settings().conn_settings; auto update_task = [should_update_disk_space, conn_settings](MariaDBServer* server) { server->update_server(should_update_disk_space, conn_settings); diff --git a/server/modules/monitor/mariadbmon/test/test_cycle_find.cc b/server/modules/monitor/mariadbmon/test/test_cycle_find.cc index 15ce47055..3d01b1133 100644 --- a/server/modules/monitor/mariadbmon/test/test_cycle_find.cc +++ b/server/modules/monitor/mariadbmon/test/test_cycle_find.cc @@ -159,7 +159,7 @@ void MariaDBMonitor::Test::init_servers(int count) { // Server contents mostly undefined auto base_server = Server::create_test_server(); - MonitorServer* mon_server = new MonitorServer(base_server, m_monitor->m_settings.disk_space_limits); + MonitorServer* mon_server = new MonitorServer(base_server, m_monitor->settings().disk_space_limits); MariaDBServer* mariadb_server = new MariaDBServer(mon_server, i - 1, m_use_hostnames, true); if (m_use_hostnames)