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

[Andy Bierman <[e-mail]>]

On Sun, Jul 15, 2018 at 1:29 PM, Robert Wilton <[e-mail]> wrote:

> Hi Lada,
>
>
> On 15/07/2018 09:48, Ladislav Lhotka wrote:
>
>> On Fri, 2018-07-13 at 08:16 -0700, Andy Bierman wrote:
>>
>>> Hi,
>>>
>>> I do not think this problem should be worked on for RESTCONF.
>>> In the future, a protocol-independent solution for
>>> concurrent edit operations might be interesting.
>>>
>> Sounds like a good plan for the next two decades. :-)
>>
>> Very strongly disagree that the /restconf/data "unified" URI should be
>>> deprecated
>>> or that requiring multiple editing steps is REST-full.
>>>
>> The editing steps are exactly the same for a given user, the only catch
>> is that
>> nothing happens as a result of the editing. I don't see anything in RFC
>> 8040
>> that prevents postponing the application of configuration changes.
>>
>> BTW, I also don't like deprecating {+restconf}/data.
>>
> My opposition to {+restconf}/data is that works nicely with operational
> state.  I.e. it has the same issues as NETCONF <get> operation, which is
> why <get-data> was introduced in the NETCONF NMDA draft.
>
>
It works for the functionality that was defined before NMDA existed.
The point of the unified /restconf/data URI is that it is the same on every
server.
There is no discovery phase and 1 of N editing models.  The server hides
those details to simplify client programming.



> Defining a version of {+restconf}/data that abstracts and hides
> <candidate>, commits, updating startup is fine, and we discussed this as
> option as part of NMDA.
>
>
This is what the RFC 8040 version of /restconf/data does


Please learn to add new functionality in a way that does not break shipping
code.

Use some other URL besides /restconf/data for new functionality.


Andy


However, I still regard automatically committing the contents of
> <candidate> from a RESTCONF operation as very risky behaviour.
>
>
>>
>>> Customers like the client-side simplicity of RESTCONF.
>>> They can use simple curl commands (or library equivalent).
>>>
>> They would be able to do the same with just one extra step - the
>> commit/reset
>> operation, which can be executed via curl easily, too.
>>
>> Early revisions of draft-ietf-netconf-restconf contained statements like
>>
>>     Applications that require more complex transaction capabilities might
>>     consider NETCONF instead of RESTCONF.
>>
>> Is it what you still suggest? My motivation for writing the present draft
>> was
>> exactly to enable some of these capabilities without resorting to NETCONF.
>>
> I would like RESTCONF to be a fully viable alternative to NETCONF.
> Particularly because it have easily handle alternative encodings.
>
>
>> Operations are 1-shot and stateless.
>>>
>>> RESTCONF has no sessions, so the NETCONF session locking described in
>>> RFC 6241
>>> does not work for RESTCONF.
>>>
>> This point that Andy raised concerns me.  If RESTCONF has no sessions
> that how does a <staging> datastore work?
>
> My draft does NOT introduce locks. Actually, after the comments by Rob and
>> Juergen, I am now inclined to use <running> rather than <intended> as the
>> commit
>> target. Then, if <running> happens to be locked (from outside, e.g.
>> NETCONF),
>> then the commit operation has to be denied, but it has nothing to do with
>> sessions.
>>
> Note that in my previous comments, I wasn't saying that RESTCONF has to
> support <candidate>, or <locks>, but I was trying to explain how private
> candidate datastores could inter-operate with them if they were supported.
>
> Thanks,
> Rob
>
>
>
>> Lada
>>
>> Andy
>>>
>>>
>>>
>>>
>>> On Fri, Jul 13, 2018 at 8:02 AM, Robert Wilton
>>> <rwilton=[e-mail]> wrote:
>>>
>>>> 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
>>>>>>>>
>>>>>>>>   _______________________________________________
>>>> Netconf mailing list
>>>> [e-mail]
>>>> https://www.ietf.org/mailman/listinfo/netconf
>>>>
>>>
>>>
>