sonic-net / sonic-buildimage

Scripts which perform an installable binary image build for SONiC
Other
735 stars 1.41k forks source link

[yang] DPB fails when BUFFER_PORT_EGRESS_PROFILE is configured #9801

Open alexrallen opened 2 years ago

alexrallen commented 2 years ago

When we configure the following buffer profiles....

    "BUFFER_PORT_EGRESS_PROFILE_LIST": {
        "Ethernet0": {
            "profile_list": "egress_lossless_profile,egress_lossy_profile"
        }
    }

Dynamic port breakout fails with...

libyang[0]: Duplicated instance of "profile_list" leaf-list ("e"). (path: /sonic-buffer-port-egress-profile-list:sonic-buffer-port-egress-profile-list/BUFFER_PORT_EGRESS_PROFILE_LIST/BUFFER_PORT_EGRESS_PROFILE_LIST_LIST[port='Ethernet0']/profile_list[.='e'])

sonic_yang(3):Data Loading Failed:Duplicated instance of "profile_list" leaf-list ("e").
Data Loading Failed
Duplicated instance of "profile_list" leaf-list ("e").

ConfigMgmt Class creation failed

Failed to break out Port. Error: Failed to load the config. Error: ConfigMgmtDPB Class creation failed

This is likely due to the fact that the Yang model is configured with the following expectation for profile_list (a JSON list) which does not match what is the appropriate configuration (a comma separated string).

"sonic-buffer-port-egress-profile-list:sonic-buffer-port-egress-profile-list": {
    "sonic-buffer-port-egress-profile-list:BUFFER_PORT_EGRESS_PROFILE_LIST": {
        "BUFFER_PORT_EGRESS_PROFILE_LIST_LIST": [
            {
                "port": "Ethernet4",
                "profile_list": ["lossless_buffer_profile", "lossless_buffer_profile2"]
            }
        ]    
    }
}

This needs to be reconciled in some manner.

zhangyanzhao commented 2 years ago

Need BUFFER_PORT_EGRESS_PROFILE YANG model owner to take a look. Viswanath will follow up.

Junchao-Mellanox commented 2 years ago

Hi @zhangyanzhao, I suppose this issue is related to function _findYangTypedValue: https://github.com/Azure/sonic-buildimage/blob/b621dafff77642db541553f5a2f8ec85e70ea695/src/sonic-yang-mgmt/sonic_yang_ext.py#L410

The issue is that when a CONFIG DB field is defined as "leaf-list", it is usually a comma separated string in CONFIG DB. In this case, the value is "egress_lossless_profile,egress_lossy_profile". Function _findYangTypedValue will iterate this string and put it to vValue. Hence, the vValue is "e,g,r,e,s,s...". Apparently, "e" is not a valid value of field "profile_list". I suppose a possible fix is to fix function _findYangTypedValue line 410 to something like:

        if leafDict[key]['__isleafList']:
            vValue = list()
            if isinstance(value, str):
                value = (x.strip() for x in value.split(','))
            for v in value:
                vValue.append(_yangConvert(v))

I did a simple test based on the fix, it works, but I suppose we still need sonic-yang-mgmt owner to confirm that this is a valid fix.

dgsudharsan commented 2 years ago

@praveen-li @zhangyanzhao @zhenggen-xu Can you please prioritize this?

praveen-li commented 2 years ago

Not sure, How the tests for PR https://github.com/Azure/sonic-buildimage/pull/7838 passed in this case ?

@AmitKaushik7, @dgsudharsan @zhangyanzhao

Seems by using a HACK in test config. I guess, SOT for config is important, because either config used in test or used in this issue is wrong.

    "BUFFER_PORT_EGRESS_PROFILE_LIST": {
        "Ethernet9": {
            "profile_list": ["egress_lossless_profile", "egress_lossy_profile"] <<<< List is not correct config?
        }
    },

Ideal fix maybe to convert String to List in Back-end i.e. in orchagent code. OR have a must condition or custom-validator here.

zhangyanzhao commented 2 years ago

@bandaru-viswanath will work with @dgsudharsan and @praveen-li on this issue.

liat-grozovik commented 2 years ago

@bandaru-viswanath any update? as this issue for 202111 can you please provide ETA?

bandaru-viswanath commented 2 years ago

@bandaru-viswanath any update? as this issue for 202111 can you please provide ETA?

@liat-grozovik We checked and asked the concerned person to work on this. Will get back to you on the ETA.

Junchao-Mellanox commented 2 years ago

Hi @liat-grozovik , I have a proposal for this issue, will include you to the loop.

qiluo-msft commented 2 years ago

@alexrallen Could you help me understand the problem statement?

what is the appropriate configuration (a comma separated string)

I checked code in src/sonic-yang-models/tests/files/sample_config_db.json, and the appropriate configuration should be a list of strings.

alexrallen commented 2 years ago

This is what is used in the test for this Yang model (this is working)

"sonic-buffer-port-egress-profile-list:sonic-buffer-port-egress-profile-list": {
    "sonic-buffer-port-egress-profile-list:BUFFER_PORT_EGRESS_PROFILE_LIST": {
        "BUFFER_PORT_EGRESS_PROFILE_LIST_LIST": [
            {
                "port": "Ethernet4",
                "profile_list": ["lossless_buffer_profile", "lossless_buffer_profile2"]
            }
        ]    
    }
}

This is what we are giving in CONFIG_DB

    "BUFFER_PORT_EGRESS_PROFILE_LIST": {
        "Ethernet0": {
            "profile_list": "egress_lossless_profile,egress_lossy_profile"
        }
    }

You can see the former is a JSON list and the latter is a comma separated list. This is a discrepancy.

AmitKaushik7 commented 2 years ago

As alexrallen pointed that BUFFER_PORT_EGRESS_PROFILE_LIST and BUFFER_PORT_INGRESS_PROFILE_LIST yang files are incorrectly having the profile_list as Lists ("profile_list": ["lossless_buffer_profile", "lossless_buffer_profile2"]). Rather these leaves should have been a comma separated string ("profile_list": "lossless_buffer_profile, lossless_buffer_profile2".).