[TPID CONFIG]TPID attribute Yang model and default TPID for Minigraph to configDB Changes #7630

gechiang · 2021-05-17T17:14:11Z

Why I did it

This PR is part of the TPID CONFIG SONiC feature set to support port/LAG TPID configuration.
TPID HLD PR: (sonic-net/SONiC#681)

There are total of 3 PRs across different submodules to achieve this feature support.

SONiC-Utilities ([TPID CONFIG] Added TPID configuration CLI support sonic-utilities#1618) - To handle TPID CLI related changes.
SONiC-Swss ([sonic-swss:TPID CONFIG]Added Orchagent TPID config support for Port/LAG sonic-swss#1747) - To handle Orchagent changes triggered from TPID configuration changes to interact with SYNCd SAI.
and this PR which handles the new port/LAG TPID attribute on Yang model and Minigraph to configDB changes.

Please refer to the TPID HLD listed above for more details on the TPID configuration feature support.
Here is a quick summary for this PR changes:
By default TPID for port/LAG is 0x8100. With HW SKU that has capability to handle TPID configuration user is allowed to configure the port/LAG to non default TPID values such as (0x9100, 0x9200, 0x88A8).
This PR is to define this new TPID attribute for Port and LAG in the Yang model as well as to ensure all the ports/LAGs in the config DB generated from Minigraph should have a default TPID value of 0x8100 .

How to verify it

Test code were added for the Yang model to test the new TPID attribute in PORT and Portchannel.
Test code were also updated in test config gen, minigraph as well as multi-asic config gen to ensure the expected default TPID is in place.
Complete feature testing of TPID functionality were also performed on DUTs that has SAI support for TPID attribute to ensure the feature is working properly.

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

201811
201911
202006
202012

Description for the changelog

TPID port/LAG attribute Yang model and default TPID for Minigraph to configDB Changes

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

… to configDB Changes

praveen-li · 2021-05-27T17:20:53Z

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

                "admin_status": "up"
            },
            "Ethernet14": {
                "alias": "Eth4/3",
                "lanes": "79",
                "description": "",
                "speed": "11100",
+                "tpid": "0x8100",


We do not need to add it in all ports, just add it in 1 or 2 ports.

Going forward after this PR is approved and merged, the config DB when generated from the Minigraph will always have TPID 0x8100. It is similar to the MTU handling where there is a default value set in config DB.
I like to keep this change to set TPID attribute for all the interfaces (port and LAG). this way it is more in sync when user inspect config DB.

gechiang · 2021-05-27T17:24:50Z

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

@@ -96,6 +96,14 @@ module sonic-port{
 						pattern "on|off";
 					}
 				}
+
+				leaf tpid {


Add description field to describe what this attribute is about

move to common so both port and LAG can share to avoid duplication

No length required. remove it.

we will not follow the open config model for this... for simplicity handling in config DB.

In the future when stateDB validation method is agreed, we can come back to this and enhance it to check capability as part of the yang model validation for TPID attribute

praveen-li · 2021-05-27T17:21:32Z

src/sonic-yang-models/yang-models/sonic-portchannel.yang

+
+                leaf tpid {
+                    type string {
+                        length 6;


See, if we can move type to stypes.yang, and use that typedef here.

Agreed. Will have this changed in next commit.

praveen-li · 2021-05-27T17:29:45Z

src/sonic-yang-models/tests/yang_model_tests/tests_config/portchannel.json

@@ -10,6 +10,7 @@
                        "lanes": "65",
                        "mtu": 9000,
                        "name": "Ethernet0",
+                        "tpid": "0x8100",


We can avoid updating old tests, since it is not mandatory field. Just add it in new tests.

I am actually leveraging this same test config file for the new attribute TPID testing. Are you saying that I should generate a new test file just for TPID purpose? since TPID is a new attribute for both port and portchannel, I thought of just expanding on existing test case instead of generating a new file for just one TPID test...
There are no other tests needed other than ensure that the model can catch invalid TPID values.

praveen-li · 2021-05-27T17:30:47Z

src/sonic-yang-models/tests/yang_model_tests/tests_config/port.json

-    }
+    },
+
+    "PORT_INVALID_TPID_TEST": {


We do not need extra test, once type is moved in stypes.

Not sure I understood your comment. moving the tpid type to a common typedef allows sharing of the same definition but testing is still required isn't it? LThis was added so that we can see that the Yang model correctly catches any attempt of using a wrong TPID value that is not specified in the model. Please clarify ...
Thanks!

praveen-li · 2021-05-27T17:31:31Z

src/sonic-yang-models/tests/yang_model_tests/tests_config/port.json

@@ -12,6 +12,7 @@
                        "mtu": 9000,
                        "pfc_asym": "off",
                        "name": "Ethernet8",
+                        "tpid": "0x8100",


No need to update old tests.

TPID will be present in all config DB after thei feature is checked in so making sure it is consistently present in all interface configs.

dgsudharsan · 2021-05-27T18:03:16Z

src/sonic-config-engine/tests/test_multinpu_cfggen.py

-            {'PortChannel0002': {'admin_status': 'up', 'min_links': '2', 'members': ['Ethernet0', 'Ethernet4'], 'mtu': '9100'},
-            'PortChannel4001': {'admin_status': 'up', 'min_links': '2', 'members': ['Ethernet-BP0', 'Ethernet-BP4'], 'mtu': '9100'},
-            'PortChannel4002': {'admin_status': 'up', 'min_links': '2', 'members': ['Ethernet-BP8', 'Ethernet-BP12'], 'mtu': '9100'}})
+                {'PortChannel0002': {'admin_status': 'up', 'min_links': '2', 'members': ['Ethernet0', 'Ethernet4'], 'mtu': '9100', 'tpid': '0x8100'},


Can you please correct the alignment in this file?

…_type typedef for both LAG and PORT tpid attribute, added description in port and portchannel leaf for the tpid attribute

…finition missing brackets during earlier merge misshandled

… to configDB Changes (sonic-net#7630) * [TPID CONFIG]TPID attribute Yang model and default TPID for Minigraph to configDB Changes * Fixed alignment spacing issue, removed length and convert to use tpid_type typedef for both LAG and PORT tpid attribute, added description in port and portchannel leaf for the tpid attribute

[TPID CONFIG]TPID attribute Yang model and default TPID for Minigraph…

0528696

… to configDB Changes

gechiang requested a review from lguohan as a code owner May 17, 2021 17:14

This was referenced May 17, 2021

[sonic-swss:TPID CONFIG]Added Orchagent TPID config support for Port/LAG sonic-net/sonic-swss#1747

Merged

[TPID CONFIG] Added TPID configuration CLI support sonic-net/sonic-utilities#1618

Merged

gechiang requested review from arlakshm and rlhui May 17, 2021 18:54

gechiang mentioned this pull request May 17, 2021

SONiC TPID Configuration Support sonic-net/SONiC#681

Merged

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

praveen-li reviewed May 27, 2021

View reviewed changes

gechiang commented May 27, 2021

View reviewed changes

praveen-li reviewed May 27, 2021

View reviewed changes

dgsudharsan reviewed May 27, 2021

View reviewed changes

gechiang added 3 commits May 28, 2021 17:27

Fixed alignment spacing issue, removed length and convert to use tpid…

33295d4

…_type typedef for both LAG and PORT tpid attribute, added description in port and portchannel leaf for the tpid attribute

Merged latest to resolve conflicts and test error due to tpid_type de…

21d1b86

…finition missing brackets during earlier merge misshandled

One more merge conflict resolution on a very busy file

3b9982d

gechiang requested a review from yxieca May 29, 2021 00:21

lguohan approved these changes May 31, 2021

View reviewed changes

gechiang merged commit cff37b0 into sonic-net:master May 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TPID CONFIG]TPID attribute Yang model and default TPID for Minigraph to configDB Changes #7630

[TPID CONFIG]TPID attribute Yang model and default TPID for Minigraph to configDB Changes #7630

gechiang commented May 17, 2021 •

edited

Loading

praveen-li May 27, 2021

gechiang May 28, 2021

gechiang May 27, 2021 •

edited

Loading

gechiang May 27, 2021

gechiang May 27, 2021

praveen-li May 27, 2021

gechiang May 28, 2021

praveen-li May 27, 2021

gechiang May 28, 2021

praveen-li May 27, 2021

gechiang May 28, 2021

praveen-li May 27, 2021

gechiang May 29, 2021

dgsudharsan May 27, 2021

gechiang May 29, 2021

[TPID CONFIG]TPID attribute Yang model and default TPID for Minigraph to configDB Changes #7630

[TPID CONFIG]TPID attribute Yang model and default TPID for Minigraph to configDB Changes #7630

Conversation

gechiang commented May 17, 2021 • edited Loading

Why I did it

How to verify it

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

Description for the changelog

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gechiang May 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gechiang commented May 17, 2021 •

edited

Loading

gechiang May 27, 2021 •

edited

Loading