Skip to content
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

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

Open
ljyfree opened this issue Mar 29, 2022 · 10 comments
Open
Labels
Triaged this issue has been triaged

Comments

@ljyfree
Copy link

ljyfree commented Mar 29, 2022

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 sonic-net/sonic-mgmt-common#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
Copy link
Contributor

maheshwari-mayank commented Mar 29, 2022

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 (sonic-net/sonic-mgmt-common#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 (sonic-net/sonic-mgmt-common#51) to be merged.

@zhangyanzhao zhangyanzhao added the YANG YANG model related changes label Mar 30, 2022
@zhangyanzhao
Copy link
Collaborator

Venkat will help to review this issue. Thanks.

@venkatmahalingam
Copy link
Collaborator

@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 sonic-net/sonic-mgmt-common#51 is merged, you can migrate to newer model.

@ljyfree
Copy link
Author

ljyfree commented Apr 1, 2022

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

@maheshwari-mayank
Copy link
Contributor

@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
Copy link
Contributor

@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.

@maheshwari-mayank maheshwari-mayank removed their assignment Apr 19, 2022
@zhangyanzhao zhangyanzhao added the Triaged this issue has been triaged label Apr 28, 2022
@zhangyanzhao
Copy link
Collaborator

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

@zhangyanzhao
Copy link
Collaborator

#10291 this PR should fixed the issue. @ljyfree can you please verify and close this issue if it works? Thanks.

@ljyfree
Copy link
Author

ljyfree commented Sep 23, 2022

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
Copy link
Collaborator

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

@zhangyanzhao zhangyanzhao removed the YANG YANG model related changes label Oct 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Triaged this issue has been triaged
Projects
None yet
Development

No branches or pull requests

5 participants