Cleanup monitor running state

include/maxscale/monitor.hh

@@ -84,13 +84,6 @@ extern const char CN_SCRIPT_TIMEOUT[];
  */
 #define MXS_MONITOR_VERSION {5, 0, 0}
 
-// Monitor state enum
-enum monitor_state_t
-{
-    MONITOR_STATE_STOPPED,
-    MONITOR_STATE_RUNNING,
-};
-
 /** Monitor events */
 enum mxs_monitor_event_t
 {
@@ -292,7 +285,19 @@ public:
      */
     static const char* get_event_name(mxs_monitor_event_t event);
 
-    virtual monitor_state_t state() const = 0;
+    /**
+     * Is the monitor running?
+     *
+     * @return True if monitor is running.
+     */
+    virtual bool is_running() const = 0;
+
+    /**
+     * Get running state as string.
+     *
+     * @return "Running" or "Stopped"
+     */
+    const char* state_string() const;
 
     const char* name() const;
 
@@ -592,28 +597,7 @@ public:
 
     virtual ~MonitorWorker();
 
-    /**
+    bool is_running() const final;
-     * @brief Current state of the monitor.
-     *
-     * Since the state is written to by the admin thread, the value returned in other threads cannot be fully
-     * trusted. The state should only be read in the admin thread or operations launched by the admin thread.
-     *
-     * @return @c MONITOR_STATE_RUNNING if the monitor is running,
-     *         @c MONITOR_STATE_STOPPED if the monitor is stopped.
-     */
-    monitor_state_t state() const override final;
-
-    /**
-     * @brief Find out whether the monitor is running.
-     *
-     * @return True, if the monitor is running, false otherwise.
-     *
-     * @see state().
-     */
-    bool is_running() const
-    {
-        return state() == MONITOR_STATE_RUNNING;
-    }
 
     /**
      * @brief Starts the monitor.

server/core/monitor.cc

@@ -152,22 +152,6 @@ private:
 
 ThisUnit this_unit;
 
-const char* monitor_state_to_string(monitor_state_t state)
-{
-    switch (state)
-    {
-    case MONITOR_STATE_RUNNING:
-        return "Running";
-
-    case MONITOR_STATE_STOPPED:
-        return "Stopped";
-
-    default:
-        mxb_assert(false);
-        return "Unknown";
-    }
-}
-
 /** Server type specific bits */
 const uint64_t server_type_bits = SERVER_MASTER | SERVER_SLAVE | SERVER_JOINED;
 
@@ -545,6 +529,11 @@ long Monitor::ticks() const
     return m_ticks.load(std::memory_order_acquire);
 }
 
+const char* Monitor::state_string() const
+{
+    return is_running() ? "Running" : "Stopped";
+}
+
 Monitor::~Monitor()
 {
     for (auto server : m_servers)
@@ -564,7 +553,7 @@ Monitor::~Monitor()
 bool Monitor::add_server(SERVER* server)
 {
     // This should only be called from the admin thread while the monitor is stopped.
-    mxb_assert(state() == MONITOR_STATE_STOPPED && is_admin_thread());
+    mxb_assert(!is_running() && is_admin_thread());
     bool success = false;
     string existing_owner;
     if (this_unit.claim_server(server->name(), m_name, &existing_owner))
@@ -598,7 +587,7 @@ void Monitor::server_removed(SERVER* server)
 void Monitor::remove_all_servers()
 {
     // This should only be called from the admin thread while the monitor is stopped.
-    mxb_assert(state() == MONITOR_STATE_STOPPED && is_admin_thread());
+    mxb_assert(!is_running() && is_admin_thread());
     for (auto mon_server : m_servers)
     {
         mxb_assert(this_unit.claimed_by(mon_server->server->name()) == m_name);
@@ -612,7 +601,7 @@ void Monitor::remove_all_servers()
 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, "State:                  %s\n", state_string());
     dcb_printf(dcb, "Times monitored:        %li\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);
@@ -631,7 +620,7 @@ void Monitor::show(DCB* dcb)
 
     dcb_printf(dcb, "\n");
 
-    if (state() == MONITOR_STATE_RUNNING)
+    if (is_running())
     {
         diagnostics(dcb);
     }
@@ -656,14 +645,13 @@ json_t* Monitor::to_json(const char* host) const
     json_object_set_new(rval, CN_TYPE, json_string(CN_MONITORS));
 
     json_object_set_new(attr, CN_MODULE, json_string(m_module.c_str()));
-    auto my_state = state();
+    json_object_set_new(attr, CN_STATE, json_string(state_string()));
-    json_object_set_new(attr, CN_STATE, json_string(monitor_state_to_string(my_state)));
     json_object_set_new(attr, CN_TICKS, json_integer(ticks()));
 
     /** Monitor parameters */
     json_object_set_new(attr, CN_PARAMETERS, parameters_to_json());
 
-    if (my_state == MONITOR_STATE_RUNNING)
+    if (is_running())
     {
         json_t* diag = diagnostics_json();
         if (diag)
@@ -1750,7 +1738,7 @@ std::vector<MonitorServer*> Monitor::get_monitored_serverlist(const string& key,
 
 bool Monitor::set_disk_space_threshold(const string& dst_setting)
 {
-    mxb_assert(state() == MONITOR_STATE_STOPPED);
+    mxb_assert(!is_running());
     SERVER::DiskSpaceLimits new_dst;
     bool rv = config_parse_disk_space_threshold(&new_dst, dst_setting.c_str());
     if (rv)
@@ -1774,7 +1762,7 @@ bool Monitor::set_server_status(SERVER* srv, int bit, string* errmsg_out)
 
     bool written = false;
 
-    if (state() == MONITOR_STATE_RUNNING)
+    if (is_running())
     {
         /* This server is monitored, in which case modifying any other status bit than Maintenance is
          * disallowed. */
@@ -1837,7 +1825,7 @@ bool Monitor::clear_server_status(SERVER* srv, int bit, string* errmsg_out)
 
     bool written = false;
 
-    if (state() == MONITOR_STATE_RUNNING)
+    if (is_running())
     {
         if (bit & ~(SERVER_MAINT | SERVER_DRAINING))
         {
@@ -1883,7 +1871,7 @@ bool Monitor::clear_server_status(SERVER* srv, int bit, string* errmsg_out)
 
 void Monitor::populate_services()
 {
-    mxb_assert(state() == MONITOR_STATE_STOPPED);
+    mxb_assert(!is_running());
 
     for (MonitorServer* pMs : m_servers)
     {
@@ -1893,7 +1881,7 @@ void Monitor::populate_services()
 
 void Monitor::deactivate()
 {
-    if (state() == MONITOR_STATE_RUNNING)
+    if (is_running())
     {
         stop();
     }
@@ -1933,21 +1921,16 @@ MonitorWorker::~MonitorWorker()
 {
 }
 
-monitor_state_t MonitorWorker::state() const
+bool MonitorWorker::is_running() const
 {
-    bool running = (Worker::state() != Worker::STOPPED);
+    return (Worker::state() != Worker::STOPPED);
-
-    return running ? MONITOR_STATE_RUNNING : MONITOR_STATE_STOPPED;
 }
 
 void MonitorWorker::do_stop()
 {
-    // This should only be called by monitor_stop(). NULL worker is allowed since the main worker may
+    // This should only be called by monitor_stop().
-    // not exist during program start/stop.
+    mxb_assert(Monitor::is_admin_thread());
-    mxb_assert(mxs_rworker_get_current() == NULL
+    mxb_assert(is_running());
-               || mxs_rworker_get_current() == mxs_rworker_get(MXS_RWORKER_MAIN));
-    mxb_assert(Worker::state() != Worker::STOPPED);
-    mxb_assert(state() != MONITOR_STATE_STOPPED);
     mxb_assert(m_thread_running.load() == true);
 
     Worker::shutdown();
@@ -1968,10 +1951,8 @@ 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.
-    mxb_assert(mxs_rworker_get_current() == NULL
+    mxb_assert(Monitor::is_admin_thread());
-               || mxs_rworker_get_current() == mxs_rworker_get(MXS_RWORKER_MAIN));
+    mxb_assert(!is_running());
-    mxb_assert(Worker::state() == Worker::STOPPED);
-    mxb_assert(state() == MONITOR_STATE_STOPPED);
     mxb_assert(m_thread_running.load() == false);
 
     if (journal_is_stale())

server/core/monitormanager.cc

@@ -137,7 +137,7 @@ void MonitorManager::debug_wait_one_tick()
 
     // Wait for all running monitors to advance at least one tick.
     this_unit.foreach_monitor([&ticks](Monitor* mon) {
-        if (mon->state() == MONITOR_STATE_RUNNING)
+        if (mon->is_running())
         {
             auto start = steady_clock::now();
             // A monitor may have been added in between the two foreach-calls (not if config changes are
@@ -150,19 +150,18 @@ void MonitorManager::debug_wait_one_tick()
                     std::this_thread::sleep_for(milliseconds(100));
                 }
             }
-      }
+        }
-      return true;
+        return true;
   });
 }
 
 void MonitorManager::destroy_all_monitors()
 {
     mxb_assert(Monitor::is_admin_thread());
-
     auto monitors = this_unit.clear();
     for (auto monitor : monitors)
     {
-        mxb_assert(monitor->state() == MONITOR_STATE_STOPPED);
+        mxb_assert(!monitor->is_running());
         delete monitor;
     }
 }
@@ -172,7 +171,7 @@ void MonitorManager::start_monitor(Monitor* monitor)
     mxb_assert(Monitor::is_admin_thread());
 
     // Only start the monitor if it's stopped.
-    if (monitor->state() == MONITOR_STATE_STOPPED)
+    if (!monitor->is_running())
     {
         if (!monitor->start())
         {
@@ -207,7 +206,7 @@ void MonitorManager::stop_monitor(Monitor* monitor)
     mxb_assert(Monitor::is_admin_thread());
 
     /** Only stop the monitor if it is running */
-    if (monitor->state() == MONITOR_STATE_RUNNING)
+    if (monitor->is_running())
     {
         monitor->stop();
     }
@@ -272,11 +271,9 @@ void MonitorManager::monitor_list(DCB* dcb)
     dcb_printf(dcb, "---------------------+---------------------\n");
 
     this_unit.foreach_monitor([dcb](Monitor* ptr) {
-                                  dcb_printf(dcb, "%-20s | %s\n",
+        dcb_printf(dcb, "%-20s | %s\n", ptr->name(), ptr->state_string());
-                                             ptr->name(),
+        return true;
-                                             ptr->state() == MONITOR_STATE_RUNNING ? "Running" : "Stopped");
+    });
-                                  return true;
-                              });
 
     dcb_printf(dcb, "---------------------+---------------------\n");
 }
@@ -310,10 +307,9 @@ std::unique_ptr<ResultSet> MonitorManager::monitor_get_list()
     mxb_assert(Monitor::is_admin_thread());
     std::unique_ptr<ResultSet> set = ResultSet::create({"Monitor", "Status"});
     this_unit.foreach_monitor([&set](Monitor* ptr) {
-                                  const char* state = ptr->state() == MONITOR_STATE_RUNNING ? "Running" : "Stopped";
+        set->add_row({ptr->m_name, ptr->state_string()});
-                                  set->add_row({ptr->m_name, state});
+        return true;
-                                  return true;
+    });
-                              });
     return set;
 }
 
@@ -409,7 +405,7 @@ bool MonitorManager::reconfigure_monitor(mxs::Monitor* monitor, const MXS_CONFIG
     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);
+    bool stopstart = monitor->is_running();
     if (stopstart)
     {
         monitor->stop();

server/modules/monitor/clustrixmon/clustrixmonitor.cc

@@ -208,7 +208,7 @@ bool ClustrixMonitor::configure(const MXS_CONFIG_PARAMETER* pParams)
 
 void ClustrixMonitor::populate_services()
 {
-    mxb_assert(state() == MONITOR_STATE_STOPPED);
+    mxb_assert(!is_running());
 
     // The servers that the Clustrix monitor has been configured with are
     // only used for bootstrapping and services will not be populated

server/modules/monitor/mariadbmon/mariadbmon.cc

@@ -695,7 +695,7 @@ bool MariaDBMonitor::check_sql_files()
 bool MariaDBMonitor::execute_manual_command(std::function<void(void)> command, json_t** error_out)
 {
     bool rval = false;
-    if (state() != MONITOR_STATE_RUNNING)
+    if (!is_running())
     {
         PRINT_MXS_JSON_ERROR(error_out, "The monitor is not running, cannot execute manual command.");
     }