sonic-cfggen should return boolean values as lowercase true and false

[bhouse-nexthop]

Why I did it

YANG mandates that boolean values are true and false, having the first letter be capital as is represented by Python would cause yang validation failures inside of libyang. Because of this, it appears a different yang type in sonic-types.yang has a secondary boolean_types that is used that allows more formats.

This can be seen, for instance, when setting:

"MGMT_VRF_CONFIG": {
  "vrf_global": {
    "mgmtVrfEnabled": "true"
  }
}

Then running config save -y. The written value is then "True", if then just running config replace /etc/sonic/config_db.json it would result in an error of:

Invalid value "True" in "mgmtVrfEnabled" element.

Work item tracking

How I did it

Capture native yang boolean values by looking up the yang schema when writing the config_db.json via sonic-cfggen and ensure they are written as lowercase.

How to verify it

See above for reproduction method

Which release branch to backport (provide reason below if selected)

• 201811
• 201911
• 202006
• 202012
• 202106
• 202111
• 202205
• 202211
• 202305
• 202411

Tested branch (Please provide the tested image version)

master as of 20250502

Description for the changelog

sonic-cfggen should return boolean values as lowercase true and false

Link to config_db schema for YANG module changes

N/A

A picture of a cute animal (not mandatory but encouraged)

Fixes #22527
Signed-off-by: Brad House bhouse@nexthop.ai

[mssonicbld]

/azp run Azure.sonic-buildimage

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[mssonicbld]

/azp run Azure.sonic-buildimage

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[bradh352]

build failures are caused by bad PR #22432

[mssonicbld]

/azp run Azure.sonic-buildimage

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[mssonicbld]

/azp run Azure.sonic-buildimage

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[bhouse-nexthop]

Thanks @lguohan I made a couple minor modifications and replied to the rest.

[mssonicbld]

/azp run Azure.sonic-buildimage

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[bradh352]

these spurious test failures in completely unrelated code paths are getting bad. I just rebased to force a rebuild.

[TafkaMax]

these spurious test failures in completely unrelated code paths are getting bad. I just rebased to force a rebuild.

Also probably needs a rebuild again?

[mssonicbld]

/azp run Azure.sonic-buildimage

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[bradh352]

these spurious test failures in completely unrelated code paths are getting bad. I just rebased to force a rebuild.

Also probably needs a rebuild again?

$!*% yep

[TafkaMax]

these spurious test failures in completely unrelated code paths are getting bad. I just rebased to force a rebuild.

Also probably needs a rebuild again?

$!*% yep

Second time it worked :-)

[bradh352]

@lguohan any chance you can look at this and let me know if you have similar concerns to @qiluo-msft ? It at most loads the yang models once per sonic-cfggen execution to do proper field type validation to prevent an invalid configdb yang schema from being written out.

[lguohan]

let me talk to qi when he is back tomorrow. do you have some measurable data to show that this won't add much overhead?

[lguohan]

btw, the pr description/title probably should update saying that introducing yang model validation in sonic-cfggen?

[bhouse-nexthop]

btw, the pr description/title probably should update saying that introducing yang model validation in sonic-cfggen?

The description does say it under the "How I did it" section.

[bhouse-nexthop]

let me talk to qi when he is back tomorrow. do you have some measurable data to show that this won't add much overhead?

Considering this is only loading the yang models and not actually performing the validation step, I'd assume the hit would be negligible. That said, I don't have hard numbers. Let me see if I can collect some before / after numbers for you on real hardware.

[qiluo-msft]

I am trying to repro the original issue mentioned in PR description, but failed to repro. I could not observe 'The written value is then "True"'

admin@sonic:~$ sonic-db-cli CONFIG_DB hset "MGMT_VRF_CONFIG|vrf_global" "mgmtVrfEnabled" true
1
admin@sonic:~$ sonic-db-cli CONFIG_DB hgetall "MGMT_VRF_CONFIG|vrf_global"
{'mgmtVrfEnabled': 'true'}
admin@sonic:~$ sudo config save -y
Running command: /usr/local/bin/sonic-cfggen -d --print-data > /etc/sonic/config_db.json
admin@sonic:~$ grep -C 2 "mgmtVrfEnabled" /etc/sonic/config_db.json
    "MGMT_VRF_CONFIG": {
        "vrf_global": {
            "mgmtVrfEnabled": "true"
        }
    },

[bhouse-nexthop]

I am trying to repro the original issue mentioned in PR description, but failed to repro. I could not observe 'The written value is then "True"'

admin@sonic:~$ sonic-db-cli CONFIG_DB hset "MGMT_VRF_CONFIG|vrf_global" "mgmtVrfEnabled" true
1
admin@sonic:~$ sonic-db-cli CONFIG_DB hgetall "MGMT_VRF_CONFIG|vrf_global"
{'mgmtVrfEnabled': 'true'}
admin@sonic:~$ sudo config save -y
Running command: /usr/local/bin/sonic-cfggen -d --print-data > /etc/sonic/config_db.json
admin@sonic:~$ grep -C 2 "mgmtVrfEnabled" /etc/sonic/config_db.json
    "MGMT_VRF_CONFIG": {
        "vrf_global": {
            "mgmtVrfEnabled": "true"
        }
    },

I modified the config_db.json and didn't play with the sonic-db-cli. I haven't looked at how sonic-db-cli writes that, but I'm guessing its writing the 'true' as a string. A string would be preserved in the original case. It actually has to be in Redis as a boolean value for this issue to be observed.

[bhouse-nexthop]

Performance numbers below.

Platform: x86_64-mlnx_msn2700-r0
HwSKU: ACS-MSN2700

Pretty slow cpu:

root@sw1:~# cat /proc/cpuinfo | grep "model name" | head -n1
model name	: Intel(R) Celeron(R) CPU 1047UE @ 1.40GHz

with patch:

root@sw1:~# time config save -y
Running command: /usr/local/bin/sonic-cfggen -d --print-data > /etc/sonic/config_db.json

real	0m1.754s
user	0m1.374s
sys	0m0.328s
root@sw1:~# time config save -y
Running command: /usr/local/bin/sonic-cfggen -d --print-data > /etc/sonic/config_db.json

real	0m1.605s
user	0m1.341s
sys	0m0.237s
root@sw1:~# time config save -y
Running command: /usr/local/bin/sonic-cfggen -d --print-data > /etc/sonic/config_db.json

real	0m1.616s
user	0m1.362s
sys	0m0.215s

without patch:

root@sw1:~# time config save -y
Running command: /usr/local/bin/sonic-cfggen -d --print-data > /etc/sonic/config_db.json

real	0m1.239s
user	0m1.044s
sys	0m0.152s
root@sw1:~# time config save -y
Running command: /usr/local/bin/sonic-cfggen -d --print-data > /etc/sonic/config_db.json

real	0m1.204s
user	0m1.004s
sys	0m0.170s
root@sw1:~# time config save -y
Running command: /usr/local/bin/sonic-cfggen -d --print-data > /etc/sonic/config_db.json

real	0m1.197s
user	0m1.014s
sys	0m0.164s

[mssonicbld]

/azp run Azure.sonic-buildimage

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[qiluo-msft]

I am trying to repro the original issue mentioned in PR description, but failed to repro. I could not observe 'The written value is then "True"'

admin@sonic:~$ sonic-db-cli CONFIG_DB hset "MGMT_VRF_CONFIG|vrf_global" "mgmtVrfEnabled" true
1
admin@sonic:~$ sonic-db-cli CONFIG_DB hgetall "MGMT_VRF_CONFIG|vrf_global"
{'mgmtVrfEnabled': 'true'}
admin@sonic:~$ sudo config save -y
Running command: /usr/local/bin/sonic-cfggen -d --print-data > /etc/sonic/config_db.json
admin@sonic:~$ grep -C 2 "mgmtVrfEnabled" /etc/sonic/config_db.json
    "MGMT_VRF_CONFIG": {
        "vrf_global": {
            "mgmtVrfEnabled": "true"
        }
    },

I modified the config_db.json and didn't play with the sonic-db-cli. I haven't looked at how sonic-db-cli writes that, but I'm guessing its writing the 'true' as a string. A string would be preserved in the original case. It actually has to be in Redis as a boolean value for this issue to be observed.

Could you provide your repro steps for the original issue? I do not understand "It actually has to be in Redis as a boolean value" since redis only support string as value. If you modify config_db.json and trigger a bug, I prefer to treat it as invalid input bug and just fix the input validation logic and fail sonic-cfggen immediately, so yang dependency is not needed.

[bhouse-nexthop]

steps to reproduce are:

1. Edit /etc/sonic/config_db.json and insert this valid section:

"MGMT_VRF_CONFIG": {
  "vrf_global": {
    "mgmtVrfEnabled": "true"
  }
}

2. Run config replace /etc/sonic/config_db.json which applies only that diff to redis successfully
3. Run config save -y to rewrite the redis database to /etc/sonic/config_db.json
4. Run config replace /etc/sonic/config_db.json, this will FAIL because step Update README.md #3 wrote "True" for mgmtVrfEnabled

The trigger to this behavior is the fact that somewhere the Pythonic True / False are being written when serializing from Redis.