gNMI subscription for translib managed YANG data #1287

sachinholla · 2023-03-15T10:56:13Z

Describes the high level design for SONiC Telemetry service and Translib infrastructure changes
to support gNMI subscriptions and wildcard paths for YANG defined paths

Code PRs:

Repo	PR title	State
sonic-mgmt-common	AppInterface enhancements for subscription
	DB Access: redis like key pattern matcher
	Return ygot strcut from app module processGet functions
	Avoid duplicate ocbinds schema loading
	DB Access: DB scan cursor support for keys and fields to iterate it
	Path validator API implementation to validate the gnmi path
	Subscribe request translation API implementation in transformer
	Apply goyang patch xfmr memory optimization
	Transformer Infra memory optimization and enhancements
	Transformer Infra Unit Tests
	DB Access: OnChange cache
	enhancements and fixes in the YGOT package
	Subscription callbacks for acl and interface app modules
	Subscribe response translation API implementation for the common app module
	Translib APIs for subscription
sonic-gnmi	Ygot diff tool
	Use PathValidator to sanitize the request paths
	Use new translib subscription APIs

linux-foundation-easycla · 2023-03-15T10:56:19Z

The committers listed above are authorized under a signed CLA.

✅ login: sachinholla / name: Sachin Holla (ff5bfa9, ced6aaf, 6826452, 3fbf82a, 8bd8d3f)
✅ login: mbalachandar / name: Balachandar Mani (5fac1c1)

Added DB table mapping info

venkatmahalingam · 2023-03-28T16:11:17Z

doc/mgmt/gnmi/gNMI_Subscription_for_YangData.md

+
+SONiC Telemetry service suports gNMI Get, Set and Subscribe RPCs for DB paths and sonic-yang based paths.
+It also suppots Get and Set for OpenConfig and IETF yang based paths that are part of **sonic-mgmt-common** repository.
+This design document describes proposed enhancements to support gNMI Subscribe RPC for such YANG paths.


Can you please be explicit here that this HLD adds the subscription feature for openconfig models based on translib infra part of sonic-mgmt-common?

venkatmahalingam · 2023-03-28T16:11:56Z

doc/mgmt/gnmi/gNMI_Subscription_for_YangData.md

+
+## 8 Unit Tests
+
+Following unit test cases require app code changes to handle subscription.


What UT/FT cases will be added for this infra code if no app code changes?

We will either enhance some of the existing app module code or introduce temporary test yangs.

lguohan · 2023-03-29T00:19:33Z

doc/mgmt/gnmi/gNMI_Subscription_for_YangData.md

+The request `target` will be made optional.
+Request will be processed as described in [GRPC Telemetry HLD](../../system-telemetry/grpc_telemetry.md)
+if the `target` is specified and matches one of the reserved keywords listed in that HLD.
+If `target` is not specified or not one of the reserved ones, the subscribe path will be treated as tarnslib YANG path.


@qiluo-msft , @zbud-msft , do we plan to use any other target, if yes, then this would break our future use cases.

@lguohan & team -- did not hear any responses for this. Anyway, I now have proposed a origin based identification of OpenConfig YANG paths. Existing target based identification of DB/event/virtual paths will be retained when client did not specify an origin. Please check.
However this target based identification is a violation of the gNMI specification. Please consider using origin instead of target.

Added new translateSubscribe() and processSubscribe() functions to appInterface as per HLD sonic-net/SONiC#1287 Also added stub implementation of these functions to all existing app modules. It blindly treats all paths as non-db. Basic subscription features, without on_change, are enabled with this mapping. Signed-off-by: Sachin Holla <[email protected]>

Added new translateSubscribe() and processSubscribe() functions to appInterface as per HLD sonic-net/SONiC#1287 Also added stub implementation of these functions to all existing app modules. It blindly treats all paths as non-db. Basic subscription features, without on_change, can be verified with this mapping. Signed-off-by: Sachin Holla <[email protected]>

qiluo-msft · 2023-03-31T18:18:41Z

doc/mgmt/gnmi/gNMI_Subscription_for_YangData.md

+
+SONiC Telemetry service suports gNMI Get, Set and Subscribe RPCs for DB paths and sonic-yang based paths.
+It also suppots Get and Set for OpenConfig and IETF yang based paths that are part of **sonic-mgmt-common** repository.
+This design document describes proposed enhancements to support gNMI Subscribe RPC for such YANG paths.


YANG paths

How about sonic-yang paths? If out of scope of this HLD, do you evaluate the possibility and effort needed?

Sonic yang paths are not considering it in this HLD. But it can be easily supported in future if needed.

I see some of the requirements are useful for sonic-yang paths, and for sonic-db paths. For example TARGET_DEFINED/POLL and ONCE/Wildcard Keys.

It would be nice if the requirement could be implemented onto all the possible path types.

Translib can handle sonic-yang paths. We are planning to implement in a future release.

sonic-db paths cannot be be handled in translib. You need to enhance the DbClient implementation to handle all other subscription modes. This HLD is applicable for TranslClient only.

qiluo-msft · 2023-03-31T18:20:13Z

doc/mgmt/gnmi/gNMI_Subscription_for_YangData.md

+
+### 1.2 Requirements
+
+#### 1.2.1 ON_CHANGE subscription for eligible paths


ON_CHANGE subscription

ON_CHANGE subscription is not limited to openconfig yang models. How about the support on sonic-yang paths, non-db paths? #Pending

This HLD only covers the yang paths handled by translib.

qiluo-msft · 2023-03-31T18:21:15Z

@hui-ma Please help review.

qiluo-msft · 2023-03-31T18:23:46Z

doc/mgmt/gnmi/gNMI_Subscription_for_YangData.md

+#### 1.2.3 TARGET_DEFINED subscription for all paths
+
+Infrastructure should support TARGET_DEFINED subscription for all YANG paths.
+Subscribe request should be treated as ON_CHANGE or SAMPLE based on the app's preferences for the target path.


app's preferences

What is "app's preferences"? #Closed

Subscription preferences for a yang path as set by the app module. Section 2.5 has more details.

qiluo-msft · 2023-03-31T18:24:38Z

doc/mgmt/gnmi/gNMI_Subscription_for_YangData.md

+
+Infrastructure should support TARGET_DEFINED subscription for all YANG paths.
+Subscribe request should be treated as ON_CHANGE or SAMPLE based on the app's preferences for the target path.
+It should split into multiple requests if the target path supports ON_CHANGE


It should split into multiple requests

Who should split what into multiple requests?

Translib infra

Could you explain "It should split into multiple requests if the target path supports ON_CHANGE"? Better with an example?

See section 2.10

qiluo-msft · 2023-03-31T18:25:43Z

doc/mgmt/gnmi/gNMI_Subscription_for_YangData.md

+
+Infrastructure should support POLL and ONCE subscriptions for all YANG paths.
+
+#### 1.2.5 Support Wildcard Keys


Wildcard Keys

wildcard is not limited to openconfig yang models. How about the support on sonic-yang paths, non-db paths, db paths? #Pending

This HLD only covers the yang paths handled by translib.

qiluo-msft · 2023-03-31T18:27:01Z

doc/mgmt/gnmi/gNMI_Subscription_for_YangData.md

+Apps should be allowed to indicate wildcard unsupported paths.
+An `INVALID_ARGUMENT` status should be returned if wildcard key cannot be supported for the target path.
+
+Wildcard in path element (like `/interfaces/*/config` or `/interfaces/.../config`) is a stretch goal.


stretch goal

You must have some basic goals. Could you list them explicitly? #Closed

Supporting wildcard keys is the basic goal (section heading). I will remove this line to avoid confusion.

qiluo-msft · 2023-03-31T18:29:49Z

doc/mgmt/gnmi/gNMI_Subscription_for_YangData.md

+### 1.3 Translib Overview
+
+Translib is a golang library for reading and writing YANG model based data to redis DB or non-DB data source.
+Applications would plugin the YANG models and their translation code into the translib.


Applications

The term "Applications" is easy to mislead to user mode process running inside sonic. #Closed

Will try to rephrase

qiluo-msft · 2023-03-31T18:30:28Z

doc/mgmt/gnmi/gNMI_Subscription_for_YangData.md

+  Apps should implement a *path transformer* to handle special cases --
+  subtree transformer or when there is no one-to-one mapping of YANG key to DB key.
+
+### 2.2 Identifying YANG Based Subscriptions


@ganglyu Please help check this section. #Closed

qiluo-msft · 2023-03-31T18:32:07Z

doc/mgmt/gnmi/gNMI_Subscription_for_YangData.md

+### 2.4 Streaming Data from Translib
+
+Unlike Get, translib will be returning multiple responses while processing the subscribe request.
+To handle this, the gNMI server would pass a queue to translib and wait for the response on it.


queue

How to you limit the queue size? Is it possible that the queue size will be infinite? Is it possible that queue is full and notification will drop internally? #Closed

There should not be any silent drops. We are re-using the existing PriorityQueue of client_subscribe.go. I do not see any queue size related handling there. Behavior will be same as that of the existing DbClient or NonDbClient (with respect to the response queue management).

jeff-yin · 2023-05-09T04:46:05Z

Please add sonic-mgmt-common#81 (Transformer UT) to the PR list. I can't edit the main text of this PR.

ganglyu · 2023-05-18T05:27:56Z

doc/mgmt/gnmi/gNMI_Subscription_for_YangData.md

+| *{empty}*       | *{empty}*       | Error (existing logic)                    |
+| *{empty}*       | *{unknown}*     | Error                                     |
+| *{unknown}*     | *{opaque}*      | Error                                     |
+


Please check https://github.com/sonic-net/SONiC/blob/master/doc/mgmt/gnmi/SONiC_GNMI_Server_Interface_Design.md 1.2.1.3 Data Schema.
This table does not align with our decision tree, would you please update decision tree?

I had referred that section earlier. That HLD only addresses schema identification for Get and Set rpcs. It does not talk about the Subscribe rpc. However, I saw few discrepancies in the implementation -- the Get & Set code is looking for origin=sonic-db only. I could not find any handling for origin=sonic-yang or origin=sonic_yang as described in the HLD.

The proposal in the current HLD is applicable for Subscribe rpc alone. It does not affect the Get and Set handling. Also, it is in alignment with decision matrix from the other HLD. It only looks different because it does not talk about origin values "sonic-db" and "sonic-yang". We do not support sonic yang based subscription as of now. Hence origin=sonic-yang is not applicable for the current HLD. Similarly, origin=sonic-db is not included because the existing code uses a target based path identification for DB, non-DB and events paths; and we cannot change it without breaking the backward compatibility.

sonic-yang is not implemented for now.
For the Subscribe rpc, we might need to support sonic-db and sonic-yang. This requires a decision tree to handle different scenarios for this rpc. The existing code uses target and origin="", so supporting origin=sonic-db should not affect backward compatibility.

And the logic of decision tree could be:

if origin == 'openconfig':
# translib subscribe
elif origin == 'sonic-db':
# sonic db subscribe
elif origin == 'sonic-yang':
# sonic yang subscribe
else:
# existing subscribe implementation

As discussed in yesterday's meeting, the scope of this HLD is to support subscription for openconfig yang paths only. Hence I have not included anything about origin=sonic-yang in this HLD to avoid confusion. We do have plans to extend it to sonic ynag paths in a future release. Decision table will be updated at that time.

For sonic-db paths, the current implementation uses a different logic and we do not want to touch it -- for backward compatibility with existing clients. The path structure used also slightly differs from your HLD; please refer to SONiC gRPC Telemetry HLD. There are other path types, like non-DB path, event path etc. Hence we are asking the owners of those components (I guess Microsoft) to enhance them.

tomek-US

LGTM

zhangyanzhao · 2023-06-14T04:56:21Z

only two open code PRs left, nice!

skg-net · 2024-01-31T03:46:34Z

@adyeung Can you please update the Quality Metric (Alpha/Beta/GA) for the feature either in this PR comments or in HLD itself based on https://github.com/sonic-net/SONiC/blob/master/doc/SONiC%20feature%20quality%20definition.md

adyeung · 2024-01-31T19:42:04Z

Please mark Quality Metric as Alpha for this feature

sachinholla and others added 6 commits March 7, 2023 20:49

gNMI Subscription for YANG data -- initial skeleton

ff5bfa9

gNMI subscription -- high level design

ced6aaf

gNMI Subscribe: add table of contents

6826452

gNMI subscribe: add translib and app interface details

3fbf82a

Added the CommonApp's interface, Transformer, and Path validator details

5fac1c1

Udpate table of contents

8bd8d3f

sachinholla and others added 3 commits March 27, 2023 22:34

gNMI subscribe: address review comments

a7ca45b

Merge branch 'master' into gnmi_subscribe_hld

d33cd06

Update gNMI_Subscription_for_YangData.md

e35feda

Added DB table mapping info

lguohan requested review from qiluo-msft and zbud-msft March 28, 2023 15:39

venkatmahalingam reviewed Mar 28, 2023

View reviewed changes

lguohan reviewed Mar 29, 2023

View reviewed changes

qiluo-msft requested a review from ganglyu March 29, 2023 20:18

sachinholla mentioned this pull request Mar 31, 2023

AppInterface enhancements for subscription sonic-net/sonic-mgmt-common#67

Merged

qiluo-msft reviewed Mar 31, 2023

View reviewed changes

sachinholla mentioned this pull request Apr 17, 2023

Return ygot strcut from app module processGet() functions sonic-net/sonic-mgmt-common#72

Merged

zhangyanzhao mentioned this pull request May 8, 2023

UMF Transformer Enhancements and Optimization #1330

Closed

venkatmahalingam previously approved these changes May 16, 2023

View reviewed changes

sachinholla mentioned this pull request May 17, 2023

Subscription callbacks for acl and interface app modules sonic-net/sonic-mgmt-common#86

Merged

ganglyu reviewed May 18, 2023

View reviewed changes

sachinholla mentioned this pull request May 24, 2023

TranslClient: use PathValidator to sanitize the request paths sonic-net/sonic-gnmi#112

Merged

6 tasks

Add a flow diagram for path identification logic

7d2b5c6

sachinholla dismissed venkatmahalingam’s stale review via 7d2b5c6 May 24, 2023 16:54

mbalachandar mentioned this pull request May 25, 2023

Subscribe response translation API implementation for the common app module sonic-net/sonic-mgmt-common#88

Closed

ganglyu approved these changes May 26, 2023

View reviewed changes

mbalachandar mentioned this pull request May 26, 2023

Subscribe response translation API implementation for the common app module sonic-net/sonic-mgmt-common#90

Merged

venkatmahalingam approved these changes May 26, 2023

View reviewed changes

tomek-US approved these changes May 26, 2023

View reviewed changes

adyeung approved these changes May 26, 2023

View reviewed changes

Merge branch 'master' into gnmi_subscribe_hld

2e72a58

adyeung merged commit 2b9a72f into sonic-net:master May 27, 2023

sachinholla mentioned this pull request Jun 13, 2023

Translib APIs for subscription sonic-net/sonic-mgmt-common#92

Merged

sachinholla mentioned this pull request Jun 14, 2023

TranslClient: Use new translib subscription APIs sonic-net/sonic-gnmi#122

Merged

6 tasks

sachinholla mentioned this pull request Jun 27, 2023

Debug grpc to fetch subscribe preferences of a yang path sonic-net/sonic-gnmi#130

Merged

6 tasks

adyeung mentioned this pull request Oct 12, 2023

UMF Infra Enhancement for SONIC-YANG #1397

Closed


		## 8 Unit Tests

		Following unit test cases require app code changes to handle subscription.


		### 1.2 Requirements

		#### 1.2.1 ON_CHANGE subscription for eligible paths


		Infrastructure should support POLL and ONCE subscriptions for all YANG paths.

		#### 1.2.5 Support Wildcard Keys

gNMI subscription for translib managed YANG data #1287

gNMI subscription for translib managed YANG data #1287

Conversation

sachinholla commented Mar 15, 2023 • edited Loading

linux-foundation-easycla bot commented Mar 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

venkatmahalingam Mar 28, 2023 • 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

qiluo-msft Mar 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sachinholla May 18, 2023 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Mar 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft commented Mar 31, 2023

qiluo-msft Mar 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Mar 31, 2023 • 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

qiluo-msft Mar 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Mar 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Mar 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Mar 31, 2023 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Mar 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeff-yin commented May 9, 2023

ganglyu May 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ganglyu May 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomek-US left a comment

Choose a reason for hiding this comment

zhangyanzhao commented Jun 14, 2023

skg-net commented Jan 31, 2024

adyeung commented Jan 31, 2024

sachinholla commented Mar 15, 2023 •

edited

Loading

linux-foundation-easycla bot commented Mar 15, 2023 •

edited

Loading

venkatmahalingam Mar 28, 2023 •

edited

Loading

qiluo-msft Mar 31, 2023 •

edited

Loading

sachinholla May 18, 2023 •

edited

Loading

qiluo-msft Mar 31, 2023 •

edited

Loading

qiluo-msft Mar 31, 2023 •

edited

Loading

qiluo-msft Mar 31, 2023 •

edited

Loading

qiluo-msft Mar 31, 2023 •

edited

Loading

qiluo-msft Mar 31, 2023 •

edited

Loading

qiluo-msft Mar 31, 2023 •

edited

Loading

qiluo-msft Mar 31, 2023 •

edited

Loading

qiluo-msft Mar 31, 2023 •

edited

Loading

ganglyu May 18, 2023 •

edited

Loading

ganglyu May 18, 2023 •

edited

Loading