Add recycling of destroyed monitors

If a destroyed monitor is created again, it will be reused. This should
prevent excessive memory growth when the same monitor is created and
destroyed again.

include/maxscale/monitor.h

@@ -198,10 +198,10 @@ struct mxs_monitor
     char *module_name;            /**< Name of the monitor module */
     void *handle;                 /**< Handle returned from startMonitor */
     size_t interval;              /**< The monitor interval */
-    bool created_online;          /**< Whether this monitor was created at runtime */
     volatile bool server_pending_changes;
     /**< Are there any pending changes to a server?
        * If yes, the next monitor loop starts early.  */
+    bool active; /**< True if monitor is active */
     struct mxs_monitor *next;     /**< Next monitor in the linked list */
 };
 

server/core/config.cc

@@ -3112,13 +3112,6 @@ int create_new_monitor(CONFIG_CONTEXT *context, CONFIG_CONTEXT *obj, HASHTABLE*
             MXS_ERROR("Failed to create monitor '%s'.", obj->object);
             error_count++;
         }
-
-        if (obj->was_persisted)
-        {
-            /** Not the cleanest way of figuring out whether the configuration
-             * was stored but it should be OK for now */
-            ((MXS_MONITOR*)obj->element)->created_online = true;
-        }
     }
     else
     {

server/core/config_runtime.cc

@@ -503,20 +503,14 @@ bool runtime_alter_monitor(MXS_MONITOR *monitor, const char *key, const char *va
                 p.value = const_cast<char*>(value);
                 monitorAddParameters(monitor, &p);
             }
-
-            add_monitor_defaults(monitor);
         }
 
         monitorStart(monitor, monitor->parameters);
     }
 
     if (valid)
-    {
-        if (monitor->created_online)
     {
         monitor_serialize(monitor);
-        }
-
         MXS_NOTICE("Updated monitor '%s': %s=%s", monitor->name, key, value);
     }
     else
@@ -852,12 +846,20 @@ bool runtime_create_monitor(const char *name, const char *module)
 
     if (monitor_find(name) == NULL)
     {
-        MXS_MONITOR *monitor = monitor_alloc((char*)name, (char*)module);
+
+        MXS_MONITOR *monitor = monitor_find_destroyed(name);
+
+        if (monitor)
+        {
+            monitor->active = true;
+        }
+        else
+        {
+            monitor = monitor_alloc(name, module);
+        }
 
         if (monitor)
         {
-            /** Mark that this monitor was created after MaxScale was started */
-            monitor->created_online = true;
             add_monitor_defaults(monitor);
 
             if (monitor_serialize(monitor))
@@ -912,8 +914,8 @@ bool runtime_destroy_monitor(MXS_MONITOR *monitor)
         {
             monitorRemoveServer(monitor, monitor->databases->server);
         }
-        MXS_NOTICE("Destroyed monitor '%s'. The monitor will be removed "
+        monitorDestroy(monitor);
-                   "after the next restart of MaxScale.", monitor->name);
+        MXS_NOTICE("Destroyed monitor '%s'", monitor->name);
     }
 
     spinlock_release(&crt_lock);

server/core/maxscale/monitor.h

@@ -40,15 +40,17 @@ typedef enum
     MONITOR_CONNECT_ATTEMPTS = 3
 } monitor_timeouts_t;
 
-MXS_MONITOR *monitor_alloc(char *, char *);
+MXS_MONITOR *monitor_alloc(const char *, const char *);
 void monitor_free(MXS_MONITOR *);
 
 void monitorStart(MXS_MONITOR *, const MXS_CONFIG_PARAMETER*);
 void monitorStop(MXS_MONITOR *);
+void monitorDestroy(MXS_MONITOR* monitor);
 void monitorStopAll();
 void monitorStartAll();
 
 MXS_MONITOR *monitor_find(const char *);
+MXS_MONITOR* monitor_find_destroyed(const char *name);
 
 void monitorShow(DCB *, MXS_MONITOR *);
 void monitorShowAll(DCB *);

server/core/monitor.cc

@@ -72,17 +72,15 @@ static unsigned int all_server_bits = SERVER_RUNNING |  SERVER_MAINT |
  * @param module        The module to load
  * @return      The newly created monitor
  */
-MXS_MONITOR *
+MXS_MONITOR* monitor_alloc(const char *name, const char *module)
-monitor_alloc(char *name, char *module)
 {
-    name = MXS_STRDUP(name);
+    char* my_name = MXS_STRDUP(name);
     char *my_module = MXS_STRDUP(module);
-
     MXS_MONITOR *mon = (MXS_MONITOR *)MXS_MALLOC(sizeof(MXS_MONITOR));
 
-    if (!name || !mon || !my_module)
+    if (!my_name || !mon || !my_module)
     {
-        MXS_FREE(name);
+        MXS_FREE(my_name);
         MXS_FREE(mon);
         MXS_FREE(my_module);
         return NULL;
@@ -90,13 +88,14 @@ monitor_alloc(char *name, char *module)
 
     if ((mon->module = (MXS_MONITOR_OBJECT*)load_module(module, MODULE_MONITOR)) == NULL)
     {
-        MXS_ERROR("Unable to load monitor module '%s'.", name);
+        MXS_ERROR("Unable to load monitor module '%s'.", my_name);
-        MXS_FREE(name);
+        MXS_FREE(my_name);
         MXS_FREE(mon);
         return NULL;
     }
+    mon->active = true;
     mon->state = MONITOR_STATE_ALLOC;
-    mon->name = name;
+    mon->name = my_name;
     mon->module_name = my_module;
     mon->handle = NULL;
     mon->databases = NULL;
@@ -108,7 +107,6 @@ monitor_alloc(char *name, char *module)
     mon->connect_attempts = DEFAULT_CONNECTION_ATTEMPTS;
     mon->interval = MONITOR_DEFAULT_INTERVAL;
     mon->parameters = NULL;
-    mon->created_online = false;
     mon->server_pending_changes = false;
     spinlock_init(&mon->lock);
     spinlock_acquire(&monLock);
@@ -193,8 +191,11 @@ void monitorStartAll()
     spinlock_acquire(&monLock);
     ptr = allMonitors;
     while (ptr)
+    {
+        if (ptr->active)
         {
             monitorStart(ptr, ptr->parameters);
+        }
         ptr = ptr->next;
     }
     spinlock_release(&monLock);
@@ -233,6 +234,13 @@ monitorStop(MXS_MONITOR *monitor)
     }
 }
 
+void monitorDestroy(MXS_MONITOR* monitor)
+{
+    spinlock_acquire(&monitor->lock);
+    monitor->active = false;
+    spinlock_release(&monitor->lock);
+}
+
 /**
  * Shutdown all running monitors
  */
@@ -244,8 +252,11 @@ monitorStopAll()
     spinlock_acquire(&monLock);
     ptr = allMonitors;
     while (ptr)
+    {
+        if (ptr->active)
         {
             monitorStop(ptr);
+        }
         ptr = ptr->next;
     }
     spinlock_release(&monLock);
@@ -427,8 +438,11 @@ monitorShowAll(DCB *dcb)
     spinlock_acquire(&monLock);
     ptr = allMonitors;
     while (ptr)
+    {
+        if (ptr->active)
         {
             monitorShow(dcb, ptr);
+        }
         ptr = ptr->next;
     }
     spinlock_release(&monLock);
@@ -517,10 +531,13 @@ monitorList(DCB *dcb)
     dcb_printf(dcb, "%-20s | Status\n", "Monitor");
     dcb_printf(dcb, "---------------------+---------------------\n");
     while (ptr)
+    {
+        if (ptr->active)
         {
             dcb_printf(dcb, "%-20s | %s\n", ptr->name,
                        ptr->state & MONITOR_STATE_RUNNING
                        ? "Running" : "Stopped");
+        }
         ptr = ptr->next;
     }
     dcb_printf(dcb, "---------------------+---------------------\n");
@@ -542,7 +559,7 @@ monitor_find(const char *name)
     ptr = allMonitors;
     while (ptr)
     {
-        if (!strcmp(ptr->name, name))
+        if (!strcmp(ptr->name, name) && ptr->active)
         {
             break;
         }
@@ -551,6 +568,30 @@ monitor_find(const char *name)
     spinlock_release(&monLock);
     return ptr;
 }
+/**
+ * Find a destroyed monitor by name
+ *
+ * @param name The name of the monitor
+ * @return  Pointer to the destroyed monitor or NULL if monitor is not found
+ */
+MXS_MONITOR* monitor_find_destroyed(const char *name)
+{
+    MXS_MONITOR* rval = NULL;
+
+    spinlock_acquire(&monLock);
+
+    for (MXS_MONITOR *ptr = allMonitors; ptr; ptr = ptr->next)
+    {
+        if (!strcmp(ptr->name, name))
+        {
+            rval = ptr;
+        }
+    }
+
+    spinlock_release(&monLock);
+
+    return rval;
+}
 
 /**
  * Set the monitor sampling interval.
@@ -763,12 +804,20 @@ void monitorAddParameters(MXS_MONITOR *monitor, MXS_CONFIG_PARAMETER *params)
 {
     while (params)
     {
-        MXS_CONFIG_PARAMETER* clone = config_clone_param(params);
+        MXS_CONFIG_PARAMETER* old = config_get_param(monitor->parameters, params->name);
-        if (clone)
+
+        if (old)
         {
+            MXS_FREE(old->value);
+            old->value = MXS_STRDUP_A(params->value);
+        }
+        else
+        {
+            MXS_CONFIG_PARAMETER* clone = config_clone_param(params);
             clone->next = monitor->parameters;
             monitor->parameters = clone;
         }
+
         params = params->next;
     }
 }
@@ -1265,6 +1314,8 @@ MXS_MONITOR* monitor_server_in_use(const SERVER *server)
     {
         spinlock_acquire(&mon->lock);
 
+        if (mon->active)
+        {
             for (MXS_MONITOR_SERVERS *db = mon->databases; db && !rval; db = db->next)
             {
                 if (db->server == server)
@@ -1272,6 +1323,7 @@ MXS_MONITOR* monitor_server_in_use(const SERVER *server)
                     rval = mon;
                 }
             }
+        }
 
         spinlock_release(&mon->lock);
     }
@@ -1292,12 +1344,6 @@ static bool create_monitor_config(const MXS_MONITOR *monitor, const char *filena
         return false;
     }
 
-    /**
-     * Only additional parameters are added to the configuration. This prevents
-     * duplication or addition of parameters that don't support it.
-     *
-     * TODO: Check for return values on all of the dprintf calls
-     */
     dprintf(file, "[%s]\n", monitor->name);
     dprintf(file, "%s=monitor\n", CN_TYPE);
     dprintf(file, "%s=%s\n", CN_MODULE, monitor->module_name);
@@ -1323,6 +1369,31 @@ static bool create_monitor_config(const MXS_MONITOR *monitor, const char *filena
         dprintf(file, "\n");
     }
 
+    const char* params[] =
+    {
+        CN_TYPE,
+        CN_MODULE,
+        CN_USER,
+        CN_PASSWORD,
+        "passwd", // TODO: Remove this
+        CN_MONITOR_INTERVAL,
+        CN_BACKEND_CONNECT_TIMEOUT,
+        CN_BACKEND_WRITE_TIMEOUT,
+        CN_BACKEND_READ_TIMEOUT,
+        CN_BACKEND_CONNECT_ATTEMPTS,
+        CN_SERVERS
+    };
+
+    std::set<std::string> param_set(params, params + sizeof(params) / sizeof(params[0]));
+
+    for (MXS_CONFIG_PARAMETER* p = monitor->parameters; p; p = p->next)
+    {
+        if (param_set.find(p->name) == param_set.end())
+        {
+            dprintf(file, "%s=%s\n", p->name, p->value);
+        }
+    }
+
     close(file);
 
     return true;
@@ -1571,6 +1642,8 @@ json_t* monitor_list_to_json(const char* host)
     spinlock_acquire(&monLock);
 
     for (MXS_MONITOR* mon = allMonitors; mon; mon = mon->next)
+    {
+        if (mon->active)
         {
             json_t *json = monitor_json_data(mon, host);
 
@@ -1579,6 +1652,7 @@ json_t* monitor_list_to_json(const char* host)
                 json_array_append_new(rval, json);
             }
         }
+    }
 
     spinlock_release(&monLock);
 
@@ -1595,6 +1669,8 @@ json_t* monitor_relations_to_server(const SERVER* server, const char* host)
     {
         spinlock_acquire(&mon->lock);
 
+        if (mon->active)
+        {
             for (MXS_MONITOR_SERVERS* db = mon->databases; db; db = db->next)
             {
                 if (db->server == server)
@@ -1603,6 +1679,7 @@ json_t* monitor_relations_to_server(const SERVER* server, const char* host)
                     break;
                 }
             }
+        }
 
         spinlock_release(&mon->lock);
     }

server/modules/routing/debugcli/debugcmd.c

@@ -1445,13 +1445,6 @@ static void alterMonitor(DCB *dcb, MXS_MONITOR *monitor, char *v1, char *v2, cha
             {
                 dcb_printf(dcb, "Error: Bad key-value parameter: %s=%s\n", key, value);
             }
-            else if (!monitor->created_online)
-            {
-                dcb_printf(dcb, "Warning: Altered monitor '%s' which is in the "
-                           "main\nconfiguration file. These changes will not be "
-                           "persisted and need\nto be manually added or set again"
-                           "after a restart.\n", monitor->name);
-            }
         }
         else
         {