libyang icon indicating copy to clipboard operation
libyang copied to clipboard
verified publisher icon CESNET

Behavior difference on diff after dup and merge compared to no merge

Open willtoth opened this issue 1 year ago • 5 comments

This is a bit of a continuation of #2138

I'll start with the example which produces the output which I am expecting.

Loading all trees using lyd_parse_data_mem(ctx, xml, LYD_XML, LYD_PARSE_STRICT, 0, &node);

Printing by

void print_node(struct lyd_node* node) {
    struct ly_out* out;
    char* result;
    ly_out_new_memory(&result, 0, &out);
    lyd_print_all(out, node, LYD_XML, 0);
    printf("%s", result);
    free(result);
}

Tree c1

<native xmlns="http://cisco.com/ns/yang/Cisco-IOS-XE-native">
  <interface>
    <Vlan>
      <name>20</name>
      <ip>
        <address>
          <primary>
            <address>20.0.0.1</address>
            <mask>255.255.0.0</mask>
          </primary>
        </address>
      </ip>
    </Vlan>
  </interface>
</native>

Tree c3

<native xmlns="http://cisco.com/ns/yang/Cisco-IOS-XE-native">
  <hostname>Blink-st-1</hostname>
  <interface>
    <Vlan>
      <name>20</name>
      <ip>
        <address>
          <primary>
            <address>20.0.0.1</address>
            <mask>255.255.0.0</mask>
          </primary>
        </address>
      </ip>
    </Vlan>
  </interface>
  <fhrp>
    <version>
      <vrrp>v3</vrrp>
    </version>
  </fhrp>
</native>

Calling lyd_diff_siblings(c1, c3, 0, &diff); produces:

<native xmlns="http://cisco.com/ns/yang/Cisco-IOS-XE-native" xmlns:yang="urn:ietf:params:xml:ns:yang:1" yang:operation="none">
  <hostname yang:operation="create">Blink-st-1</hostname>
  <fhrp yang:operation="create">
    <version>
      <vrrp>v3</vrrp>
    </version>
  </fhrp>
</native>

This is exactly what I expect, a yang:create for hostname and fhrp. Note that fhrp is a type ENUM and has a default value of v2.

Now the part that I don't quite understand. Instead of creating c3, it is built using a merge operation.

Tree c2:

<native xmlns="http://cisco.com/ns/yang/Cisco-IOS-XE-native" xmlns:yang="urn:ietf:params:xml:ns:yang:1" yang:operation="none">
  <hostname>Blink-st-1</hostname>
  <fhrp>
    <version>
      <vrrp>v3</vrrp>
    </version>
  </fhrp>
</native>

Tree merged is created as the merge of c1 and c2

    lyd_dup_siblings(c1, NULL, LYD_DUP_RECURSIVE | LYD_DUP_WITH_FLAGS | LYD_DUP_WITH_PARENTS, &merged);
    lyd_merge_module(&merged, c2, NULL, NULL, NULL, LYD_MERGE_WITH_FLAGS);

Printing this tree matches c3 above, as expected. However now when I call the diff function I get:

<native xmlns="http://cisco.com/ns/yang/Cisco-IOS-XE-native" xmlns:yang="urn:ietf:params:xml:ns:yang:1" yang:operation="none">
  <hostname yang:operation="create">Blink-st-1</hostname>
</native>

The expected create is no longer printed for fhrp. It is present as a default value, and not printed, so I believe this all comes down the handling of default and explicit values. And there are some knobs for this. What is surprising to me however is using the same settings, I can print the merged and c3 trees, they both match, however the printed result of the diff tree does not. Adding an explicit validate stage to merged does not change anything. This is my primary question, why do two trees which appear to the exactly the same, produce different diffs?

Digging in a little deeper, I print out which nodes have the LYD_DEFAULT flag set in the merged and c3 trees and I see the following do not have the LYD_DEFAULT:

void print_explicit(const struct lyd_node* other) {
    struct lyd_node* tmp_node;

    printf("Starting DFS on OTHER\r\n");
    LYD_TREE_DFS_BEGIN(other, tmp_node)
    {
        if (!(tmp_node->flags & LYD_DEFAULT)) {
            printf("NOT DEFAULT: %s\r\n", tmp_node->schema->name);
        }
        LYD_TREE_DFS_END(other, tmp_node);
    }
}

c3:

NOT DEFAULT: fhrp
NOT DEFAULT: version
NOT DEFAULT: vrrp

merged:

NOT DEFAULT: vrrp

I believe this is some kind of bug, maybe with LYD_MERGE_WITH_FLAGS? fhrp and version should not have the LYD_DEFAULT flag in the merged output. In the merged tree, fhrp and version are still marked as default. In the c3 tree, they are not marked as default.

Some things I have tried, and the issues with them:

  1. Adding the default value of fhrp as v2 into tree c1 always produces the expected diff. I can see fhrp is not marked default.
  2. Adding or removing all combinations of LYD_MERGE_WITH_FLAGS and/or LYD_MERGE_DEFAULTS did not make any difference
  3. Adding the flag LYD_DIFF_DEFAULTS also produced the correct output. However this flag is not ideal, as it causes many leaves to be left in after the diff without any operation on them, and I don't see a way to not print these. This is similar to #2138 and I can find an example here if needed.
  4. Change the print options to LYD_PRINT_WD_IMPL_TAG however this adds a huge amount of noise, since there are many defaults that would be created. This doesn't show up in the above example, but anything more complex in tree c2 does.
  5. After the merge operation, simply recreate the tree by printing the merge result as XML, and re-parsing it as a new tree. This always results in the expected diff, but is quite wasteful since it must output and re-parse the tree.

Workaround: Before doing the diff, find all nodes in c2 which are not marked as default, and remove the corresponding LYD_DEFAULT flag in the merged tree.

void mark_explicit(struct lyd_node* node, const struct lyd_node* other) {
    struct lyd_node* tmp_node;

    LYD_TREE_DFS_BEGIN(other, tmp_node)
    {
        if (!(tmp_node->flags & LYD_DEFAULT)) {
            struct lyd_node* node_to_update;
            char* path = lyd_path(tmp_node, LYD_PATH_STD, NULL, 0);
            lyd_find_path(node, path, false, &node_to_update);
            node_to_update->flags &= ~LYD_DEFAULT;
            free(path);
        }
        LYD_TREE_DFS_END(other, tmp_node);
    }
}

mark_explicit(merged, c2);

This produces the expected diff, and expected default state in the merged tree.

test.zip

willtoth avatar Oct 18 '24 15:10 willtoth

Please try to reduce the problem to just the example (thanks for that) and what should be the expected output (or behavior) instead of the one the example prints. You are talking about a bug but I failed to decipher what should it be exactly.

michalvasko avatar Oct 21 '24 12:10 michalvasko

Sure thing, let me frame it based on the example.

Line 142 is my workaround for the bug. To re-create the bug, comment out line 142 mark_explicit(merged, c2);

The example prints:

  • tree c1
  • tree c2
  • merged tree of c1 and c2
  • diff tree of merged and c1
====> c1 <====
<native xmlns="http://cisco.com/ns/yang/Cisco-IOS-XE-native">
  <interface>
    <Vlan>
      <name>20</name>
      <ip>
        <address>
          <primary>
            <address>20.0.0.1</address>
            <mask>255.255.0.0</mask>
          </primary>
        </address>
      </ip>
    </Vlan>
  </interface>
</native>
====> c2 <====
<native xmlns="http://cisco.com/ns/yang/Cisco-IOS-XE-native" xmlns:yang="urn:ietf:params:xml:ns:yang:1" yang:operation="none">
  <hostname>Blink-st-1</hostname>
  <fhrp>
    <version>
      <vrrp>v3</vrrp>
    </version>
  </fhrp>
</native>
====> Merged Tree <====
<native xmlns="http://cisco.com/ns/yang/Cisco-IOS-XE-native">
  <hostname>Blink-st-1</hostname>
  <interface>
    <Vlan>
      <name>20</name>
      <ip>
        <address>
          <primary>
            <address>20.0.0.1</address>
            <mask>255.255.0.0</mask>
          </primary>
        </address>
      </ip>
    </Vlan>
  </interface>
  <fhrp>
    <version>
      <vrrp>v3</vrrp>
    </version>
  </fhrp>
</native>
====> Diff Merged Tree <====
<native xmlns="http://cisco.com/ns/yang/Cisco-IOS-XE-native" xmlns:yang="urn:ietf:params:xml:ns:yang:1" yang:operation="none">
  <hostname yang:operation="create">Blink-st-1</hostname>
</native>

Uncomment line 142 (example as-is above) gives the expected output:

====> c1 <====
<native xmlns="http://cisco.com/ns/yang/Cisco-IOS-XE-native">
  <interface>
    <Vlan>
      <name>20</name>
      <ip>
        <address>
          <primary>
            <address>20.0.0.1</address>
            <mask>255.255.0.0</mask>
          </primary>
        </address>
      </ip>
    </Vlan>
  </interface>
</native>
====> c2 <====
<native xmlns="http://cisco.com/ns/yang/Cisco-IOS-XE-native" xmlns:yang="urn:ietf:params:xml:ns:yang:1" yang:operation="none">
  <hostname>Blink-st-1</hostname>
  <fhrp>
    <version>
      <vrrp>v3</vrrp>
    </version>
  </fhrp>
</native>
====> Merged Tree <====
<native xmlns="http://cisco.com/ns/yang/Cisco-IOS-XE-native">
  <hostname>Blink-st-1</hostname>
  <interface>
    <Vlan>
      <name>20</name>
      <ip>
        <address>
          <primary>
            <address>20.0.0.1</address>
            <mask>255.255.0.0</mask>
          </primary>
        </address>
      </ip>
    </Vlan>
  </interface>
  <fhrp>
    <version>
      <vrrp>v3</vrrp>
    </version>
  </fhrp>
</native>
====> Diff Merged Tree <====
<native xmlns="http://cisco.com/ns/yang/Cisco-IOS-XE-native" xmlns:yang="urn:ietf:params:xml:ns:yang:1" yang:operation="none">
  <hostname yang:operation="create">Blink-st-1</hostname>
  <fhrp yang:operation="create">
    <version>
      <vrrp>v3</vrrp>
    </version>
  </fhrp>
</native>

The bug is, without the workaround the following part of the diff is not printed. I suspect it is due to fhrp and version having flags LYD_DEFAULT in the merged tree.

  <fhrp yang:operation="create">
    <version>
      <vrrp>v3</vrrp>
    </version>
  </fhrp>

willtoth avatar Oct 21 '24 14:10 willtoth

Yes, it is because of that, you have to use LYD_DIFF_DEFAULTS to get diff on default values. But you had a problem with that, but I just do not understand how do you expect it should work, even in theory. How is libyang supposed to know you want diff on some default nodes but not on others?

michalvasko avatar Oct 22 '24 08:10 michalvasko

This was my initial thought too, sorry if this is not clear. In the above Merged Tree the node vrrp is not default. So it should remain not default and eventually printed. v2 is the default value for that node. The difference is that fhrp and version is default for some reason, but it should not be.

Going back to the original example, the variable c3_xml is the same xml printed by the merge operation. If I comment out line 142 mark_explicit(merged, c2); and on line 125 set to #if 0 - this will load the xml instead of using the result of the merge operation without any kind of workaround.

Let me modify and attach a new example, as I believe it is the result of the merge operation that is causing this issue.

In example2

  1. I load tree c1, c2, and c3
  2. Then I dup c1 and merge the dup and c2 (c1 and c2 are unchanged).
  3. I print c1, c2, c3, and merged. NOTE: c3 and the merged tree should be the same
  4. I print out all nodes in c3 and the merged tree without the LYD_DEFAULT flag
====> c1 <====
<native xmlns="http://cisco.com/ns/yang/Cisco-IOS-XE-native">
  <interface>
    <Vlan>
      <name>20</name>
      <ip>
        <address>
          <primary>
            <address>20.0.0.1</address>
            <mask>255.255.0.0</mask>
          </primary>
        </address>
      </ip>
    </Vlan>
  </interface>
</native>
====> c2 <====
<native xmlns="http://cisco.com/ns/yang/Cisco-IOS-XE-native" xmlns:yang="urn:ietf:params:xml:ns:yang:1" yang:operation="none">
  <hostname>Blink-st-1</hostname>
  <fhrp>
    <version>
      <vrrp>v3</vrrp>
    </version>
  </fhrp>
</native>
====> c3 <====
<native xmlns="http://cisco.com/ns/yang/Cisco-IOS-XE-native">
  <hostname>Blink-st-1</hostname>
  <interface>
    <Vlan>
      <name>20</name>
      <ip>
        <address>
          <primary>
            <address>20.0.0.1</address>
            <mask>255.255.0.0</mask>
          </primary>
        </address>
      </ip>
    </Vlan>
  </interface>
  <fhrp>
    <version>
      <vrrp>v3</vrrp>
    </version>
  </fhrp>
</native>
====> Merged Tree <====
<native xmlns="http://cisco.com/ns/yang/Cisco-IOS-XE-native">
  <hostname>Blink-st-1</hostname>
  <interface>
    <Vlan>
      <name>20</name>
      <ip>
        <address>
          <primary>
            <address>20.0.0.1</address>
            <mask>255.255.0.0</mask>
          </primary>
        </address>
      </ip>
    </Vlan>
  </interface>
  <fhrp>
    <version>
      <vrrp>v3</vrrp>
    </version>
  </fhrp>
</native>
====> Non-Default Nodes in c3: <====
NOT DEFAULT: native
NOT DEFAULT: hostname
NOT DEFAULT: interface
NOT DEFAULT: Vlan
NOT DEFAULT: name
NOT DEFAULT: ip
NOT DEFAULT: address
NOT DEFAULT: primary
NOT DEFAULT: address
NOT DEFAULT: mask
NOT DEFAULT: fhrp
NOT DEFAULT: version
NOT DEFAULT: vrrp
====> Non-Default Nodes in Merged Tree: <====
NOT DEFAULT: native
NOT DEFAULT: hostname
NOT DEFAULT: interface
NOT DEFAULT: Vlan
NOT DEFAULT: name
NOT DEFAULT: ip
NOT DEFAULT: address
NOT DEFAULT: primary
NOT DEFAULT: address
NOT DEFAULT: mask
NOT DEFAULT: vrrp

Note for merged and for c3 who look and should be functionally equivalent, there is a difference in the set of non-default values. Specifically, merged is missing fhrp and version.

I believe this then leads to the diff and printing treating the node as default in the resulting diff tree of example 1.

Example attached. example2.zip

willtoth avatar Oct 22 '24 14:10 willtoth

Thanks for the explanation and you are right, the default flag was not being updated properly because of obsolete merge code. Should be fixed now.

michalvasko avatar Oct 23 '24 10:10 michalvasko