[sysrepo-devel] Confirmed Commit Capability (RFC 6241 8.4)

[Radek Krejčí]

Hi Adam,
sorry for the late response, I knew that I forget to do something during last week :(

Dne 10.4.2018 v 23:26 ADAM WEAST napsal(a):
> Mikael, Radek thank you for your quick responses! Please see our proposal below; however, if this mailing list is not the ideal place to communicate design, then feel free to propose an alternative method.
>
> Confirmed commit proposal
>
> To avoid confusion, we'll use the term sr_commit to refer to sysrepo's commit functionality and <commit> to refer to the NETCONF's RFC functionality. A "commit context" refers to a dm_commit_context_t used during a call to rp_dt_commit.
>
> We see two main options for implementing confirmed commit functionality within sysrepo. 
> 1. Place the "confirmed" logic in sr_copy_config.
>   * This could give us the ability to perform a NETCONF confirmed <commit> with less changes to sysrepo.
>   * On the other hand, sysrepo itself would only have "confirmed copy-config" support, which could be confusing.
> 2. Place the "confirmed" logic in sr_commit.
>   * This would probably require some deeper changes.
>   * This provides "confirmed commit" capabilites at the sysrepo level in addition to NETCONF confirmed <commit> through rp_dt_copy_config's use of rp_dt_commit.
>
> Option (2) could be more useful in the sysrepo context. It appears that rp_dt_copy_config_to_running uses rp_dt_commit, so we would still gain the NETCONF <commit> functionality. The rp_dt_commit state machine also seems like a good place to include the logic. 
>
> We have done some preliminary investigations into option (2), but we would like your input. We were not sure what your vision of confirmed commit for sysrepo involved.

First, I'm not familiar with these internals. My colleague Michal, who knows this best will be back in office at the end of April and he will provide better feedback.

Personally, when I was thinking about confirmed commit, it was much more NETCONF feature to me. The idea was to keep just basic operations in sysrepo and implement more complicated NETCONF features in Netopeer server. That's why e.g. filtering on <get> or <get-config> is done in Netopeer server and not in sysrepo. So actually I see option (3) - implement confirmed commit in Netopeer server using the current sysrepo API. But I didd not investigate the option more deeply, so I'm not sure if all the confirmed commit functionality is implementable there. Of course, drawback of this option is that the feature is available only via NETCONF, not for the applications directly connected via sysrepo. But I'm not sure if it is needed. What is your use case?

Anyway, here is some feedback to the proposed solution

> Proposed interface changes:
> * sr_commit - add 3 arguments
>   * bool - is_confirmed - true if this is a confirmed commit.
>   * int - timeout - seconds of timeout before the confirmed commit is reverted. Does nothing if this is not a confirmed commit.
>   * char* - persist_id - value of the <persist> or <persist-id> tag in a <commit> message. Can be NULL. Does nothing if this is not a confirmed commit.
> * sr_copy_config would also receive the same three arguments, but only to pass the values down to the rp_dt_commit call.
> * sr_cancel_commit - new function to cancel current commit
>   * char* - persist_id - value of <persist-id>. Can be NULL. Does nothing if this is not a confirmed commit. 

What about keeping sr_commit() as it is and introducing sr_confirmed_commit()? Of course, internally the part of the code will be shared, but this way we could preserve (just extend) the current API.

> Behavior
> Due to the nature of existing sysrepo commands, it makes sense that a confirmed sr_commit would operate on the current datastore session. This would mean that a user could have up to three active confirmed sr_commits at a time (on startup, running, and candidate).
>
> Our current idea is to add a few additional states to the rp_dt_commit state machine. The existing logic will be followed through the DM_COMMIT_NOTIFY_APPLY state. From that point, one of two steps could be taken.
> 1. If this is a commit that needs to be confirmed, go to DM_COMMIT_WAIT_FOR_CONFIRM
> 2. If this is a normal commit, go to DM_COMMIT_CONFIRM_COMMIT
>
> We would also expect an external source to be able to set the commit context's state and resume the commit logic, similar to how the Request Processor can set the state to DM_COMMIT_WRITE or DM_COMMIT_NOTIFY_ABORT and resume depending on the responses from the notifications.
> 1. If a confirming commit is received, the original commit context can be set to DM_COMMIT_CONFIRM_COMMIT and resumed to allow the original commit context to cleanup after itself.
> 2. If a timeout occurs or a cancel-commit equivalent command is received, the original commit context is set to DM_COMMIT_CANCEL_COMMIT and resumed.
>
> DM_COMMIT_WAIT_FOR_CONFIRM
> This state will need to ensure that the confirm context is saved correctly. Any information needed to revert the datastore will need to be saved during this time.
> In NETCONF <commit>, multiple confirmed <commit>s can be called in sequence. Any following <cancel-commit> command would need to revert all outstanding confirmed <commit>s. We believe that we can save the original commit context from the first call to sr_commit to revert all outstanding changes.
>
> DM_COMMIT_CONFIRM_COMMIT
> If there is a commit context waiting to be confirmed, take the necessary actions to clean it up because it is no longer needed. If there are no commit contexts waiting to be confirmed, do nothing.
>
> DM_COMMIT_CANCEL_COMMIT and following states
> During this state, we need to write the original configuration back to storage (if necessary) and send ABORT notifications. We might be able to reuse the DM_COMMIT_NOTIFY_ABORT stage here easily. To write the configuration back to storage, we would either reuse DM_COMMIT_WRITE or create a new, similar stage depending on the amount of change necessary.

from the first sight, it looks good to me. Anyway, I would like to discuss it also with Michal.

> Data Structures
> We are still becoming familiar with the different data structures used throughout sysrepo. We are interested in the commit context's prev_data_trees property. If we can hold onto this structure, it appears that we can restore the original datastore values relatively easily.
> We see that commit contexts are saved into the data manager context's commit_ctxs tree, but this does not appear to be persistent. Given the fact that we need to save state between multiple calls to sysrepo, and eventually between sessions for persistent confirmed <commit>s, will we need to start using the Persistence Manager to store our state, or are there other ways?

sysrepo's work with contexts is still kind of magic to me, so I'm not able to answer. Actually all that destroying and creating new contexts done by sysrepo seems to me as a main issue here and I'm not sure how much of the logic will need changes to make the confirmed commit working.

So, the summary from my side is
- I don't see any nonsense in your proposal, the main issue seems to be handling of contexts
- consider possibility to move the implementation into netopeer2-server
- I want Michal's feedback since he knows these internals the best

Radek


> Thanks,
> Adam Weast 
>
> This message has been classified Public by ADAM WEAST on Tuesday, April 10, 2018 at 4:26:57 PM.
> -----Original Message-----
> From: Mikael Abrahamsson [mailto:swmike at swm.pp.se] 
> Sent: Tuesday, April 10, 2018 1:23 AM
> To: ADAM WEAST <ADAM.WEAST at adtran.com>
> Cc: sysrepo-devel at sysrepo.org
> Subject: Re: [sysrepo-devel] Confirmed Commit Capability (RFC 6241 8.4)
>
> On Mon, 9 Apr 2018, ADAM WEAST wrote:
>
>> Since NACM has been already implemented and confirmed-commit was the 
>> next item on the TODO list, I thought it would be a good idea to ask.
> Hi, and welcome. As Radek said, confirmed-commit is a feature we all want, but currently we do not have developer resources to implement it in the near term. If you (or someone else) is interested enough in this feature and want to put in developer resources to do so, we'd very much like to collaborate and be part of the design process.
>
> We welcome more developers for all parts of the Sysrepo suite, feel free to reach out to us if anyone wants to participate more.
>