Adding support for OC YANG interfaces. by nagarwal03 · Pull Request #9 · nagarwal03/oc-yang-intf

nagarwal03 · 2024-02-13T20:29:40Z

What we did:
Added transformer support for OpenConfig Interfaces.

Why we did it?
Earlier, intf_app.go was used for interfaces configuration. Replacing it with xfmr_intf.go to add transformer support for OpenConfig YANG interfaces configuration via REST and gNMI.

How did we verify the changes?
Verified by adding interface unit test files for REST and gNMI.

Unit test file changes for REST:
interfaces_openconfig_test.go – To test the parameters in the openconfig-interfaces.yang.
(https://github.com/nagarwal03/oc-yang-intf/files/14440648/feb28_testapp.log)

Log files for REST manual run:
REST_ETHERNET.log
REST_SUBINTERFACE.log
REST_TOP.log

Log files for gNMI manual run:
GNMI_ETH.log
GNMI_SUB.log
GNMI_TOP.log

Translib.test Logs:
https://github.com/nagarwal03/oc-yang-intf/files/14440650/feb28_translib.log

Support added:
Ethernet interface parameters can be modified via REST and gNMI.

sonic-port xfmr support

…f github.com:nagarwal03/oc-yang-intf into Nikita_subintf_oc_yang

Adding support for OC yang interfaces.

Remove IntfType related code for any non-supported types

Code Clean-up - Remove unsupported interface related code

Merge branch 'oc-intf-common-code-Jan30' into Nikita_subintf_oc_yang

Removing unnecessary code.

rathnasabapathyv · 2024-02-14T01:30:55Z

models/yang/openconfig-if-ip.yang

       "The length of the subnet prefix.";
    }
+
+    leaf family  {


Let's not introduce "family" yang attribute in OC-yang, since it was present in sonic-yang. If we introduce a new attribute in OC-yang, we should have a justification for that. From OC-yang perspective, "family" attribute is not needed, since OC have separate container already for IPv4 (subinterfaces/subinterface/ipv4) & IPv6 (subinterfaces/subinterface/ipv6), which is already intuitive. But in Sonic-yang, there is no separate table for each address-family, hence family attribute is needed in sonic-yang alone. Based on the incoming OC-yang URI for IP-config, we can populate family attribute field in sonic-yang/config-db accordingly in YangToDb_intf_ip_addr_xfmr

I've removed the family attribute from OC Yang and xfmr_intf.go

rathnasabapathyv · 2024-02-14T05:02:57Z

translib/utils/utils.go


 import (
 	"fmt"
-	//"github.com/Azure/sonic-mgmt-common/translib/db"


why we need this change? this import remove is already present in community file. Check for white-space

This line was originally added twice in the community file and one of the lines was commented out so I deleted that commented line.

rathnasabapathyv · 2024-02-14T05:05:09Z

models/yang/openconfig-if-ip.yang

      description
        "The length of the subnet prefix.";
    }
+    leaf family  {


Same as above

rathnasabapathyv · 2024-02-14T05:06:43Z

translib/sys_app.go

 	procTable   map[uint64]dbEntry
 }

+type reqType int


Do we really need this change in this file? It's not been used in xfmr_intf.go

Used to be used, seems to be removed with one of the code changes to remove unnecessary code. Will double check & remove.

@rathnasabapathyv It is required in the sys_app.go file and pfm_app.go file. It was earlier defined in the intf_app.go file and since we deleted that file, we were seeing the following errors so had to redefine it here:

./pfm_app.go:38:28: undefined: dbEntry ./pfm_app.go:168:39: undefined: dbEntry ./pfm_app.go:184:39: undefined: dbEntry ./sys_app.go:42:25: undefined: dbEntry ./sys_app.go:43:25: undefined: dbEntry ./sys_app.go:269:36: undefined: dbEntry ./sys_app.go:285:33: undefined: dbEntry ./sys_app.go:290:34: undefined: dbEntry ./sys_app.go:307:24: undefined: dbEntry make[3]: *** [Makefile:59: /sonic/src/sonic-mgmt-common/build/tests/translib/testapp.test] Error 2

As part of this enhancement, we don't have any enhancements in pfm_app.go & sys_app.go. Without this change, existing community code pfm_app.go, how it's compiling?

Looks like in existing community code, these two typedefs reqType & dbEntry were present earlier in intf_app.go, which has been removed by us as part of this PR and we have to park it somewhere. It's ok then.

…yang-intf into satoru-port-jan30

Adding automation file for subinterfaces.

…tion error

Add Interface-level and Ethernet UT scripts + Merge all interface related tests to 1 file

Addressing review comments. Removed family attribute from OC Yang.

…Jan30' into Nikita_subintf_oc_yang

kwangsuk · 2024-02-21T22:02:17Z

You may have to update go.mod and go.sum files. Please check them in the following repos - sonic-mgmt-common, sonic-mgmt-framework.

kwangsuk · 2024-02-21T22:09:53Z

models/yang/extensions/openconfig-interfaces-ext.yang

The extension YANG file is intended to extend the existing OC YANG definitions to support Dell specific nodes. Since this file does not have such extended node definitions but just deviations, you do not need this file. Please move the deviate statements to openconfig-interfaces-deviation.yang.

go.mod and go.sum files have been updated with the latest build.
Extention yang has been deleted and all deviations has been moved to the openconfig-interfaces-deviation.yang

kwangsuk · 2024-02-21T22:48:25Z

translib/transformer/xfmr_intf.go

+	XlateFuncBind("DbToYang_intf_enabled_xfmr", DbToYang_intf_enabled_xfmr)
+	XlateFuncBind("YangToDb_intf_eth_port_config_xfmr", YangToDb_intf_eth_port_config_xfmr)
+	XlateFuncBind("DbToYang_intf_eth_port_config_xfmr", DbToYang_intf_eth_port_config_xfmr)
+	XlateFuncBind("DbToYangPath_intf_eth_port_config_path_xfmr", DbToYangPath_intf_eth_port_config_path_xfmr)


The path-xmfr callback is designed for subscription support. Just note that earlier code upstream for sflow app includes the subscription support. To be consistent, please plan to add subscription support.
For your reference, here is the PR - sonic-net#91

We are not planning to support subscription as part of this initial PR. Planning to do next. We can remove path-xfmr code and relevant annotations from here.

Okay, that's fine. Just mention the phased plan for community reviewers.

rathnasabapathyv · 2024-02-21T20:39:39Z

models/yang/extensions/openconfig-interfaces-ext.yang

@@ -0,0 +1,147 @@
+module openconfig-interfaces-ext {
+
+    yang-version "1.1";


Let's move all deviations present in this file to openconfig-interfaces-deviation.yang

rathnasabapathyv · 2024-02-21T20:40:08Z

models/yang/extensions/openconfig-interfaces-deviation.yang

+
+  revision "2024-01-19" {
+    description
+      "Add Yang deviations for unsupported subinterfaces.";


Let's change the revision number as 0.1.0 and update description accordingly.

rathnasabapathyv · 2024-02-21T23:19:08Z

models/yang/openconfig-if-ethernet.yang

+    reference "2.13.0";
+  }
+
+  revision "2022-04-20" {


Like we have discussed, let's remove all other revisions except "2.12.1" & "2.8.0"

rathnasabapathyv · 2024-02-21T23:26:35Z

You may have to update go.mod and go.sum files. Please check them in the following repos - sonic-mgmt-common, sonic-mgmt-framework.

What kind of update, do we need @kwangsuk on this? Looks like existing one itself is working for us. Do you think anything is missing for this code changes in PR?

kwangsuk · 2024-02-21T23:36:59Z

You may have to update go.mod and go.sum files. Please check them in the following repos - sonic-mgmt-common, sonic-mgmt-framework.

What kind of update, do we need @kwangsuk on this? Looks like existing one itself is working for us. Do you think anything is missing for this code changes in PR?

The xfmr_intf.go file adds a new package - "inet.af/netaddr", that it will update go.mod accordingly. Please check.

kwangsuk · 2024-02-21T23:43:18Z

Please run your unit test binary, sonic-mgmt-common/build/tests/translib/translib.test, to see the test result.

rathnasabapathyv · 2024-02-21T23:44:05Z

You may have to update go.mod and go.sum files. Please check them in the following repos - sonic-mgmt-common, sonic-mgmt-framework.

What kind of update, do we need @kwangsuk on this? Looks like existing one itself is working for us. Do you think anything is missing for this code changes in PR?

The xfmr_intf.go file adds a new package - "inet.af/netaddr", that it will update go.mod accordingly. Please check.

Got it. Thanks @kwangsuk . If we don't include that "inet.af/netaddr" in go.mod, what kind of issue, we will see? Compilation error or functionality error? Compilation & functional test seems fine, that's why asking. If that was the case, can we assume that pkg is already included through other inclusions?

rathnasabapathyv · 2024-02-21T23:45:28Z

Please run your unit test binary, sonic-mgmt-common/build/tests/translib/translib.test, to see the test result.
Team has written new test specific to interfaces and they are going to attach those automation-logs for reference in PR.

kwangsuk · 2024-02-21T23:46:12Z

You may have to update go.mod and go.sum files. Please check them in the following repos - sonic-mgmt-common, sonic-mgmt-framework.

What kind of update, do we need @kwangsuk on this? Looks like existing one itself is working for us. Do you think anything is missing for this code changes in PR?

The xfmr_intf.go file adds a new package - "inet.af/netaddr", that it will update go.mod accordingly. Please check.

Got it. Thanks @kwangsuk . If we don't include that "inet.af/netaddr" in go.mod, what kind of issue, we will see? Compilation error or functionality error? Compilation & functional test seems fine, that's why asking. If that was the case, can we assume that pkg is already included through other inclusions?

It will fail in azure pipeline.

kwangsuk · 2024-02-21T23:49:59Z

Please run your unit test binary, sonic-mgmt-common/build/tests/translib/translib.test, to see the test result.
Team has written new test specific to interfaces and they are going to attach those automation-logs for reference in PR.

Yes, I know. Since the "testapp" is tagged to the test.go files, it is compiled into translib.test, which is expected. More importantly, the azure pipeline will trigger running tests at time of merge. So, all testcase should pass. Please refer to the azure-pipelines.yml.

Updating automation files. Removed family attribute.

…slib.test

Adding fixes based on review. Adding go.mod and go.sum files for tran…

Satoru-Shinohara · 2024-02-26T23:47:49Z

The PR with the latest change has been merged to the internal common branch.
Those changes should be reflected in this PR now.

Please review when possible, thank you so much!

translib/transformer/xfmr_intf.go

kwangsuk · 2024-02-28T21:06:06Z

As commented earlier, please attach the test result of the testapp.test, which is relevant to the PR. Please replace the translib.test log with testapp.test log. Please refer to the testapp.test binary.
https://github.com/sonic-net/sonic-mgmt-common/blob/99052691c3aa2c9ddaf5f69104af12677a387d4d/translib/Makefile#L67C3-L67C27

…se formatting

Address additional review comments regarding port-speed and switch ca…

kwangsuk · 2024-03-04T23:29:47Z

SONiC community is far more likely to use the SONiC yangs. Like Sflow app, lets upstream interfacs_sonic_test.go as well.

rathnasabapathyv · 2024-03-04T23:31:51Z

SONiC community is far more likely to use the SONiC yangs. Like Sflow app, lets upstream interfacs_sonic_test.go as well.
Why we need to write test cases for those @kwangsuk, since this PR is about transformer changes from OC-yang to Sonic-yang? Even i had that question in SFLOW XFMR PR as well? Even without these xfmr application changes, sonic-yang test via mgmt-framework infra is expected to work as it is.

kwangsuk · 2024-03-04T23:37:15Z

SONiC community is far more likely to use the SONiC yangs. Like Sflow app, lets upstream interfacs_sonic_test.go as well.
Why we need to write test cases for those @kwangsuk, since this PR is about transformer changes from OC-yang to Sonic-yang? Even i had that question in SFLOW XFMR PR as well? Even without these xfmr application changes, sonic-yang test via mgmt-framework infra is expected to work as it is.

The upstream from both Dell and Broadcom is also aimed to encourage the community to adapt the sonic-mgmt-common infrastructure including transformer, cvl and DB components. So, the app code will serve as a show case.

rathnasabapathyv · 2024-03-04T23:46:36Z

SONiC community is far more likely to use the SONiC yangs. Like Sflow app, lets upstream interfacs_sonic_test.go as well.
Why we need to write test cases for those @kwangsuk, since this PR is about transformer changes from OC-yang to Sonic-yang? Even i had that question in SFLOW XFMR PR as well? Even without these xfmr application changes, sonic-yang test via mgmt-framework infra is expected to work as it is.

The upstream from both Dell and Broadcom is also aimed to encourage the community to adapt the sonic-mgmt-common infrastructure including transformer, cvl and DB components. So, the app code will serve as a show case.

Since this PR changes are more about transformer application code changes to enable user to configure via OC-yang, we want to spend effort for writing automation test cases for that cases alone. Without this xfmr application changes, config via OC-yang is not possible. Other existing working scenarios like config via sonic-yang is fine, we don't want to spend time writing automation for those. If we want to show-case some sample test application via sonic-yang, we can let them refer SFLOW automation-test code for sonic-yang.

kwangsuk · 2024-03-05T00:16:03Z

SONiC community is far more likely to use the SONiC yangs. Like Sflow app, lets upstream interfacs_sonic_test.go as well.
Why we need to write test cases for those @kwangsuk, since this PR is about transformer changes from OC-yang to Sonic-yang? Even i had that question in SFLOW XFMR PR as well? Even without these xfmr application changes, sonic-yang test via mgmt-framework infra is expected to work as it is.

The upstream from both Dell and Broadcom is also aimed to encourage the community to adapt the sonic-mgmt-common infrastructure including transformer, cvl and DB components. So, the app code will serve as a show case.

Since this PR changes are more about transformer application code changes to enable user to configure via OC-yang, we want to spend effort for writing automation test cases for that cases alone. Without this xfmr application changes, config via OC-yang is not possible. Other existing working scenarios like config via sonic-yang is fine, we don't want to spend time writing automation for those. If we want to show-case some sample test application via sonic-yang, we can let them refer SFLOW automation-test code for sonic-yang.

Since SONiC yang is 1:1 translation performed by transformer, the YANG to ABNF is pretty straightforward to demonstrate the SONiC yang path for sonic-port.yang and sonic-interface.yang. I believe the interface is most useful show case to the community. But, if you think it's overloaded effort, that would be fine. You can plan it next phase.

Satoru-Shinohara and others added 14 commits January 30, 2024 15:44

sonic-port xfmr support

b7c0d8d

Merge pull request #3 from nagarwal03/satoru-port-jan30

e1f8d71

sonic-port xfmr support

Adding support for OC yang intf.

04b7076

Resolving merge conflicts. Merge branch 'oc-intf-common-code-Jan30' o…

26ca597

…f github.com:nagarwal03/oc-yang-intf into Nikita_subintf_oc_yang

Merge pull request #5 from nagarwal03/Nikita_subintf_oc_yang

c399ed1

Adding support for OC yang interfaces.

remove unneeded code for unsupported interface types

0bc8f73

remove functions that are no longet necessary

8ff5872

Merge pull request #6 from nagarwal03/satoru-port-jan30

d98bb8d

Remove IntfType related code for any non-supported types

Code Clean-up - Remove unsupported interface related code

754c0d3

Add back needed code

e52935c

Merge pull request #7 from nagarwal03/satoru-port-jan30

1cdada6

Code Clean-up - Remove unsupported interface related code

Removing unnecessary code.

5d36584

Resolve clean up conflicts.

adfd683

Merge branch 'oc-intf-common-code-Jan30' into Nikita_subintf_oc_yang

Merge pull request #8 from nagarwal03/Nikita_subintf_oc_yang

f895d66

Removing unnecessary code.

nagarwal03 requested a review from Satoru-Shinohara February 13, 2024 20:29

rathnasabapathyv self-requested a review February 13, 2024 23:02

rathnasabapathyv reviewed Feb 14, 2024

View reviewed changes

Satoru-Shinohara and others added 6 commits February 19, 2024 17:13

UT Tests for openconfig-interfaces (Top-Level Interfaces + Ethernet)

3888202

Merge branch 'oc-intf-common-code-Jan30' of github.com:nagarwal03/oc-…

f4b07de

…yang-intf into satoru-port-jan30

Adding automation file for subinterfaces.

ca4bd9e

Merge pull request #10 from nagarwal03/Nikita_subintf_oc_yang

c8c5abb

Adding automation file for subinterfaces.

Resolve merge conflict of all interface-related tests + Fixed descrip…

699daad

…tion error

Merge pull request #11 from nagarwal03/satoru-port-jan30

7ae1b7c

Add Interface-level and Ethernet UT scripts + Merge all interface related tests to 1 file

nagarwal03 requested review from kwangsuk and rathnasabapathyv February 20, 2024 21:27

Nikita Agarwal and others added 4 commits February 20, 2024 15:41

Addressing review comments. Removed family attribute from OC Yang.

0ba1402

Merge pull request #12 from nagarwal03/Nikita_subintf_oc_yang

b4328f2

Addressing review comments. Removed family attribute from OC Yang.

Updating automation files. Removed family attribute.

07c6f21

Resolve automation file conflicts. Merge branch 'oc-intf-common-code-…

95734a1

…Jan30' into Nikita_subintf_oc_yang

kwangsuk reviewed Feb 21, 2024

View reviewed changes

rathnasabapathyv reviewed Feb 21, 2024

View reviewed changes

nagarwal03 and others added 3 commits February 22, 2024 14:43

Merge pull request #13 from nagarwal03/Nikita_subintf_oc_yang

c250eb9

Updating automation files. Removed family attribute.

Adding fixes based on review. Adding go.mod and go.sum files for tran…

4cbde32

…slib.test

Merge pull request #14 from nagarwal03/satoru-port-jan30

dded84e

Adding fixes based on review. Adding go.mod and go.sum files for tran…

kwangsuk reviewed Feb 28, 2024

View reviewed changes

translib/transformer/xfmr_intf.go Show resolved Hide resolved

kwangsuk reviewed Feb 28, 2024

View reviewed changes

translib/transformer/xfmr_intf.go Show resolved Hide resolved

Satoru-Shinohara and others added 2 commits February 28, 2024 17:44

Address additional review comments regarding port-speed and switch ca…

e264cf6

…se formatting

Merge pull request #15 from nagarwal03/satoru-port-jan30

b4c7204

Address additional review comments regarding port-speed and switch ca…

nagarwal03 requested review from kwangsuk and rathnasabapathyv March 4, 2024 20:26

rathnasabapathyv approved these changes Mar 6, 2024

View reviewed changes

nagarwal03 merged commit 0a95d5d into internal-review-ocintf Mar 6, 2024

		@@ -0,0 +1,147 @@
		module openconfig-interfaces-ext {

		yang-version "1.1";

Conversation

nagarwal03 commented Feb 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nagarwal03 Feb 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kwangsuk commented Feb 21, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rathnasabapathyv commented Feb 21, 2024

Uh oh!

kwangsuk commented Feb 21, 2024

Uh oh!

kwangsuk commented Feb 21, 2024

Uh oh!

rathnasabapathyv commented Feb 21, 2024

Uh oh!

rathnasabapathyv commented Feb 21, 2024

Uh oh!

kwangsuk commented Feb 21, 2024

Uh oh!

kwangsuk commented Feb 21, 2024

Uh oh!

Satoru-Shinohara commented Feb 26, 2024

Uh oh!

Uh oh!

Uh oh!

kwangsuk commented Feb 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kwangsuk commented Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rathnasabapathyv commented Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kwangsuk commented Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rathnasabapathyv commented Mar 4, 2024

Uh oh!

kwangsuk commented Mar 5, 2024

Uh oh!

Reviewers

Assignees

nagarwal03 commented Feb 13, 2024 •

edited

Loading

nagarwal03 Feb 19, 2024 •

edited

Loading

kwangsuk commented Feb 28, 2024 •

edited

Loading

kwangsuk commented Mar 4, 2024 •

edited

Loading

rathnasabapathyv commented Mar 4, 2024 •

edited

Loading

kwangsuk commented Mar 4, 2024 •

edited

Loading