Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gNMI update with explicit deletion #201

Merged
merged 5 commits into from
Jul 10, 2024

Conversation

gwizdms
Copy link
Contributor

@gwizdms gwizdms commented Nov 7, 2023

Added clarity for delete path discussed in gnmi repo issue 145.

Updated wording to clarify explicit deletion section 3.5.2.3.
Eliminated a proposed change.
@ccole-juniper
Copy link

Which implementations currently send deletions when in sample mode?

@gwizdms
Copy link
Contributor Author

gwizdms commented Nov 16, 2023

@ccole-juniper the purpose of this proposed change to the wording of the specification is not to call out who currently implements what, it is to clarify wording to as to eliminate a broader spectrum of interpretations. Posting this change is also a time for the audience to weigh in on the proposed change. If there is a specific reason why you would not be in favor of supporting such change, please contribute so it can be reviewed by the group.

@ccole-juniper
Copy link

@ccole-juniper the purpose of this proposed change to the wording of the specification is not to call out who currently implements what, it is to clarify wording to as to eliminate a broader spectrum of interpretations. Posting this change is also a time for the audience to weigh in on the proposed change. If there is a specific reason why you would not be in favor of supporting such change, please contribute so it can be reviewed by the group.

Asking for current and prior art is the intention, and I'd still like to hear from other vendors and operators re: their particular implementation. From my experience with SNMP and similar frameworks I'm not familiar with the mixture of change tracking into the equivalent of sample mode. Previous and current implementations will help to inform intention when there's ambiguity.

@ccole-juniper
Copy link

When would deletions be expected to be delivered: promptly on deletion ("out of sample") or at the next sample ("in sample")? If "in sample", I would expect that the notification timestamp would need to reflect the time of deletion which can significantly precede the time of the sample.

@dplore dplore changed the title Gwizdms patch explicit deletion gNMI update with explicit deletion Nov 16, 2023
@dplore
Copy link
Member

dplore commented Nov 16, 2023

This was reviewed in Nov 14, 2023 OC Operators meeting without objection. Added to Dec 7, 2023 OC Community meeting for broader awareness.

@hellt
Copy link
Contributor

hellt commented Nov 17, 2023 via email

@dplore
Copy link
Member

dplore commented Nov 17, 2023

Thanks Darren, upon further review we noticed that rollback reset functionality is not available. Here is the review comment I left for this #196 (review)

I think this comment is misplaced? This PR (#201) is about gNMI Update and Notification messages.

@hellt
Copy link
Contributor

hellt commented Nov 17, 2023

pardon, replied to a wrong thread indeed, @dplore

@dplore dplore requested a review from robshakir December 7, 2023 03:42
@ashu-ciena
Copy link

I posted on question on openconfig google group on what should be the expectation from a spec perspective for a delete notification.
Should the deletion notification be sent on the deletion of a config leaf/list-instance delete even if user subscribes to the state node of that object instance, something like this - /interfaces/interface[name=intf1]/state/counters ?
AND
If the subscription is on a leaf node under /interfaces/interface[name=intf1]/state/counters, then should the delete notification be sent for /interfaces/interface[name=intf1]/state/counters or the subscribed leaf XPath, knowing the fact that even other leaf nodes would cease to exist after the deletion of "intf1" ?

Assume here that interface is not limited to a physically present port on the router and can be a logical construct representing some L2/L3 service endpoint.

@dplore
Copy link
Member

dplore commented Jan 17, 2024

When a node in the tree is removed, a DELETE must be sent. This includes /interfaces/interface/state. It is ok to DELETE a parent node and expect clients to recursively remove all the children nodes. See gnmi reference 3.4.6

In practice there are scenarios where physical interfaces can be persistent or ephemeral. For example, a persistent interface could be a fixed 100GE port. A configuration for an IP address could be added or removed from this interface, but the interface will still exist as state.

An ephemeral interface could be a breakout port. Take a 400GE Ethernet which has a breakout capable transceiver inserted. When a breakout is configured, new interfaces are created, including their state containers. If the breakout deleted, the breakout physical interfaces will no longer exist and the relevant state containers should be deleted. It would be ok to delete the /interfaces/interface[name='breakout1'] container for the breakouts that were deleted.

I would expect logical interfaces to be treated the same way as the breakout port example above.

@ejbrever
Copy link

I had two other suggestions for notes that would be good to add here:

  1. For timing of the delete message, I think it is fine for it to be asynchronous during a SAMPLE subscription, but it likely does not matter too much as long as the delete is sent. However, I can imagine there be a preference to send it asynchronously as this might alleviate the need for a caching mechanism to store a delete until the next SAMPLE interval.
  2. In certain events (i.e. failure or disable power to component), specific leaves likely need to be deleted. As an example, if you consider an interface that measures optical power levels. If the system is unable to read optical power levels off of the hardware during an event, we have an interesting decision to make on the optical power level leaf. One option is not report anything further, however, this likely means you have a stale value that can be misinterpreted. Another option is to report a default value (i.e. -40dBm), but that would make an assumption of the value which seems problematic as well. In that case, perhaps the right thing to do is send a delete for that leaf?

@gwizdms
Copy link
Contributor Author

gwizdms commented Feb 5, 2024

Changed wording for required in ON-CHANGE subscription mode and optional for SAMPLE subscription mode after OpenConfig Community meeting discussions.

@DanG100
Copy link

DanG100 commented Mar 25, 2024

Can we clarify the behavior for TARGET_DEFINED streams?

When a client makes a Subscribe request with mode TARGET_DEFINED, it doesn't know if server is going to reply with ON_CHANGE or SAMPLE (per leaf). The client then can't know if the absence of updates means some paths were implicitly deleted or that haven't been any changes.

@dplore
Copy link
Member

dplore commented Mar 26, 2024

Can we clarify the behavior for TARGET_DEFINED streams?

When a client makes a Subscribe request with mode TARGET_DEFINED, it doesn't know if server is going to reply with ON_CHANGE or SAMPLE (per leaf). The client then can't know if the absence of updates means some paths were implicitly deleted or that haven't been any changes.

Good idea, we should attempt to address delete behavior when TARGET_DEFINED is used.

In practice, TARGET_DEFINED behavior is expected to be "do the right thing" where rapidly changing leaf values (like counters) are sampled and infrequently changing leaves (like the state copy of config leaves) are sent ON_CHANGE. Likewise one way to define this is the DELETE behavior is also "TARGET_DEFINED". We could elaborate that a target is expected to send delete for leaves which are updated only when they change and optionally send a DELETE for leaves which are updated periodically (ie: even if they don't change).

We will touch on this topic at the March 26,2024 OC Operators meeting.

@dplore
Copy link
Member

dplore commented May 28, 2024

No objections were recorded regarding TARGET_DEFINED and explicit deletes. @gwizdms please add some text for explicitly defining what should happen with delete in TARGET_DEFINED.

Added explicit deletion verbiage for TARGET-DEFINED.
Copy link
Contributor Author

@gwizdms gwizdms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added verbiage for TARGET-DEFINED explicit deletion.

@dplore
Copy link
Member

dplore commented May 30, 2024

Thanks @gwizdms . This change is set to last call for June 11, 2024

@dplore dplore merged commit 067d53c into openconfig:master Jul 10, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants