MXS-1883 Maintenance is now the only user-modifiable bit for a monitored server

The request to turn maintenance off/on is a separate flag, although the actual
status is still contained in the status bitfield.

include/maxscale/atomic.h

@@ -121,4 +121,13 @@ static inline void atomic_synchronize()
  */
 bool atomic_cas_ptr(void **variable, void** old_value, void *new_value);
 
+/**
+ * Atomic read-and-write. Writes new value into the given memory address and returns the old value.
+ *
+ * @param variable The variable which is overwritten
+ * @param new_value The value to write
+ * @return The value before writing
+ */
+int atomic_exchange_int(int *variable, int new_value);
+
 MXS_END_DECLS

include/maxscale/server.h

@@ -41,6 +41,13 @@ extern const char CN_PERSISTMAXTIME[];
 extern const char CN_PERSISTPOOLMAX[];
 extern const char CN_PROXY_PROTOCOL[];
 
+/**
+ * Maintenance mode request constants.
+ */
+const int MAINTENANCE_OFF = -100;
+const int MAINTENANCE_NO_CHANGE = 0;
+const int MAINTENANCE_ON = 100;
+
 /**
  * The server parameters used for weighting routing decissions
  */
@@ -137,9 +144,9 @@ typedef struct server
     int            last_event;     /**< The last event that occurred on this server */
     int64_t        triggered_at;   /**< Time when the last event was triggered */
     bool           active_event;   /**< Was MaxScale active when last event was observed */
-    // Values updated mainly by monitor
+    // Status descriptors. Updated automatically by a monitor or manually by the admin
     uint64_t       status;         /**< Current status flag bitmap */
-    uint64_t       status_pending; /**< Temporary status, usually written to current status once ready */
+    int            maint_request;  /**< Is admin requesting Maintenance=ON/OFF on the server? */
     char           version_string[MAX_SERVER_VERSION_LEN]; /**< Server version string as given by backend */
     uint64_t       version;        /**< Server version numeric representation */
     server_type_t  server_type;    /**< Server type (MariaDB or MySQL), deduced from version string */
@@ -372,8 +379,6 @@ extern uint64_t server_map_status(const char *str);
 extern void server_set_version_string(SERVER* server, const char* version_string);
 extern void server_set_version(SERVER* server, const char* version_string, uint64_t version);
 extern uint64_t server_get_version(const SERVER* server);
-extern void server_set_status(SERVER *server, int bit);
-extern void server_clear_status(SERVER *server, int bit);
 
 extern void printServer(const SERVER *);
 extern void printAllServers();

include/maxscale/server.hh

@@ -0,0 +1,24 @@
+#pragma once
+/*
+ * Copyright (c) 2016 MariaDB Corporation Ab
+ *
+ * Use of this software is governed by the Business Source License included
+ * in the LICENSE.TXT file and at www.mariadb.com/bsl11.
+ *
+ * Change Date: 2020-01-01
+ *
+ * On the date above, in accordance with the Business Source License, use
+ * of this software will be governed by version 2 or later of the General
+ * Public License.
+ */
+
+#include <maxscale/cppdefs.hh>
+
+#include <string>
+#include <maxscale/server.h>
+
+namespace maxscale
+{
+    bool server_set_status(SERVER *server, int bit, std::string* errmsg_out = NULL);
+    bool server_clear_status(SERVER *server, int bit, std::string* errmsg_out = NULL);
+}

server/core/atomic.cc

@@ -182,3 +182,19 @@ bool atomic_cas_ptr(void **variable, void** old_value, void *new_value)
     return __sync_bool_compare_and_swap(variable, *old_value, new_value);
 #endif
 }
+
+int atomic_exchange_int(int *variable, int new_value)
+{
+#ifdef MXS_USE_ATOMIC_BUILTINS
+    return __atomic_exchange_n(variable, new_value, __ATOMIC_SEQ_CST);
+#else
+    /* The __sync-functions have a questionable exchange-operation (__sync_lock_test_and_set) which
+     * doesn't always write the requested value. Using compare-and-swap instead. */
+    int old_val = *variable;
+    while (!__sync_bool_compare_and_swap(variable, old_val, new_value))
+    {
+        old_val = *variable;
+    }
+    return old_val;
+#endif
+}

server/core/monitor.cc

@@ -1737,9 +1737,9 @@ void release_monitor_servers(MXS_MONITOR *monitor)
     }
 }
 /**
-  * Sets the current status of all servers monitored by this monitor to
+  * Check if admin is requesting setting or clearing maintenance status on the server and act accordingly.
-  * the pending status. This should only be called at the beginning of
+  * Should be called at the beginning of a monitor loop.
-  * a monitor loop, after the servers are locked.
+  *
   * @param monitor The target monitor
   */
 void servers_status_pending_to_current(MXS_MONITOR *monitor)
@@ -1747,11 +1747,22 @@ void servers_status_pending_to_current(MXS_MONITOR *monitor)
     MXS_MONITORED_SERVER *ptr = monitor->monitored_servers;
     while (ptr)
     {
-        ptr->server->status = ptr->server->status_pending;
+        // 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;
     }
     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
@@ -1763,7 +1774,6 @@ void servers_status_current_to_pending(MXS_MONITOR *monitor)
     MXS_MONITORED_SERVER *ptr = monitor->monitored_servers;
     while (ptr)
     {
-        ptr->server->status_pending = ptr->server->status;
         ptr = ptr->next;
     }
 }
@@ -2165,7 +2175,6 @@ static const char* process_server(MXS_MONITOR *monitor, const char *data, const
 
             uint64_t status = maxscale::get_byteN(sptr, MMB_LEN_SERVER_STATUS);
             db->mon_prev_status = status;
-            db->server->status_pending = status;
             server_set_status_nolock(db->server, status);
             monitor_set_pending_status(db, status);
             break;

server/core/resource.cc

@@ -25,6 +25,7 @@
 #include <maxscale/adminusers.h>
 #include <maxscale/modulecmd.h>
 #include <maxscale/semaphore.hh>
+#include <maxscale/server.hh>
 
 #include "internal/httprequest.hh"
 #include "internal/httpresponse.hh"
@@ -658,9 +659,16 @@ HttpResponse cb_set_server(const HttpRequest& request)
 
     if (opt)
     {
-        server_set_status(server, opt);
+        string errmsg;
+        if (mxs::server_set_status(server, opt, &errmsg))
+        {
             return HttpResponse(MHD_HTTP_NO_CONTENT);
         }
+        else
+        {
+            return HttpResponse(MHD_HTTP_FORBIDDEN, mxs_json_error(errmsg.c_str()));
+        }
+    }
 
     return HttpResponse(MHD_HTTP_FORBIDDEN,
                         mxs_json_error("Invalid or missing value for the `%s` "
@@ -674,9 +682,16 @@ HttpResponse cb_clear_server(const HttpRequest& request)
 
     if (opt)
     {
-        server_clear_status(server, opt);
+        string errmsg;
+        if (mxs::server_clear_status(server, opt, &errmsg))
+        {
             return HttpResponse(MHD_HTTP_NO_CONTENT);
         }
+        else
+        {
+            return HttpResponse(MHD_HTTP_FORBIDDEN, mxs_json_error(errmsg.c_str()));
+        }
+    }
 
     return HttpResponse(MHD_HTTP_FORBIDDEN,
                         mxs_json_error( "Invalid or missing value for the `%s` "

server/core/server.cc

@@ -26,7 +26,6 @@
 #include <maxscale/config.h>
 #include <maxscale/service.h>
 #include <maxscale/session.h>
-#include <maxscale/server.h>
 #include <maxscale/spinlock.h>
 #include <maxscale/dcb.h>
 #include <maxscale/poll.h>
@@ -40,11 +39,13 @@
 #include <maxscale/clock.h>
 #include <maxscale/http.hh>
 #include <maxscale/maxscale.h>
+#include <maxscale/server.hh>
 
 #include "internal/monitor.h"
 #include "internal/poll.h"
 #include "internal/routingworker.hh"
 
+
 using maxscale::Semaphore;
 using maxscale::Worker;
 using maxscale::WorkerTask;
@@ -62,7 +63,10 @@ const char CN_PROXY_PROTOCOL[]     = "proxy_protocol";
 
 static SPINLOCK server_spin = SPINLOCK_INIT;
 static SERVER *allServers = NULL;
-
+static const char ERR_CANNOT_MODIFY[] = "The server is monitored, so only the maintenance status can be "
+                                        "set/cleared manually. Status was not modified.";
+static const char WRN_REQUEST_OVERWRITTEN[] = "Previous maintenance request was not yet read by the monitor "
+                                              "and was overwritten.";
 static void spin_reporter(void *, char *, int);
 static void server_parameter_free(SERVER_PARAM *tofree);
 
@@ -119,7 +123,7 @@ SERVER* server_alloc(const char *name, const char *address, unsigned short port,
     server->auth_instance = auth_instance;
     server->port = port;
     server->status = SERVER_RUNNING;
-    server->status_pending = SERVER_RUNNING;
+    server->maint_request = MAINTENANCE_NO_CHANGE;
     server->node_id = -1;
     server->rlag = MAX_RLAG_UNDEFINED;
     server->master_id = -1;
@@ -1306,28 +1310,47 @@ SERVER* server_repurpose_destroyed(const char *name, const char *protocol, const
  * @param server        The server to update
  * @param bit           The bit to set for the server
  */
-void server_set_status(SERVER *server, int bit)
+bool mxs::server_set_status(SERVER *server, int bit, string* errmsg_out)
 {
+    bool written = false;
     /* First check if the server is monitored. This isn't done under a lock
      * but the race condition cannot cause significant harm. Monitors are never
-     * freed so the pointer stays valid.
+     * freed so the pointer stays valid. */
-     */
     MXS_MONITOR *mon = monitor_server_in_use(server);
     spinlock_acquire(&server->lock);
     if (mon && mon->state == MONITOR_STATE_RUNNING)
     {
-        /* Set a pending status bit. It will be activated on the next monitor
+        /* This server is monitored, in which case modifying any other status bit than Maintenance is
-         * loop. Also set a flag so the next loop happens sooner.
+         * disallowed. Maintenance is set/cleared using a special variable which the monitor reads when
-         */
+         * starting the next update cycle. Also set a flag so the next loop happens sooner. */
-        server->status_pending |= bit;
+        if (bit & ~SERVER_MAINT)
+        {
+            MXS_ERROR(ERR_CANNOT_MODIFY);
+            if (errmsg_out)
+            {
+                *errmsg_out = ERR_CANNOT_MODIFY;
+            }
+        }
+        else if (bit & SERVER_MAINT)
+        {
+            // Warn if the previous request hasn't been read.
+            int previous_request = atomic_exchange_int(&server->maint_request, MAINTENANCE_ON);
+            written = true;
+            if (previous_request != MAINTENANCE_NO_CHANGE)
+            {
+                MXS_WARNING(WRN_REQUEST_OVERWRITTEN);
+            }
             mon->server_pending_changes = true;
         }
+    }
     else
     {
         /* Set the bit directly */
         server_set_status_nolock(server, bit);
+        written = true;
     }
     spinlock_release(&server->lock);
+    return written;
 }
 /**
  * Clear a status bit in the server under a lock. This ensures synchronization
@@ -1337,24 +1360,42 @@ void server_set_status(SERVER *server, int bit)
  * @param server        The server to update
  * @param bit           The bit to clear for the server
  */
-void server_clear_status(SERVER *server, int bit)
+bool mxs::server_clear_status(SERVER *server, int bit, string* errmsg_out)
 {
+    bool written = false;
     MXS_MONITOR *mon = monitor_server_in_use(server);
     spinlock_acquire(&server->lock);
     if (mon && mon->state == MONITOR_STATE_RUNNING)
     {
-        /* Clear a pending status bit. It will be activated on the next monitor
+        // See server_set_status().
-         * loop. Also set a flag so the next loop happens sooner.
+        if (bit & ~SERVER_MAINT)
-         */
+        {
-        server->status_pending &= ~bit;
+            MXS_ERROR(ERR_CANNOT_MODIFY);
+            if (errmsg_out)
+            {
+                *errmsg_out = ERR_CANNOT_MODIFY;
+            }
+        }
+        else if (bit & SERVER_MAINT)
+        {
+            // Warn if the previous request hasn't been read.
+            int previous_request = atomic_exchange_int(&server->maint_request, MAINTENANCE_OFF);
+            written = true;
+            if (previous_request != MAINTENANCE_NO_CHANGE)
+            {
+                MXS_WARNING(WRN_REQUEST_OVERWRITTEN);
+            }
             mon->server_pending_changes = true;
         }
+    }
     else
     {
         /* Clear bit directly */
         server_clear_status_nolock(server, bit);
+        written = true;
     }
     spinlock_release(&server->lock);
+    return written;
 }
 
 bool server_is_mxs_service(const SERVER *server)

server/modules/monitor/mariadbmon/mariadbmon.cc

@@ -476,7 +476,6 @@ void MariaDBMonitor::main_loop()
                 auto new_status = mon_srv->pending_status;
                 auto srv = mon_srv->server;
                 srv->status = new_status;
-                srv->status_pending = new_status;
             }
 
             /* Check if monitor events need to be launched. */

server/modules/protocol/MySQL/mariadbbackend/mysql_backend.cc

@@ -22,6 +22,7 @@
 #include <maxscale/protocol.h>
 #include <maxscale/protocol/mysql.h>
 #include <maxscale/router.h>
+#include <maxscale/server.hh>
 #include <maxscale/utils.h>
 
 /*
@@ -329,7 +330,7 @@ static void handle_error_response(DCB *dcb, GWBUF *buffer)
                   "mode.", dcb->server->name,
                   dcb->server->address, dcb->server->port);
 
-        server_set_status(dcb->server, SERVER_MAINT);
+        mxs::server_set_status(dcb->server, SERVER_MAINT, NULL);
     }
     else if (errcode == ER_ACCESS_DENIED_ERROR ||
              errcode == ER_DBACCESS_DENIED_ERROR ||

server/modules/routing/debugcli/debugcmd.cc

@@ -46,7 +46,7 @@
 #include <maxscale/maxscale.h>
 #include <maxscale/modulecmd.h>
 #include <maxscale/router.h>
-#include <maxscale/server.h>
+#include <maxscale/server.hh>
 #include <maxscale/service.h>
 #include <maxscale/spinlock.h>
 #include <maxscale/users.h>
@@ -2233,7 +2233,11 @@ set_server(DCB *dcb, SERVER *server, char *bit)
 
     if ((bitvalue = server_map_status(bit)) != 0)
     {
-        server_set_status(server, bitvalue);
+        std::string errmsg;
+        if (!mxs::server_set_status(server, bitvalue, &errmsg))
+        {
+            dcb_printf(dcb, "%s\n", errmsg.c_str());
+        }
     }
     else
     {
@@ -2256,7 +2260,11 @@ clear_server(DCB *dcb, SERVER *server, char *bit)
 
     if ((bitvalue = server_map_status(bit)) != 0)
     {
-        server_clear_status(server, bitvalue);
+        std::string errmsg;
+        if (!mxs::server_clear_status(server, bitvalue, &errmsg))
+        {
+            dcb_printf(dcb, "%s", errmsg.c_str());
+        }
     }
     else
     {

server/modules/routing/maxinfo/maxinfo_exec.cc

@@ -41,6 +41,7 @@
 #include <maxscale/modutil.h>
 #include <maxscale/resultset.h>
 #include <maxscale/router.h>
+#include <maxscale/server.hh>
 #include <maxscale/service.h>
 #include <maxscale/spinlock.h>
 #include <maxscale/version.h>
@@ -392,10 +393,17 @@ void exec_set_server(DCB *dcb, MAXINFO_TREE *tree)
         int status = server_map_status(tree->right->value);
         if (status != 0)
         {
-            server_set_status(server, status);
+            std::string errmsgs;
+            if (mxs::server_set_status(server, status, &errmsgs))
+            {
                 maxinfo_send_ok(dcb);
             }
             else
+            {
+                maxinfo_send_error(dcb, 0, errmsgs.c_str());
+            }
+        }
+        else
         {
             if (strlen(tree->right->value) > 80) // Prevent buffer overrun
             {
@@ -473,10 +481,17 @@ void exec_clear_server(DCB *dcb, MAXINFO_TREE *tree)
         int status = server_map_status(tree->right->value);
         if (status != 0)
         {
-            server_clear_status(server, status);
+            std::string errmsgs;
+            if (mxs::server_clear_status(server, status, &errmsgs))
+            {
                 maxinfo_send_ok(dcb);
             }
             else
+            {
+                maxinfo_send_error(dcb, 0, errmsgs.c_str());
+            }
+        }
+        else
         {
             if (strlen(tree->right->value) > 80) // Prevent buffer overrun
             {