Adding support for OC YANG interfaces #125

nagarwal03 · 2024-03-08T00:04:27Z

What we did:
Added transformer support for OpenConfig Interfaces as specified in the HLD

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 unit test files for REST and gNMI.

Unit test file changes:

interfaces_openconfig_test.go – To test the parameters in the openconfig-interfaces.yang via REST.
testapp.log
gNMI is tested manually.

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:
translib.test log

Support added:

Provide support for OpenConfig Interfaces YANG models.
Replace translib App based implementation with a Transformer based implementation of:
- Get Ethernet interface attributes.
- Configure/ Set Ethernet interface attributes.
- Delete Ethernet interface attributes.
Ethernet interface parameters can be modified via REST and gNMI.
Support port-speed and auto-negotiation on Ethernet interfaces via REST and gNMI.
Support IPv4 and IPv6 address configuration on Ethernet interfaces via REST and gNMI.
This does not cover gNMI subscription.

linux-foundation-easycla · 2024-03-08T00:04:31Z

The committers listed above are authorized under a signed CLA.

✅ login: nagarwal03 / name: Nikita Agarwal (a757a11, 471426b, 3857b9e, 5b2f5fa, 4cd984f, e1a05bc, 6ecb456, 3ec7c4d)

adyeung · 2024-03-15T01:00:36Z

@sayedsaquibbrcm will help review

adyeung · 2024-03-15T01:01:53Z

@nagarwal03 pls clear EasyCLA and build failures

nagarwal03 · 2024-03-19T00:41:55Z

@adyeung Fixed EasyCLA and build failures.

Co-authored-by: Satoru-Shinohara <satoru.shinohara@dell.com> Co-authored-by: Nikita Agarwal <nikita_agarwal1@dell.com>

sayedsaquibbrcm · 2024-03-21T10:40:48Z

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

+    deviate not-supported;
+  }
+
+  deviation /oc-intf:interfaces/oc-intf:interface/oc-intf:subinterfaces/oc-intf:subinterface/oc-ip:ipv6/oc-ip:state {


Openconfig mandates that every attribute under "config" container should be mirrored in "state" container too.

Here, interfaces/interface/subinterfaces/subinterface/ipv4/config/enabled is present(see line 164),
but interfaces/interface/subinterfaces/subinterface/ipv4/state is pruned.

This is inconsistent with OpenConfig guidelines.

This needs to be corrected

Thanks for reviewing this @sayedsaquibbrcm.

The two ipv4 containers are not supported and are marked correctly in the PR. (Attached lines)

interfaces/interface/subinterfaces/subinterface/ipv4/config

sonic-mgmt-common/models/yang/extensions/openconfig-interfaces-deviation.yang

Line 132 in 471426b

deviation /oc-intf:interfaces/oc-intf:interface/oc-intf:subinterfaces/oc-intf:subinterface/oc-ip:ipv4/oc-ip:config {

interfaces/interface/subinterfaces/subinterface/ipv4/state

sonic-mgmt-common/models/yang/extensions/openconfig-interfaces-deviation.yang

Line 136 in 471426b

deviation /oc-intf:interfaces/oc-intf:interface/oc-intf:subinterfaces/oc-intf:subinterface/oc-ip:ipv4/oc-ip:state {

The ipv6 config enabled attribute is supported and the corresponding ipv6 state attribute is now marked as supported.

interfaces/interface/subinterfaces/subinterface/ipv6/config/enabled ===> Supported already

interfaces/interface/subinterfaces/subinterface/ipv6/state/enabled ===> Addressed in a new commit and marked as supported.

sayedsaquibbrcm · 2024-03-21T10:48:19Z

translib/transformer/xfmr_intf.go

+
+	idx := pathInfo.Var("index")
+
+	if idx != "0" && idx != "*" && idx != "" {


HLD mentions that routed subinterfaces are not supported. We should remove this code.

Removed the code.

sayedsaquibbrcm · 2024-03-21T10:49:08Z

translib/transformer/xfmr_intf.go

+	}
+	var idx string
+
+	if strings.Contains(inParams.key, ".") {


As per HLD, we do not support subinterfaces

Updated this.

sayedsaquibbrcm · 2024-03-26T06:12:27Z

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

+    deviate not-supported;
+  }
+
+  deviation /oc-intf:interfaces/oc-intf:interface/oc-intf:subinterfaces/oc-intf:subinterface/oc-ip:ipv6/oc-ip:state {


This needs to be corrected

sayedsaquibbrcm · 2024-04-08T10:42:42Z

translib/transformer/xfmr_intf.go

+
+	if idx != "" {
+		i64, _ := strconv.ParseUint(idx, 10, 32)
+		i32 = uint32(i64)


maybe we should return error if i32 != 0, because subinterface is not supported

@sayedsaquibbrcm I've addressed the review comments.

sayedsaquibbrcm · 2024-04-08T10:43:00Z

translib/transformer/xfmr_intf.go

+			log.Errorf("Extracting Interface type for Interface: %s failed!", ifName)
+			return "", tlerr.New(ierr.Error())
+		}
+		err = performIfNameKeyXfmrOp(&inParams, &requestUriPath, &ifName, intfType, i32)


this function is also not returning error if subintfid != 0.
We should ensure that operations on URIs with subinterface index > 0, return error, instead of succeeding and modifying unintended config.

sayedsaquibbrcm · 2024-04-08T10:50:49Z

translib/transformer/xfmr_intf.go

+		if xpath == "/openconfig-interfaces:interfaces/interface/subinterfaces/subinterface/ipv6/config/enabled" {
+			if len(inParams.subOpDataMap) > 0 {
+				dbMap := make(map[string]map[string]db.Value)
+				if inParams.subOpDataMap[4] != nil && (*inParams.subOpDataMap[4])[db.ConfigDB] != nil {


we should use const for brevity
https://github.com/sonic-net/sonic-mgmt-common/blob/master/translib/transformer/xconst.go#L73
const (
GET Operation = iota + 1
CREATE
REPLACE
UPDATE
DELETE
SUBSCRIBE
MAXOPER
)

sayedsaquibbrcm · 2024-04-08T10:52:07Z

translib/transformer/xfmr_intf.go

+				(*inParams.dbDataMap)[db.ConfigDB]["SUBINTF_TBL"]["0"].Field["NULL"] = "NULL"
+			}
+			tblList = append(tblList, "SUBINTF_TBL")
+		}


in the else case, should we return error is idx != 0 to avoid inadvertent issues?

sayedsaquibbrcm · 2024-04-08T10:55:14Z

translib/transformer/xfmr_intf.go

+
+	idx := pathInfo.Var("index")
+
+	subintf_key = idx


Same here, idx > 0 should return error...
getIntfTypeByName(ifName) will not be returning error for subif id > 0, which make the type of interface as subinterface.

sayedsaquibbrcm · 2024-04-08T10:55:25Z

translib/transformer/xfmr_intf.go

+	rmap := make(map[string]interface{})
+	var err error
+	i64, _ := strconv.ParseUint(idx, 10, 32)
+	rmap["index"] = i64


sayedsaquibbrcm · 2024-04-08T10:55:48Z

translib/transformer/xfmr_intf.go

+	id := pathInfo.Var("index")
+	log.Info("DbToYang_subif_index_xfmr: Sub-interface Index = ", id)
+	i64, _ := strconv.ParseUint(id, 10, 32)
+	res_map["index"] = i64


sayedsaquibbrcm · 2024-04-08T10:58:33Z

translib/transformer/xfmr_intf.go

+
+		var subIdxStr string
+		var name string
+		if strings.Contains(intfName, ".") {


this code may be irrelevant ... pls remove

sayedsaquibbrcm

pls see previous comments

This reverts commit 85119e8.

Why I did it Build pipeline is broken, and build error is introduced bye sonic-mgmt-common: sonic-net/sonic-mgmt-common#125 How I did it Disable related test How to verify it Run azure pipeline to verify build

nagarwal03 mentioned this pull request Mar 8, 2024

Adding HLD for OC Yang interfaces. sonic-net/SONiC#1628

Merged

kwangsuk assigned kwangsuk and anand-kumar-subramanian Mar 12, 2024

Satoru-Shinohara force-pushed the Add-xfmr-OC-Yang-Intf-Mar-07 branch from dc1deea to 1803bc2 Compare March 13, 2024 01:43

kwangsuk assigned kwangsuk and unassigned kwangsuk and anand-kumar-subramanian Mar 14, 2024

nagarwal03 force-pushed the Add-xfmr-OC-Yang-Intf-Mar-07 branch from ee21a68 to b13d276 Compare March 19, 2024 00:02

nagarwal03 and others added 3 commits March 19, 2024 12:33

Adding transformer support for OC yang intf.

6ecb456

Co-authored-by: Satoru-Shinohara <satoru.shinohara@dell.com> Co-authored-by: Nikita Agarwal <nikita_agarwal1@dell.com>

Adding fix for sanity check errors.

3ec7c4d

Co-authored-by: Satoru-Shinohara <satoru.shinohara@dell.com> Co-authored-by: Nikita Agarwal <nikita_agarwal1@dell.com>

Adding dbinit.py to azure-pipelines.yml

471426b

nagarwal03 force-pushed the Add-xfmr-OC-Yang-Intf-Mar-07 branch from a25aa92 to 471426b Compare March 19, 2024 19:44

sayedsaquibbrcm reviewed Mar 21, 2024

View reviewed changes

jeff-yin mentioned this pull request Mar 25, 2024

UMF: OpenConfig YANG support for Physical Interfaces #131

Closed

sayedsaquibbrcm suggested changes Mar 26, 2024

View reviewed changes

nagarwal03 added 2 commits March 28, 2024 12:42

Merge branch 'sonic-net:master' into Add-xfmr-OC-Yang-Intf-Mar-07

a757a11

Addressing review comments.

3857b9e

rathnasabapathyv approved these changes Mar 30, 2024

View reviewed changes

sayedsaquibbrcm reviewed Apr 8, 2024

View reviewed changes

sayedsaquibbrcm suggested changes Apr 8, 2024

View reviewed changes

Addressing review comments for subinterfaces.

5b2f5fa

nagarwal03 force-pushed the Add-xfmr-OC-Yang-Intf-Mar-07 branch 2 times, most recently from 41a6d8a to 5b2f5fa Compare April 25, 2024 22:03

nagarwal03 added 2 commits April 25, 2024 16:42

Merge branch 'master' into Add-xfmr-OC-Yang-Intf-Mar-07

e1a05bc

Adding go.mod file.

4cd984f

sayedsaquibbrcm approved these changes Apr 29, 2024

View reviewed changes

kwangsuk approved these changes Apr 29, 2024

View reviewed changes

kwangsuk merged commit 85119e8 into sonic-net:master Apr 29, 2024
2 checks passed

ganglyu mentioned this pull request Apr 30, 2024

Fix build error from sonic-mgmt-common sonic-net/sonic-gnmi#224

Merged

6 tasks

kwangsuk added a commit that referenced this pull request Apr 30, 2024

Revert "Adding support for OC YANG interfaces (#125)"

f956ff4

This reverts commit 85119e8.

kwangsuk mentioned this pull request Apr 30, 2024

Revert "Adding support for OC YANG interfaces" #137

Closed

rathnasabapathyv mentioned this pull request May 13, 2024

WIP: Add Ethernet intf subscription support nagarwal03/oc-yang-intf#29

Open

ganglyu mentioned this pull request Jun 18, 2024

[202311] Fix build error from sonic-mgmt-common sonic-net/sonic-gnmi#248

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support for OC YANG interfaces #125

Adding support for OC YANG interfaces #125

nagarwal03 commented Mar 8, 2024

linux-foundation-easycla bot commented Mar 8, 2024 •

edited

Loading

adyeung commented Mar 15, 2024

adyeung commented Mar 15, 2024

nagarwal03 commented Mar 19, 2024

sayedsaquibbrcm Mar 21, 2024

sayedsaquibbrcm Mar 26, 2024

nagarwal03 Mar 29, 2024

sayedsaquibbrcm Mar 21, 2024

nagarwal03 Mar 29, 2024

sayedsaquibbrcm Mar 21, 2024

nagarwal03 Mar 29, 2024

sayedsaquibbrcm Mar 26, 2024

sayedsaquibbrcm Apr 8, 2024

nagarwal03 Apr 25, 2024

sayedsaquibbrcm Apr 8, 2024

sayedsaquibbrcm Apr 8, 2024 •

edited

Loading

sayedsaquibbrcm Apr 8, 2024 •

edited

Loading

sayedsaquibbrcm Apr 8, 2024 •

edited

Loading

sayedsaquibbrcm Apr 8, 2024

sayedsaquibbrcm Apr 8, 2024

sayedsaquibbrcm Apr 8, 2024

sayedsaquibbrcm left a comment


		idx := pathInfo.Var("index")

		if idx != "0" && idx != "*" && idx != "" {

Adding support for OC YANG interfaces #125

Adding support for OC YANG interfaces #125

Conversation

nagarwal03 commented Mar 8, 2024

linux-foundation-easycla bot commented Mar 8, 2024 • edited Loading

adyeung commented Mar 15, 2024

adyeung commented Mar 15, 2024

nagarwal03 commented Mar 19, 2024

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

sayedsaquibbrcm Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

sayedsaquibbrcm Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

sayedsaquibbrcm Apr 8, 2024 • 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

sayedsaquibbrcm left a comment

Choose a reason for hiding this comment

linux-foundation-easycla bot commented Mar 8, 2024 •

edited

Loading

sayedsaquibbrcm Apr 8, 2024 •

edited

Loading

sayedsaquibbrcm Apr 8, 2024 •

edited

Loading

sayedsaquibbrcm Apr 8, 2024 •

edited

Loading