sonic-net / sonic-buildimage

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

[YANG]sonic-dot1p-tc-map.yang would cause failed deployment via mgmt-framework #10386

Open ljyfree opened 2 years ago

ljyfree commented 2 years ago

Description

There are nested LISTs in ./yang-models/sonic-dot1p-tc-map.yang as follows:

container sonic-dot1p-tc-map {  

        container DOT1P_TO_TC_MAP {

            description "DOT1P_TO_TC_MAP part of config_db.json";

            list DOT1P_TO_TC_MAP_LIST {

                key "name";

                leaf name {
                    type string {
                        pattern '[a-zA-Z0-9]{1}([-a-zA-Z0-9_]{0,31})';
                        length 1..32 {
                            error-message "Invalid length for map name.";
                            error-app-tag map-name-invalid-length;
                        }
                    }
                }

                list DOT1P_TO_TC_MAP { //this is list inside list for storing mapping between two fields

                    key "dot1p";

                    leaf dot1p {
                        type string {
                            pattern "[0-7]?" {
                                error-message "Invalid DOT1P";
                                error-app-tag dot1p-invalid;
                            }
                        }
                    }

                    leaf tc {
                        type string {
                            pattern "[0-7]?"{
                                error-message "Invalid Traffic Class";
                                error-app-tag tc-invalid;
                            }
                        }
                    }
                }
            }
        }
    }

The target configuration into configDB as follows:

        "DOT1P_TO_TC_MAP": {
           "Dot1p_to_tc_map1": { 
              "1": "2",
              "3": "4"
            }

It means dot1p-1 maps to tc-2 and dot1p-3 maps to tc-4. However,mgmt-framework's CVL function would check if field(here is "1" and "3") exist in the schema sourced from yang. It would definitely failed.

Same issue for dscp-to-tc-map.

I found similar description in SONiC_YANG_Model_Guidelines , which use "map-list ". https://github.com/Azure/SONiC/blob/master/doc/mgmt/SONiC_YANG_Model_Guidelines.md#11-mapping-tables-in-redis-are-defined-using-nested-list-use-sonic-extmap-list-true-to-indicate-that-the-list-is-used-for-mapping-table-the-outer-list-is-used-for-multiple-instances-of-mapping-the-inner-list-is-used-for-mapping-entries-for-each-outer-list-instance

But "map-list" was removed in https://github.com/Azure/sonic-mgmt-common/pull/51 .

@ohu1 @maheshwari-mayank

Steps to reproduce the issue:

1.compile mgmt-framework with ./yang-models/sonic-dot1p-tc-map.yang 2.Modify xlate_to_db.go to support nested list 3.try to deploy dot1p-to-tc

Describe the results you received:

CVL failed

Describe the results you expected:

dot1p-to-tc configuration should be

Output of show version:

(paste your output here)

Output of show techsupport:

(paste your output here or download and attach the file here )

Additional information you deem important (e.g. issue happens only occasionally):

maheshwari-mayank commented 2 years ago

When sonic-dot1p-tc-map.yang was uploaded to sonic-yang-models, the community didn't agreed to use CVL extension "map-list". In review, it was told to remove this extension from yang. So this PR (https://github.com/Azure/sonic-mgmt-common/pull/51) was raised which removes the support of CVL extension "map-list" support from CVL. This PR is still not merged.

To fix this issue, either sonic yang should be allowed with "map-list" extension, or PR (https://github.com/Azure/sonic-mgmt-common/pull/51) to be merged.

zhangyanzhao commented 2 years ago

Venkat will help to review this issue. Thanks.

venkatmahalingam commented 2 years ago

@ljyfree SONiC YANG models present https://github.com/Azure/sonic-mgmt-common/tree/master/models/yang/sonic and https://github.com/Azure/sonic-buildimage/tree/master/src/sonic-yang-models/yang-models are not same, we still honor map-list in mgmt-common repo, please continue to use it, if https://github.com/Azure/sonic-mgmt-common/pull/51 is merged, you can migrate to newer model.

ljyfree commented 2 years ago

@maheshwari-mayank Have you ever compiled mgmt-common with mgmt-framework and validate if deploy dot1p-to-tc work via RESTConf works?

maheshwari-mayank commented 2 years ago

@ljyfree
Sorry I didn't depolyed dot1p-to-tc yang as I am not owner of this yang. @AshokDaparthi @ohu1 Please kindly check this yang.

AshokDaparthi commented 2 years ago

@ljyfree - We are not deployed dot1p-to-tc or any qos maps with sonic-yang with restconf. We defined YANG models for qos maps and we did not sing up for testing YANG with restconf etc with sonic yang's. Defining YANG models does not mean it will work from mgmt-common for RESTconf unless YANG and DB formats is same.

Here YANG model and DB formats are different. In YANG, key is "dot1p"; and values is leaf "tc". But in DB, everything is field and value pairs. May be mgmt-common needs special handling to convert YANG to DB format and vice versa.
@maheshwari-mayank If i am not wrong, map-list yang extension will not solve issue here this will be useful for validation in CVL only.

zhangyanzhao commented 2 years ago

@ljyfree can you please check if this issue still there? or any other info do you need? Thanks.

zhangyanzhao commented 2 years ago

https://github.com/sonic-net/sonic-buildimage/pull/10291 this PR should fixed the issue. @ljyfree can you please verify and close this issue if it works? Thanks.

ljyfree commented 2 years ago

Hi @zhangyanzhao I don't think they are the same issue, since #10291 fix the issue on leaf-list 's seperation while here is the map-list issue.

zhangyanzhao commented 1 year ago

CVL failure is not fixed yet, need BRCM to help. But this is not YANG issue. @adyeung