From 512c3c018d5df64f4fb9ce2cc841a39bc6512b00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Thu, 3 Aug 2017 09:13:20 +0300 Subject: [PATCH] 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 | 2 +- server/core/config.cc | 7 - server/core/config_runtime.cc | 26 ++-- server/core/maxscale/monitor.h | 4 +- server/core/monitor.cc | 149 ++++++++++++++++----- server/modules/routing/debugcli/debugcmd.c | 7 - 6 files changed, 131 insertions(+), 64 deletions(-) diff --git a/include/maxscale/monitor.h b/include/maxscale/monitor.h index d9a9cb3f5..06d76a067 100644 --- a/include/maxscale/monitor.h +++ b/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 */ }; diff --git a/server/core/config.cc b/server/core/config.cc index 90ec6962c..981611169 100644 --- a/server/core/config.cc +++ b/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 { diff --git a/server/core/config_runtime.cc b/server/core/config_runtime.cc index 1bcc0cdf0..d1770cd93 100644 --- a/server/core/config_runtime.cc +++ b/server/core/config_runtime.cc @@ -503,8 +503,6 @@ bool runtime_alter_monitor(MXS_MONITOR *monitor, const char *key, const char *va p.value = const_cast(value); monitorAddParameters(monitor, &p); } - - add_monitor_defaults(monitor); } monitorStart(monitor, monitor->parameters); @@ -512,11 +510,7 @@ bool runtime_alter_monitor(MXS_MONITOR *monitor, const char *key, const char *va if (valid) { - if (monitor->created_online) - { - monitor_serialize(monitor); - } - + 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 " - "after the next restart of MaxScale.", monitor->name); + monitorDestroy(monitor); + MXS_NOTICE("Destroyed monitor '%s'", monitor->name); } spinlock_release(&crt_lock); diff --git a/server/core/maxscale/monitor.h b/server/core/maxscale/monitor.h index e859cc0e7..448e1e821 100644 --- a/server/core/maxscale/monitor.h +++ b/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 *); diff --git a/server/core/monitor.cc b/server/core/monitor.cc index 995089c9b..aaed7a86d 100644 --- a/server/core/monitor.cc +++ b/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 * -monitor_alloc(char *name, char *module) +MXS_MONITOR* monitor_alloc(const char *name, const 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_FREE(name); + MXS_ERROR("Unable to load monitor module '%s'.", my_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); @@ -194,7 +192,10 @@ void monitorStartAll() ptr = allMonitors; while (ptr) { - monitorStart(ptr, ptr->parameters); + 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 */ @@ -245,7 +253,10 @@ monitorStopAll() ptr = allMonitors; while (ptr) { - monitorStop(ptr); + if (ptr->active) + { + monitorStop(ptr); + } ptr = ptr->next; } spinlock_release(&monLock); @@ -428,7 +439,10 @@ monitorShowAll(DCB *dcb) ptr = allMonitors; while (ptr) { - monitorShow(dcb, ptr); + if (ptr->active) + { + monitorShow(dcb, ptr); + } ptr = ptr->next; } spinlock_release(&monLock); @@ -518,9 +532,12 @@ monitorList(DCB *dcb) dcb_printf(dcb, "---------------------+---------------------\n"); while (ptr) { - dcb_printf(dcb, "%-20s | %s\n", ptr->name, - ptr->state & MONITOR_STATE_RUNNING - ? "Running" : "Stopped"); + 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); - if (clone) + MXS_CONFIG_PARAMETER* old = config_get_param(monitor->parameters, params->name); + + 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,11 +1314,14 @@ MXS_MONITOR* monitor_server_in_use(const SERVER *server) { spinlock_acquire(&mon->lock); - for (MXS_MONITOR_SERVERS *db = mon->databases; db && !rval; db = db->next) + if (mon->active) { - if (db->server == server) + for (MXS_MONITOR_SERVERS *db = mon->databases; db && !rval; db = db->next) { - rval = mon; + if (db->server == server) + { + rval = mon; + } } } @@ -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 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; @@ -1572,11 +1643,14 @@ json_t* monitor_list_to_json(const char* host) for (MXS_MONITOR* mon = allMonitors; mon; mon = mon->next) { - json_t *json = monitor_json_data(mon, host); - - if (json) + if (mon->active) { - json_array_append_new(rval, json); + json_t *json = monitor_json_data(mon, host); + + if (json) + { + json_array_append_new(rval, json); + } } } @@ -1595,12 +1669,15 @@ json_t* monitor_relations_to_server(const SERVER* server, const char* host) { spinlock_acquire(&mon->lock); - for (MXS_MONITOR_SERVERS* db = mon->databases; db; db = db->next) + if (mon->active) { - if (db->server == server) + for (MXS_MONITOR_SERVERS* db = mon->databases; db; db = db->next) { - mxs_json_add_relation(rel, mon->name, CN_MONITORS); - break; + if (db->server == server) + { + mxs_json_add_relation(rel, mon->name, CN_MONITORS); + break; + } } } diff --git a/server/modules/routing/debugcli/debugcmd.c b/server/modules/routing/debugcli/debugcmd.c index 0acebfa1c..06eaf2a02 100644 --- a/server/modules/routing/debugcli/debugcmd.c +++ b/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 {