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

[Andy Bierman <[e-mail]>]

On Sun, Jul 15, 2018 at 6:48 AM, Ladislav Lhotka <[e-mail]> 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. :-)
>
>
Your draft ignores the concurrency problem. E.G:

At time T0, config is /foo
Client1 and Client2 both start editing

At T1, Client1 merges /foo/A (config is /foo/A)

At T2, Client2 replaces /foo with /foo/B

At this point all of edits from Client1 are lost.
Imagine if git worked this way :-(

The edit from Client2 has to be rejected instead of accepted.
A clever system will allow Client2 to resync to time T1 and then try the
commit again


>
> > 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.
>
> >
> > 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.
>
>
yes because NETCONF has sessions and locking and lock management built-in to
session management


> > Operations are 1-shot and stateless.
> >
> > RESTCONF has no sessions, so the NETCONF session locking described in
> RFC 6241
> > does not work for RESTCONF.
>
> 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.
>
>
Correct. Your draft introduces concurrent candidate datastores for RESTCONF
only.
Private staging areas do not work well if simplistic "last one wins"
commits are done.
I am not sure RESTCONF needs any scratchpad datastore. This was created
to assist human CLI datastore editing in NETCONF.  RESTCONF has YANG Patch
to easily collect edits to apply all at once.

RESTCONF for NMDA allows access to the real candidate datastore.
The /restconf/operations resource allows RESTCONF access to any
YANG-defined RPC
(which includes ietf-netconf RPCs).   I don't see any problem to solve in
order
to make RESTCONF be more like NETCONF.



Lada
>
>
Andy


> >
> > 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
> >
> >
> --
> Ladislav Lhotka
> Head, CZ.NIC Labs
> PGP Key ID: 0xB8F92B08A9F76C67
>