diff --git a/include/maxscale/monitor.h b/include/maxscale/monitor.h index eece36a96..c7d1281ed 100644 --- a/include/maxscale/monitor.h +++ b/include/maxscale/monitor.h @@ -259,9 +259,7 @@ struct mxs_monitor char *module_name; /**< Name of the monitor module */ MXS_MONITOR_INSTANCE *instance; /**< Instance returned from startMonitor */ size_t interval; /**< The monitor interval */ - volatile bool server_pending_changes; - /**< Are there any pending changes to a server? - * If yes, the next monitor loop starts early. */ + int check_maintenance_flag; /**< Set when admin requests a maintenance status change. */ bool active; /**< True if monitor is active */ time_t journal_max_age; /**< Maximum age of journal file */ uint32_t script_timeout; /**< Timeout in seconds for the monitor scripts */ @@ -322,8 +320,7 @@ bool check_monitor_permissions(MXS_MONITOR* monitor, const char* query); void monitor_clear_pending_status(MXS_MONITORED_SERVER *ptr, uint64_t bit); void monitor_set_pending_status(MXS_MONITORED_SERVER *ptr, uint64_t bit); -void servers_status_pending_to_current(MXS_MONITOR *monitor); -void servers_status_current_to_pending(MXS_MONITOR *monitor); +void monitor_check_maintenance_requests(MXS_MONITOR *monitor); bool mon_status_changed(MXS_MONITORED_SERVER* mon_srv); bool mon_print_fail_status(MXS_MONITORED_SERVER* mon_srv); @@ -333,9 +330,6 @@ bool mon_connection_is_ok(mxs_connect_result_t connect_result); void mon_log_connect_error(MXS_MONITORED_SERVER* database, mxs_connect_result_t rval); const char* mon_get_event_name(mxs_monitor_event_t event); -void lock_monitor_servers(MXS_MONITOR *monitor); -void release_monitor_servers(MXS_MONITOR *monitor); - /** * Alter monitor parameters * diff --git a/include/maxscale/server.h b/include/maxscale/server.h index 5ae3d5fa9..13a5f2b56 100644 --- a/include/maxscale/server.h +++ b/include/maxscale/server.h @@ -48,7 +48,8 @@ extern const char CN_PROXY_PROTOCOL[]; const int MAINTENANCE_OFF = -100; const int MAINTENANCE_NO_CHANGE = 0; const int MAINTENANCE_ON = 100; - +const int MAINTENANCE_FLAG_NOCHECK = 0; +const int MAINTENANCE_FLAG_CHECK = -1; /** * The server parameters used for weighting routing decissions */ diff --git a/server/core/monitor.cc b/server/core/monitor.cc index b32a5fedd..6b0b673f9 100644 --- a/server/core/monitor.cc +++ b/server/core/monitor.cc @@ -136,7 +136,7 @@ MXS_MONITOR* monitor_create(const char *name, const char *module) mon->journal_max_age = DEFAULT_JOURNAL_MAX_AGE; mon->script_timeout = DEFAULT_SCRIPT_TIMEOUT; mon->parameters = NULL; - mon->server_pending_changes = false; + mon->check_maintenance_flag = MAINTENANCE_FLAG_NOCHECK; memset(mon->journal_hash, 0, sizeof(mon->journal_hash)); mon->disk_space_threshold = NULL; mon->disk_space_check_interval = 0; @@ -1712,73 +1712,35 @@ void mon_report_query_error(MXS_MONITORED_SERVER* db) db->server->port, mysql_error(db->con)); } -/** - * Acquire locks on all servers monitored by this monitor. There should - * only be max 1 monitor per server. - * @param monitor The target monitor - */ -void lock_monitor_servers(MXS_MONITOR *monitor) -{ - MXS_MONITORED_SERVER *ptr = monitor->monitored_servers; - while (ptr) - { - spinlock_acquire(&ptr->server->lock); - ptr = ptr->next; - } -} -/** - * Release locks on all servers monitored by this monitor. There should - * only be max 1 monitor per server. - * @param monitor The target monitor - */ -void release_monitor_servers(MXS_MONITOR *monitor) -{ - MXS_MONITORED_SERVER *ptr = monitor->monitored_servers; - while (ptr) - { - spinlock_release(&ptr->server->lock); - ptr = ptr->next; - } -} /** * Check if admin is requesting setting or clearing maintenance status on the server and act accordingly. * Should be called at the beginning of a monitor loop. * * @param monitor The target monitor */ -void servers_status_pending_to_current(MXS_MONITOR *monitor) +void monitor_check_maintenance_requests(MXS_MONITOR *monitor) { - MXS_MONITORED_SERVER *ptr = monitor->monitored_servers; - while (ptr) + /* 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(&monitor->check_maintenance_flag, MAINTENANCE_FLAG_NOCHECK); + if (flags_changed != MAINTENANCE_FLAG_NOCHECK) { - // The only server status bit the admin may change is the [Maintenance] bit. - int admin_msg = atomic_exchange_int(&ptr->server->maint_request, MAINTENANCE_NO_CHANGE); - if (admin_msg == MAINTENANCE_ON) + MXS_MONITORED_SERVER *ptr = monitor->monitored_servers; + while (ptr) { - // TODO: Change to writing MONITORED_SERVER->pending status instead once cleanup done. - server_set_status_nolock(ptr->server, SERVER_MAINT); + // The only server status bit the admin may change is the [Maintenance] bit. + int admin_msg = atomic_exchange_int(&ptr->server->maint_request, MAINTENANCE_NO_CHANGE); + if (admin_msg == MAINTENANCE_ON) + { + // TODO: Change to writing MONITORED_SERVER->pending status instead once cleanup done. + server_set_status_nolock(ptr->server, SERVER_MAINT); + } + else if (admin_msg == MAINTENANCE_OFF) + { + server_clear_status_nolock(ptr->server, SERVER_MAINT); + } + ptr = ptr->next; } - else if (admin_msg == MAINTENANCE_OFF) - { - server_clear_status_nolock(ptr->server, SERVER_MAINT); - } - ptr = ptr->next; - } - monitor->server_pending_changes = false; -} - -/** - * Sets the pending status of all servers monitored by this monitor to - * the current status. This should only be called at the end of - * a monitor loop, before the servers are released. - * @param monitor The target monitor - */ -void servers_status_current_to_pending(MXS_MONITOR *monitor) -{ - MXS_MONITORED_SERVER *ptr = monitor->monitored_servers; - while (ptr) - { - ptr = ptr->next; } } @@ -2778,8 +2740,7 @@ void MonitorInstance::main() while (!m_shutdown) { - lock_monitor_servers(m_monitor); - servers_status_pending_to_current(m_monitor); + monitor_check_maintenance_requests(m_monitor); tick(); @@ -2790,15 +2751,13 @@ void MonitorInstance::main() mon_process_state_changes(m_monitor, m_script.empty() ? NULL : m_script.c_str(), m_events); mon_hangup_failed_servers(m_monitor); - servers_status_current_to_pending(m_monitor); store_server_journal(m_monitor, m_master); - release_monitor_servers(m_monitor); /** Sleep until the next monitoring interval */ unsigned int ms = 0; while (ms < m_monitor->interval && !m_shutdown) { - if (m_monitor->server_pending_changes) + if (atomic_load_int(&m_monitor->check_maintenance_flag) != MAINTENANCE_FLAG_NOCHECK) { // Admin has changed something, skip sleep break; diff --git a/server/core/server.cc b/server/core/server.cc index 96124f3a2..72fbabe1f 100644 --- a/server/core/server.cc +++ b/server/core/server.cc @@ -1342,7 +1342,7 @@ bool mxs::server_set_status(SERVER *server, int bit, string* errmsg_out) { MXS_WARNING(WRN_REQUEST_OVERWRITTEN); } - mon->server_pending_changes = true; + atomic_store_int(&mon->check_maintenance_flag, MAINTENANCE_FLAG_CHECK); } } else @@ -1387,7 +1387,7 @@ bool mxs::server_clear_status(SERVER *server, int bit, string* errmsg_out) { MXS_WARNING(WRN_REQUEST_OVERWRITTEN); } - mon->server_pending_changes = true; + atomic_store_int(&mon->check_maintenance_flag, MAINTENANCE_FLAG_CHECK); } } else diff --git a/server/modules/monitor/mariadbmon/mariadbmon.cc b/server/modules/monitor/mariadbmon/mariadbmon.cc index 454ca5b58..89ffeaed6 100644 --- a/server/modules/monitor/mariadbmon/mariadbmon.cc +++ b/server/modules/monitor/mariadbmon/mariadbmon.cc @@ -308,12 +308,8 @@ void MariaDBMonitor::main() /* Coarse time has resolution ~1ms (as opposed to 1ns) but this is enough. */ clock_gettime(CLOCK_MONOTONIC_COARSE, &loop_start); - lock_monitor_servers(m_monitor); /* Read any admin status changes from SERVER->status_pending. */ - if (m_monitor->server_pending_changes) - { - servers_status_pending_to_current(m_monitor); - } + monitor_check_maintenance_requests(m_monitor); /* Update MXS_MONITORED_SERVER->pending_status. This is where the monitor loop writes it's findings. * Also, backup current status so that it can be compared to any deduced state. */ for (auto mon_srv = m_monitor->monitored_servers; mon_srv; mon_srv = mon_srv->next) @@ -322,9 +318,6 @@ void MariaDBMonitor::main() mon_srv->pending_status = status; mon_srv->mon_prev_status = status; } - /* Avoid reading/writing SERVER->status while servers are not locked. Routers read the state - * all the time. */ - release_monitor_servers(m_monitor); // Query all servers for their status. Update the server id array. m_servers_by_id.clear(); @@ -415,67 +408,44 @@ void MariaDBMonitor::main() measure_replication_lag(root_master); } - /* Servers have been queried and states deduced. Lock servers for a short while again while we update - * the shared server states and make the changes visible. There is a problem though: the admin could - * have changed the shared state while the queries were running. In this rare case, the deduced - * server states no longer apply and may be inconsistent. The most straightforward solution to this - * is to discard the results of the latest monitor loop and start another immediately. Cluster - * modifications are not performed until the next loop. The discarding of results is not perfect as - * some data has already been written. But this data is only used by the monitor itself and will be - * rewritten next loop. TODO: make versions of log_master_changes() and mon_process_state_changes() - * which read from MXS_MONITORED_SERVER->pending_status instead of SERVER->status. Then this locking - * and releasing can be moved after failover etc and the user changes can be applied gracefully. */ - lock_monitor_servers(m_monitor); - if (m_monitor->server_pending_changes) + // Update shared status. The next functions read the shared status. TODO: change the following + // functions to read "pending_status" instead. + for (auto mon_srv = m_monitor->monitored_servers; mon_srv; mon_srv = mon_srv->next) { - MXS_WARNING("MaxAdmin or REST API modified the state of a server monitored by '%s' during a " - "monitor iteration. Discarding the monitor results and retrying.", - m_monitor->name); - release_monitor_servers(m_monitor); + mon_srv->server->status = mon_srv->pending_status; } - else + + /* Check if monitor events need to be launched. */ + mon_process_state_changes(m_monitor, m_script.c_str(), m_events); + m_cluster_modified = false; + if (m_auto_failover) { - // Update shared status. - for (auto mon_srv = m_monitor->monitored_servers; mon_srv; mon_srv = mon_srv->next) - { - auto new_status = mon_srv->pending_status; - auto srv = mon_srv->server; - srv->status = new_status; - } - - /* Check if monitor events need to be launched. */ - mon_process_state_changes(m_monitor, m_script.c_str(), m_events); - m_cluster_modified = false; - if (m_auto_failover) - { - m_cluster_modified = handle_auto_failover(); - } - - /* log master detection failure of first master becomes available after failure */ - log_master_changes(root_master, &log_no_master); - - // Do not auto-join servers on this monitor loop if a failover (or any other cluster modification) - // has been performed, as server states have not been updated yet. It will happen next iteration. - if (!config_get_global_options()->passive && m_auto_rejoin && !m_cluster_modified && - cluster_can_be_joined()) - { - // Check if any servers should be autojoined to the cluster and try to join them. - handle_auto_rejoin(); - } - - /* Check if any slave servers have read-only off and turn it on if user so wishes. Again, do not - * perform this if cluster has been modified this loop since it may not be clear which server - * should be a slave. */ - if (!config_get_global_options()->passive && m_enforce_read_only_slaves && !m_cluster_modified) - { - enforce_read_only_on_slaves(); - } - - mon_hangup_failed_servers(m_monitor); - store_server_journal(m_monitor, m_master ? m_master->m_server_base : NULL); - release_monitor_servers(m_monitor); + m_cluster_modified = handle_auto_failover(); } + /* log master detection failure of first master becomes available after failure */ + log_master_changes(root_master, &log_no_master); + + // Do not auto-join servers on this monitor loop if a failover (or any other cluster modification) + // has been performed, as server states have not been updated yet. It will happen next iteration. + if (!config_get_global_options()->passive && m_auto_rejoin && !m_cluster_modified && + cluster_can_be_joined()) + { + // Check if any servers should be autojoined to the cluster and try to join them. + handle_auto_rejoin(); + } + + /* Check if any slave servers have read-only off and turn it on if user so wishes. Again, do not + * perform this if cluster has been modified this loop since it may not be clear which server + * should be a slave. */ + if (!config_get_global_options()->passive && m_enforce_read_only_slaves && !m_cluster_modified) + { + enforce_read_only_on_slaves(); + } + + mon_hangup_failed_servers(m_monitor); + store_server_journal(m_monitor, m_master ? m_master->m_server_base : NULL); + // Check how much the monitor should sleep to get one full monitor interval. timespec loop_end; clock_gettime(CLOCK_MONOTONIC_COARSE, &loop_end); @@ -486,7 +456,8 @@ void MariaDBMonitor::main() sleep_time_remaining = MXS_MAX(MXS_MON_BASE_INTERVAL_MS, sleep_time_remaining); /* Sleep in small increments to react faster to some events. This should ideally use some type of * notification mechanism. */ - while (sleep_time_remaining > 0 && !should_shutdown() && !m_monitor->server_pending_changes) + while (sleep_time_remaining > 0 && !should_shutdown() && + atomic_load_int(&m_monitor->check_maintenance_flag) == MAINTENANCE_FLAG_NOCHECK) { int small_sleep_ms = (sleep_time_remaining >= MXS_MON_BASE_INTERVAL_MS) ? MXS_MON_BASE_INTERVAL_MS : sleep_time_remaining;