After some additional research we discovered that it is not important that the cib is large. The value of attribute "crm_feature_set" of the element "cib" is what matters .

> with crm_feature_set="3.0.8"
[vm-rhel72-1 ~] $ cat cib.short.original.xml
<cib epoch="576" num_updates="0" admin_epoch="0" validate-with="pacemaker-2.0" crm_feature_set="3.0.8" update-origin="rh7-3" update-client="crmd" cib-last-written="Thu Aug 23 16:49:17 2012" have-quorum="0" dc-uuid="2">
  <configuration>
    <crm_config/>
    <nodes/>
    <resources>
      <group id="MyGroup">
        <primitive class="ocf" id="A2" provider="heartbeat" type="Dummy">
          <operations>
            <op id="A2-monitor-interval-10" interval="10" name="monitor" timeout="20"/>
          </operations>
        </primitive>
      </group>
    </resources>
    <constraints/>
  </configuration>
  <status/>
</cib>

[vm-rhel72-1 ~] $ cp cib.short.original.xml cib.short.new.xml
[vm-rhel72-1 ~] $ sed -i 's#<group id="MyGroup">#<group id="MyGroup"><primitive class="ocf" id="A8" provider="heartbeat" type="Dummy"><operations><op id="A8-monitor-interval-10" interval="10" name="monitor" timeout="20"/></operations></primitive>#' cib.short.new.xml

[vm-rhel72-1 ~] $ crm_diff --original cib.short.original.xml --new cib.short.new.xml --no-version
<diff crm_feature_set="3.0.12" format="1" digest="303628457fcbe83eff1b2b11dbdd2a41">
  <diff-removed>
    <cib/>
  </diff-removed>
  <diff-added>
    <cib validate-with="pacemaker-2.0" crm_feature_set="3.0.8" update-origin="rh7-3" update-client="crmd" cib-last-written="Thu Aug 23 16:49:17 2012" have-quorum="0" dc-uuid="2">
      <configuration>
        <resources>
          <group id="MyGroup">
            <primitive class="ocf" id="A8" provider="heartbeat" type="Dummy" __crm_diff_marker__="added:top">
              <operations>
                <op id="A8-monitor-interval-10" interval="10" name="monitor" timeout="20"/>
              </operations>
            </primitive>
          </group>
        </resources>
      </configuration>
    </cib>
  </diff-added>
</diff>

[vm-rhel72-1 ~] $ cp cib.short.original.xml cib.short.live.xml
[vm-rhel72-1 ~] $ crm_diff --original cib.short.original.xml --new cib.short.new.xml --no-version | CIB_file=cib.short.live.xml cibadmin --patch --verbose --xml-pipe
Call failed: Application of an update diff failed


> with crm_feature_set="3.0.12"
[vm-rhel72-1 ~] $ sed -i 's#3\.0\.8#3.0.12#' cib.short.original.xml
[vm-rhel72-1 ~] $ sed -i 's#3\.0\.8#3.0.12#' cib.short.new.xml
[vm-rhel72-1 ~] $ crm_diff --original cib.short.original.xml --new cib.short.new.xml --no-version
<diff format="2">
  <change operation="create" path="/cib/configuration/resources/group[@id=&apos;MyGroup&apos;]" position="0">
    <primitive class="ocf" id="A8" provider="heartbeat" type="Dummy">
      <operations>
        <op id="A8-monitor-interval-10" interval="10" name="monitor" timeout="20"/>
      </operations>
    </primitive>
  </change>
</diff>

[vm-rhel72-1 ~] $ cp cib.short.original.xml cib.short.live.xml
[vm-rhel72-1 ~] $ crm_diff --original cib.short.original.xml --new cib.short.new.xml --no-version | CIB_file=cib.short.live.xml cibadmin --patch --verbose --xml-pipe
[vm-rhel72-1 ~] $ echo $?
0

This turned out to be an ancient known bug that was fixed long ago, but the old behavior is preserved for compatibility. Hence it only affects CIBs with crm_feature_set less than or equal to 3.0.8.

There are two patch formats used by pacemaker, "v1" (<=3.0.8) and "v2" (>3.0.8, which should be anything created in RHEL 7.1 and later). The v1 format has a bug where it can't handle adding the first child of an element -- in this case, adding the resource as the first member of the group is what makes it fail.

I see two options:

* pcs could automatically call "cibadmin --upgrade" for anything with crm_feature_set <=3.0.8, before applying any changes. This is probably the best option.

* We could add a "force v2" option to crm_diff, that pcs can use. I'd like this to become the default behavior in upstream Pacemaker 2.0, which is likely to make it into RHEL 7 at some point. So the option would be a temporary measure. But if you want to avoid upgrading the CIB, we could go this route.

Let me know which you'd prefer.

(In reply to Ken Gaillot from comment #4)
> * We could add a "force v2" option to crm_diff, that pcs can use. I'd like
> this to become the default behavior in upstream Pacemaker 2.0, which is
> likely to make it into RHEL 7 at some point. So the option would be a
> temporary measure. But if you want to avoid upgrading the CIB, we could go
> this route.

A better way to do the above would be a downstream patch to make crm_diff always use v2 in RHEL, which wouldn't need any changes in pcs. Once upstream adopted the behavior, the patch would go away.

I still think anyone with a config that old would benefit from a cibadmin --upgrade, so they can use current features, but again, let me know which you prefer.

It seems that pacemaker itself upgrade the crm_feature_set:

[vm-rhel72-1 ~] $ cat /var/lib/pacemaker/cib/cib.xml | grep crm_feature_set
<cib crm_feature_set="3.0.12" validate-with="pacemaker-2.8" epoch="21" num_updates="0" admin_epoch="0" cib-last-written="Fri Oct 20 09:25:11 2017" update-origin="vm-rhel72-1" update-client="cibadmin" update-user="root" have-quorum="1">

[vm-rhel72-1 ~] $ pcs cluster stop --all
vm-rhel72-1: Stopping Cluster (pacemaker)...
vm-rhel72-3: Stopping Cluster (pacemaker)...
vm-rhel72-1: Stopping Cluster (corosync)...
vm-rhel72-3: Stopping Cluster (corosync)...

[vm-rhel72-1 ~] $ sed -i 's/crm_feature_set="3.0.12"/crm_feature_set="3.0.8"/' /var/lib/pacemaker/cib/cib.xml
[vm-rhel72-1 ~] $ rm /var/lib/pacemaker/cib/cib.xml.sig
[vm-rhel72-1 ~] $ ssh vm-rhel72-3 'sed -i "s/crm_feature_set=\"3.0.12\"/crm_feature_set=\"3.0.8\"/" /var/lib/pacemaker/cib/cib.xml'
[vm-rhel72-1 ~] $ ssh vm-rhel72-3 'rm /var/lib/pacemaker/cib/cib.xml.sig'

[vm-rhel72-1 ~] $ pcs cluster start --all
vm-rhel72-1: Starting Cluster...
vm-rhel72-3: Starting Cluster...

[vm-rhel72-1 ~] $ cat /var/lib/pacemaker/cib/cib.xml | grep crm_feature_set
<cib crm_feature_set="3.0.12" validate-with="pacemaker-2.8" epoch="21" num_updates="0" admin_epoch="0" cib-last-written="Fri Oct 20 09:59:20 2017" update-origin="vm-rhel72-1" update-client="cibadmin" update-user="root" have-quorum="1">

> "force v2"
If pacemaker always upgrade crm_feature_set (is it so?) the problem should not appear with a new pacemaker. So "force v2" is not required in this case. If pcs meet an older version of pacemaker then it is without "force v2".

> upgrade in pcs
The workflow with CIB is: load (upgrade reload)? edit push. It is possible that the version of crm_feature_set is still unsufficient after the upgrade. So in the push must deal with an old crm_feature_set anyway. So the upgrade does not solve problem.

> There is an another option:
Pcs can check crm_feature_set during the push and it can push CIB without the crm_diff (i.e. full replacement + warning) if crm_feature_set is not sufficient.

This option should be enough:
1) New pcs should not meet an old pacemaker in RHEL. So the live CIB will be pushed always via diff in new RHEL.
2) When the pcs meet an old pacemaker then the live CIB will be fully replaced as before (see bug 1441673).
3) If there is CIB_file with an old crm_feature_set then the full replacement will be applied. The reasons for pushing via diff are described in bug 1441673: 1) concurrent editing  2) acl issue 3) performance. These should be no real problem in the case of CIB_file.

So does pacemaker always upgrade the crm_feature_set? If so then this bz could be reassigned to pcs.

> There is an another option:
> Pcs can check crm_feature_set during the push and it can push CIB without the
> crm_diff (i.e. full replacement + warning) if crm_feature_set is not
> sufficient.

Ah, I didn't think of that. That would be fine, too.

> So does pacemaker always upgrade the crm_feature_set?

No, pacemaker never does it automatically, only if someone/something calls cibadmin --upgrade.

Your test in Comment 6 didn't remove all previous CIBs (*.raw *.sig), so most likely, pacemaker used one of them instead of the unsigned cib.xml.

pcs does automatically call cibadmin --upgrade if the user uses a newer feature that their current feature set doesn't support. The "upgrade in pcs" suggestion is to do that unconditionally if the feature set is <= 3.0.8 (which should only be configurations created in 7.0 that have never been upgraded). But if we want to avoid unnecessary upgrades at all cost, we could instead do Comment 5 (the "force v2" option) or the full replacement.

> The workflow with CIB is: load (upgrade reload)? edit push. It is possible that
> the version of crm_feature_set is still unsufficient after the upgrade. So in
> the push must deal with an old crm_feature_set anyway. So the upgrade does not
> solve problem.

cibadmin --upgrade always upgrades to the most recent schema possible. It should be extremely rare that there any failures in that process.

Whoops, I confused crm_feature_set and the XML schema version. cibadmin --upgrade won't help at all here. crm_feature_set is set automatically based on the version of the node that's currently DC. So, we should only ever see crm_feature_set on a mixed-version cluster in a rolling upgrade from 7.0 to the current version, or in a CIB_file -- probably not a significant concern in practice.

Our options are to do a full replace when crm_feature_set <= 3.0.8, and/or to always force v2 patches downstream.

(In reply to Ken Gaillot from comment #8)
> Whoops, I confused crm_feature_set and the XML schema version. cibadmin
> --upgrade won't help at all here. crm_feature_set is set automatically based
> on the version of the node that's currently DC. So, we should only ever see
> crm_feature_set on a mixed-version cluster in a rolling upgrade from 7.0 to
> the current version, or in a CIB_file -- probably not a significant concern
> in practice.
> 
> Our options are to do a full replace when crm_feature_set <= 3.0.8, and/or
> to always force v2 patches downstream.

If I understand well:
The crm_feature_set could be <= 3.0.8 in the case that one node have an old pacemaker and this node is DC. 
Pushing the CIB via diff can fail in such cluster.

1) If pcs make full replacement of CIB in case of crm_feature_set <= 3.0.8 it will succeed. But the concurrency issue appears here.

2) If cibadmin adds "force v2" option then pushing CIB via diff succeeds on node with new pacemaker. But the full replacement is performed on a node with an old pacemaker (since an old cibadmin is without "force v2"; not mentioned that there is installed an old pcs version most probably). So the concurrency issue is solved only partialy.

I think that pcs should decide push mechanism according to crm_feature_set. I think it is not absolutely necessary to add "force v2" to cibadmin.

(In reply to Ivan Devat from comment #9)
> If I understand well:
> The crm_feature_set could be <= 3.0.8 in the case that one node have an old
> pacemaker and this node is DC. 
> Pushing the CIB via diff can fail in such cluster.

Correct. FYI, the oldest node will always be selected as the DC -- that ensures that all nodes support the features it uses.

> 1) If pcs make full replacement of CIB in case of crm_feature_set <= 3.0.8
> it will succeed. But the concurrency issue appears here.
> 
> 2) If cibadmin adds "force v2" option then pushing CIB via diff succeeds on
> node with new pacemaker. But the full replacement is performed on a node
> with an old pacemaker (since an old cibadmin is without "force v2"; not
> mentioned that there is installed an old pcs version most probably). So the
> concurrency issue is solved only partialy.
> 
> I think that pcs should decide push mechanism according to crm_feature_set.
> I think it is not absolutely necessary to add "force v2" to cibadmin.

Yes, that sounds good. Re-assigning to pcs.

Created attachment 1380384 [details]
proposed fix + tests

Test:

This can only be tested using pcs commands overhauled to the new pcs architecture. Commands not yet overhauled always replace the hole CIB and never do a diff. 

1. Use a CIB with crm_feature_set > 3.0.8. Run a pcs command, e.g. "pcs resource manage" with --debug and verify the CIB is pushed as a diff.
2. Use a CIB with crm_feature_set <= 3.0.8. Run a pcs command, e.g. "pcs resource manage" with --debug and verify the CIB is replaced as a whole and a warning is displayed proposing pacemaker update due to this.

Additional details leading to a more real-world like test scenario are described in comment 4.

After Fix:

[ant ~] $ rpm -q pcs pcs-snmp
pcs-0.9.165-1.el7.x86_64
pcs-snmp-0.9.165-1.el7.x86_64

>  with `crm_feature_set` >= 3.0.8: cibadmin is used only for loading cib and for pushing cib `crm_diff` is used

[ant ~] $ pcs cluster cib /tmp/cib.xml
[ant ~] $ cat /tmp/cib.xml|grep -m1 crm_feature_set
<cib crm_feature_set="3.0.12" validate-with="pacemaker-2.8" epoch="7" num_updates="5" admin_epoch="0" cib-last-written="Mon Jun 25 10:06:39 2018" update-origin="ant" update-client="cibadmin" update-user="root" have-quorum="1" dc-uuid="2">

[ant ~] $ pcs resource -f /tmp/cib.xml manage R1 --debug | grep "/usr/sbin/cibadmin\|crm_diff"
Running: /usr/sbin/cibadmin --local --query
Finished running: /usr/sbin/cibadmin --local --query
Running: /usr/sbin/crm_diff --original /tmp/tmpJQuNyA.pcs --new /tmp/tmpSyOMqj.pcs --no-version
Finished running: /usr/sbin/crm_diff --original /tmp/tmpJQuNyA.pcs --new /tmp/tmpSyOMqj.pcs --no-version

>  with `crm_feature_set` < 3.0.8: cibadmin is used for both - loading and pushing cib

[ant ~] $ sed -i 's#crm_feature_set="3.0.12"#crm_feature_set="3.0.7"#' /tmp/cib.xml
[ant ~] $ cat /tmp/cib.xml|grep -m1 crm_feature_set
<cib crm_feature_set="3.0.7" validate-with="pacemaker-2.8" epoch="7" num_updates="5" admin_epoch="0" cib-last-written="Mon Jun 25 10:06:39 2018" update-origin="ant" update-client="cibadmin" update-user="root" have-quorum="1" dc-uuid="2">

[ant ~] $ pcs resource -f /tmp/cib.xml manage R1 --debug | grep "/usr/sbin/cibadmin\|crm_diff"
Running: /usr/sbin/cibadmin --local --query
Finished running: /usr/sbin/cibadmin --local --query
Running: /usr/sbin/cibadmin --replace --verbose --xml-pipe --scope configuration
Finished running: /usr/sbin/cibadmin --replace --verbose --xml-pipe --scope configuration

Does pcs properly guard against

  pcs cluster cib-push new.xml diff-against=original.xml

when the operation would possibly fail because current crm_feature_set
is not sufficiently recent enough?

If not, it definitely should (i.e., full surface of this bug would
preferably get cleared).

There might even be an optimizing heuristic per the buggy behaviour
detailed above:

  push the complete version instead
  iff
  (
    patch adds first child of an element
    AND
    subsequently queried current crm_feature_set <= 3.0.8
  )

and otherwise, just send the delta.

Alternatively, fail in the non-straightforward cases
(yet allow --force?), suggesting full-fledged

  pcs cluster cib-push <new.xml> --config

* * *

Speaking of "queried crm_feature_set", it looks like the current
implemenation doesn't check what version is used within cluster
right now, which is bad when one takes new.xml and original.xml
from a completely different, possibly newer cluster.
(That's another fragility that could be prevented with the
suggested "integrity pipelines scheme" per [bug 1359057]).

(In reply to Jan Pokorný from comment #17)
> Does pcs properly guard against
> 
>   pcs cluster cib-push new.xml diff-against=original.xml
> 
> when the operation would possibly fail because current crm_feature_set
> is not sufficiently recent enough?

No, it does not. Here is a reproducer:

[ant ~] $ pcs cluster cib /tmp/cib.original.xml
[ant ~] $ cp /tmp/cib.original.xml /tmp/cib.new.xml
[ant ~] $ pcs resource -f /tmp/cib.new.xml create R1 ocf:heartbeat:Dummy
[ant ~] $ sed -i 's#crm_feature_set="3.0.14"#crm_feature_set="3.0.7"#' /tmp/cib.original.xml
[ant ~] $ pcs cluster cib-push /tmp/cib.new.xml diff-against=/tmp/cib.original.xml
Error: unable to push cib
Call cib_apply_diff failed (-206): Application of an update diff failed

Thank you for pointing out.

> 
> If not, it definitely should (i.e., full surface of this bug would
> preferably get cleared).
> 
> There might even be an optimizing heuristic per the buggy behaviour
> detailed above:
> 
>   push the complete version instead
>   iff
>   (
>     patch adds first child of an element
>     AND
>     subsequently queried current crm_feature_set <= 3.0.8
>   )
> 
> and otherwise, just send the delta.

The `diff-against` option was introduced in context of bug 1404233. So I'm not sure if pushing complete version is the right way.

Created attachment 1475608 [details]
fix for "pcs cluster cib-push"

Test for the patch from comment 20 is in comment 19, expected results after fix:
# pcs cluster cib-push bbb.xml diff-against=aaa.xml
Error: unable to diff against original cib 'aaa.xml': the 'crm_feature_set' version is '3.0.1' but at least version '3.0.9' is required

After fix
[root@rhel76-node1 ~]# rpm -q pcs
pcs-0.9.165-4.el7.x86_64

[root@rhel76-node1 ~]# pcs cluster cib cib.orig.xml
[root@rhel76-node1 ~]# cp cib.orig.xml cib.new.xml
[root@rhel76-node1 ~]# pcs resource -f cib.new.xml create R1 ocf:heartbeat:Dummy
[root@rhel76-node1 ~]# sed -i 's#crm_feature_set="3.0.14"#crm_feature_set="3.0.7"#' cib.orig.xml
[root@rhel76-node1 ~]# pcs cluster cib-push cib.new.xml diff-against=cib.orig.xml
Error: unable to diff against original cib 'cib.orig.xml': the 'crm_feature_set' version is '3.0.7' but at least version '3.0.9' is required

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:3066