Re: [Netconf] Comments on draft-lhotka-netconf-restconf-transactions-00

[Robert Wilton <[e-mail]>]

On 13/07/2018 15:19, Ladislav Lhotka wrote:
> Robert Wilton <[e-mail]> writes:
>
>> Hi Lada,
>>
>>
>> On 12/07/2018 18:22, Ladislav Lhotka wrote:
>>> Hi Rob,
>>>
>>> thanks for your comments, please see inline.
>>>
>>> Robert Wilton <[e-mail]> writes:
>>>
>>>> Hi Lada,
>>>>
>>>> I've had a read of this draft, and have provided some comments below.
>>>>
>>>> So, my top level comment is that I don't know whether or not RESTCONF
>>>> needs this functionality or not.  I've heard some operators state that
>>>> they think that clients can just construct an "atomic" change, and hence
>>>> don't have the need for a server side staging area.  Perhaps a good
>>>> question to ask in Montreal?
>>> I think what you mean is an analogy to git, where all changes are
>>> applied on the client's side and then new commits are pushed to the
>>> server. However, git was designed for this mode of operation - I think
>>> with RESTCONF it wouldn't be so efficient. And also, the client
>>> functionality would be probably difficult to implement in a plain
>>> browser whereas browser-based clients can be easily used with RESTCONF
>>> extended according to my draft.
>> No, I wasn't thinking that they would be separate commits, but a single
>> client commit.
>>
>> I guess it may depend on whether it is a machine constructing the
>> configuration change (in which case merging it into a single request
>> should be plausibly straight forward), or human's doing the interaction,
>> although even then I still wonder whether creating an edit buffer on the
>> client side, and then pushing that to the server as a single update
>> isn't a slightly cleaner paradigm.
> I agree that a lot can be done on the client side, but eventually the
> data has to be sent to the server, and it is possible that the target
> config datastore has changed in the mean time by another client - this
> is a conflict that has to be resolved somehow.
This is resolved at the time that any config change is merged into 
<running>.  Either the config change can be merged without errors and 
validates successfully (via <intended>), or the merge fails, or 
validation fails.  If either the merge or validate fails then <running> 
is not changed, the config change is rejected, and the client notified.

>
>> Perhaps the draft could have a background that explains some of the
>> expected usages of private candidate datastores.
> The aim is to enable transactions and concurrent R/W access of multiple
> clients. This drafts attempts to solve it on the server side, somebody
> else may want to propose a client-side solution.
Clients can already do it today, as per my previous answer.  I don't 
think that there is anything to standardize here.

>
> I think both may be potentially useful - one can have capable
> servers and restricted clients, or vice versa.
>
>>>> The rest of my comments below, apply to the proposed technical solution,
>>>> and obviously only apply if this is a needed enhancement. :-)
>>>>
>>>> 1) Generally, I definitely prefer the idea of per session staging areas
>>>> (aka private candidates) described in this draft over a shared lockable
>>>> candidate datastore.  This follows my belief that loosely coupled
>>>> concurrent systems are more robust than tightly coupled ones (e.g. with
>>>> shared locking).
>>>>
>>>> 2) I don't think that this draft needs to mention <intended> at all.
>>>> Instead, everywhere you mention <intended> then you should be saying
>>>> <running>.  I.e. your staging datastores should update <running> on a
>>>> commit operation, just like a commit of <candidate> updates <running>.
>>>> <intended> is always just updated as a side effect of a write to
>>>> <running>, and as such is a tangential consideration.
>>> The main reason for using <intended> is that the target datastore into
>>> which staging datastores are merged has to be valid at all
>>> times. <running> has somewhat fuzzy semantics both in NETCONF and under
>>> NMDA. But yes, the text also says that essentially we have <running> and
>>> <intended> being the same. NMDA explicitly permits this simplification.
>> <running> has the configuration supplied by the user before any template
>> expansion, or inactive config removal.
>> <intended> is the same configuration data, but after template expansion,
>> inactive config removal, and any other random config manipulations that
>> the server might do.
>>
>> If the device doesn't do "template expansion, inactive config removal,
>> and any other random config manipulations", then <intended> is trivially
>> the same as <running>.
>>
>> Whenever <running> is due to be changed, <intended> is also updated at
>> the same time, and validated.
>>
>> Hence <intended> is always valid, and by implication, so is <running>,
>> since you cannot make a change to <running> without also updating, and
>> validating <intended> at the exact same time.  I.e. they succeed or fail
>> together.
>>
>> I think that your <staging> datastore design works much better with NMDA
>> if you update <running> instead of <intended>.
>>
> Yes, but <running> can be writable or not, may be locked and may be
> invalid.
Yes, and that is all fine.

>
> If RESTCONF is the only protocol, then it is perhaps just a matter of
> naming, but if NETCONF is used along with RESTCONF on the same device, I
> want to avoid their interference as much as possible.
You can't.  Ultimately there are two mechanisms writing the same data, 
they need to be sympathetic to each other.

>   My idea is that
> contributions from NETCONF and RESTCONF only meet at <intended>.
Alas. I don't think that fits well with the NMDA architecture at all.  
The NMDA architecture assumes that all conventional client configuration 
operations combine at <running> rather than <intended>.  This is also 
the merge point today when both NETCONF and RESTCONF are being used.

The purpose of <intended> is as a mechanism to handle template 
expansion, inactive config, and possibly other default server config.  
It isn't meant to be another configuration merge point.

>
>>>> 3) Rather than having clients interact via {+restconf}/data, I think
>>>> that it would be much better to require NMDA and then have clients
>>>> interact via {+restconf}/ds/ietf-restconf-transactions:staging, as per
>>>> draft-ietf-netconf-nmda-restconf-04 section 3.1.  The new staging
>>>> datastore identity should also be defined in your module to inherit from
>>>> ietf-datastores:datastore identity.  I think that this probably also
>>>> more closely aligns to restful principals.
>>> Again, in RESTCONF it is unclear what the "unified" datastore really
>>> is. We wanted to make the semantics clear and explicit and, in
>>> particular, permit configuration edits only via the staging
>>> datastore. With your suggestion, it is not clear to me whether the
>>> client could also interact with {+restconf}/data.
>> The problem with {+restconf}/data is that is combines the *desired*
>> configuration with the *actual* operational state.  This combination
>> cannot always be done in a sane way if the system isn't in a steady state.
>>
>> I think that we should be trying to deprecate {+restconf}/data, I think
>> that cleaner/simpler semantics can be achieved by interacting via
>> explicit datastores.
> If this is done, then it would make sense to do what you suggest. For
> the time being, the advantage is that clients only suporting RFC 8040
> can be used with my enhancements - the commit and reset operations can
> be added separately, e.g as simple curl scripts.

>> E.g.
>> (1) If a RESTCONF client wants to make an atomic update to the
>> configuration, then it just writes to <running>.
>> (2) If a RESTCONF client wants private staged configuration then it does
>> it via <staging> and a commit to <running>.  From a system perspective
>> this is pretty much the same as (1) any way.
>> (3 ) If a shared candidate datastore is required, then a client writes
>> to <candidate> and then commits configuration to <running>.
>> (4) If <running> can be locked, then attempts by other clients to commit
>> to <running> when it is locked must fail.
> This is all very complicated, I don't want to force RESTCONF users into
> learning NETCONF first. Keep it simple, stupid.
It is not complicated, particularly if the server doesn't implement 
locking of shared candidate.

I prefer explicit behavior.

E.g. I don't think that RESTCONF auto-magically committing the contents 
of a shared <candidate> datastore makes the two protocols work together 
simpler.  More likely it was occasionally cause very surprising, and 
potentially very bad, things happening to a devices configuration (e.g. 
if the NETCONF client isn't employing locking).

>>>> 4) So, I think that the <staging> datastore itself only contains the
>>>> proposed changes (additions, modifications, and deletes) to <running>
>>>> when they are committed.  I think that clients may also want to see the
>>>> combined configuration of the current contents of <running> with the
>>>> delta held in <staging> applied.  This could be exposed either as (i) a
>>>> new RPC, (ii) as an extra query parameter or (iii) As another read-only
>>>> datastore.  A new RPC has the disadvantage that it probably wouldn't
>>>> support all the query parameters, so my instinctive preference would be
>>>> to one of the other two latter options.
>>> Do you mean to be able to see the result of a "dry run" of a commit?
>>> This would be certainly possible and, in fact, in our implementation it
>>> is pretty trivial.
>> let me ask two different question first:
>>
>> (1) If I call GET on <staging> then do I see just what I have changed
>> (and explicitly don't see anything that I haven't changed), or do I see
>> all of the base configuration with my private changes merged in?
> After you do commit or reset, your staging repository becomes
> (conceptually) an exact, private and writable copy of <intended>. If you
> do some changes, you see them along with the other config data (modulo
> NACM).  However, you don't see any changes that have been done to
> <intended> in the mean time.
OK, so I think that it is useful to be able to get/see the delta against 
the base copy, and perhaps an operation for it to sync and merge with 
the latest baseline version.  Obviously, there would need to be a 
mechanism to report merge conflicts.

>
>> (2) If the answer to Q1 is you see the base configuration + private
>> changes merged in, then is it the base configuration fixed from the
>> point in time that <staging> was initialized? Or does it float, i.e. it
>> always updates to the latest committed base configuration in running?
> In our implementation, it is the data from the point of time when
> <staging> was last initialized (after commit or reset). I think it would
> be possible to let <staging> track the changes in intended as long as
> the user doesn't start editing it.
>
>>>> 5) If private candidate datastores are being added to RESTCONF, then
>>>> should they also be added to NETCONF?  If they are added to both then I
>>>> think that they should be added in the same way, as much as possible,
>>>> perhaps both could be updated in a single draft to save repetitive
>>>> text?  In general, I like (Kent's?) idea of NETCONF WG writing a RFC
>>>> that describes all the common parts of NETCONF and RESTCONF that the
>>>> individual protocol docs can then reference rather than writing similar
>>>> or equivalent text in two places.
>>> But private candidates are already an option in NETCONF, right? One
>>> possibility would be to make it the ONLY option, because shared candidates
>>> have known problems.
>> How do you do private candidate in NETCONF?  I thought that it was only
>> shared candidate that had been standardized.
> RFC 6241 says this in sec. 8.3.1:
>
>     The candidate configuration can be shared among multiple sessions.
>     Unless a client has specific information that the candidate
>     configuration is not shared, it MUST assume that other sessions are
>     able to modify the candidate configuration at the same time.
This implies to me that NETCONF's candidate datastore is generally 
regarded as being shared, not private.

Thanks,
Rob


>
> Lada
>
>> Thanks,
>> Rob
>>
>>
>>> Thanks, Lada
>>>
>>>> But otherwise, I think that it is an interesting idea, and certainly
>>>> warrants some WG discussion.
>>>>
>>>> Thanks,
>>>> Rob
>>>>