Skip to content

libyang3 schema default value not honored ... randomly ... bug #2350

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
bradh352 opened this issue Feb 12, 2025 · 13 comments
Open

libyang3 schema default value not honored ... randomly ... bug #2350

bradh352 opened this issue Feb 12, 2025 · 13 comments
Labels
is:bug Bug description. status:invalid Issue is not reproducible or the behavior is intended.

Comments

@bradh352
Copy link

bradh352 commented Feb 12, 2025

While porting SONiC from libyang 1.0.73 to 3.7.8, I ran across a peculiar issue. Unless I'm evaluating it wrong, it looks like a bug in libyang's handling of default values but simply reordering the leaf nodes in the container magically fixes it. I've only found a single instance of this in all of SONiC's schema.

If I reorder the leaf nodes in the schema, then the default value shows up and works.

I have a commit here showing the schema reorder I made to make the tests pass:

sonic-net/sonic-buildimage@dd1dddb

The schema leafs impacted are:

leaf default_bgp_status {
    type enumeration {
        enum up;
        enum down;
    }
    default up;
}
leaf docker_routing_config_mode {
    description "This leaf allows different configuration modes for FRR:
                - separated: FRR config generated from ConfigDB, each FRR daemon has its own config file
                - unified: FRR config generated from ConfigDB, single FRR config file
                - split: FRR config not generated from ConfigDB, each FRR daemon has its own config file
                - split-unified: FRR config not generated from ConfigDB, single FRR config file";
    type string {
        pattern "separated|unified|split|split-unified";
    }
    default "unified";
}

both tests do an xpath query for /sonic-device_metadata:sonic-device_metadata/DEVICE_METADATA/localhost/hostname then load the child nodes into a dictionary, and validate the keys of sonic-device_metadata:default_bgp_status and sonic-device_metadata:docker_routing_config_mode exist and are of the expected values.

This is happening in libyang-python with code similar to this:

nodes = node.find_all("/sonic-device_metadata:sonic-device_metadata/DEVICE_METADATA/localhost/hostname")
for dnode in nodes:
  if (xpath == dnode.path()):
    data = dnode.print_mem("json", with_siblings=True, pretty=True, include_implicit_defaults=True)
    data = json.loads(data)
    assert ("sonic-device_metadata:default_bgp_status" in data)
    assert (data["sonic-device_metadata:default_bgp_status"] == "up")

I'm not sure what the underlying cause is so I don't know how to submit a reduced test case.

@michalvasko
Copy link
Member

That is weird indeed and it would be great if I could reproduce it somehow. What could perhaps affect this is the forced ordering of nodes in newer libyang when they are always following the order in the YANG module. Some functions may create nodes "before" your pointer to the data tree, which may one not always expect and it can result in some "missing" nodes. But no idea whether this can really be an issue or how this is handled in the Python bindings.

@michalvasko michalvasko added the is:bug Bug description. label Feb 14, 2025
@bradh352
Copy link
Author

I don't see how this could be python-specific, since the bindings on these functions are pretty minimal. I've got to imagine its in the C libyang itself, that said, I can't say if maybe its in print_mem() not emitting the default values somehow or in the parsed data tree. Once I finish up the SONiC porting and get the other PRs accepted here in libyang we depend on I can probably spend some time to write up a test case for this in C (though I'll probably include the entire yang schema and json config in the test case unless I can fairly quickly come up with a reduced set that reproduces the issue).

@michalvasko
Copy link
Member

That would be great, no problem with including a YANG schema and a data file, even if large.

@Krisscut
Copy link

Krisscut commented Feb 19, 2025

I'm encountering a similar issue with the default values not being reported in a stack using these software (trying to upgrade from older versions):

netopeer2-server-2.2.35
libnetconf2-3.5.5
libyang-3.7.8
sysrepo-3.3.10

I'm not yet 100% sure on my end if the issue lies in libyang itself or one of those other components.

In my case, a netconf client perform a get but and is missing a default-value leaf when previous to the upgrade it was returned.
Enabling the debug traces in netopeer2-server, I can see that in the reply sent to the client it is already missing:

See schema here, and notably the leaf maximum-number-of-transport-flows:
Schema on an open gerrit

What was sent before in an edit config:

<processing-elements xmlns="urn:o-ran:processing-element:1.0">
  <transport-session-type>ETH-INTERFACE</transport-session-type>
  <ru-elements>
    <name>DFE1</name>
    <transport-flow>
      <interface-name>m-vlan24</interface-name>
      <eth-flow>
          <ru-mac-address>02:42:ac:11:00:02</ru-mac-address>
          <vlan-id>24</vlan-id>
          <o-du-mac-address>00:77:e4:2d:2a:9a</o-du-mac-address>
      </eth-flow>
    </transport-flow>
  </ru-elements>
</processing-elements>

Then what is fetched using a get rpc on the processing-elements module:

netopeer2-server(PID:2703): [DBG]: LN: Session 4: Received message:
netopeer2-server(PID:2703): <?xml version="1.0" encoding="UTF-8"?><rpc xmlns="urn:ietf:params:xml:ns:netconf:base:1.0" message-id="urn:uuid:d8e64d99-66bf-41d1-b945-bccd35d50f4f"><get><filter type="subtree"><processing-elements xmlns="urn:o-ran:processing-element:1.0"/></filter><ns0:with-defaults xmlns:ns0="urn:ietf:params:xml:ns:yang:ietf-netconf-with-defaults">report-all</ns0:with-defaults></get></rpc>
netopeer2-server(PID:2703): [DBG]: LN: Session 4: Sending message:
netopeer2-server(PID:2703): #553
netopeer2-server(PID:2703): [DBG]: LN: Session 4: Sending message:
netopeer2-server(PID:2703): <rpc-reply xmlns="urn:ietf:params:xml:ns:netconf:base:1.0" message-id="urn:uuid:d8e64d99-66bf-41d1-b945-bccd35d50f4f"><data><processing-elements xmlns="urn:o-ran:processing-element:1.0"><transport-session-type>ETH-INTERFACE</transport-session-type><ru-elements><name>DFE1</name><transport-flow><interface-name>m-vlan24</interface-name><eth-flow><ru-mac-address>02:42:ac:11:00:02</ru-mac-address><vlan-id>24</vlan-id><o-du-mac-address>00:77:e4:2d:2a:9a</o-du-mac-address></eth-flow></transport-flow></ru-elements></processing-elements></data></rpc-reply>

Note that in my example, using sysrepocfg to dump the content of the operational datastore with
sysrepocfg -X -x /processing-elements -d o -e report-all

Returns nothing with new versions but was returning the default values before.

I will try to have a repro step in a small and controlled environment to facilitate investigation as well !

@Krisscut
Copy link

Krisscut commented Feb 20, 2025

On my end this is reproduceable using the attached yang model (I used the processing-element one but removed external dependencies) and this script (uses sysrepoctl to load it and get with defaults)

#!/usr/bin/env bash
source $(dirname $0)/setup_env.sh

set -xe

echo "List installed modules"
$SYSREPOCTL_EXECUTABLE -l

echo "Install custom yang file"
$SYSREPOCTL_EXECUTABLE -i $(dirname $0)/yang_file.yang

echo "List installed modules after"
$SYSREPOCTL_EXECUTABLE -l

echo "List processingelements"
$SYSREPOCFG_EXECUTABLE -X -x /processing-elements -d o -e report-all

With previous stack running
sysrepo-2.2.150
libyang-2.1.148
netopeer2-2.2.13

I have this result:

+ echo 'List installed modules'
List installed modules
+ /opt/project/bin/sysrepoctl -l
Sysrepo repository: /var/tmp/sysrepo/repository

Module Name                | Revision   | Flags | Startup Owner  | Startup Perms | Running Perms | Submodules | Features                 
-----------------------------------------------------------------------------------------------------------------------------------------
ietf-datastores            | 2018-02-14 | I     | user:everybody | 444           | 444           |            |                          
ietf-factory-default       | 2020-08-31 | I     | user:everybody | 600           | 600           |            | factory-default-datastore
ietf-inet-types            | 2013-07-15 | i     |                |               |               |            |                          
ietf-netconf               | 2013-09-29 | I     | user:everybody | 644           | 644           |            |                          
ietf-netconf-acm           | 2018-02-14 | I     | user:everybody | 600           | 600           |            |                          
ietf-netconf-notifications | 2012-02-06 | I     | user:everybody | 644           | 644           |            |                          
ietf-netconf-with-defaults | 2011-06-01 | I     | user:everybody | 444           | 444           |            |                          
ietf-origin                | 2018-02-14 | I     | user:everybody | 444           | 444           |            |                          
ietf-yang-library          | 2019-01-04 | I     | user:everybody | 644           | 644           |            |                          
ietf-yang-metadata         | 2016-08-05 | i     |                |               |               |            |                          
ietf-yang-schema-mount     | 2019-01-14 | I     | user:everybody | 644           | 644           |            |                          
ietf-yang-structure-ext    | 2020-06-17 | i     |                |               |               |            |                          
ietf-yang-types            | 2013-07-15 | i     |                |               |               |            |                          
sysrepo-factory-default    | 2023-02-23 | I     | user:everybody | 600           | 600           |            |                          
sysrepo-monitoring         | 2023-08-11 | I     | user:everybody | 600           | 600           |            |                          
sysrepo-plugind            | 2022-08-26 | I     | user:everybody | 644           | 644           |            |                          
yang                       | 2022-06-16 | I     | user:everybody | 444           | 444           |            |                          

Flags meaning: I - Installed/i - Imported; R - Replay support

+ echo 'Install custom yang file'
Install custom yang file
++ dirname /workspace/Services/Netopeer2/basic_test.sh
+ /opt/project/bin/sysrepoctl -i /workspace/Services/Netopeer2/yang_file.yang
+ echo 'List installed modules after'
List installed modules after
+ /opt/project/bin/sysrepoctl -l
Sysrepo repository: /var/tmp/sysrepo/repository

Module Name                | Revision   | Flags | Startup Owner  | Startup Perms | Running Perms | Submodules | Features                 
-----------------------------------------------------------------------------------------------------------------------------------------
ietf-datastores            | 2018-02-14 | I     | user:everybody | 444           | 444           |            |                          
ietf-factory-default       | 2020-08-31 | I     | user:everybody | 600           | 600           |            | factory-default-datastore
ietf-inet-types            | 2013-07-15 | i     |                |               |               |            |                          
ietf-netconf               | 2013-09-29 | I     | user:everybody | 644           | 644           |            |                          
ietf-netconf-acm           | 2018-02-14 | I     | user:everybody | 600           | 600           |            |                          
ietf-netconf-notifications | 2012-02-06 | I     | user:everybody | 644           | 644           |            |                          
ietf-netconf-with-defaults | 2011-06-01 | I     | user:everybody | 444           | 444           |            |                          
ietf-origin                | 2018-02-14 | I     | user:everybody | 444           | 444           |            |                          
ietf-yang-library          | 2019-01-04 | I     | user:everybody | 644           | 644           |            |                          
ietf-yang-metadata         | 2016-08-05 | i     |                |               |               |            |                          
ietf-yang-schema-mount     | 2019-01-14 | I     | user:everybody | 644           | 644           |            |                          
ietf-yang-structure-ext    | 2020-06-17 | i     |                |               |               |            |                          
ietf-yang-types            | 2013-07-15 | i     |                |               |               |            |                          
o-ran-processing-element   | 2020-04-17 | I     | user:everybody | 600           | 600           |            |                          
sysrepo-factory-default    | 2023-02-23 | I     | user:everybody | 600           | 600           |            |                          
sysrepo-monitoring         | 2023-08-11 | I     | user:everybody | 600           | 600           |            |                          
sysrepo-plugind            | 2022-08-26 | I     | user:everybody | 644           | 644           |            |                          
yang                       | 2022-06-16 | I     | user:everybody | 444           | 444           |            |                          

Flags meaning: I - Installed/i - Imported; R - Replay support

+ echo 'List processingelements'
List processingelements
+ /opt/project/bin/sysrepocfg -X -x /processing-elements -d o -e report-all
<processing-elements xmlns="urn:o-ran:processing-element:1.0">
  <maximum-number-of-transport-flows>4094</maximum-number-of-transport-flows>
</processing-elements>

With new stack running :
sysrepo-3.3.10
libyang-3.7.8
netopeer2-server-2.2.35

List installed modules
+ /opt/project/bin/sysrepoctl -l
Sysrepo repository: /var/tmp/sysrepo/repository

Module Name                | Revision   | Flags | Startup Owner  | Startup Perms | Running Perms | Submodules | Features                 
-----------------------------------------------------------------------------------------------------------------------------------------
ietf-datastores            | 2018-02-14 | I     | user:everybody | 444           | 444           |            |                          
ietf-factory-default       | 2020-08-31 | I     | user:everybody | 600           | 600           |            | factory-default-datastore
ietf-inet-types            | 2013-07-15 | i     |                |               |               |            |                          
ietf-netconf               | 2013-09-29 | I     | user:everybody | 644           | 644           |            |                          
ietf-netconf-acm           | 2018-02-14 | I     | user:everybody | 600           | 600           |            |                          
ietf-netconf-notifications | 2012-02-06 | I     | user:everybody | 644           | 644           |            |                          
ietf-netconf-with-defaults | 2011-06-01 | I     | user:everybody | 444           | 444           |            |                          
ietf-origin                | 2018-02-14 | I     | user:everybody | 444           | 444           |            |                          
ietf-yang-library          | 2019-01-04 | I     | user:everybody | 644           | 644           |            |                          
ietf-yang-metadata         | 2016-08-05 | i     |                |               |               |            |                          
ietf-yang-schema-mount     | 2019-01-14 | I     | user:everybody | 644           | 644           |            |                          
ietf-yang-structure-ext    | 2020-06-17 | i     |                |               |               |            |                          
ietf-yang-types            | 2013-07-15 | i     |                |               |               |            |                          
sysrepo-factory-default    | 2024-05-02 | I     | user:everybody | 600           | 600           |            |                          
sysrepo-monitoring         | 2023-08-11 | I     | user:everybody | 600           | 600           |            |                          
sysrepo-plugind            | 2022-08-26 | I     | user:everybody | 644           | 644           |            |                          
yang                       | 2022-06-16 | I     | user:everybody | 444           | 444           |            |                          

Flags meaning: I - Installed/i - Imported; R - Replay support

+ echo 'Install custom yang file'
Install custom yang file
++ dirname /workspace/Services/Netopeer2/basic_test.sh
+ /opt/project/bin/sysrepoctl -i /workspace/Services/Netopeer2/yang_file.yang
+ echo 'List installed modules after'
List installed modules after
+ /opt/project/bin/sysrepoctl -l
Sysrepo repository: /var/tmp/sysrepo/repository

Module Name                | Revision   | Flags | Startup Owner  | Startup Perms | Running Perms | Submodules | Features                 
-----------------------------------------------------------------------------------------------------------------------------------------
ietf-datastores            | 2018-02-14 | I     | user:everybody | 444           | 444           |            |                          
ietf-factory-default       | 2020-08-31 | I     | user:everybody | 600           | 600           |            | factory-default-datastore
ietf-inet-types            | 2013-07-15 | i     |                |               |               |            |                          
ietf-netconf               | 2013-09-29 | I     | user:everybody | 644           | 644           |            |                          
ietf-netconf-acm           | 2018-02-14 | I     | user:everybody | 600           | 600           |            |                          
ietf-netconf-notifications | 2012-02-06 | I     | user:everybody | 644           | 644           |            |                          
ietf-netconf-with-defaults | 2011-06-01 | I     | user:everybody | 444           | 444           |            |                          
ietf-origin                | 2018-02-14 | I     | user:everybody | 444           | 444           |            |                          
ietf-yang-library          | 2019-01-04 | I     | user:everybody | 644           | 644           |            |                          
ietf-yang-metadata         | 2016-08-05 | i     |                |               |               |            |                          
ietf-yang-schema-mount     | 2019-01-14 | I     | user:everybody | 644           | 644           |            |                          
ietf-yang-structure-ext    | 2020-06-17 | i     |                |               |               |            |                          
ietf-yang-types            | 2013-07-15 | i     |                |               |               |            |                          
o-ran-processing-element   | 2020-04-17 | I     | user:everybody | 600           | 600           |            |                          
sysrepo-factory-default    | 2024-05-02 | I     | user:everybody | 600           | 600           |            |                          
sysrepo-monitoring         | 2023-08-11 | I     | user:everybody | 600           | 600           |            |                          
sysrepo-plugind            | 2022-08-26 | I     | user:everybody | 644           | 644           |            |                          
yang                       | 2022-06-16 | I     | user:everybody | 444           | 444           |            |                          

Flags meaning: I - Installed/i - Imported; R - Replay support

+ echo 'List processingelements'
List processingelements
+ /opt/project/bin/sysrepocfg -X -x /processing-elements -d o -e report-all

yang_file.zip

@michalvasko
Copy link
Member

Okay, thanks for the reproducer and files, based on which I have learned that you are talking about a config false node with a default value. For these the current behavior is intentional and was obviously changed compared to the older version. The logic is that the operational state (config false nodes) must be fully reported by applications including any default values that they are using.

@michalvasko michalvasko added the status:invalid Issue is not reproducible or the behavior is intended. label Feb 20, 2025
@Krisscut
Copy link

Okay, thanks for the reproducer and files, based on which I have learned that you are talking about a config false node with a default value. For these the current behavior is intentional and was obviously changed compared to the older version. The logic is that the operational state (config false nodes) must be fully reported by applications including any default values that they are using.

Ok, thanks for the reply, then I will make adaptations on my end !

Note that @bradh352 case might not be the same, sorry If I hijacked this thread and his issue is still not addressed but it seemed similar.

@bradh352
Copy link
Author

Yeah, my issue I'm pretty sure is different. Finishing up porting the last SONiC module to libyang3 (GOlang using cgo, yuck) ... then I'll loop back around on providing a test case.

@bradh352
Copy link
Author

bradh352 commented Mar 6, 2025

Ok, I finally had time to write a test case. I haven't had a chance to try to mess with the yang to minimize it at all, I just took all the SONiC yang models. See attached issue-2350.tar.gz

Extract and compile with something like:

cc -Wall -W -I/usr/local/include -L/usr/local/bin -Wl,-rpath=/usr/local/lib -o issue_2350 issue_2350.c -lyang

I've embedded both good and bad YANG models, with the diff as previously mentioned which shows just a reorder of nodes:

diff -ruN yang-models-bad yang-models-good
diff -ruN yang-models-bad/sonic-device_metadata.yang yang-models-good/sonic-device_metadata.yang
--- yang-models-bad/sonic-device_metadata.yang	2025-03-06 11:04:49.287561091 +0000
+++ yang-models-good/sonic-device_metadata.yang	2025-03-06 13:41:10.395131606 +0000
@@ -34,7 +34,7 @@
 
             description "DEVICE_METADATA part of config_db.json";
 
-            container localhost{
+            container localhost {
 
                 leaf hwsku {
                     type stypes:hwsku;
@@ -47,6 +47,20 @@
                     description "asic_id is unique identifier of the asic used by SAI for initialization.";
                 }
 
+                leaf hostname {
+                    type stypes:hostname;
+                }
+
+                leaf platform {
+                    type string {
+                        length 1..255;
+                    }
+                }
+
+                leaf mac {
+                    type yang:mac-address;
+                }
+
                 leaf default_bgp_status {
                     type enumeration {
                         enum up;
@@ -67,20 +81,6 @@
                     default "unified";
                 }
 
-                leaf hostname {
-                    type stypes:hostname;
-                }
-
-                leaf platform {
-                    type string {
-                        length 1..255;
-                    }
-                }
-
-                leaf mac {
-                    type yang:mac-address;
-                }
-
                 leaf default_pfcwd_status {
                     type enumeration {
                         enum disable;

It does one pass on the good yang model and one pass on the bad yang model to extract the json then compares them, output of running the command looks like:

./issue_2350 
======================
Expected: 
{
  "sonic-device_metadata:hostname": "DUT-ASW",
  "sonic-device_metadata:platform": "Stone-DX010",
  "sonic-device_metadata:default_bgp_status": "up",
  "sonic-device_metadata:docker_routing_config_mode": "separated",
  "sonic-device_metadata:default_pfcwd_status": "disable",
  "sonic-device_metadata:bgp_asn": 65000,
  "sonic-device_metadata:frr_mgmt_framework_config": false,
  "sonic-device_metadata:synchronous_mode": "enable",
  "sonic-device_metadata:yang_config_validation": "disable",
  "sonic-device_metadata:suppress-fib-pending": "disabled",
  "sonic-device_metadata:timezone": "UTC",
  "sonic-device_metadata:nexthop_group": "disabled",
  "sonic-device_metadata:ring_thread_enabled": false
}

======================
Received: 
{
  "sonic-device_metadata:hostname": "DUT-ASW",
  "sonic-device_metadata:platform": "Stone-DX010",
  "sonic-device_metadata:default_pfcwd_status": "disable",
  "sonic-device_metadata:bgp_asn": 65000,
  "sonic-device_metadata:frr_mgmt_framework_config": false,
  "sonic-device_metadata:synchronous_mode": "enable",
  "sonic-device_metadata:yang_config_validation": "disable",
  "sonic-device_metadata:suppress-fib-pending": "disabled",
  "sonic-device_metadata:timezone": "UTC",
  "sonic-device_metadata:nexthop_group": "disabled",
  "sonic-device_metadata:ring_thread_enabled": false
}

Notice how the Received is missing sonic-device_metadata:default_bgp_status and sonic-device_metadata:docker_routing_config_mode.

@bradh352
Copy link
Author

bradh352 commented Mar 8, 2025

@michalvasko now that I've provided a test case to reproduce, at least please remove the status:invalid tag :)

@michalvasko
Copy link
Member

michalvasko commented Mar 10, 2025

Thanks for the example. So it is just as I thought, only a misunderstanding of the expected behavior. When the NODE_XPATH is changed to

#define NODE_XPATH "/sonic-device_metadata:sonic-device_metadata/DEVICE_METADATA/localhost"

it prints all the default values just in a different order. But the flag LYD_PRINT_WITHSIBLINGS is correctly documented and mentions printing only the following siblings.

@bradh352
Copy link
Author

I'm honestly not sure why the original author wrote the tests with that xpath, I agree it is odd : https://github.com/sonic-net/sonic-buildimage/blob/master/src/sonic-yang-models/tests/yang_model_tests/tests/device_metadata.json

looks like in libyang1 it would print all siblings with the LYP_WITHSIBLINGS flag, not just ones after that point. Didn't realize there was a behavior change.

@michalvasko
Copy link
Member

looks like in libyang1 it would print all siblings with the LYP_WITHSIBLINGS flag, not just ones after that point. Didn't realize there was a behavior change.

It is (unfortunately) not mentioned in the transition docs, but it seems it did change. The expectation is, though, that you either print all the siblings and use the first one (why would you use another?) or print only a specific subtree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is:bug Bug description. status:invalid Issue is not reproducible or the behavior is intended.
Projects
None yet
Development

No branches or pull requests

3 participants