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 config subscription extension documentation #213

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

karimra
Copy link

@karimra karimra commented Jul 29, 2024

Documentation page to complement openconfig/gnmi#169

@karimra
Copy link
Author

karimra commented Aug 19, 2024

@robshakir @dplore, would you mind taking a look at this PR?

Copy link
Member

@dplore dplore left a comment

Choose a reason for hiding this comment

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

A few minor comments for you

rpc/gnmi/gnmi-config-subscriptions.md Outdated Show resolved Hide resolved
rpc/gnmi/gnmi-config-subscriptions.md Outdated Show resolved Hide resolved
rpc/gnmi/gnmi-config-subscriptions.md Outdated Show resolved Hide resolved
rpc/gnmi/gnmi-config-subscriptions.md Show resolved Hide resolved
rpc/gnmi/gnmi-config-subscriptions.md Outdated Show resolved Hide resolved
@dplore dplore self-assigned this Aug 20, 2024
Copy link
Contributor

@robshakir robshakir left a comment

Choose a reason for hiding this comment

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

Thanks for the specification doc. It's very useful to understand this further.

rpc/gnmi/gnmi-config-subscriptions.md Outdated Show resolved Hide resolved
rpc/gnmi/gnmi-config-subscriptions.md Outdated Show resolved Hide resolved
rpc/gnmi/gnmi-config-subscriptions.md Outdated Show resolved Hide resolved
rpc/gnmi/gnmi-config-subscriptions.md Outdated Show resolved Hide resolved
rpc/gnmi/gnmi-config-subscriptions.md Outdated Show resolved Hide resolved
rpc/gnmi/gnmi-config-subscriptions.md Outdated Show resolved Hide resolved
`SubscribeResponse` to indicate a commit boundary to the client.
A commit boundary marks the completion point of a specific set of
configuration changes.
It indicates that all changes within that set have been committed and
Copy link
Contributor

Choose a reason for hiding this comment

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

What does committed mean here? To me, I would interpret this as meaning that all of the values have been applied to the intended configuration -- but there seems a need to define these semantics clearly to allow for interoperable implementations.

Copy link
Author

Choose a reason for hiding this comment

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

Committed means the interface (gNMI, netconf, CLI,...) that was used to change the values returned a nil/zero error.

Copy link
Contributor

Choose a reason for hiding this comment

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

let's clarify this in the text.

Copy link
Author

Choose a reason for hiding this comment

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

added:

It indicates that all changes within that set have been committed -- meaning
the mechanism used to apply the changes reported no errors -- and
that all notifications triggered by the commit have been streamed to
the client.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like the fact that the 'commit' semantics are defined by the definition per interface -- so can we say "gNMI, NETCONF, CLI etc., returned a non-zero value" explicitly?

Copy link
Author

Choose a reason for hiding this comment

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

Done

rpc/gnmi/gnmi-config-subscriptions.md Outdated Show resolved Hide resolved
confirmed option is used.
* `server_commit_id`: An optional internal ID assigned by the target.

In the case a commit happens before the `sync_response: true` and the server
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding a second thread here -- what happens if we have two configuration changes -- i.e.:

  • client A makes a change
  • client B makes a change
  • client C has a ConfigSubscription open

It seems like, in this case, sync_response: true might be returned at any point in the lifetime of the subscription (when whatever existed in the data tree at the time client C created its subscription). We want ConfigSubscriptionSyncDone to be returned at the point that client A and client B have had their changes "committed" (let's clarify semantics here) -- this is rather independent to when sync_response: true is sent.

Another implication here is that we cannot tell the exact provenance of an update within the Subscribe stream if there are changes being made that do not block on the "completion" of a previous change.

Copy link
Author

Choose a reason for hiding this comment

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

The goal here is to delay any ConfigSubscriptionSyncDone until after sync_response: true is received.
As for overlapping updates from 2 different commits, typically config stores do not support parallel commits. Assuming updates to be streamed are added to the send buffer in sequence, I see little chance for overlapping updates.

If we want to keep the door open for concurrent commits we have to add the commit_confirmed_id and and server_commit_id to every update. This won't solve the case none of those 2 IDs exist like for a change from the CLI.

Copy link
Contributor

Choose a reason for hiding this comment

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

the config store does not need to support parallel commits for this situation to happen.

assume we take N seconds to stream all updates corresponding to a config change.

t=0 - config change 1 happens and is finished
t=1 - streaming for updates from config change 1 starts
t=Y (Y<N) - config change 2 happens and is finished
t=Y+1 - streaming for updates from config change 2 starts
t=N - streaming for updates from config change 1 ends
t=Y+N - streaming for updates from config change 2 ends

in this scenario -- it's the streaming of the updates that overlaps, not the commits.

when the client receives the ConfigSubscriptionSyncDone with the commit ID of config change 1 in it, it may already have received updates for paths that were impacted only by config change 2 -- so the semantic that this marker means "you have the state that reflects the state only after change 1" is broken.

the only way to avoid this is by saying that the config store locks until all updates are streamed (i.e., N seconds) (which seems bad for performance), or there is a strict send buffer with no coalescing possible (which I don't think is the right behaviour -- and is already broken by existing gnmi implementations).

why does coalescing matter? well, if both change 1 and change 2 change /foo/bar and the update for /foo/bar hasn't been sent, then it's acceptable for an gNMI server to coalesce the two, and only send the value for /foo/bar that corresponds to the value after the second change.

i think the semantics that you're trying to define here for ConfigSubscriptionSyncDone are problematic with any kind of concurrency. to me, it's better to say that the gNMI server gives looser semantics about what it can guarantee -- i.e., it guarantees only that it is "done" with processing the changes that are covered by a particular commit. if the client really wants to ensure that its config subscription reflects exactly that intended state, it can implement locking between changes without adding complexity at the server side.

Copy link
Author

Choose a reason for hiding this comment

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

I think this definition works: it guarantees only that it is "done" with processing the changes that are covered by a particular commit.. The streaming of changes depends on whether they were coalesced.

Copy link
Contributor

Choose a reason for hiding this comment

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

ACK -- suggest to update the text here to describe that this is the semantic meaning of receiving a "done". I might also suggest adding the example of concurrency that means that this design decision is required in an appendix for future readers.

Copy link
Author

Choose a reason for hiding this comment

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

Updated and added appendix

rpc/gnmi/gnmi-config-subscriptions.md Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants