Bug 1561617

Created attachment 1414291 [details]
original unchanged cib

Created attachment 1414293 [details]
cib with a noop update applied to it

With https://github.com/ClusterLabs/pacemaker/commit/42dc2bb

[08:38 PM] beekhof@minidell ~/sources/pacemaker/upstream ☹  # PCMK_trace_functions=__xml_diff_object tools/crm_diff -o input1.xml -n input2.xml -Vc
(       xml.c:4209  )    info: __xml_diff_object:	nvpair.nodes-3-haproxy-role moved from 4 to 3
(       xml.c:4209  )    info: __xml_diff_object:	nvpair.nodes-3-redis-role moved from 3 to 4
<diff format="2" digest="a49a3002b8a2c872d3f0c6d1a09c528d">
  <version>
    <source admin_epoch="0" epoch="104" num_updates="12"/>
    <target admin_epoch="0" epoch="104" num_updates="12"/>
  </version>
  <change operation="move" path="/cib/configuration/nodes/node[@id=&apos;3&apos;]/instance_attributes[@id=&apos;nodes-3&apos;]/nvpair[@id=&apos;nodes-3-haproxy-role&apos;]" position="3"/>
  <change operation="move" path="/cib/configuration/nodes/node[@id=&apos;3&apos;]/instance_attributes[@id=&apos;nodes-3&apos;]/nvpair[@id=&apos;nodes-3-redis-role&apos;]" position="4"/>
</diff>

 : git : ✗ master@f73f955 Test: rhbz#1565187 - Ensure failures that cause fencing are not removed until after fencing completes
[08:38 PM] beekhof@minidell ~/sources/pacemaker/upstream ☹  # PCMK_trace_functions=__xml_diff_object tools/crm_diff -o input1.xml -n input2.xml -V
(       xml.c:4126  )    info: __xml_diff_object:	Moved nvpair@id (0 -> 1)
(       xml.c:4126  )    info: __xml_diff_object:	Moved nvpair@name (1 -> 0)
(       xml.c:4209  )    info: __xml_diff_object:	nvpair.nodes-3-haproxy-role moved from 4 to 3
(       xml.c:4209  )    info: __xml_diff_object:	nvpair.nodes-3-redis-role moved from 3 to 4
<diff format="2">
  <version>
    <source admin_epoch="0" epoch="104" num_updates="12"/>
    <target admin_epoch="0" epoch="104" num_updates="12"/>
  </version>
  <change operation="modify" path="/cib/configuration/nodes/node[@id=&apos;2&apos;]/instance_attributes[@id=&apos;nodes-2&apos;]/nvpair[@id=&apos;nodes-2-haproxy-role&apos;]">
    <change-list>
      <change-attr name="name" operation="set" value="haproxy-role"/>
      <change-attr name="id" operation="set" value="nodes-2-haproxy-role"/>
    </change-list>
    <change-result>
      <nvpair name="haproxy-role" id="nodes-2-haproxy-role" value="true"/>
    </change-result>
  </change>
  <change operation="move" path="/cib/configuration/nodes/node[@id=&apos;3&apos;]/instance_attributes[@id=&apos;nodes-3&apos;]/nvpair[@id=&apos;nodes-3-haproxy-role&apos;]" position="3"/>
  <change operation="move" path="/cib/configuration/nodes/node[@id=&apos;3&apos;]/instance_attributes[@id=&apos;nodes-3&apos;]/nvpair[@id=&apos;nodes-3-redis-role&apos;]" position="4"/>
</diff>

Turns out I explicitly wanted attribute ordering changes detected in https://github.com/ClusterLabs/pacemaker/commit/5125ecbdf, so that behaviour is retained everywhere except crm_diff and even then only when comparing with -c or --cib

What's so appealing on a library feature above and beyond what
the standard mandates?  This also means not working on plain XML
basis (with all of implications of that model) anymore.
Original model has other means of working with order-sensitivity
where desired.

https://www.w3.org/TR/2008/REC-xml-20081126/#sec-starttags

> Note that the order of attribute specifications in a start-tag
> or empty-element tag is not significant.

I can see that deterministic/preserving serialization/deserialization
is very useful in some contexts (e.g. for libxslt), but when the
question stands "are these XML instances equivalent", the original
model has a clear answer about the isomorphism on attributes
(just as it has about whitespace related differences etc.).

So I built a pacemaker package and installed it:
pacemaker-1.1.18.crm_diff.0-11.el7.x86_64

Definitely an improvement:
pcs cluster cib cib.xml
cp cib.xml cib.xml.orig
# This below is effectively a noop:
pcs -f cib.xml resource update ip-172.16.11.97 cidr_netmask=32

[root@foobar-0 crm_diff_tests]# crm_diff --cib --original cib.xml.orig --new cib.xml
<diff format="2" digest="e9eb1ba31a86c41a406e86f79bcad861">
  <version>
    <source admin_epoch="0" epoch="28" num_updates="13"/>
    <target admin_epoch="0" epoch="29" num_updates="0"/>
  </version>
  <change operation="modify" path="/cib">
    <change-list>
      <change-attr name="epoch" operation="set" value="29"/>
      <change-attr name="num_updates" operation="set" value="0"/>
    </change-list>
    <change-result>
      <cib crm_feature_set="3.0.14" validate-with="pacemaker-2.10" epoch="29" num_updates="0" admin_epoch="0" cib-last-written="Mon Apr 16 16:41:38 2018" update-origin="foobar-1" update-client="crmd" update-user="hacluster" have-quorum="1" dc-uuid="2"/>
    </change-result>
  </change>
  <change operation="create" path="/cib/configuration/resources/primitive[@id=&apos;ip-172.16.11.97&apos;]" position="2">
    <meta_attributes id="ip-172.16.11.97-meta_attributes"/>
  </change>
</diff>


I wonder if we can avoid crm_diff signaling that the empty meta attributes tag ha been added:
[root@foobar-0 crm_diff_tests]# diff -u cib.xml.orig cib.xml
--- cib.xml.orig        2018-04-16 16:42:32.997958929 +0200
+++ cib.xml     2018-04-16 16:42:50.590301314 +0200
@@ -1,4 +1,4 @@
-<cib crm_feature_set="3.0.14" validate-with="pacemaker-2.10" epoch="28" num_updates="13" admin_epoch="0" cib-last-written="Mon Apr 16 16:41:38 2018" update-origin="foobar-1" update-client="crmd" update-user="hacluster" have-quorum="1" dc-uuid="2">
+<cib crm_feature_set="3.0.14" validate-with="pacemaker-2.10" epoch="29" num_updates="0" admin_epoch="0" cib-last-written="Mon Apr 16 16:41:38 2018" update-origin="foobar-1" update-client="crmd" update-user="hacluster" have-quorum="1" dc-uuid="2">
   <configuration>
     <crm_config>
       <cluster_property_set id="cib-bootstrap-options">
@@ -84,6 +84,7 @@
           <op id="ip-172.16.11.97-start-interval-0s" interval="0s" name="start" timeout="20s"/>
           <op id="ip-172.16.11.97-stop-interval-0s" interval="0s" name="stop" timeout="20s"/>
         </operations>
+        <meta_attributes id="ip-172.16.11.97-meta_attributes"/>
       </primitive>
       <clone id="rabbitmq-clone">
         <primitive class="ocf" id="rabbitmq" provider="heartbeat" type="rabbitmq-cluster">


If not I'll have to add some code to ignore empty tags (although I can't predict yet how many and which ones are relevant and which aren't)

Do we need a 7.5.z for this?

(In reply to Michele Baldessari from comment #8)
> I wonder if we can avoid crm_diff signaling that the empty meta attributes
> tag ha been added:

Nope :)

> If not I'll have to add some code to ignore empty tags (although I can't
> predict yet how many and which ones are relevant and which aren't)

Seems like something pcs could avoid creating in the first place.

(In reply to Ken Gaillot from comment #9)
> Do we need a 7.5.z for this?

Yes please

(In reply to Jan Pokorný from comment #7)
> What's so appealing on a library feature above and beyond what
> the standard mandates? 

Because the cib comparision and digest algorithms are built on it.
Do we have your permission now?

(In reply to Andrew Beekhof from comment #10)
> (In reply to Michele Baldessari from comment #8)
> > I wonder if we can avoid crm_diff signaling that the empty meta attributes
> > tag ha been added:
> 
> Nope :)

Slacker ;)

> > If not I'll have to add some code to ignore empty tags (although I can't
> > predict yet how many and which ones are relevant and which aren't)
> 
> Seems like something pcs could avoid creating in the first place.

Right, but I still need to care for that case because a) there might be other occurrences where pcs might do this and we might not slay them all and b) I'd have to conditionalize code on is pcs running a fixed version -> user crm_diff, otherwise use crm_simulate, which would become a pita to maintain and test.

Let me explore this on the ruby side of things and let's see how it goes.
Thanks a lot for the fix in the meantime!

(In reply to Michele Baldessari from comment #13)
> (In reply to Andrew Beekhof from comment #10)
> > (In reply to Michele Baldessari from comment #8)
> > > I wonder if we can avoid crm_diff signaling that the empty meta attributes
> > > tag ha been added:
> > 
> > Nope :)
> 
> Slacker ;)
> 
> > > If not I'll have to add some code to ignore empty tags (although I can't
> > > predict yet how many and which ones are relevant and which aren't)
> > 
> > Seems like something pcs could avoid creating in the first place.

This happens in old pcs code at several places I think. In new code (new pcs library) we take care not to leave empty elements in the CIB. Creating an empty meta_attributes on resource update was easy to fix. So here you are: bz1568353

> Right, but I still need to care for that case because a) there might be
> other occurrences where pcs might do this and we might not slay them all and

Yes, there probably are a few more.

> b) I'd have to conditionalize code on is pcs running a fixed version -> user
> crm_diff, otherwise use crm_simulate, which would become a pita to maintain
> and test.
> 
> Let me explore this on the ruby side of things and let's see how it goes.
> Thanks a lot for the fix in the meantime!

(In reply to Jan Pokorný from comment #7)
> What's so appealing on a library feature above and beyond what
> the standard mandates?  This also means not working on plain XML
> basis (with all of implications of that model) anymore.
> Original model has other means of working with order-sensitivity
> where desired.
> 
> https://www.w3.org/TR/2008/REC-xml-20081126/#sec-starttags
> 
> > Note that the order of attribute specifications in a start-tag
> > or empty-element tag is not significant.
> 
> I can see that deterministic/preserving serialization/deserialization
> is very useful in some contexts (e.g. for libxslt), but when the
> question stands "are these XML instances equivalent", the original
> model has a clear answer about the isomorphism on attributes
> (just as it has about whitespace related differences etc.).

crm_diff is a CIB tool, not an XML tool, i.e. it operates at a higher level than pure XML. There are two useful questions it can be used to answer:

The first is: Given two CIB inputs, what changes are necessary to apply to the second one to make it identical to the first one, as verified by a hash of the contents? For the hash verification, the order of XML attributes is consequential, even if it is not semantically.

The second is: Given two CIB inputs, are they functionally equivalent? In this case, the order of XML attributes is not consequential.

The fix here is to allow the user to choose via the --cib option. I'm not sure it's the best option to overload that, but it maintains backward compatibility and works for the necessary purpose.

re [comment 18]:

> crm_diff is a CIB tool, not an XML tool, i.e. it operates
> at a higher level than pure XML.

This is exactly why CIB would ideally _not_ break the assumptions
set forth with the lower level (in the natural order of things,
CIB would not attempt to be superset of XML, overridding its natural
axioms, when it's conceptually its subset) :-)


re [comment 12]:

> Because the cib comparison and digest algorithms are built on it.

That's not the root cause I was after.  I can presume the answer
is "because it was _easy_ and without quadratic complexity at the
attribute level comparison".  Well, it's not necessarily quadratic:

For elements A, B of lenghts m, n, respectively:

* for each element, iterate through the attributes, for each attribute,
  compute (not necessarily cryptographic) hash of "<name>=<value>"
  (for instance), XOR it to the accumulated value for the particular
  element
  - complexity:
    m * ("mean length of "<name>=<value>" in A" + "constant for XOR") \
    + n * (<mean length of "<name>=<value>" in B + "constant for XOR")

* compare the resulting hashes
  - complexity: constant

-> if we, conservatively, deem "(mean length of "<name>=<value>" in X)"
   is 64, we get overall complexity:
   (64 + "constant for XOR") * (m + n) + "constant for comparison",
   and because constants are less significant order of the complexity
   growth, it can be neglected: m + n, i.e., linear complexity
   (see above, having the luxury of being _subset_ of XML, CIB could
   arbitrarily impose a limit of either name/value being 64 characters
   at maximum to guarantee predictable timing/avoid DoS'ing when
   processing CIB)

The overall complexity would further decrease if the hashes were
maintained throughout the life scope of the XML object systemically.

Digest algorithm could then directly leverage the already precomputed
hashes of the attribute sets, moreover this precomputation could be
leveraged in bottom-up level, i.e., at the root, aggregate precomputed
(or ad-hoc recursively computed) hashes of its subtrees.

Such a design could also prevent headaches with sensitive data
contributing directly to the digest computation, [bug 1164861 comment 6],
but then, said hash function would have to be cryptographically strong
so as to avoid bruteforcing pre-image (omitted raw passwords) easily.


> Do we have your permission now?

Anytime, barring a possibility of a time machine, apparently I cannot
revert the history :-)

before:
=======

> [root@virt-242 ~]# rpm -q pacemaker pcs
> pacemaker-1.1.18-11.el7.x86_64
> pcs-0.9.165-1.el7.x86_64
> [root@virt-242 ~]# wget -qO cib.xml https://bugzilla.redhat.com/attachment.cgi?id=1414291
> [root@virt-242 ~]# sha1sum cib.xml
> 58e12006c0facee54fedd5c57c63f62a986183d5  cib.xml
> [root@virt-242 ~]# cp cib.xml cib.xml.orig
> [root@virt-242 ~]# pcs -f cib.xml resource update ip-172.17.1.15 cidr_netmask=32 ip=172.17.1.15   
> [root@virt-242 ~]# crm_diff --cib --original cib.xml.orig --new cib.xml
> <diff format="2" digest="b64d3000a4d06df03baf6e769fbdeeb2">
>   <version>
>     <source admin_epoch="0" epoch="86" num_updates="125"/>
>     <target admin_epoch="0" epoch="87" num_updates="0"/>
>   </version>
>   <change operation="modify" path="/cib">
>     <change-list>
>       <change-attr name="epoch" operation="set" value="87"/>
>       <change-attr name="num_updates" operation="set" value="0"/>
>     </change-list>
>     <change-result>
>       <cib crm_feature_set="3.0.14" validate-with="pacemaker-2.10" epoch="87" num_updates="0" admin_epoch="0" cib-last-written="Tue Mar 27 08:12:24 2018" update-origin="controller-0" update-client="cibadmin" update-user="root" have-quorum="1" dc-uuid="2"/>
>     </change-result>
>   </change>
>   <change operation="modify" path="/cib/configuration/resources/bundle[@id=&apos;galera-bundle&apos;]/storage/storage-mapping[@id=&apos;mysql-foo&apos;]">
>     <change-list>
>       <change-attr name="id" operation="set" value="mysql-foo"/>
>       <change-attr name="target-dir" operation="set" value="/foo"/>
>     </change-list>
>     <change-result>
>       <storage-mapping id="mysql-foo" options="rw" source-dir="/foo" target-dir="/foo"/>
>     </change-result>
>   </change>
>   <change operation="modify" path="/cib/configuration/resources/bundle[@id=&apos;galera-bundle&apos;]/storage/storage-mapping[@id=&apos;mysql-bar&apos;]">
>     <change-list>
>       <change-attr name="id" operation="set" value="mysql-bar"/>
>       <change-attr name="target-dir" operation="set" value="/bar"/>
>     </change-list>
>     <change-result>
>       <storage-mapping id="mysql-bar" options="rw" source-dir="/bar" target-dir="/bar"/>
>     </change-result>
>   </change>
> </diff>
> 
> [root@virt-242 ~]# echo $?
> 1
> [root@virt-242 ~]# crm_diff --no-version --original cib.xml.orig --new cib.xml
> <diff format="2">
>   <change operation="modify" path="/cib/configuration/resources/bundle[@id=&apos;galera-bundle&apos;]/storage/storage-mapping[@id=&apos;mysql-foo&apos;]">
>     <change-list>
>       <change-attr name="id" operation="set" value="mysql-foo"/>
>       <change-attr name="target-dir" operation="set" value="/foo"/>
>     </change-list>
>     <change-result>
>       <storage-mapping id="mysql-foo" options="rw" source-dir="/foo" target-dir="/foo"/>
>     </change-result>
>   </change>
>   <change operation="modify" path="/cib/configuration/resources/bundle[@id=&apos;galera-bundle&apos;]/storage/storage-mapping[@id=&apos;mysql-bar&apos;]">
>     <change-list>
>       <change-attr name="id" operation="set" value="mysql-bar"/>
>       <change-attr name="target-dir" operation="set" value="/bar"/>
>     </change-list>
>     <change-result>
>       <storage-mapping id="mysql-bar" options="rw" source-dir="/bar" target-dir="/bar"/>
>     </change-result>
>   </change>
> </diff>
> 
> [root@virt-242 ~]# echo $?
> 1



after:
======

> [root@virt-241 ~]# rpm -q pacemaker pcs
> pacemaker-1.1.19-3.el7.x86_64
> pcs-0.9.165-1.el7.x86_64
> [root@virt-241 ~]# wget -qO cib.xml https://bugzilla.redhat.com/attachment.cgi?id=1414291
> [root@virt-241 ~]# sha1sum cib.xml
> 58e12006c0facee54fedd5c57c63f62a986183d5  cib.xml
> [root@virt-241 ~]# cp cib.xml cib.xml.orig
> [root@virt-241 ~]# pcs -f cib.xml resource update ip-172.17.1.15 cidr_netmask=32 ip=172.17.1.15
> [root@virt-241 ~]# crm_diff --cib --original cib.xml.orig --new cib.xml
> <diff format="2" digest="b64d3000a4d06df03baf6e769fbdeeb2">
>   <version>
>     <source admin_epoch="0" epoch="86" num_updates="125"/>
>     <target admin_epoch="0" epoch="87" num_updates="0"/>
>   </version>
>   <change operation="modify" path="/cib">
>     <change-list>
>       <change-attr name="epoch" operation="set" value="87"/>
>       <change-attr name="num_updates" operation="set" value="0"/>
>     </change-list>
>     <change-result>
>       <cib crm_feature_set="3.0.14" validate-with="pacemaker-2.10" epoch="87" num_updates="0" admin_epoch="0" cib-last-written="Tue Mar 27 08:12:24 2018" update-origin="controller-0" update-client="cibadmin" update-user="root" have-quorum="1" dc-uuid="2"/>
>     </change-result>
>   </change>
> </diff>
> 
> [root@virt-241 ~]# echo $?                                             
> 1
> [root@virt-241 ~]# crm_diff --no-version --original cib.xml.orig --new cib.xml
> <diff format="2">
>   <change operation="modify" path="/cib/configuration/resources/bundle[@id=&apos;galera-bundle&apos;]/storage/storage-mapping[@id=&apos;mysql-foo&apos;]">
>     <change-list>
>       <change-attr name="id" operation="set" value="mysql-foo"/>
>       <change-attr name="target-dir" operation="set" value="/foo"/>
>     </change-list>
>     <change-result>
>       <storage-mapping id="mysql-foo" options="rw" source-dir="/foo" target-dir="/foo"/>
>     </change-result>
>   </change>
>   <change operation="modify" path="/cib/configuration/resources/bundle[@id=&apos;galera-bundle&apos;]/storage/storage-mapping[@id=&apos;mysql-bar&apos;]">
>     <change-list>
>       <change-attr name="id" operation="set" value="mysql-bar"/>
>       <change-attr name="target-dir" operation="set" value="/bar"/>
>     </change-list>
>     <change-result>
>       <storage-mapping id="mysql-bar" options="rw" source-dir="/bar" target-dir="/bar"/>
>     </change-result>
>   </change>
> </diff>
> 
> [root@virt-241 ~]# echo $?
> 1


Passing the '-c/--cib' flag to crm_diff now shows less differences (effectively, only the expected version/epoch metadata changes) between original cib and cib with a no-op resource update applied.
The output with '-u/--no-version' remains unchanged, as different diffing logic is used when this flag is passed.
Please note that the missing empty meta attribute change before the fix as compared to the original bug description is due to bz#1568353 being fixed (hence the rpm -q for pcs package).

Marking as verified in 1.1.19-3.el7.

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHBA-2018:3055