From ae780515cd4db1e6f23db9f75a628ce9c39bdd49 Mon Sep 17 00:00:00 2001 From: Ken Gaillot Date: Fri, 9 Jun 2017 12:35:36 -0500 Subject: [PATCH 1/8] Refactor: libcrmcommon: remember when IPC client is root or cluster user will allow using a different eviction threshold --- include/crm/common/ipcs.h | 3 ++- lib/common/ipc.c | 13 ++++++++++--- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/include/crm/common/ipcs.h b/include/crm/common/ipcs.h index 52338e3..43b7b60 100644 --- a/include/crm/common/ipcs.h +++ b/include/crm/common/ipcs.h @@ -60,7 +60,8 @@ struct crm_remote_s { enum crm_client_flags { - crm_client_flag_ipc_proxied = 0x00001, /* ipc_proxy code only */ + crm_client_flag_ipc_proxied = 0x00001, /* ipc_proxy code only */ + crm_client_flag_ipc_privileged = 0x00002, /* root or cluster user */ }; struct crm_client_s { diff --git a/lib/common/ipc.c b/lib/common/ipc.c index 50980ec..e0a7a5c 100644 --- a/lib/common/ipc.c +++ b/lib/common/ipc.c @@ -314,6 +314,7 @@ crm_client_alloc(void *key) crm_client_t * crm_client_new(qb_ipcs_connection_t * c, uid_t uid_client, gid_t gid_client) { + static gid_t uid_cluster = 0; static gid_t gid_cluster = 0; crm_client_t *client = NULL; @@ -323,11 +324,12 @@ crm_client_new(qb_ipcs_connection_t * c, uid_t uid_client, gid_t gid_client) return NULL; } - if (gid_cluster == 0) { - if(crm_user_lookup(CRM_DAEMON_USER, NULL, &gid_cluster) < 0) { + if (uid_cluster == 0) { + if (crm_user_lookup(CRM_DAEMON_USER, &uid_cluster, &gid_cluster) < 0) { static bool have_error = FALSE; if(have_error == FALSE) { - crm_warn("Could not find group for user %s", CRM_DAEMON_USER); + crm_warn("Could not find user and group IDs for user %s", + CRM_DAEMON_USER); have_error = TRUE; } } @@ -347,6 +349,11 @@ crm_client_new(qb_ipcs_connection_t * c, uid_t uid_client, gid_t gid_client) client->kind = CRM_CLIENT_IPC; client->pid = crm_ipcs_client_pid(c); + if ((uid_client == 0) || (uid_client == uid_cluster)) { + /* Remember when a connection came from root or hacluster */ + set_bit(client->flags, crm_client_flag_ipc_privileged); + } + crm_debug("Connecting %p for uid=%d gid=%d pid=%u id=%s", c, uid_client, gid_client, client->pid, client->id); #if ENABLE_ACL -- 1.8.3.1 From f93ce6bdd6be5d2670ab0cd8dd10d3b8b9972a65 Mon Sep 17 00:00:00 2001 From: Ken Gaillot Date: Fri, 9 Jun 2017 14:46:16 -0500 Subject: [PATCH 2/8] Low: libcrmcommon: add function for testing daemon name --- include/crm/common/util.h | 2 ++ lib/common/utils.c | 21 +++++++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/include/crm/common/util.h b/include/crm/common/util.h index 682b346..aa192f9 100644 --- a/include/crm/common/util.h +++ b/include/crm/common/util.h @@ -26,6 +26,7 @@ # include # include +# include # include # include # include @@ -127,6 +128,7 @@ gboolean did_rsc_op_fail(lrmd_event_data_t * event, int target_rc); char *crm_md5sum(const char *buffer); char *crm_generate_uuid(void); +bool crm_is_daemon_name(const char *name); int crm_user_lookup(const char *name, uid_t * uid, gid_t * gid); diff --git a/lib/common/utils.c b/lib/common/utils.c index 27ed60d..a652197 100644 --- a/lib/common/utils.c +++ b/lib/common/utils.c @@ -1959,6 +1959,27 @@ crm_generate_uuid(void) return buffer; } +/*! + * \brief Check whether a string represents a cluster daemon name + * + * \param[in] name String to check + * + * \return TRUE if name is standard client name used by daemons, FALSE otherwise + */ +bool +crm_is_daemon_name(const char *name) +{ + return (name && + (!strcmp(name, CRM_SYSTEM_CRMD) + || !strcmp(name, CRM_SYSTEM_STONITHD) + || !strcmp(name, T_ATTRD) + || !strcmp(name, CRM_SYSTEM_CIB) + || !strcmp(name, CRM_SYSTEM_MCP) + || !strcmp(name, CRM_SYSTEM_DC) + || !strcmp(name, CRM_SYSTEM_TENGINE) + || !strcmp(name, CRM_SYSTEM_LRMD))); +} + #include char * -- 1.8.3.1 From 19497fa9785c854084253167b7dc54a8c026e1ad Mon Sep 17 00:00:00 2001 From: Ken Gaillot Date: Fri, 9 Jun 2017 11:11:50 -0500 Subject: [PATCH 3/8] Low: libcrmcommon: support setting max queue length per IPC client --- include/crm/common/ipcs.h | 3 +++ lib/common/ipc.c | 39 ++++++++++++++++++++++++++------------- 2 files changed, 29 insertions(+), 13 deletions(-) diff --git a/include/crm/common/ipcs.h b/include/crm/common/ipcs.h index 43b7b60..d2db212 100644 --- a/include/crm/common/ipcs.h +++ b/include/crm/common/ipcs.h @@ -19,6 +19,7 @@ #ifndef CRM_COMMON_IPCS__H # define CRM_COMMON_IPCS__H +# include # include # ifdef HAVE_GNUTLS_GNUTLS_H # undef KEYFILE @@ -95,6 +96,7 @@ struct crm_client_s { struct crm_remote_s *remote; /* TCP/TLS */ unsigned int queue_backlog; /* IPC queue length after last flush */ + unsigned int queue_max; /* Evict client whose queue grows this big */ }; extern GHashTable *client_connections; @@ -110,6 +112,7 @@ crm_client_t *crm_client_alloc(void *key); crm_client_t *crm_client_new(qb_ipcs_connection_t * c, uid_t uid, gid_t gid); void crm_client_destroy(crm_client_t * c); void crm_client_disconnect_all(qb_ipcs_service_t *s); +bool crm_set_client_queue_max(crm_client_t *client, const char *qmax); void crm_ipcs_send_ack(crm_client_t * c, uint32_t request, uint32_t flags, const char *tag, const char *function, int line); diff --git a/lib/common/ipc.c b/lib/common/ipc.c index e0a7a5c..3110334 100644 --- a/lib/common/ipc.c +++ b/lib/common/ipc.c @@ -37,8 +37,9 @@ #define PCMK_IPC_VERSION 1 -/* Evict clients whose event queue grows this large */ -#define PCMK_IPC_MAX_QUEUE 500 +/* Evict clients whose event queue grows this large (by default) */ +#define PCMK_IPC_DEFAULT_QUEUE_MAX 500 +#define PCMK_IPC_DEFAULT_QUEUE_MAX_S "500" struct crm_ipc_response_header { struct qb_ipc_response_header qb; @@ -409,6 +410,24 @@ crm_client_destroy(crm_client_t * c) free(c); } +/*! + * \brief Raise IPC eviction threshold for a client, if allowed + * + * \param[in,out] client Client to modify + * \param[in] queue_max New threshold (as string) + * + * \return TRUE if change was allowed, FALSE otherwise + */ +bool +crm_set_client_queue_max(crm_client_t *client, const char *qmax) +{ + if (is_set(client->flags, crm_client_flag_ipc_privileged)) { + client->queue_max = crm_parse_int(qmax, PCMK_IPC_DEFAULT_QUEUE_MAX_S); + return TRUE; + } + return FALSE; +} + int crm_ipcs_client_pid(qb_ipcs_connection_t * c) { @@ -553,18 +572,12 @@ crm_ipcs_flush_events(crm_client_t * c) } if (queue_len) { - /* We want to allow clients to briefly fall behind on processing - * incoming messages, but drop completely unresponsive clients so the - * connection doesn't consume resources indefinitely. - * - * @TODO It is possible that the queue could reasonably grow large in a - * short time. An example is a reprobe of hundreds of resources on many - * nodes resulting in a surge of CIB replies to the crmd. We could - * possibly give cluster daemons a higher threshold here, and/or prevent - * such a surge by throttling LRM history writes in the crmd. - */ - if (queue_len > PCMK_IPC_MAX_QUEUE) { + /* Allow clients to briefly fall behind on processing incoming messages, + * but drop completely unresponsive clients so the connection doesn't + * consume resources indefinitely. + */ + if (queue_len > QB_MAX(c->queue_max, PCMK_IPC_DEFAULT_QUEUE_MAX)) { if ((c->queue_backlog <= 1) || (queue_len < c->queue_backlog)) { /* Don't evict for a new or shrinking backlog */ crm_warn("Client with process ID %u has a backlog of %u messages " -- 1.8.3.1 From 324e241d9d9666dfe6543ddf88d965b12d5a164f Mon Sep 17 00:00:00 2001 From: Ken Gaillot Date: Mon, 12 Jun 2017 17:47:12 -0500 Subject: [PATCH 4/8] Low: libcib: always use current values when unpacking config Previously, cib_read_config() called unpack_instance_attributes() with overwrite=FALSE. This meant that changes to an option would not take effect unless the option was not set before. The only significant use of cib_read_config() was in cib_acl_enabled(), which used a new, empty hash table for every call, so the issue didn't matter. The cib daemon also used cib_read_config() to maintain a global config_hash, which was affected by the issue, but didn't matter because it was never used. This change will allow config_hash to be used. --- lib/cib/cib_utils.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/cib/cib_utils.c b/lib/cib/cib_utils.c index ab48f16..8aeed67 100644 --- a/lib/cib/cib_utils.c +++ b/lib/cib/cib_utils.c @@ -743,7 +743,7 @@ cib_read_config(GHashTable * options, xmlNode * current_cib) config = get_object_root(XML_CIB_TAG_CRMCONFIG, current_cib); if (config) { unpack_instance_attributes(current_cib, config, XML_CIB_TAG_PROPSET, NULL, options, - CIB_OPTIONS_FIRST, FALSE, now); + CIB_OPTIONS_FIRST, TRUE, now); } verify_cib_options(options); -- 1.8.3.1 From b986acbe131216aaa372681e97dfc0f2ef8f70ad Mon Sep 17 00:00:00 2001 From: Ken Gaillot Date: Tue, 13 Jun 2017 16:03:36 -0500 Subject: [PATCH 5/8] Low: libcib: correctly search for v2 patchset changes cib_internal_config_changed() was never updated for v2 patch format --- lib/cib/cib_utils.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/lib/cib/cib_utils.c b/lib/cib/cib_utils.c index 8aeed67..f639ada 100644 --- a/lib/cib/cib_utils.c +++ b/lib/cib/cib_utils.c @@ -794,23 +794,24 @@ cib_apply_patch_event(xmlNode * event, xmlNode * input, xmlNode ** output, int l return rc; } +/* v2 and v2 patch formats */ +#define XPATH_CONFIG_CHANGE \ + "//" XML_CIB_TAG_CRMCONFIG " | " \ + "//" XML_DIFF_CHANGE "[contains(@" XML_DIFF_PATH ",'/" XML_CIB_TAG_CRMCONFIG "/')]" + gboolean -cib_internal_config_changed(xmlNode * diff) +cib_internal_config_changed(xmlNode *diff) { gboolean changed = FALSE; - xmlXPathObject *xpathObj = NULL; - if (diff == NULL) { - return FALSE; - } + if (diff) { + xmlXPathObject *xpathObj = xpath_search(diff, XPATH_CONFIG_CHANGE); - xpathObj = xpath_search(diff, "//" XML_CIB_TAG_CRMCONFIG); - if (numXpathResults(xpathObj) > 0) { - changed = TRUE; + if (numXpathResults(xpathObj) > 0) { + changed = TRUE; + } + freeXpathObject(xpathObj); } - - freeXpathObject(xpathObj); - return changed; } -- 1.8.3.1 From 3e5cdd9b329b3328a88912db48c4e4a4d6f94563 Mon Sep 17 00:00:00 2001 From: Ken Gaillot Date: Fri, 9 Jun 2017 15:33:27 -0500 Subject: [PATCH 6/8] Feature: cib,libcib: support option for IPC eviction threshold Only the cib utilizes the option currently, but the other daemons could easily do the same, if they are subject to large IPC bursts. --- cib/callbacks.c | 13 +++++++++++++ cib/callbacks.h | 10 ++++++++++ lib/cib/cib_utils.c | 24 ++++++++++++++++++++---- 3 files changed, 43 insertions(+), 4 deletions(-) diff --git a/cib/callbacks.c b/cib/callbacks.c index 4708f10..544a920 100644 --- a/cib/callbacks.c +++ b/cib/callbacks.c @@ -281,6 +281,19 @@ cib_common_callback(qb_ipcs_connection_t * c, void *data, size_t size, gboolean cib_client->name = crm_itoa(cib_client->pid); } else { cib_client->name = strdup(value); + if (crm_is_daemon_name(value)) { + set_bit(cib_client->options, cib_is_daemon); + } + } + } + + /* Allow cluster daemons more leeway before being evicted */ + if (is_set(cib_client->options, cib_is_daemon)) { + const char *qmax = cib_config_lookup("cluster-ipc-limit"); + + if (crm_set_client_queue_max(cib_client, qmax)) { + crm_trace("IPC threshold for %s[%u] is now %u", + cib_client->name, cib_client->pid, cib_client->queue_max); } } diff --git a/cib/callbacks.h b/cib/callbacks.h index b4d48d6..bddff09 100644 --- a/cib/callbacks.h +++ b/cib/callbacks.h @@ -19,6 +19,7 @@ #include #include #include +#include #include #include @@ -43,6 +44,9 @@ enum cib_notifications cib_notify_replace = 0x0004, cib_notify_confirm = 0x0008, cib_notify_diff = 0x0010, + + /* not a notification, but uses the same IPC bitmask */ + cib_is_daemon = 0x1000, /* whether client is another cluster daemon */ }; /* *INDENT-ON* */ @@ -80,3 +84,9 @@ extern void cib_ha_peer_callback(HA_Message * msg, void *private_data); extern int cib_ccm_dispatch(gpointer user_data); extern void cib_ccm_msg_callback(oc_ed_t event, void *cookie, size_t size, const void *data); #endif + +static inline const char * +cib_config_lookup(const char *opt) +{ + return g_hash_table_lookup(config_hash, opt); +} diff --git a/lib/cib/cib_utils.c b/lib/cib/cib_utils.c index f639ada..adbf9c8 100644 --- a/lib/cib/cib_utils.c +++ b/lib/cib/cib_utils.c @@ -699,10 +699,26 @@ cib_native_notify(gpointer data, gpointer user_data) } pe_cluster_option cib_opts[] = { - /* name, old-name, validate, default, description */ - {"enable-acl", NULL, "boolean", NULL, "false", &check_boolean, - "Enable CIB ACL", NULL} - , + /* + * name, legacy name, + * type, allowed values, default, validator, + * short description, + * long description + */ + { + "enable-acl", NULL, + "boolean", NULL, "false", &check_boolean, + "Enable CIB ACL", + NULL + }, + { + "cluster-ipc-limit", NULL, + "integer", NULL, "500", &check_positive_number, + "Maximum IPC message backlog before disconnecting a cluster daemon", + "Raise this if log has \"Evicting client\" messages for cluster daemon" + " PIDs (a good value is the number of resources in the cluster" + " multiplied by the number of nodes)" + }, }; void -- 1.8.3.1 From 3f855794adbe9ad46aaca3848d591f5dc26d3371 Mon Sep 17 00:00:00 2001 From: Ken Gaillot Date: Tue, 13 Jun 2017 18:07:45 -0500 Subject: [PATCH 7/8] Doc: Pacemaker Explained: document cluster-ipc-limit --- doc/Pacemaker_Explained/en-US/Ch-Options.txt | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/doc/Pacemaker_Explained/en-US/Ch-Options.txt b/doc/Pacemaker_Explained/en-US/Ch-Options.txt index f4e1af7..ec0c6b9 100644 --- a/doc/Pacemaker_Explained/en-US/Ch-Options.txt +++ b/doc/Pacemaker_Explained/en-US/Ch-Options.txt @@ -267,6 +267,15 @@ take effect, we can optionally poll the cluster's status for changes. A value of 0 disables polling. Positive values are an interval (in seconds unless other SI units are specified, e.g. 5min). +| cluster-ipc-limit | 500 | +indexterm:[cluster-ipc-limit,Cluster Option] +indexterm:[Cluster,Option,cluster-ipc-limit] +The maximum IPC message backlog before one cluster daemon will disconnect +another. This is of use in large clusters, for which a good value is the number +of resources in the cluster multiplied by the number of nodes. The default of +500 is also the minimum. Raise this if you see "Evicting client" messages for +cluster daemon PIDs in the logs. + | pe-error-series-max | -1 | indexterm:[pe-error-series-max,Cluster Option] indexterm:[Cluster,Option,pe-error-series-max] -- 1.8.3.1 From bc6d723a94221419b15650a52e8ed1d843a32ff1 Mon Sep 17 00:00:00 2001 From: Ken Gaillot Date: Thu, 15 Jun 2017 14:20:27 -0500 Subject: [PATCH 8/8] Refactor: libcrmcommon: avoid redundant constant definitions --- lib/common/ipc.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/common/ipc.c b/lib/common/ipc.c index 3110334..c238bca 100644 --- a/lib/common/ipc.c +++ b/lib/common/ipc.c @@ -39,7 +39,6 @@ /* Evict clients whose event queue grows this large (by default) */ #define PCMK_IPC_DEFAULT_QUEUE_MAX 500 -#define PCMK_IPC_DEFAULT_QUEUE_MAX_S "500" struct crm_ipc_response_header { struct qb_ipc_response_header qb; @@ -422,8 +421,12 @@ bool crm_set_client_queue_max(crm_client_t *client, const char *qmax) { if (is_set(client->flags, crm_client_flag_ipc_privileged)) { - client->queue_max = crm_parse_int(qmax, PCMK_IPC_DEFAULT_QUEUE_MAX_S); - return TRUE; + int qmax_int = crm_int_helper(qmax, NULL); + + if ((errno == 0) && (qmax_int > 0)) { + client->queue_max = qmax_int; + return TRUE; + } } return FALSE; } -- 1.8.3.1