-
Notifications
You must be signed in to change notification settings - Fork 196
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
Reset rollback duration on subsequent commit confirm operation. #161
base: master
Are you sure you want to change the base?
Conversation
// commit action creates a new commit. If a commit is on-going, server resets | ||
// the rollback duration to a new provided value or sets the default value | ||
// if one is omitted. |
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.
On the oc call at 07-12-2023 there was a comment made by @kidiyoor in support of having a separate action for the resetting of the rollback duration.
Imagine a case where a gNMI client sends a Set request with commit confirmed extension triggering the NOS to enter in the commit-confirmed phase.
The response from the NOS, though, gets lost and never reaches the client.The client assumes that the request never made the NOS as it didn't see a response, so it resends the same Set Request message causing the already ongoing commit to reset the timeout.
With a separate message such case won't be happening since the reset message would only be sent when the client knows that the commit is ongoing.
If we stick to the same CommitRequest
message as the current PR proposes for the sake of being in line with NETCONF implementation, we won't actually be vulnerable to the above explained behavior.
The initial Set Request is sent with the payload containing the intended changes, whereas the CommitConfirmed extension that is meant to reset the timeout MUST be sent without any payload. This guarantees, that the client can only reset the timeout once it knows that the commit confirmed has already been requested.
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 NETCONF definition doesn't address this use-case as this use-case is not valid/relevant in a session based CLI interaction. Hence NETCONF definition is inadequate to define the behavior when a remote automated system client fails due to error in response flow, so I think it is ok to NOT adhere to NETCONF here.
I agree that with additional validation and checks it is possible to overcome the issue. However,
If the only argument to re-use the CommitRequest
for resetting the rollback duration is to adhere with NETCONF, then I would like us to consider if introducing a new action would simplify the server/client implementation and lead to a cleaner API definition. Please point out demerits with a new action.
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.
We have no objections using a separate message.
I can refactor this PR, but first I wanted to see if we should wait for others to comment
@earies and others whos gh handles I don't know/remember...
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.
This proposal brings rollback reset functionality to the gNMI Commit Confirmed extension.
References to prior art
Any NOS compliant with RFC6241 Netconf Commit Confirmed capability supports resetting the rollback timeout on receiving new Commit Confirmed rpc:
A quote from the linked chapter:
/cc @dplore @kidiyoor