SONiC QoS MAPs and PORT_QOS_MAP Yang #7375

ohu1 · 2021-04-20T00:15:21Z

Why I did it

Created SONiC Yang model for the following QOS MAPs and PORT QOS MAP:
DSCP_TO_TC_MAP
DOT1P_TO_TC_MAP
TC_TO_QUEUE_MAP
TC_TO_PRIORITY_GROUP_MAP
MAP_PFC_PRIORITY_TO_QUEUE
PORT_QOS_MAP

How I did it

Defined Yang models for QOS MAPs based on Guideline doc:
https://github.com/Azure/SONiC/blob/master/doc/mgmt/SONiC_YANG_Model_Guidelines.md
and
https://github.com/Azure/sonic-utilities/blob/master/doc/Command-Reference.md

How to verify it

sonic_yang_models package build
The infra code was modified to handle the nested list definition in YANG and translate it to a flat list in DB.

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

201811
201911
202006
202012

Description for the changelog

ghost · 2021-04-20T00:15:32Z

All CLA requirements met.

rathnasabapathyv · 2021-05-13T20:39:34Z

src/sonic-yang-models/yang-models/sonic-dscp-tc-map.yang

+                }
+
+                list DSCP_TO_TC_MAP { //this is list inside list for storing mapping between two fields
+                    key "dscp tc_num";


As discussed in the community meeting today (May-13), please get back to us whether you are going to change the QOS backend to support the yang model for DSCP_TO_TC_MAP_LIST. Like we discussed, nested-list (list-inside-list) can't be supported for the current QOS-backend DB contents expectation. More details below.

With current QOS backend code's DB expectation, we can't represent that in yang-format for the inner list (DSCP_TO_TC_MAP) using nested list (list-inside-list) with keys being dscp & tc_num. As shown below in Redis DB, dot1p & tc_num shouldn't go to key-portion, instead it should go to feild/value portion. Table below has db representation for clarity. Current QOS backend expectation.

To solve, either we should change the QOS backend to accept flat-table as like other tables instead of nested-list. Here the 2nd list can be separated out as an independent list with "map-name" (parent-key with leaf-ref) and DSCP as key with tc_num as mandatory leaf. Or the other approach is to introduce the sonic-extensions we proposed earlier (map-list & map-leaf) to take of writing these special yang cases to DB as expected by current QOS backend.

Same problem exists for DOT1P_TO_TC_MAP as well

Each DSCP is mapped to only 1 TC, so we can use only DSCP as key. If both fields are keys, that makes 24:3 and 24:4 a valid combination.

rathnasabapathyv · 2021-05-13T20:41:10Z

src/sonic-yang-models/yang-models/sonic-dot1p-tc-map.yang

+                }
+
+                list DOT1P_TO_TC_MAP { //this is list inside list for storing mapping between two fields
+                    key "dot1p tc_num";


The problem mentioned below for DSCP_TO_TC_MAP exists for DOT1P_TO_TC_MAP as well.

anshuv-mfst · 2021-06-03T17:50:07Z

Hi @praveen-li - please confirm QOS backend change approach and please share with the team.

praveen-li · 2021-06-17T17:46:06Z

src/sonic-yang-models/tests/files/sample_config_db.json

+
+        "DSCP_TO_TC_MAP": {
+	        "Dscp_to_tc_map1": { 
+		        "24": "3", 


This representation is different than other lists, I think in future we should change it as below, so that the translation code is not burdened by special cases: @lguohan @renukamanavalan for comment.

"DSCP_TO_TC_MAP": { "Dscp_to_tc_map1": { "24": { "tc_num": "3" } } }

what does the redis entry looks like?

It looks like exactly like line 693: "24": "3", in which "24" is the field name, "3" is the value.

zhangyanzhao · 2021-09-02T17:19:39Z

depends on another PR which @AshokDaparthi will add the link here.

zhangyanzhao · 2021-09-02T17:21:43Z

depends on sonic-net/sonic-utilities#1626

zhangyanzhao · 2021-09-02T21:51:34Z

sonic-net/sonic-utilities#1626 is merged

zhangyanzhao · 2021-09-09T17:31:07Z

depends on #8683

zhangyanzhao · 2021-09-09T17:55:37Z

depends on sonic-net/sonic-swss#1754 as well

zhangyanzhao · 2021-09-16T17:16:58Z

Need close the dependent PRs before moving forward.

stephenxs

PFC_PRIORITY_TO_PRIORITY_GROUP_MAP should also be defined here, right?

stephenxs · 2021-09-24T14:15:20Z

src/sonic-yang-models/yang-models/sonic-pfc-priority-queue-map.yang

+
+                    leaf pfc_priority {
+                        type string {
+                            pattern "[0-9]?";


pfc_priority should be in range [0-7]?

smaheshm · 2021-09-27T21:32:12Z

@ohu1 There are few unresolved comments, can you address them.

smaheshm · 2021-09-27T21:33:41Z

src/sonic-yang-models/setup.py

+                         './yang-models/sonic-tc-dot1p-map.yang',
+                         './yang-models/sonic-tc-priority-group-map.yang',
+                         './yang-models/sonic-tc-queue-map.yang',
+                         './yang-models/sonic-pfc-priority-queue-map.yang',


I don't see yang model for PORT_QOS_MAP (unless I missed it). Is there a separate PR for it.

@smaheshs - We have not done this because PORT_QOS_MAP is reference to maps. I will raise it once format changes are merged.

AshokDaparthi · 2021-09-29T21:56:29Z

@praveen-li - As per last discussion i remember, you are adding special handling in libyang for handling nested list. Can you confirm this is done or not?

zhangyanzhao · 2021-09-30T15:50:48Z

depends on Azure/sonic-swss#1754 as well

Azure/sonic-swss#1754 is merged

zhangyanzhao · 2021-09-30T17:11:49Z

@praveen-li - As per last discussion i remember, you are adding special handling in libyang for handling nested list. Can you confirm this is done or not?

@praveen-li would you please help to share your comments?

zhangyanzhao · 2021-09-30T17:48:53Z

@AshokDaparthi will follow-up based on the discussion in today's meeting.

zhangyanzhao · 2021-09-30T17:54:15Z

proceed with current implementation, nested list can be added later as long term solution.

neethajohn

Please update the PR description to reflect all the tables added as part of this PR. Also I do not see testcases added for these changes.
There is also a 'PFC_PRIORITY_TO_PRIORITY_GROUP_MAP' table used by some vendors which is missing here. Is there a way to add a constraint based on the asic type in yang?

src/sonic-yang-models/yang-models/sonic-dot1p-tc-map.yang

src/sonic-yang-models/yang-models/sonic-pfc-priority-queue-map.yang

src/sonic-yang-models/yang-models/sonic-tc-dot1p-map.yang

src/sonic-yang-models/yang-models/sonic-tc-dscp-map.yang

src/sonic-yang-models/yang-models/sonic-dot1p-tc-map.yang

src/sonic-yang-models/yang-models/sonic-port-qos-map.yang

AshokDaparthi · 2021-10-06T19:17:46Z

@neethajohn - This PR is work in Progress. sonic-ext for qos-map will be removed because it not supported in python validation code. I am looking at infra code changes needed in python validator for qos map(nested list). If we think it will take time and not much advantage we will modify yang as flat model like discussed in last call. based on yang i will write test cases.

AshokDaparthi · 2021-10-14T23:46:58Z

@neethajohn @praveen-li - Added test cases. Please review.

lgtm-com · 2021-10-15T01:19:06Z

This pull request introduces 2 alerts when merging f55687389964d9d2336c03f48af93996b2152e84 into c374705 - view on LGTM.com

new alerts:

2 for Unused local variable

add dot1p-tc, pfc-pg, tc-dot1p, tc-dscp, tc-pg, tc-queue maps

lgtm-com · 2021-10-18T19:38:53Z

This pull request introduces 2 alerts when merging fb9c26a2f3c09e43e5739316b9ebf6f4572c4a55 into a99d78d - view on LGTM.com

new alerts:

2 for Unused local variable

praveen-li

LGTM, we can merge it after test passes. Thx.

AshokDaparthi · 2021-10-19T16:00:50Z

@neethajohn , @smaheshm - Can you re-approve and merge.

AshokDaparthi · 2021-10-19T19:19:31Z

@smaheshm - Can you merge this

This reverts commit 459d3d1.

This reverts commit 9c172a5.

…ernal branch Related work items: sonic-net#525, sonic-net#534, sonic-net#909, sonic-net#927, sonic-net#930, sonic-net#934, sonic-net#935, sonic-net#937, sonic-net#938, sonic-net#939, sonic-net#940, sonic-net#943, sonic-net#944, sonic-net#945, sonic-net#946, sonic-net#948, sonic-net#1702, sonic-net#1923, sonic-net#1936, sonic-net#1942, sonic-net#2315, sonic-net#7281, sonic-net#7375, sonic-net#8659, sonic-net#8799, sonic-net#8868, sonic-net#8875, sonic-net#8935, sonic-net#8942, sonic-net#8956, sonic-net#8961, sonic-net#8966, sonic-net#8969, sonic-net#8971, sonic-net#8978, sonic-net#8991, sonic-net#8996, sonic-net#9001, sonic-net#9008, #3146150

#### What I did Fixes #2034 Which lists where handled before in ConfigDb to YANG conversion? - Dictionary of key to Object e.g. ``` "TACPLUS_SERVER": { "1.1.1.1": { <= Key: Object (i.e. {...}) "priority": "1", "tcp_port": "49" }, "1.1.1.2": { "priority": "1", "tcp_port": "49" }, "1.1.1.3": { "priority": "1", "tcp_port": "49" } } ``` - List of values ``` "VLAN": { "Vlan1000": { "vlanid": "1000", "dhcp_servers": [ <= Values "192.0.0.1", "192.0.0.2", "192.0.0.3", "192.0.0.4" ] } } ``` - But there is no handling of Dictionary of Key to Value ``` "DOT1P_TO_TC_MAP": { "Dot1p_to_tc_map1": { <= Key: Value "1": "1", "2": "2", "3": "1", "4": "2" } }, ``` Refer to sonic-net/sonic-buildimage#7375 for more info about how Type 1 list ConfigDb is getting converted to SonicYang. After checking how type1 is handled during ConfigDb to SonicYang conversion. Check the unit-tests here for converting ConfigDb Path to SonicYang xpath. Also added CABLE_LENGTH to ADD_RACK and REMOVE_RACK tests. #### How I did it When handling a list, check if it is of type1. If that's the case call type1 list handling. #### How to verify it unit-test #### Previous command output (if the output of a command-line utility has changed) #### New command output (if the output of a command-line utility has changed)

ohu1 requested a review from lguohan as a code owner April 20, 2021 00:15

lguohan added the YANG YANG model related changes label Apr 20, 2021

rathnasabapathyv reviewed May 13, 2021

View reviewed changes

praveen-li reviewed Jun 17, 2021

View reviewed changes

zhangyanzhao mentioned this pull request Sep 9, 2021

Modified test cases by QOS DB reference format remove sonic-net/sonic-mgmt#3824

Merged

stephenxs suggested changes Sep 24, 2021

View reviewed changes

smaheshm reviewed Sep 27, 2021

View reviewed changes

AshokDaparthi force-pushed the qos-map branch 3 times, most recently from ee254d7 to 638e64f Compare September 29, 2021 21:27

AshokDaparthi force-pushed the qos-map branch from 638e64f to 189aeb5 Compare September 30, 2021 17:03

neethajohn suggested changes Oct 6, 2021

View reviewed changes

AshokDaparthi dismissed praveen-li’s stale review via f556873 October 14, 2021 23:44

ohu1 and others added 4 commits October 18, 2021 10:58

SONiC QoS DSCP-TO-TC-MAP Yang

5e300bd

add dot1p-tc, pfc-pg, tc-dot1p, tc-dscp, tc-pg, tc-queue maps

Modified infra code to handle qos maps yang

cdf19b0

Addressed review comments

98db480

Added qos maps yang test cases

c132df1

ohu1 force-pushed the qos-map branch from f556873 to fb9c26a Compare October 18, 2021 18:06

Alignment

703f978

ohu1 force-pushed the qos-map branch from fb9c26a to 703f978 Compare October 18, 2021 22:14

praveen-li approved these changes Oct 19, 2021

View reviewed changes

neethajohn approved these changes Oct 19, 2021

View reviewed changes

neethajohn merged commit 459d3d1 into sonic-net:master Oct 19, 2021

smaheshm mentioned this pull request Oct 28, 2021

[CBF] Added configuration templates to generate configs for CBF #8689

Merged

dgsudharsan added a commit to dgsudharsan/sonic-buildimage that referenced this pull request Oct 29, 2021

Revert "SONiC QoS MAPs and PORT_QOS_MAP Yang (sonic-net#7375)"

9c172a5

This reverts commit 459d3d1.

dgsudharsan added a commit to dgsudharsan/sonic-buildimage that referenced this pull request Oct 29, 2021

Revert "Revert "SONiC QoS MAPs and PORT_QOS_MAP Yang (sonic-net#7375)""

fc6a975

This reverts commit 9c172a5.

ghooo mentioned this pull request May 18, 2022

[GCU] Handling type1 lists sonic-net/sonic-utilities#2171

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SONiC QoS MAPs and PORT_QOS_MAP Yang #7375

SONiC QoS MAPs and PORT_QOS_MAP Yang #7375

ohu1 commented Apr 20, 2021 •

edited

Loading

ghost commented Apr 20, 2021 •

edited by ghost

Loading

rathnasabapathyv May 13, 2021 •

edited

Loading

praveen-li Jun 17, 2021

rathnasabapathyv May 13, 2021

anshuv-mfst commented Jun 3, 2021

praveen-li Jun 17, 2021 •

edited

Loading

lguohan Jun 17, 2021

ohu1 Jun 17, 2021

zhangyanzhao commented Sep 2, 2021

zhangyanzhao commented Sep 2, 2021

zhangyanzhao commented Sep 2, 2021

zhangyanzhao commented Sep 9, 2021

zhangyanzhao commented Sep 9, 2021

zhangyanzhao commented Sep 16, 2021

stephenxs left a comment

stephenxs Sep 24, 2021

smaheshm commented Sep 27, 2021

smaheshm Sep 27, 2021

AshokDaparthi Sep 27, 2021

AshokDaparthi commented Sep 29, 2021

zhangyanzhao commented Sep 30, 2021

zhangyanzhao commented Sep 30, 2021

zhangyanzhao commented Sep 30, 2021

zhangyanzhao commented Sep 30, 2021

neethajohn left a comment

AshokDaparthi commented Oct 6, 2021

AshokDaparthi commented Oct 14, 2021

lgtm-com bot commented Oct 15, 2021

lgtm-com bot commented Oct 18, 2021

praveen-li left a comment

AshokDaparthi commented Oct 19, 2021

AshokDaparthi commented Oct 19, 2021

SONiC QoS MAPs and PORT_QOS_MAP Yang #7375

SONiC QoS MAPs and PORT_QOS_MAP Yang #7375

Conversation

ohu1 commented Apr 20, 2021 • edited Loading

Why I did it

How I did it

How to verify it

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

Description for the changelog

ghost commented Apr 20, 2021 • edited by ghost Loading

rathnasabapathyv May 13, 2021 • edited Loading

Choose a reason for hiding this comment

praveen-li Jun 17, 2021

Choose a reason for hiding this comment

rathnasabapathyv May 13, 2021

Choose a reason for hiding this comment

anshuv-mfst commented Jun 3, 2021

praveen-li Jun 17, 2021 • edited Loading

Choose a reason for hiding this comment

lguohan Jun 17, 2021

Choose a reason for hiding this comment

ohu1 Jun 17, 2021

Choose a reason for hiding this comment

zhangyanzhao commented Sep 2, 2021

zhangyanzhao commented Sep 2, 2021

zhangyanzhao commented Sep 2, 2021

zhangyanzhao commented Sep 9, 2021

zhangyanzhao commented Sep 9, 2021

zhangyanzhao commented Sep 16, 2021

stephenxs left a comment

Choose a reason for hiding this comment

stephenxs Sep 24, 2021

Choose a reason for hiding this comment

smaheshm commented Sep 27, 2021

smaheshm Sep 27, 2021

Choose a reason for hiding this comment

AshokDaparthi Sep 27, 2021

Choose a reason for hiding this comment

AshokDaparthi commented Sep 29, 2021

zhangyanzhao commented Sep 30, 2021

zhangyanzhao commented Sep 30, 2021

zhangyanzhao commented Sep 30, 2021

zhangyanzhao commented Sep 30, 2021

neethajohn left a comment

Choose a reason for hiding this comment

AshokDaparthi commented Oct 6, 2021

AshokDaparthi commented Oct 14, 2021

lgtm-com bot commented Oct 15, 2021

lgtm-com bot commented Oct 18, 2021

praveen-li left a comment

Choose a reason for hiding this comment

AshokDaparthi commented Oct 19, 2021

AshokDaparthi commented Oct 19, 2021

ohu1 commented Apr 20, 2021 •

edited

Loading

ghost commented Apr 20, 2021 •

edited by ghost

Loading

rathnasabapathyv May 13, 2021 •

edited

Loading

praveen-li Jun 17, 2021 •

edited

Loading