From 33b3b7e7004e3ec5501c32a2217ec32775a5a487 Mon Sep 17 00:00:00 2001
From: Ken Gaillot <kgaillot@redhat.com>
Date: Mon, 24 Apr 2017 10:38:50 -0500
Subject: [PATCH 1/6] Refactor: libcib: functionize destroying op callback
table
reduces code duplication
---
lib/cib/cib_client.c | 29 ++++++++++++-----------------
1 file changed, 12 insertions(+), 17 deletions(-)
diff --git a/lib/cib/cib_client.c b/lib/cib/cib_client.c
index 5ded943..0f53330 100644
--- a/lib/cib/cib_client.c
+++ b/lib/cib/cib_client.c
@@ -206,6 +206,15 @@ cib_destroy_op_callback(gpointer data)
free(blob);
}
+static void
+destroy_op_callback_table()
+{
+ if (cib_op_callback_table != NULL) {
+ g_hash_table_destroy(cib_op_callback_table);
+ cib_op_callback_table = NULL;
+ }
+}
+
char *
get_shadow_file(const char *suffix)
{
@@ -348,14 +357,7 @@ cib_new_variant(void)
new_cib = calloc(1, sizeof(cib_t));
- if (cib_op_callback_table != NULL) {
- g_hash_table_destroy(cib_op_callback_table);
- cib_op_callback_table = NULL;
- }
- if (cib_op_callback_table == NULL) {
- cib_op_callback_table = g_hash_table_new_full(g_direct_hash, g_direct_equal,
- NULL, cib_destroy_op_callback);
- }
+ remove_cib_op_callback(0, TRUE); /* remove all */
new_cib->call_id = 1;
new_cib->variant = cib_undefined;
@@ -419,10 +421,7 @@ cib_delete(cib_t * cib)
free(client);
}
- if(cib_op_callback_table) {
- g_hash_table_destroy(cib_op_callback_table);
- cib_op_callback_table = NULL;
- }
+ destroy_op_callback_table();
if(cib) {
cib->cmds->free(cib);
@@ -639,13 +638,9 @@ void
remove_cib_op_callback(int call_id, gboolean all_callbacks)
{
if (all_callbacks) {
- if (cib_op_callback_table != NULL) {
- g_hash_table_destroy(cib_op_callback_table);
- }
-
+ destroy_op_callback_table();
cib_op_callback_table = g_hash_table_new_full(g_direct_hash, g_direct_equal,
NULL, cib_destroy_op_callback);
-
} else {
g_hash_table_remove(cib_op_callback_table, GINT_TO_POINTER(call_id));
}
--
1.8.3.1
From 360cf350722f9646581e47b4d73d971aa81e6cec Mon Sep 17 00:00:00 2001
From: Ken Gaillot <kgaillot@redhat.com>
Date: Fri, 21 Apr 2017 11:31:01 -0500
Subject: [PATCH 2/6] Low: crmd: avoid use-after-free when disconnecting from
CIB
crmd_exit() freed the CIB connection, then drained the mainloop.
However, the mainloop could call functions (such as do_cib_control() or
cib_native_destroy()) that would use the connection object.
This would lead to a segfault, though the harm was minimal, since the crmd was
already exiting at this point.
Also log a notice comparable to what's done for the crmd's other disconnects.
---
crmd/cib.c | 17 ++++++-----------
crmd/control.c | 10 ++++++++--
include/crm/cib.h | 1 +
lib/cib/cib_client.c | 35 ++++++++++++++++++++++++-----------
4 files changed, 39 insertions(+), 24 deletions(-)
diff --git a/crmd/cib.c b/crmd/cib.c
index 40fed4a..41f4f3d 100644
--- a/crmd/cib.c
+++ b/crmd/cib.c
@@ -159,19 +159,16 @@ do_cib_replaced(const char *event, xmlNode * msg)
register_fsa_input(C_FSA_INTERNAL, I_ELECTION, NULL);
}
-/* A_CIB_STOP, A_CIB_START, A_CIB_RESTART, */
+/* A_CIB_STOP, A_CIB_START, O_CIB_RESTART */
void
do_cib_control(long long action,
enum crmd_fsa_cause cause,
enum crmd_fsa_state cur_state,
enum crmd_fsa_input current_input, fsa_data_t * msg_data)
{
- struct crm_subsystem_s *this_subsys = cib_subsystem;
+ CRM_ASSERT(fsa_cib_conn != NULL);
- long long stop_actions = A_CIB_STOP;
- long long start_actions = A_CIB_START;
-
- if (action & stop_actions) {
+ if (action & A_CIB_STOP) {
if (fsa_cib_conn->state != cib_disconnected && last_resource_update != 0) {
crm_info("Waiting for resource update %d to complete", last_resource_update);
@@ -181,7 +178,6 @@ do_cib_control(long long action,
crm_info("Disconnecting CIB");
clear_bit(fsa_input_register, R_CIB_CONNECTED);
- CRM_ASSERT(fsa_cib_conn != NULL);
fsa_cib_conn->cmds->del_notify_callback(fsa_cib_conn, T_CIB_DIFF_NOTIFY, do_cib_updated);
@@ -189,15 +185,14 @@ do_cib_control(long long action,
fsa_cib_conn->cmds->set_slave(fsa_cib_conn, cib_scope_local);
fsa_cib_conn->cmds->signoff(fsa_cib_conn);
}
+ crm_notice("Disconnected from the CIB");
}
- if (action & start_actions) {
+ if (action & A_CIB_START) {
int rc = pcmk_ok;
- CRM_ASSERT(fsa_cib_conn != NULL);
-
if (cur_state == S_STOPPING) {
- crm_err("Ignoring request to start %s after shutdown", this_subsys->name);
+ crm_err("Ignoring request to start the CIB after shutdown");
return;
}
diff --git a/crmd/control.c b/crmd/control.c
index af9c2c2..a9c0b73 100644
--- a/crmd/control.c
+++ b/crmd/control.c
@@ -355,8 +355,11 @@ crmd_exit(int rc)
election_fini(fsa_election);
fsa_election = NULL;
- cib_delete(fsa_cib_conn);
- fsa_cib_conn = NULL;
+ /* Tear down the CIB connection, but don't free it yet -- it could be used
+ * when we drain the mainloop later.
+ */
+ cib_free_callbacks(fsa_cib_conn);
+ fsa_cib_conn->cmds->signoff(fsa_cib_conn);
verify_stopped(fsa_state, LOG_WARNING);
clear_bit(fsa_input_register, R_LRM_CONNECTED);
@@ -485,6 +488,9 @@ crmd_exit(int rc)
mainloop_destroy_signal(SIGCHLD);
}
+ cib_delete(fsa_cib_conn);
+ fsa_cib_conn = NULL;
+
/* Graceful */
return rc;
}
diff --git a/include/crm/cib.h b/include/crm/cib.h
index a1246d1..ec52602 100644
--- a/include/crm/cib.h
+++ b/include/crm/cib.h
@@ -172,6 +172,7 @@ cib_t *cib_new_no_shadow(void);
char *get_shadow_file(const char *name);
cib_t *cib_shadow_new(const char *name);
+void cib_free_callbacks(cib_t *cib);
void cib_delete(cib_t * cib);
void cib_dump_pending_callbacks(void);
diff --git a/lib/cib/cib_client.c b/lib/cib/cib_client.c
index 0f53330..907bb5a 100644
--- a/lib/cib/cib_client.c
+++ b/lib/cib/cib_client.c
@@ -406,24 +406,37 @@ cib_new_variant(void)
return new_cib;
}
+/*!
+ * \brief Free all callbacks for a CIB connection
+ *
+ * \param[in] cib CIB connection to clean up
+ */
void
-cib_delete(cib_t * cib)
+cib_free_callbacks(cib_t *cib)
{
- GList *list = NULL;
- if(cib) {
- list = cib->notify_list;
- }
+ if (cib) {
+ GList *list = cib->notify_list;
- while (list != NULL) {
- cib_notify_client_t *client = g_list_nth_data(list, 0);
+ while (list != NULL) {
+ cib_notify_client_t *client = g_list_nth_data(list, 0);
- list = g_list_remove(list, client);
- free(client);
+ list = g_list_remove(list, client);
+ free(client);
+ }
}
-
destroy_op_callback_table();
+}
- if(cib) {
+/*!
+ * \brief Free all memory used by CIB connection
+ *
+ * \param[in] cib CIB connection to delete
+ */
+void
+cib_delete(cib_t *cib)
+{
+ cib_free_callbacks(cib);
+ if (cib) {
cib->cmds->free(cib);
}
}
--
1.8.3.1
From a6383a30d3258856efef1c067790faa78d1a5787 Mon Sep 17 00:00:00 2001
From: Ken Gaillot <kgaillot@redhat.com>
Date: Fri, 21 Apr 2017 11:33:53 -0500
Subject: [PATCH 3/6] Low: crmd: don't destroy election structure twice
didn't cause any harm, but unnecessary
---
crmd/control.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/crmd/control.c b/crmd/control.c
index a9c0b73..5d91b1d 100644
--- a/crmd/control.c
+++ b/crmd/control.c
@@ -388,7 +388,6 @@ crmd_exit(int rc)
free(integration_timer); integration_timer = NULL;
free(finalization_timer); finalization_timer = NULL;
free(election_trigger); election_trigger = NULL;
- election_fini(fsa_election);
free(shutdown_escalation_timer); shutdown_escalation_timer = NULL;
free(wait_timer); wait_timer = NULL;
free(recheck_timer); recheck_timer = NULL;
--
1.8.3.1
From 8183dbf57bc92a79b54c7e56942e4e3bda36654f Mon Sep 17 00:00:00 2001
From: Ken Gaillot <kgaillot@redhat.com>
Date: Mon, 1 May 2017 14:58:58 -0500
Subject: [PATCH 4/6] Log: libcib: downgrade ACL status message to trace
gets logged 1-3 times for every CIB op
---
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 1908401..ab48f16 100644
--- a/lib/cib/cib_utils.c
+++ b/lib/cib/cib_utils.c
@@ -282,7 +282,7 @@ cib_acl_enabled(xmlNode *xml, const char *user)
g_hash_table_destroy(options);
}
- crm_debug("CIB ACL is %s", rc ? "enabled" : "disabled");
+ crm_trace("CIB ACL is %s", rc ? "enabled" : "disabled");
#endif
return rc;
}
--
1.8.3.1
From f1f9bf7ceb3737ed4d731669c2e5744a9e234a04 Mon Sep 17 00:00:00 2001
From: Ken Gaillot <kgaillot@redhat.com>
Date: Mon, 1 May 2017 17:06:03 -0500
Subject: [PATCH 5/6] Low: libcrmcommon: don't delay next flush by more than 5
seconds
If an IPC client falls behind on processing incoming events, we set a
progressive delay between queue flushes. However, at the maximum backlog
of 500 events, this would be 51 seconds. Cap this delay at 5 seconds.
---
lib/common/ipc.c | 22 +++++++++++++++++++---
1 file changed, 19 insertions(+), 3 deletions(-)
diff --git a/lib/common/ipc.c b/lib/common/ipc.c
index 2949837..c9e3da2 100644
--- a/lib/common/ipc.c
+++ b/lib/common/ipc.c
@@ -463,12 +463,28 @@ crm_ipcs_flush_events_cb(gpointer data)
return FALSE;
}
+/*!
+ * \internal
+ * \brief Add progressive delay before next event queue flush
+ *
+ * \param[in,out] c Client connection to add delay to
+ * \param[in] queue_len Current event queue length
+ */
+static inline void
+delay_next_flush(crm_client_t *c, unsigned int queue_len)
+{
+ /* Delay a maximum of 5 seconds */
+ guint delay = (queue_len < 40)? (1000 + 100 * queue_len) : 5000;
+
+ c->event_timer = g_timeout_add(delay, crm_ipcs_flush_events_cb, c);
+}
+
ssize_t
crm_ipcs_flush_events(crm_client_t * c)
{
- int sent = 0;
ssize_t rc = 0;
- int queue_len = 0;
+ unsigned int sent = 0;
+ unsigned int queue_len = 0;
if (c == NULL) {
return pcmk_ok;
@@ -523,8 +539,8 @@ crm_ipcs_flush_events(crm_client_t * c)
qb_ipcs_disconnect(c->ipcs);
return rc;
}
+ delay_next_flush(c, queue_len);
- c->event_timer = g_timeout_add(1000 + 100 * queue_len, crm_ipcs_flush_events_cb, c);
}
return rc;
--
1.8.3.1
From 5d94da7af947810ca4b12dc1d9dce18a7d638f73 Mon Sep 17 00:00:00 2001
From: Ken Gaillot <kgaillot@redhat.com>
Date: Mon, 1 May 2017 17:39:02 -0500
Subject: [PATCH 6/6] Fix: libcrmcommon: avoid evicting IPC client if messages
spike briefly
Before, an IP server would evict a client if its event queue grew to 500
messages. Now, we avoid evicting if the backlog is new (a quick spike of many
messages that immediately crosses the threshold).
---
include/crm/common/ipcs.h | 5 ++++-
lib/common/ipc.c | 41 ++++++++++++++++++++++++++++++++---------
2 files changed, 36 insertions(+), 10 deletions(-)
diff --git a/include/crm/common/ipcs.h b/include/crm/common/ipcs.h
index 2fec931..ba1ccef 100644
--- a/include/crm/common/ipcs.h
+++ b/include/crm/common/ipcs.h
@@ -73,6 +73,8 @@ struct crm_client_s {
char *name;
char *user;
+ /* Provided for server use (not used by library) */
+ /* @TODO merge options, flags, and kind (reserving lower bits for server) */
long long options;
int request_id;
@@ -80,7 +82,7 @@ struct crm_client_s {
void *userdata;
int event_timer;
- GList *event_queue;
+ GList *event_queue; /* @TODO use GQueue instead */
/* Depending on the value of kind, only some of the following
* will be populated/valid
@@ -91,6 +93,7 @@ struct crm_client_s {
struct crm_remote_s *remote; /* TCP/TLS */
+ unsigned int backlog_len; /* IPC queue length after last flush */
};
extern GHashTable *client_connections;
diff --git a/lib/common/ipc.c b/lib/common/ipc.c
index c9e3da2..d32e373 100644
--- a/lib/common/ipc.c
+++ b/lib/common/ipc.c
@@ -37,6 +37,9 @@
#define PCMK_IPC_VERSION 1
+/* Evict clients whose event queue grows this large */
+#define PCMK_IPC_MAX_QUEUE 500
+
struct crm_ipc_response_header {
struct qb_ipc_response_header qb;
uint32_t size_uncompressed;
@@ -523,24 +526,44 @@ crm_ipcs_flush_events(crm_client_t * c)
}
queue_len -= sent;
- if (sent > 0 || c->event_queue) {
+ if (sent > 0 || queue_len) {
crm_trace("Sent %d events (%d remaining) for %p[%d]: %s (%lld)",
sent, queue_len, c->ipcs, c->pid,
pcmk_strerror(rc < 0 ? rc : 0), (long long) rc);
}
- if (c->event_queue) {
- if (queue_len % 100 == 0 && queue_len > 99) {
- crm_warn("Event queue for %p[%d] has grown to %d", c->ipcs, c->pid, queue_len);
+ 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.
+ */
- } else if (queue_len > 500) {
- crm_err("Evicting slow client %p[%d]: event queue reached %d entries",
- c->ipcs, c->pid, queue_len);
- qb_ipcs_disconnect(c->ipcs);
- return rc;
+ if (queue_len > PCMK_IPC_MAX_QUEUE) {
+ if ((c->backlog_len <= 1) || (queue_len < c->backlog_len)) {
+ /* Don't evict for a new or shrinking backlog */
+ crm_warn("Client with process ID %u has a backlog of %u messages "
+ CRM_XS " %p", c->pid, queue_len, c->ipcs);
+ } else {
+ crm_err("Evicting client with process ID %u due to backlog of %u messages "
+ CRM_XS " %p", c->pid, queue_len, c->ipcs);
+ c->backlog_len = 0;
+ qb_ipcs_disconnect(c->ipcs);
+ return rc;
+ }
}
+
+ c->backlog_len = queue_len;
delay_next_flush(c, queue_len);
+ } else {
+ /* Event queue is empty, there is no backlog */
+ c->backlog_len = 0;
}
return rc;
--
1.8.3.1