Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
base: master
Are you sure you want to change the base?
gNMI config subscription extension documentation #213
Changes from 1 commit
f6d20ec
532ed52
ddac514
56c5312
7c754d7
f53dbd6
d0a4930
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added:
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
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.:
ConfigSubscription
openIt 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 wantConfigSubscriptionSyncDone
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 whensync_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.There was a problem hiding this comment.
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 aftersync_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 andserver_commit_id
to every update. This won't solve the case none of those 2 IDs exist like for a change from the CLI.There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated and added appendix