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.

Sign up for GitHub

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

Jump to bottom

Reset rollback duration on subsequent commit confirm operation. #161

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Reset rollback duration on subsequent commit confirm operation. #161

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion proto/gnmi_ext/gnmi_ext.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,9 @@ message Commit {
// Required.
string id = 1;
oneof action {
// commit action creates a new commit. If a commit is on-going, server returns error.
// 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.
Comment on lines +109 to +111
Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kidiyoor @dplore due to missing feedback from the community members outside of the OC community call, I went with creating the alternative PR #164 with an explicit Set Rollback Duration action.

This seems to be more in line with @kidiyoor view, so maybe we can ship #164 instead.

CommitRequest commit = 2;
// confirm action will confirm an on-going commit, the ID provided during confirm
// should match the on-going commit ID.
Expand Down