From 8afa8c2c5a2e358072a8914537a75488ce0f7d19 Mon Sep 17 00:00:00 2001
From: Johan Wikman <johan.wikman@mariadb.com>
Date: Thu, 7 Jun 2018 11:25:44 +0300
Subject: [PATCH] MXS-1775 Add MonitorInstanceSimple class

MonitorInstanceSimple is intended for simple monitors that
probe servers in a straightforward fashion. More complex monitors
can be derived directly from MonitorInstance.
---
 include/maxscale/monitor.hh                   | 79 +++++++++++++------
 server/core/monitor.cc                        | 14 +++-
 server/modules/monitor/auroramon/auroramon.cc |  2 +-
 server/modules/monitor/auroramon/auroramon.hh |  2 +-
 server/modules/monitor/galeramon/galeramon.cc |  6 +-
 server/modules/monitor/galeramon/galeramon.hh |  4 +-
 server/modules/monitor/grmon/grmon.cc         |  2 +-
 server/modules/monitor/grmon/grmon.hh         |  2 +-
 .../modules/monitor/mariadbmon/mariadbmon.cc  |  7 --
 .../modules/monitor/mariadbmon/mariadbmon.hh  |  1 -
 server/modules/monitor/mmmon/mmmon.cc         |  6 +-
 server/modules/monitor/mmmon/mmmon.hh         |  4 +-
 .../monitor/ndbclustermon/ndbclustermon.cc    |  2 +-
 .../monitor/ndbclustermon/ndbclustermon.hh    |  2 +-
 14 files changed, 82 insertions(+), 51 deletions(-)

diff --git a/include/maxscale/monitor.hh b/include/maxscale/monitor.hh
index 92912a0fb..ab745bde0 100644
--- a/include/maxscale/monitor.hh
+++ b/include/maxscale/monitor.hh
@@ -168,14 +168,6 @@ protected:
      */
     virtual bool has_sufficient_permissions() const;
 
-    /**
-     * @brief Update server information
-     *
-     * The implementation should probe the server in question and update
-     * the server status bits.
-     */
-    virtual void update_server_status(MXS_MONITORED_SERVER* pMonitored_server) = 0;
-
     /**
      * @brief Flush pending server status to each server.
      *
@@ -189,23 +181,9 @@ protected:
      * @brief Monitor the servers
      *
      * This function is called once per monitor round, and the concrete
-     * implementation should probe all servers, i.e. call @c update_server_status
-     * on each server.
-     *
-     * The default implementation will for each server:
-     *   - Do nothing, if the server is in maintenance.
-     *   - Before calling, store the previous status of the server.
-     *   - Before calling, set the pending status of the monitored server object
-     *     to the status of the corresponding server object.
-     *   - Ensure that there is a connection to the server.
-     *     If there is, @c update_server_status is called.
-     *     If there is not, the pending status will be updated accordingly and
-     *     @c update_server_status will *not* be called.
-     *   - After the call, update the error count of the server if it is down.
-     *
-     * Finally, it will call @c flush_server_status.
+     * implementation should probe all servers and set server status bits.
      */
-    virtual void tick();
+    virtual void tick() = 0;
 
     /**
      * @brief Called before the monitor loop is started
@@ -244,6 +222,59 @@ private:
     static void main(void* pArg);
 };
 
+class MonitorInstanceSimple : public MonitorInstance
+{
+public:
+    MonitorInstanceSimple(const MonitorInstanceSimple&) = delete;
+    MonitorInstanceSimple& operator = (const MonitorInstanceSimple&) = delete;
+
+protected:
+    MonitorInstanceSimple(MXS_MONITOR* pMonitor)
+        : MonitorInstance(pMonitor)
+    {
+    }
+
+    /**
+     * @brief Update server information
+     *
+     * The implementation should probe the server in question and update
+     * the server status bits.
+     */
+    virtual void update_server_status(MXS_MONITORED_SERVER* pMonitored_server) = 0;
+
+    /**
+     * @brief Called right at the beginning of @c tick().
+     *
+     * The default implementation does nothing.
+     */
+    virtual void pre_tick();
+
+    /**
+     * @brief Called right before the end of @c tick().
+     *
+     * The default implementation does nothing.
+     */
+    virtual void post_tick();
+
+private:
+    /**
+     * @brief Monitor the servers
+     *
+     * This function is called once per monitor round and will for each server:
+     *
+     *   - Do nothing, if the server is in maintenance.
+     *   - Store the previous status of the server.
+     *   - Set the pending status of the monitored server object
+     *     to the status of the corresponding server object.
+     *   - Ensure that there is a connection to the server.
+     *     If there is, @c update_server_status() is called.
+     *     If there is not, the pending status will be updated accordingly and
+     *     @c update_server_status() will *not* be called.
+     *   - After the call, update the error count of the server if it is down.
+     */
+    void tick(); // final
+};
+
 /**
  * The purpose of the template MonitorApi is to provide an implementation
  * of the monitor C-API. The template is instantiated with a class that
diff --git a/server/core/monitor.cc b/server/core/monitor.cc
index 62bb0d547..427afa772 100644
--- a/server/core/monitor.cc
+++ b/server/core/monitor.cc
@@ -2827,8 +2827,18 @@ void MonitorInstance::flush_server_status()
     }
 }
 
-void MonitorInstance::tick()
+void MonitorInstanceSimple::pre_tick()
 {
+}
+
+void MonitorInstanceSimple::post_tick()
+{
+}
+
+void MonitorInstanceSimple::tick()
+{
+    pre_tick();
+
     for (MXS_MONITORED_SERVER *pMs = m_monitor->monitored_servers; pMs; pMs = pMs->next)
     {
         if (!SERVER_IN_MAINT(pMs->server))
@@ -2892,6 +2902,8 @@ void MonitorInstance::tick()
             }
         }
     }
+
+    post_tick();
 }
 
 void MonitorInstance::pre_loop()
diff --git a/server/modules/monitor/auroramon/auroramon.cc b/server/modules/monitor/auroramon/auroramon.cc
index f4709af48..f7ebfa06f 100644
--- a/server/modules/monitor/auroramon/auroramon.cc
+++ b/server/modules/monitor/auroramon/auroramon.cc
@@ -26,7 +26,7 @@
 
 
 AuroraMonitor::AuroraMonitor(MXS_MONITOR* monitor)
-    : maxscale::MonitorInstance(monitor)
+    : maxscale::MonitorInstanceSimple(monitor)
 {
 }
 
diff --git a/server/modules/monitor/auroramon/auroramon.hh b/server/modules/monitor/auroramon/auroramon.hh
index ee5b8c135..7bae000f7 100644
--- a/server/modules/monitor/auroramon/auroramon.hh
+++ b/server/modules/monitor/auroramon/auroramon.hh
@@ -20,7 +20,7 @@
  * @file auroramon.hh - The Aurora monitor
  */
 
-class AuroraMonitor : public maxscale::MonitorInstance
+class AuroraMonitor : public maxscale::MonitorInstanceSimple
 {
 public:
     AuroraMonitor(const AuroraMonitor&) = delete;
diff --git a/server/modules/monitor/galeramon/galeramon.cc b/server/modules/monitor/galeramon/galeramon.cc
index 9d21eb3ce..afcb92e87 100644
--- a/server/modules/monitor/galeramon/galeramon.cc
+++ b/server/modules/monitor/galeramon/galeramon.cc
@@ -47,7 +47,7 @@ static void nodeval_free(GALERA_NODE_INFO *);
 static bool using_xtrabackup(MXS_MONITORED_SERVER *database, const char* server_string);
 
 GaleraMonitor::GaleraMonitor(MXS_MONITOR *mon)
-    : maxscale::MonitorInstance(mon)
+    : maxscale::MonitorInstanceSimple(mon)
     , m_id(MXS_MONITOR_DEFAULT_ID)
     , m_disableMasterFailback(0)
     , m_availableWhenDonor(0)
@@ -293,10 +293,8 @@ void GaleraMonitor::update_server_status(MXS_MONITORED_SERVER* monitored_server)
     }
 }
 
-void GaleraMonitor::tick()
+void GaleraMonitor::post_tick()
 {
-    MonitorInstance::tick();
-
     int is_cluster = 0;
 
     /* Try to set a Galera cluster based on
diff --git a/server/modules/monitor/galeramon/galeramon.hh b/server/modules/monitor/galeramon/galeramon.hh
index de95e0db5..f47b4f99d 100644
--- a/server/modules/monitor/galeramon/galeramon.hh
+++ b/server/modules/monitor/galeramon/galeramon.hh
@@ -52,7 +52,7 @@ typedef struct galera_cluster_info
 } GALERA_CLUSTER_INFO;
 
 
-class GaleraMonitor : public maxscale::MonitorInstance
+class GaleraMonitor : public maxscale::MonitorInstanceSimple
 {
 public:
     GaleraMonitor(const GaleraMonitor&) = delete;
@@ -67,7 +67,7 @@ protected:
     bool configure(const MXS_CONFIG_PARAMETER* param);
     bool has_sufficient_permissions() const;
     void update_server_status(MXS_MONITORED_SERVER* monitored_server);
-    void tick();
+    void post_tick();
 
 private:
     unsigned long m_id;                 /**< Monitor ID */
diff --git a/server/modules/monitor/grmon/grmon.cc b/server/modules/monitor/grmon/grmon.cc
index f6a4caef8..1dcaba4fa 100644
--- a/server/modules/monitor/grmon/grmon.cc
+++ b/server/modules/monitor/grmon/grmon.cc
@@ -27,7 +27,7 @@
 
 
 GRMon::GRMon(MXS_MONITOR* monitor)
-    : MonitorInstance(monitor)
+    : MonitorInstanceSimple(monitor)
 {
 }
 
diff --git a/server/modules/monitor/grmon/grmon.hh b/server/modules/monitor/grmon/grmon.hh
index d0734d51a..3cf974c57 100644
--- a/server/modules/monitor/grmon/grmon.hh
+++ b/server/modules/monitor/grmon/grmon.hh
@@ -20,7 +20,7 @@
  * @file grmon.hh A MySQL Group Replication cluster monitor
  */
 
-class GRMon : public maxscale::MonitorInstance
+class GRMon : public maxscale::MonitorInstanceSimple
 {
 public:
     GRMon(const GRMon&) = delete;
diff --git a/server/modules/monitor/mariadbmon/mariadbmon.cc b/server/modules/monitor/mariadbmon/mariadbmon.cc
index 6fc90b33a..eb1e3d4a9 100644
--- a/server/modules/monitor/mariadbmon/mariadbmon.cc
+++ b/server/modules/monitor/mariadbmon/mariadbmon.cc
@@ -332,13 +332,6 @@ void MariaDBMonitor::update_server(MariaDBServer& server)
     mon_srv->mon_err_count = (is_running || in_maintenance) ? 0 : mon_srv->mon_err_count + 1;
 }
 
-void MariaDBMonitor::update_server_status(MXS_MONITORED_SERVER* monitored_server)
-{
-    // Not used and should not be called. Is there a way to check this "statically" (without
-    // template magic)?
-    ss_dassert(!true);
-}
-
 void MariaDBMonitor::pre_loop()
 {
     // MonitorInstance loaded from the journal the current master into its
diff --git a/server/modules/monitor/mariadbmon/mariadbmon.hh b/server/modules/monitor/mariadbmon/mariadbmon.hh
index 8a86e341f..416d9137d 100644
--- a/server/modules/monitor/mariadbmon/mariadbmon.hh
+++ b/server/modules/monitor/mariadbmon/mariadbmon.hh
@@ -163,7 +163,6 @@ private:
     MariaDBServer* get_server_info(MXS_MONITORED_SERVER* db);
     MariaDBServer* get_server(int64_t id);
     void update_server(MariaDBServer& server);
-    void update_server_status(MXS_MONITORED_SERVER* pMonitored_server); // Not used
 
     // Cluster discovery and status assignment methods
     MariaDBServer* find_root_master();
diff --git a/server/modules/monitor/mmmon/mmmon.cc b/server/modules/monitor/mmmon/mmmon.cc
index f902fba15..29450658a 100644
--- a/server/modules/monitor/mmmon/mmmon.cc
+++ b/server/modules/monitor/mmmon/mmmon.cc
@@ -36,7 +36,7 @@ static void detectStaleMaster(void *, int);
 static bool isMySQLEvent(mxs_monitor_event_t event);
 
 MMMonitor::MMMonitor(MXS_MONITOR *monitor)
-    : maxscale::MonitorInstance(monitor)
+    : maxscale::MonitorInstanceSimple(monitor)
     , m_id(MXS_MONITOR_DEFAULT_ID)
     , m_detectStaleMaster(false)
 {
@@ -318,10 +318,8 @@ void MMMonitor::update_server_status(MXS_MONITORED_SERVER* monitored_server)
     }
 }
 
-void MMMonitor::tick()
+void MMMonitor::post_tick()
 {
-    MonitorInstance::tick();
-
     /* Get Master server pointer */
     MXS_MONITORED_SERVER *root_master = get_current_master();
 
diff --git a/server/modules/monitor/mmmon/mmmon.hh b/server/modules/monitor/mmmon/mmmon.hh
index 06c97d134..8390d69ac 100644
--- a/server/modules/monitor/mmmon/mmmon.hh
+++ b/server/modules/monitor/mmmon/mmmon.hh
@@ -20,7 +20,7 @@
  * @file mmmon.hh - The Multi-Master monitor
  */
 
-class MMMonitor : public maxscale::MonitorInstance
+class MMMonitor : public maxscale::MonitorInstanceSimple
 {
 public:
     MMMonitor(const MMMonitor&) = delete;
@@ -35,7 +35,7 @@ protected:
     bool configure(const MXS_CONFIG_PARAMETER* params);
     bool has_sufficient_permissions() const;
     void update_server_status(MXS_MONITORED_SERVER* monitored_server);
-    void tick();
+    void post_tick();
 
 private:
     unsigned long m_id;      /**< Monitor ID */
diff --git a/server/modules/monitor/ndbclustermon/ndbclustermon.cc b/server/modules/monitor/ndbclustermon/ndbclustermon.cc
index 757eef8ad..c1027620a 100644
--- a/server/modules/monitor/ndbclustermon/ndbclustermon.cc
+++ b/server/modules/monitor/ndbclustermon/ndbclustermon.cc
@@ -23,7 +23,7 @@
 
 
 NDBCMonitor::NDBCMonitor(MXS_MONITOR *monitor)
-    : maxscale::MonitorInstance(monitor)
+    : maxscale::MonitorInstanceSimple(monitor)
     , m_id(MXS_MONITOR_DEFAULT_ID)
 {
 }
diff --git a/server/modules/monitor/ndbclustermon/ndbclustermon.hh b/server/modules/monitor/ndbclustermon/ndbclustermon.hh
index 971eb41c1..f89d07472 100644
--- a/server/modules/monitor/ndbclustermon/ndbclustermon.hh
+++ b/server/modules/monitor/ndbclustermon/ndbclustermon.hh
@@ -20,7 +20,7 @@
  * @file ndbcclustermon.hh A NDBC cluster monitor
  */
 
-class NDBCMonitor : public maxscale::MonitorInstance
+class NDBCMonitor : public maxscale::MonitorInstanceSimple
 {
 public:
     NDBCMonitor(const NDBCMonitor&) = delete;