Low: daemons: Fix removing transient attrs on rolling upgrades.

The previous patches to fix race conditions in removing transient
attributes rely on this sequence of events:

* First, attrd notices that a peer has left the cluster at the corosync
  level.  This causes it to set the values of all transient attributes
  to NULL.
* Second, based updates the CIB to remove transient attributes, which
  triggers a callback in attrd.  This callback removes all attributes
  whose value is NULL.

This seems to work reliably in same-version clusters.  However, in mixed
version clusters (like if you're doing a rolling upgrade), this doesn't
always happen.  In fact in my testing, almost every time, the order of
those two operations is reversed.

What's happening here is that in the mixed version cluster, the DC
(which will still be the old, un-upgraded cluster version) still has a
controld that deletes transient attributes.  It will be sending out
requests that based performs this change to the CIB, which conflicts
with the entire point of these patches - that controld is no longer in
the business of deleting transient attributes.

I think there's no way around this problem.  No matter how you update
cluster nodes, you'll always have a DC that's the old version with an
old controld.  If you take the existing DC out of the cluster to update
it, another existing node will become the DC and it will be the problem
node.

The fix here is to bump the attrd protocol version so we know if a node
has been updated or not.  This is probably reasonable to do for the
change in the first place.  Then, attrd can inspect the protocol version
of the peer that is telling it to remove a transient attribute.  If the
peer version is old enough, attrd can simply remove the transient
attribute immediately instead of doing the NULL-then-delete process.

Author: clumens

daemons/attrd/attrd_attributes.c

@@ -312,16 +312,23 @@ attrd_for_cib(const attribute_t *a)
  *                      combination
  */
 static void
-drop_removed_values(const char *cib_id, const char *set_type,
+drop_removed_values(const char *cib_id, const char *set_type, int peer_attrd_ver,
                     bool (*func)(const attribute_t *, const char *,
                                  const char *))
 {
+    int our_attrd_ver;
+    bool drop_immediately = false;
     attribute_t *a = NULL;
     GHashTableIter attr_iter;
     const char *entry_type = pcmk__s(set_type, "status entry"); // for log
 
     CRM_CHECK((cib_id != NULL) && (func != NULL), return);
 
+    pcmk__scan_min_int(ATTRD_PROTOCOL_VERSION, &our_attrd_ver, -1);
+    drop_immediately = (peer_attrd_ver != -1)
+                        && ATTRD_SUPPORTS_CLEARING_CIB(our_attrd_ver)
+                        && !ATTRD_SUPPORTS_CLEARING_CIB(peer_attrd_ver);
+
     // Check every attribute ...
     g_hash_table_iter_init(&attr_iter, attributes);
     while (g_hash_table_iter_next(&attr_iter, NULL, (gpointer *) &a)) {
@@ -338,7 +345,7 @@ drop_removed_values(const char *cib_id, const char *set_type,
         while (g_hash_table_iter_next(&value_iter, NULL, (gpointer *) &v)) {
             const char *id = NULL;
 
-            if (v->current != NULL) {
+            if ((v->current != NULL) && !drop_immediately) {
                 continue;
             }
 
@@ -355,11 +362,17 @@ drop_removed_values(const char *cib_id, const char *set_type,
             }
 
             if (!func(a, id, cib_id)) {
+                crm_trace("%s != %s", id, cib_id);
                 continue;
             }
 
-            crm_debug("Dropping %s[%s] after CIB erasure of %s %s",
-                      a->id, v->nodename, entry_type, cib_id);
+            if (drop_immediately) {
+                crm_debug("Dropping %s[%s] immediately", a->id, v->nodename);
+            } else {
+                crm_debug("Dropping %s[%s] after CIB erasure of %s %s",
+                          a->id, v->nodename, entry_type, cib_id);
+            }
+
             g_hash_table_iter_remove(&value_iter);
         }
     }
@@ -394,7 +407,7 @@ nvpair_matches(const attribute_t *a, const char *xml_id, const char *cib_id)
 void
 attrd_drop_removed_value(const char *set_type, const char *cib_id)
 {
-    drop_removed_values(cib_id, set_type, nvpair_matches);
+    drop_removed_values(cib_id, set_type, -1, nvpair_matches);
 }
 
 /*!
@@ -431,7 +444,7 @@ set_id_matches(const attribute_t *a, const char *xml_id, const char *cib_id)
 void
 attrd_drop_removed_set(const char *set_type, const char *cib_id)
 {
-    drop_removed_values(cib_id, set_type, set_id_matches);
+    drop_removed_values(cib_id, set_type, -1, set_id_matches);
 }
 
 /*!
@@ -457,7 +470,7 @@ node_matches(const attribute_t *a, const char *xml_id, const char *cib_id)
  * \param[in] cib_id  ID of node state that was removed from CIB
  */
 void
-attrd_drop_removed_values(const char *cib_id)
+attrd_drop_removed_values(const char *cib_id, int peer_attrd_ver)
 {
-    drop_removed_values(cib_id, NULL, node_matches);
+    drop_removed_values(cib_id, NULL, peer_attrd_ver, node_matches);
 }

daemons/attrd/attrd_cib.c

@@ -113,7 +113,7 @@ drop_values_in_deletion(xmlNode *xml, void *data)
         attrd_drop_removed_set(set_type, set_id);
 
     } else if (!pcmk__str_empty(node_id)) {
-        attrd_drop_removed_values(node_id);
+        attrd_drop_removed_values(node_id, *(int *) data);
     }
 
 done:
@@ -149,6 +149,9 @@ attrd_cib_updated_cb(const char *event, xmlNode *msg)
 
     client_name = pcmk__xe_get(msg, PCMK__XA_CIB_CLIENTNAME);
     if (!cib__client_triggers_refresh(client_name)) {
+        const char *peer = pcmk__xe_get(msg, PCMK__XA_SRC);
+        int peer_attrd_ver = attrd_get_peer_protocol_ver(peer);
+
         /* This change came from a source that ensured the CIB is consistent
          * with our attributes table, so we don't need to write anything out.
          * If a removed attribute has been erased, we can forget it now.
@@ -165,7 +168,7 @@ attrd_cib_updated_cb(const char *event, xmlNode *msg)
          * function signature.
          */
         pcmk__xe_foreach_child((xmlNode *) patchset, PCMK_XE_CHANGE,
-                               drop_values_in_deletion, NULL);
+                               drop_values_in_deletion, &peer_attrd_ver);
         return;
     }
 

daemons/attrd/attrd_utils.c

@@ -238,6 +238,23 @@ attrd_remove_peer_protocol_ver(const char *host)
     }
 }
 
+int
+attrd_get_peer_protocol_ver(const char *host)
+{
+    gpointer key;
+
+    if (peer_protocol_vers == NULL) {
+        return -1;
+    }
+
+    key = g_hash_table_lookup(peer_protocol_vers, host);
+    if (key == NULL) {
+        return -1;
+    }
+
+    return GPOINTER_TO_INT(key);
+}
+
 /*!
  * \internal
  * \brief When a peer node broadcasts a message with its protocol version, keep

daemons/attrd/pacemaker-attrd.h

@@ -44,11 +44,13 @@
  *     5       2.1.5    Peers can request confirmation of a sent message
  *     6       2.1.7    PCMK__ATTRD_CMD_PEER_REMOVE supports PCMK__XA_REAP
  *     7       3.0.0    "flush" support dropped
+ *     8       3.0.2    attrd clears transient attributes from the CIB as well
  */
-#define ATTRD_PROTOCOL_VERSION "7"
+#define ATTRD_PROTOCOL_VERSION "8"
 
 #define ATTRD_SUPPORTS_MULTI_MESSAGE(x) ((x) >= 4)
 #define ATTRD_SUPPORTS_CONFIRMATION(x)  ((x) >= 5)
+#define ATTRD_SUPPORTS_CLEARING_CIB(x)  ((x) >= 8)
 
 #define attrd_send_ack(client, id, flags)                       \
     pcmk__ipc_send_ack((client), (id), (flags), PCMK__XE_ACK,   \
@@ -217,7 +219,7 @@ char *attrd_nvpair_id(const attribute_t *attr, const char *node_state_id);
 bool attrd_for_cib(const attribute_t *a);
 void attrd_drop_removed_value(const char *set_type, const char *cib_id);
 void attrd_drop_removed_set(const char *set_type, const char *cib_id);
-void attrd_drop_removed_values(const char *cib_id);
+void attrd_drop_removed_values(const char *cib_id, int peer_attrd_ver);
 
 enum attrd_write_options {
     attrd_write_changed         = 0,
@@ -231,6 +233,7 @@ void attrd_write_or_elect_attribute(attribute_t *a);
 extern int minimum_protocol_version;
 void attrd_remove_peer_protocol_ver(const char *host);
 void attrd_update_minimum_protocol_ver(const char *host, const char *value);
+int attrd_get_peer_protocol_ver(const char *host);
 
 mainloop_timer_t *attrd_add_timer(const char *id, int timeout_ms, attribute_t *attr);