Bug: tools: Clear all prefer constraints when performing a move

A move is implemented in terms of perfer constraints.  If those
constraints contain something like a lifetime expression, and older
prefer constraints are not cleared out, the result is a mess.  The XML
that is attempted to insert into the CIB will contain both the older
constraint and then the new lifetime expression as sub-nodes of that
constraint.  This is invalid, so the CIB will throw it out.

The fix is to make sure there are no prefer constraints for any nodes
when a move is done.

Most ban constraints are left alone, because they may still be valid -
you may want to move a resource to one node while preserving the ban on
another node.  Taking care of this is the bulk of the complexity in this
patch.

One further note - any ban constraints on the destination still need to
be removed.  Having both a ban and a prefer constraint on the same node
may technically be valid XML, but doesn't make any sense.

See rhbz#1648620

tools/crm_resource.c

@@ -982,10 +982,10 @@ main(int argc, char **argv)
                 rc = -pcmk_err_node_unknown;
                 goto bail;
             }
-            rc = cli_resource_clear(rsc_id, dest->details->uname, NULL, cib_conn);
+            rc = cli_resource_clear(rsc_id, dest->details->uname, NULL, cib_conn, TRUE);
 
         } else {
-            rc = cli_resource_clear(rsc_id, NULL, data_set->nodes, cib_conn);
+            rc = cli_resource_clear(rsc_id, NULL, data_set->nodes, cib_conn, TRUE);
         }
 
     } else if (rsc_cmd == 'M' && host_uname) {

tools/crm_resource.h

@@ -38,7 +38,8 @@ extern const char *attr_set_type;
 /* ban */
 int cli_resource_prefer(const char *rsc_id, const char *host, cib_t * cib_conn);
 int cli_resource_ban(const char *rsc_id, const char *host, GListPtr allnodes, cib_t * cib_conn);
-int cli_resource_clear(const char *rsc_id, const char *host, GListPtr allnodes, cib_t * cib_conn);
+int cli_resource_clear(const char *rsc_id, const char *host, GListPtr allnodes, cib_t * cib_conn,
+                       bool clear_ban_constraints);
 int cli_resource_clear_all_expired(xmlNode *root, cib_t *cib_conn, const char *rsc, const char *node, bool scope_master);
 
 /* print */

tools/crm_resource_ban.c

@@ -228,15 +228,19 @@ resource_clear_node_in_expr(const char *rsc_id, const char *host, cib_t * cib_co
 }
 
 static int
-resource_clear_node_in_location(const char *rsc_id, const char *host, cib_t * cib_conn)
+resource_clear_node_in_location(const char *rsc_id, const char *host, cib_t * cib_conn,
+                                bool clear_ban_constraints)
 {
     int rc = pcmk_ok;
     xmlNode *fragment = NULL;
     xmlNode *location = NULL;
 
     fragment = create_xml_node(NULL, XML_CIB_TAG_CONSTRAINTS);
-    location = create_xml_node(fragment, XML_CONS_TAG_RSC_LOCATION);
-    crm_xml_set_id(location, "cli-ban-%s-on-%s", rsc_id, host);
+
+    if (clear_ban_constraints == TRUE) {
+        location = create_xml_node(fragment, XML_CONS_TAG_RSC_LOCATION);
+        crm_xml_set_id(location, "cli-ban-%s-on-%s", rsc_id, host);
+    }
 
     location = create_xml_node(fragment, XML_CONS_TAG_RSC_LOCATION);
     crm_xml_set_id(location, "cli-prefer-%s", rsc_id);
@@ -255,7 +259,8 @@ resource_clear_node_in_location(const char *rsc_id, const char *host, cib_t * ci
 }
 
 int
-cli_resource_clear(const char *rsc_id, const char *host, GListPtr allnodes, cib_t * cib_conn)
+cli_resource_clear(const char *rsc_id, const char *host, GListPtr allnodes, cib_t * cib_conn,
+                   bool clear_ban_constraints)
 {
     int rc = pcmk_ok;
 
@@ -271,7 +276,7 @@ cli_resource_clear(const char *rsc_id, const char *host, GListPtr allnodes, cib_
          * to try the second clear method.
          */
         if (rc == pcmk_ok) {
-            rc = resource_clear_node_in_location(rsc_id, host, cib_conn);
+            rc = resource_clear_node_in_location(rsc_id, host, cib_conn, clear_ban_constraints);
         }
 
     } else {
@@ -283,7 +288,7 @@ cli_resource_clear(const char *rsc_id, const char *host, GListPtr allnodes, cib_
         for(; n; n = n->next) {
             node_t *target = n->data;
 
-            rc = cli_resource_clear(rsc_id, target->details->uname, NULL, cib_conn);
+            rc = cli_resource_clear(rsc_id, target->details->uname, NULL, cib_conn, clear_ban_constraints);
             if (rc != pcmk_ok) {
                 break;
             }

tools/crm_resource_runtime.c

@@ -1378,7 +1378,7 @@ cli_resource_restart(pe_resource_t *rsc, const char *host, int timeout_ms,
     }
 
     if (stop_via_ban) {
-        rc = cli_resource_clear(rsc_id, host, NULL, cib);
+        rc = cli_resource_clear(rsc_id, host, NULL, cib, TRUE);
 
     } else if (orig_target_role) {
         rc = cli_resource_update_attribute(rsc, rsc_id, NULL, NULL,
@@ -1460,7 +1460,7 @@ cli_resource_restart(pe_resource_t *rsc, const char *host, int timeout_ms,
 
   failure:
     if (stop_via_ban) {
-        cli_resource_clear(rsc_id, host, NULL, cib);
+        cli_resource_clear(rsc_id, host, NULL, cib, TRUE);
     } else if (orig_target_role) {
         cli_resource_update_attribute(rsc, rsc_id, NULL, NULL,
                                       XML_RSC_ATTR_TARGET_ROLE,
@@ -1874,8 +1874,11 @@ cli_resource_move(resource_t *rsc, const char *rsc_id, const char *host_name,
         }
     }
 
-    /* Clear any previous constraints for 'dest' */
-    cli_resource_clear(rsc_id, dest->details->uname, data_set->nodes, cib);
+    /* Clear any previous prefer constraints across all nodes. */
+    cli_resource_clear(rsc_id, NULL, data_set->nodes, cib, FALSE);
+
+    /* Clear any previous ban constraints on 'dest'. */
+    cli_resource_clear(rsc_id, dest->details->uname, data_set->nodes, cib, TRUE);
 
     /* Record an explicit preference for 'dest' */
     rc = cli_resource_prefer(rsc_id, dest->details->uname, cib);