reference icon indicating copy to clipboard operation
reference copied to clipboard

Update gnmi-specification.md Target_Defined mode description

Open ashu-ciena opened this issue 5 months ago • 16 comments

Add clarity on using sample-interval in target defined mode.

ashu-ciena avatar Jul 16 '25 19:07 ashu-ciena

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

google-cla[bot] avatar Jul 16 '25 19:07 google-cla[bot]

@dplore Can you please add relevant reviewers to this PR? Any information missing in the PR ?

ashu-ciena avatar Jul 28 '25 06:07 ashu-ciena

I've added this to the queue for OC Operator review and pinged a couple of individual reviewers.

Making this mandatory would make it a breaking change to the base GNMI specification which we should not allow.

I don't think we expect or require an sample_interval for TARGET_DEFINED. The target may choose it's own sample_interval. I can see this as possibly an optional field for TARGET_DEFINED subscriptions.

dplore avatar Jul 29 '25 00:07 dplore

Reviewed in July 29, 2025 OC Operators meeting.

An additional idea is that this "if the target decides to use sample, the client suggests a sample interval" idea should be implemented in a new gnmi subscription mode. This way the target can communicate if the "suggested sample interval" feature is supported or not.

I think this PR would benefit from a more information about the operational use cases we are trying to solve for before we can provide better feedback of what should be implemented.

dplore avatar Jul 29 '25 17:07 dplore

Reviewed in July 29, 2025 OC Operators meeting.

An additional idea is that this "if the target decides to use sample, the client suggests a sample interval" idea should be implemented in a new gnmi subscription mode. This way the target can communicate if the "suggested sample interval" feature is supported or not.

I think this PR would benefit from a more information about the operational use cases we are trying to solve for before we can provide better feedback of what should be implemented.

Are you suggesting a new subscription mode? I was wondering if the user subscribes in target-defined mode and also provides the sample interval, the target can then communicate if the "suggested sample interval" is viable or not on the subscribed xpath. If , there is no sample interval provided, target can choose.

ashu-ciena avatar Jul 30 '25 12:07 ashu-ciena

Reviewed in July 29, 2025 OC Operators meeting. An additional idea is that this "if the target decides to use sample, the client suggests a sample interval" idea should be implemented in a new gnmi subscription mode. This way the target can communicate if the "suggested sample interval" feature is supported or not. I think this PR would benefit from a more information about the operational use cases we are trying to solve for before we can provide better feedback of what should be implemented.

Are you suggesting a new subscription mode? I was wondering if the user subscribes in target-defined mode and also provides the sample interval, the target can then communicate if the "suggested sample interval" is viable or not on the subscribed xpath. If , there is no sample interval provided, target can choose.

@dplore and @robshakir do you want more discussion on this in Thursday meeting? If not, can we give our final set of comments on the latest upload and we can proceed to merge?

ashu-ciena avatar Aug 04 '25 04:08 ashu-ciena

Following up on this thread, are there any open questions/comments? This PR requires approval, looking forward to it.

ashu-ciena avatar Aug 28 '25 19:08 ashu-ciena

I am note sure why this PR is in "Waiting For Author" state? I think it should be moved to "Ready to discuss" or "In Progress" mode. I have incorporated all the comments, I received so far.

ashu-ciena avatar Oct 09 '25 19:10 ashu-ciena

Moving to ready-to-discuss.

The ability to specify an interval when setting the value of TARGET_DEFINED is surprising to me? But we'll see what folks at the Operators Meeting think.

ElodinLaarz avatar Oct 09 '25 19:10 ElodinLaarz

Reviewed in Oct 14, 2025 OC Operators meeting.

General feedback is:

  • When using TARGET_DEFINED, a target is expected to choose and change a sample interval dynamically, although this is undefined in the specification. We would be happy to accept an update to the specification that this is the expectation.

  • There's no expectation that a client can set a sample_interval in a TARGET_DEFINED subscription mode. The gnmi.proto specifies that sample_interval is used in SAMPLE mode (ref).

dplore avatar Oct 14 '25 16:10 dplore

* There's no expectation that a client can set a `sample_interval` in a `TARGET_DEFINED` subscription mode.  The gnmi.proto specifies that `sample_interval` is used in `SAMPLE` mode ([ref](https://github.com/openconfig/gnmi/blob/891c5a8cdcd670528157283cde220adb0f43f63e/proto/gnmi/gnmi.proto#L303)).

Suggestion for discussion: If sample_interval is set to 0, have the element choose acceptable intervals. If set to non zero, treat the same as sample mode where the device can choose to honor/error

earies avatar Oct 14 '25 17:10 earies

@dplore and @earies the text in the current state of spec is only focusing on the type of subscription in target defined mode. So, we have the liberty to add clarifications regarding the sample interval, if target chose to treat the subscription as sample mode. Not specifying the sample interval is equivalent to specifying it as "0" value. The Subscription proto is same for both sample and target defined mode, so we can not restrict users to provide a sample interval even in target defined mode. I somewhat agree that if user does, target can choose to accept or reject the subscription with "Invalid Parameter" error.

If you all agree, I can update my PR with these clarifications.

ashu-ciena avatar Oct 15 '25 04:10 ashu-ciena

The issue is the currently proposed text to be added for TARGET_DEFINED conflicts with the gnmi.proto field definition for sample_interval. (which is: uint64 sample_interval = 3; // ns between samples in SAMPLE mode.).

It is ok to propose text that says use of sample_interval is only defined for Subscription.mode is SAMPLE. If sample_interval is used in Subscription.mode TARGET_DEFINED , the target should reject the subscription with grpc status code INVALID_ARGUMENT.

dplore avatar Oct 16 '25 00:10 dplore

If you are very interested in allowing a client to optionally choose a sample_interval when using TARGET_DEFINED mode a gnmi extension might be a better place for this. We have managed to not make any changes to the core gnmi protocol in some time and would like to have any additions appear as extensions.

dplore avatar Oct 16 '25 00:10 dplore

If you are very interested in allowing a client to optionally choose a sample_interval when using TARGET_DEFINED mode a gnmi extension might be a better place for this. We have managed to not make any changes to the core gnmi protocol in some time and would like to have any additions appear as extensions.

With TARGET_DEFINED, as the name suggests the "TARGET" should define the sample interval as well. Implementors may choose to use "default" sample interval in TARGET_DEFINED mode, just like when sample_interval is set to zero in SAMPLE mode. Thus no change to the gnmi notification message is required either.

raghubk avatar Oct 27 '25 19:10 raghubk

I have updated the PR with the suggested text. Please do a final review. @raghubk if sample interval is not provided, it is equivalent to 0, so just like sample mode, target can choose the sample interval. In contrast to sample mode of subscription with sample interval 0, the target is not bound to choose the lowest minimum possible value of sample interval imo.

ashu-ciena avatar Nov 14 '25 09:11 ashu-ciena