Remove MonitorManager calls from Monitor functions

Also adds admin thread checks to MonitorManager functions and combines
anonymous namespaces.

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;
 

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"
 

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"
 

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"
 

server/core/internal/monitor.hh

@@ -17,9 +17,6 @@
  */
 
 #include <maxscale/monitor.hh>
-#include <maxscale/resultset.hh>
-#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;
-};

server/core/internal/monitormanager.hh

@@ -14,6 +14,8 @@
 
 #include <maxscale/monitor.hh>
 
+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;
+};

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 */

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);
     }
 }
+
+}

server/core/monitormanager.cc

@@ -15,10 +15,13 @@
 
 #include <maxscale/json_api.hh>
 #include <maxscale/paths.h>
+#include <maxscale/resultset.hh>
 
 #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<Monitor*, uint64_t> 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);
+    mxb_assert(Monitor::is_admin_thread());
-
-    Guard guard(monitor->m_lock);
 
     // 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);
+    mxb_assert(Monitor::is_admin_thread());
-
-    Guard guard(monitor->m_lock);
 
     /** 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
+    // "servers"-setting of the base monitor.
-    // to cleanly "deactivate" inherited objects.
+    monitor->deactivate();
-    stop_monitor(monitor);
-    monitor->remove_all_servers();
     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<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";
@@ -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();

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"

server/core/server.cc

@@ -45,6 +45,7 @@
 #include <maxscale/routingworker.hh>
 
 #include "internal/monitor.hh"
+#include "internal/monitormanager.hh"
 #include "internal/poll.hh"
 #include "internal/config.hh"
 #include "internal/service.hh"

server/core/test/test_modulecmd.cc

@@ -24,6 +24,7 @@
 #include <maxscale/json_api.hh>
 
 #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)
 

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"

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"

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"