[netconf] Re: WGLC for privcand-06

["Rob Wilton (rwilton)" <[e-mail]>]

Hi all,

James, Rob, thank you both for your work on this document, and sorry that it has taken a while for the WG LC review (but there are a lot of them happening at the moment). I think that given that many implementations support (or even default to) private candidate semantics, then I think that this work is important and good … and ideally I would like to get rid of shared candidate support altogether over time.

However, I don’t think that this document is quite ready.  I think that I am fine with the core concepts, and I think that the solution is on the right track, but I think this document needs tightening of the specification in places and additional clarity, and hence in my review I have lots of comments – sorry!

For me, I think that the biggest concern is that the “auto-update” mode feels like it somewhat tacked on the side, but from a client perspective, it is really important to know whether a server is running in manual mode or automatic mode, since that will change how they interact with the server.  Hence, I think that the document needs to more explicitly and clearly identify and pull out these two different modes and give some clear guidance to clients about how interactions will differ.


Moderate level comments:



(1) p 0, sec



                       NETCONF Private Candidates

                     draft-ietf-netconf-privcand-06



I think that it would be helpful to have Datastore in the draft title.



This document generally refers to "private candidates" rather than "private candidate datastore" because we no longer give it a separate private-candidate datastore name, and instead, reuse candidate.



However, I think the document was be clearer to refer to it as private candidate datastores, but then explain that the datastore reuses the "candidate" datastore name to aid backwards compatibility and simplify the APIs, but gives different, private, semantics to a session specific candidate datastores.





(2) p 0, sec



                       NETCONF Private Candidates

                     draft-ietf-netconf-privcand-06



Does this draft need to have any text to describe how the NETCONF validate operation works on a private candidate datastore?





(3) p 3, sec 2.1.  Session specific datastore



   A session specific datastore is a configuration datastore that,

   unlike the candidate and running configuration datastores which have

   only one per system, is bound to the specific NETCONF session.



Probably be helpful to include a reference to RFC 8342 for candidate and running configuration datastores.  I note that you don't use the <> notation used in other documents to reference datastores, but that is reasonable here because they are being used for RPC operations instead.  However, it probably means that you should always refer to the them a XXX datastore rather than just XXX.  I.e., "running datastore" rather than "running".





(4) p 4, sec 3.  Limitations using the shared candidate configuration for multiple

    clients



In terms of document structure, I think that this section would flow better as a subsection of the introduction (e.g., either as a section 1.4 or even inserted before the existing 1.2 section).  I.e., the document flow feels a bit strange that it is talking about definitions and behaviour for the solution, and then is going back to the problem definition as to why it was defined in the first place.



I would also suggest that the Definitions and Terminology just give very brief definitions, and the more detailed descriptions move into the "core" part of the document, i.e., from Chapter 4 onwards.  The document also covers common concepts and NETCONF and RESTCONF specific aspects.  I would suggest that putting NETCONF and RESTCONF specific details each into their own subsections (even if they stay in the same sections of the document) would make the document easier/cleaner to see what applies to which protocols.





(5) p 10, sec 4.5.  Interaction between running and private-candidate(s)



   *  An implementation chooses to perform an <update> operation after a

      change to the running configuration by any other client.



What about the auto-update operation?  I think that would also end up updating the private-candidate datastore.





(6) p 10, sec 4.5.  Interaction between running and private-candidate(s)



   An implementation may choose, optionally, to automatically perform an

   <update> operation automatically on each private candidate

   configuration datastore after a change to the running configuration

   from another client.  However, this choice should be made with

   caution as it will replace, overwrite, or otherwise alter (depending

   on the servers system resolution mode, discussed later) the private

   candidate configuration without notifying the client.



A client needs to know whether a server is going to be doing auto-update or not since this impacts how clients must interact with the server.  Probably there needs be a capability to advertise this.  I also think that you should drop the second sentence of this paragraph, or otherwise, make it more balanced, and list the downsides of doing the explicit branch management (e.g., Jan's previously indicated that NSO supports both modes and 90%+ of customers choose the auto-update mode ...)





(7) p 12, sec 4.6.2.  Detecting and reporting conflicts



   *  A <commit> operation (that MUST trigger an automatic <update>

      operation immediately before) MUST fail irrespective of any

      system-wide default resolution-mode.  It MUST inform the client of

      the conflict and SHOULD detail the location of the conflict(s).



I agree that an update MUST be performed, but I don't agree that the commit operation MUST fail if the system is set up to automatically resolve conflicts.  This doesn't make sense to me.



There is also no data model that allows a client to determine what, if any, automatic conflict resolution mode is in effect.  I think that this meeds to be added.



The update RPC has no out parameters and hence does not allow a list of conflicts to be reported, and hence I think that this aspect of the draft is under specified (due to the "MUST inform the client of the conflict").



Also need to consider semantics on automatic update before a commit vs after a commit.  I.e., a given client probably wouldn't be able to know if this has happened and hence needs the same semantics either way.





(8) p 12, sec 4.6.2.  Detecting and reporting conflicts



   The location of the conflict(s) should be reported as a list of

   xpaths and values.



As per above.  I think that this, as it stands, is under specified, and further text/examples are needed to ensure consistency in implementations.





(9) p 18, sec 4.6.3.3.  Revert-on-conflict



   This resolution method MUST be the default resolution method as it

   provides for the highest level of visibility and control to ensure

   operational stability.



I'm confused here, and I'm not sure that I agree, and I don't think that you need this text.  The "update" RPC has this as the default semantics which is fine.





(10) p 25, sec 5.  IANA Considerations



   *  urn:ietf:params:netconf:capability:private-candidate:1.0 (Version

      1.0)



The IANA considerations section needs to also register the YANG model and its associated namespace.







Minor level comments:



(11) p 2, sec 1.  Introduction



   NETCONF [RFC6241] and RESTCONF [RFC8040] both provide a mechanism for

   one or more clients to make configuration changes to a device running

   as a NETCONF/RESTCONF server.  Each client has the ability to make

   one or more configuration changes to the server's shared candidate

   configuration.



Maybe the introduction should take one step further back and briefly describe what candidate configuration is?





(12) p 3, sec 1.3.  Updates to RFC 9144



   This document updates [RFC9144] to augment the <compare> operation to

   describe how it works with the private candidate datastore.  These

   updates are described in Section 4.7.2.7.



This document should indicate that it is updating RFC 8040.  Further details later in my comments.





(13) p 3, sec 2.2.  Shared candidate configuration



   The candidate configuration datastore defined in [RFC6241] is

   referenced as the shared candidate configuration in this document.



I think that it would be much clearer to refer to it as "shared candidate configuration datastore"





(14) p 3, sec 2.3.  Private candidate configuration



   A private candidate configuration is a session specific candidate

   configuration datastore.



Again, I think that you should call it "private candidate configuration datastore", dropping the datastore noun makes it a more jarring read, at least to me.





(15) p 4, sec 2.3.  Private candidate configuration



   Obtaining this private candidate over NETCONF or RESTCONF will

   display the entire configuration, including all changes made to it.

   Performing a <commit> operation will merge the changes from the

   private candidate into the running configuration (the same as a merge

   in source code management systems).  This merge is performed in two

   steps and is described further in Section 4.7.2.1.  A <discard-

   changes> operation will revert the private candidate to the branch's

   initial state or it's state at the last <update> (whichever is most

   recent).



As per previous comments this paragraph seems to mix comments related to NETCONF and RESTCONF and then continue with operations that are NETCONF specific.  I.e., presumably the discard-changes operation (at least) is NETCONF specific, presuming that you don't also intend to support that RPC for RESTCONF.





(16) p 4, sec 2.3.  Private candidate configuration



   All changes made to this private candidate configuration are held

   separately from any other candidate configuration changes, whether

   made by other users to the shared candidate or any other private

   candidate, and are not visible to or accessible by anyone else.



Rather than "by anyone else", perhaps "by any other configuration session"?





(17) p 4, sec 3.1.  Issues



I would merge 3.1 and 3.1.1 under a "Issue:  Unintended deployment of alternate users configuration changes" section.





(18) p 5, sec 3.2.  Current mitigation strategies



Rather than "Current mitigation strategies" perhaps "Mitigation strategies when using a shared candidate datastore"





(19) p 5, sec 3.2.3.  Fine-grained locking



   Partial locking additionally requires that the client is aware of any

   interdependencies within the servers YANG models in order to lock all

   parts of the tree.



I'm also not sure how widely implemented RFC5717 is.





(20) p 6, sec 4.1.  What is a private candidate



   A private candidate is defined earlier in the definitions and

   terminology section of this document.



As mentioned previously, it would be better to put the full definition of the private candidate datastore here, and only have a one/two line description in the terminology, which could then forward reference to this section.  That way all the core explanations are in one place.





(21) p 6, sec 4.2.  When is a private candidate created



             +----------------------------> private candidate

            /

           /

   +------+-------------------------------> running configuration

          ^

        Private candidate created



I think that we need to be somewhat careful with comparing this to a source code repository.  Since, e.g., git, currently the most prevalent source control system doesn't actually work this way.  In git, a branch isn't actually a copy of the repository contents at the point that the branch was created from, but instead, it is just a pointer into a sequence of changes.  So I suggest perhaps loosening the text to "This is somewhat analogous to creating a branch in a source code repository" may be more correct.





(22) p 7, sec 4.3.  When is a private candidate destroyed



   A private candidate is valid for the duration of the NETCONF session,

   or the duration of the RESTCONF request.  Issuing a <commit>

   operation will not close the private candidate but will issue an

   implicit <update> operation resyncing changes from the running

   configuration.  More details on this later in this document.



running configuration -> running configuration datastore.





(23) p 7, sec 4.4.1.  Server



   As support for the shared candidate configuration datastore is a pre-

   requisite for the private candidate functionlity, a server MUST also

   advertise the :candidate capability as defined in [RFC6241].



Why is it necessary to also support shared candidate capability?  I.e., isn't the ultimate goal here to get rid of shared candidate.  I think that it would be good if there was a mechanism for a server to be able to indicate that it supports private-candidate and not shared candidate.  Nit: "functionality"





(24) p 7, sec 4.4.2.  NETCONF client



   In order for the use of private candidates to be established using

   this approach both the NETCONF server and the NETCONF client MUST

   advertise this capability.



If requiring shared candidate is really needed then this text may be better to be more explicit that both the private-candidate and candidate capabilities are both exposed.





(25) p 8, sec 4.4.2.  NETCONF client



   *  Close the session as the requested functionality cannot be

      provided.



I think that we should only choose one of these, and the NETCONF RFC capabilities exchange mandates the first behaviour from the capability.  I.e., you only get private-candidate behaviour if both client and server choose it.  Hence, it would be up to the client to close the session if the client requires it and the server doesn't support it.  A future version of NETCONF could handle this differently.





(26) p 8, sec 4.4.2.  NETCONF client



   All RPC requests that target or impact the shared candidate

   configuration datastore will implicitly target or impact the

   session's private candidate configuration datastore instead, and

   operate in exactly the same way.



I think that the first part of this sentence needs to be reworded to make it more clear, since the RPC isn't actually impacting the shared candidate configuration datastore.  Perhaps "All RPC requests that notionally target or impact ...", or "All RPCs that are defined to target ..."





(27) p 8, sec 4.4.3.  RESTCONF client



   RESTCONF does not provide a mechanism for the client to advertise a

   capability.  Therefore when a RESTCONF server advertises the

   :private-candidate capability, all client requests will use a private

   candidate when the client references the "{+restconf}/data" resource

   described in Section 3.3.1 of [RFC8040].  All edits are made to the

   client's private candidate, and the private candidate is

   automatically committed.



I think that this is the right solution, but probably needs some more explanation:



- There should be justification as to why it is okay to change client behaviour without the client being able to opt-in, to which the answer is that it is just a better solution, in that RFC 8040 RESTCONF only auto commits the shared candidate because it has no other option.



- This document (in chapter 1) needs to update RFC 8040 section 1.4 (I think), to indicate the change in behaviour.



- This document should mention RFC 8527, but say that it isn't impacted because it doesn't expose a candidate datastore at all.





(28) p 9, sec 4.5.  Interaction between running and private-candidate(s)



   As described earlier, the client MUST be aware of changes to it's

   private candidate configuration so it can be assured that it is only

   committing its own modifications.  It should also be aware of any

   changes to the current running configuration.



I think that we need to be careful with the language here.  I think that the MUST statement here conflicts with the auto-update mode described below.





(29) p 10, sec 4.6.1.  What is a conflict?



   A conflict is when the intent of the client may have been different

   had it had a different starting point.  In configuration terms, a

   conflict occurs when the same set of nodes in a configuration being

   altered by one user are changed between the start of the

   configuration preparation (the first <edit-config>/<edit-data>

   operation) and the conclusion of this configuration activity

   (terminated by a <commit> operation).



It isn't necessarily first edit operation since you create the private candidate from a <get-config>/<get-data> operation against candidate, and hence that could be the starting impact point.





(30) p 10, sec 4.6.1.  What is a conflict?



   When this happens a conflict occurs for each node modified in the

   running configuration that is also modified in the private candidate

   configuration.



I find this text somewhat unclear, and I'm not sure if it is right.



My reading of this description is that if a config leaf started with value A, and then it was changed to value B both in running and also changed to B in private candidate then that would fail at commit time as a conflict.



Similarly, if an interface was removed from a private candidate session at the same time as being removed from running then this would be reported as a conflict.  I.e., acting like a delete operation rather than a remove, and differing from the semantics if running was being edited directly.



Hence, I think that you may want to allow changes where the final state for a given data node ends up being the same in both running and private candidate.



I also think that you need to have some text that takes into account <update> operations since this would presumable reset the point at which you are doing the comparison to see if it modified rather than when private candidate was first created.





(31) p 11, sec 4.6.1.  What is a conflict?



   *  There is a change to the order of any items in a leaf-list

      configured as "ordered-by user"



This could be merged with the "list" one, as "order of any list or leaf-list items".





(32) p 11, sec 4.6.1.  What is a conflict?



   *  There is a change of existance (or otherwise) of a leaf with an

      Empty type definition



Shouldn't this be change of existence or value of any leaf? Which would then mean that it could be merged with the first entry in your list and would mean that you don't need this one.  Also note s/existance/existence/.





(33) p 11, sec 4.6.1.  What is a conflict?



   *  An interface has been deleted in the running configuration that

      existed when the private candidate was created.  A change to a

      child node of this specific interface is made in the private

      candidate using the default merge operation would, instead of

      changing the child node, both recreate the interface and then set

      the child node.



You mention the merge operation here, but I assume that the NETCONF operations only affect how the private candidate datastore is updated, not any subsequent conflict/detection or resolution.  Does my understanding align with the authors?  If so, merging the netconf operation here, may cause confusion.





(34) p 12, sec 4.6.2.  Detecting and reporting conflicts



   *  Manually triggered by the <update> NETCONF operation

   *  Automatically triggered by the server running an <update>

      operation, such as when a <commit> operation is performed by the

      client in the private candidate session.



This should also include a server that automatically performs an update on each commit to running.





(35) p 12, sec 4.6.2.  Detecting and reporting conflicts



   When a conflict is identified that node is marked by the server as

   "in conflict" in the private candidate.  This "in conflict" status

   does not propagate back up the tree to the parent node(s).  Each node

   in the ancestral tree is evaluated as in conflict or otherwise on its

   own merits.  The "in conflict" marker remains until the conflict is

   resolved on that node.



Perhaps "internally marked by the server", so that this text doesn't imply that the client can see or directly query the marked node (e.g., though a YANG metadata extension).





(36) p 12, sec 4.6.2.  Detecting and reporting conflicts



   *  An <update> operation triggered by a client (excluding updates

      triggered by the <commit> operation) MUST fail unless the server

      has explicitly configured a system-wide resolution-mode of prefer-

      candidate or prefer-running (discussed later in this document).



server has explicitly configured ... do you mean the client has explicitly configured?  Why does this config need to be explicit?





(37) p 12, sec 4.6.2.  Detecting and reporting conflicts



   Note: If a server implementation has chosen to automatically issue an

   <update> operation every time a change is made to the running

   configuration, the server will use the system-wide system resolution

   mode.  If this resolution mode is prefer-candidate or prefer-running

   the conflicts will be resolved using those rules.  If the resolution

   mode is set to revert-on-conflict the semantics are the same as the

   prefer-candidate method, however, all changes, whether in conflict or

   otherwise will be marked in the private candidate as "in-conflict".

   This means that any subsequent <commit> will fail until the client

   makes a conscious choice to resolve them.



Note, I presume that means that once marked as being 'in-conflict' that means that even if the configuration was changed in running such that it was no longer in conflict it is still required for the client to explicitly resolve it?  E.g., an alternative approach would be during any update operation to also check and clear "in-conflicit" flags if they are no longer in conflict.





(38) p 18, sec 4.6.3.3.  Revert-on-conflict



   This resolution method MUST be supported by a server.



I think that this text would be better in the supported resolution mode section.





(39) p 20, sec 4.6.4.  System resolution mode and advertisement of this mode



   The system resolution mode is revert-on-conflict.  However, a system

   MAY choose to select a different system resolution mode.



This paragraph doesn't make sense to me.





(40) p 20, sec 4.6.4.  System resolution mode and advertisement of this mode



   urn:ietf:params:netconf:capability:private-candidate:1.0

                 ?default-resolution-mode=prefer-running



Rather than default-resolution-mode, shouldn't this be the automatic resolution mode and it is only advertised if the server performs automatic resolution.



I'm concerned that the default server behaviour is left down to the server, and that there is no way for a client to change this (e.g., via an IETF YANG configuration model).





(41) p 20, sec 4.6.5.  Supported resolution modes



   urn:ietf:params:netconf:capability:private-candidate:1.0

                 ?supported-resolution-modes=revert-on-conflict,prefer-candidate



Really these are the supported modes for the manual update operation, and the naming of the capability would better reflect that.





(42) p 23, sec 4.7.2.5.  <get-data>



   Performing a <get-data> operation with the candidate configuration

   datastore as the datastore, the configuration from the private

   candidate will be returned.  If no private candidate configuration

   has been created, one will be created at this point.



I'm cautious that this "read" operation should change the state on the server by creating the private candidate, likewise for <compare> and <get-config>.



On one hand, the impact of this change by its very nature is restricted to that session, and for a server that implements auto-update and doesn't allow the more advanced version of compare that you describe below, then the server doesn't need to create the private candidate datastore at that point in time at all, it only needs to act as if it exists.  I.e, pragmatically, I think it can just read from <running> instead.



If you don't have the auto-update semantics then I guess that it gives the client the opportunity to get the baseline to find what its edits are relative to, i.e., if you were to treat it as a read from running instead then the start could end up being different than was is expected.





(43) p 23, sec 4.7.2.7.1.  <reference-point> parameter



   This document adds the optional <reference-point> node to the input

   of the <compare> operation that accepts the following values:



I'm not convinced that this additional complexity of adding this extra parameter to the compare operation is really needed.  Do we know if anyone has asked for this?  Otherwise perhaps it would be better to just keep the basic compare operation that only compares against datastores rather than increasing the complexity of the compare operation and also increasing the complexity of what servers must store/maintain if they support this option.  If there is not a clear demand then RFC9144 could be bis-ed in future to add in the extra option.





(44) p 27, sec Appendix A.  YANG modules



       feature private-candidate {

           description

               "NETCONF :private-candidate capability;

               If the server advertises the :private-candidate

               capability for a session, then this feature must

               also be enabled for that session.  Otherwise,

               this feature must not be enabled.";

           reference

               "draft-ietf-netconf-privcand";

       }



I think this feature serves no purpose, since just advertising support for this module would be sufficient to indicate that the update rpc is supported.



Also, you cannot enable features for a session, or at least, I don't think the IETF standards allow for this because it would require a different version of YANG library to be published per session (even if some vendor implementations do).  I think that it is much safer to assume that a given device only has one set of features that are in effect at a given point in time.





(45) p 28, sec Appendix A.  YANG modules



       augment /cmp:compare/cmp:input {

           leaf reference-point {

               reference "draft-ietf-netconf-privcand";

               if-feature "private-candidate";

               description

                   "When this leaf is provided and the source or

                   destination is the candidate datastore, operating

                   in private candidate mode, the comparison will

                   either occur between the last-update point of

                   the private candidate or the creation-point of

                   the private candidate.";

               default last-update;

               type enumeration {

                   enum last-update;

                   enum creation-point;

               }

           }

       }



As per my previous comments, I'm not sure that that the compare operation should be extended in this way, or at least not in this document.





(46) p 28, sec Appendix A.  YANG modules



   The authors would like to thank Andy Bierman, Jan Lindblad, Lori-Ann

   McGrath, Dylan Sadoun, Jason Sterne, Kent Watsen and Rob Wilton for

   their contributions and reviews.



As a minor comment, I was surprised to get a request for an IPR declaration on this draft because the chairs thought that I was a contributor, which I think may be because of the title of this section.  It might be better to just call this section Acknowledgements, and perhaps change "their contributions and reviews" to "their reviews and feedback".







Nit level comments:



(47) p 9, sec 4.5.  Interaction between running and private-candidate(s)



   As described earlier, the client MUST be aware of changes to it's

   private candidate configuration so it can be assured that it is only

   committing its own modifications.  It should also be aware of any

   changes to the current running configuration.



s/it's/its/





(48) p 11, sec 4.6.1.  What is a conflict?



   A server MAY choose to add additional checks over and above the above

   list.



Suggest: "additional checks in addition to the list above".





(49) p 21, sec 4.7.2.1.  <commit>



   The behaviour of the <commit> operation is updated such that the

   issuing a <commit> MUST follow a two step process in order to copy

   the proposed private configuration into the running configuration.

   These steps are:



Probably "that issuing a <commit> operation" or "the issuing of a <commit> operation".





(50) p 21, sec 4.7.2.1.  <commit>



   1.  An implicit update operation MUST be performed by the server

       using the resolution-mode revert-on-conflict (described later in

       this document).



Nit, the resolution is described earlier in the document rather than later.





(51) p 23, sec 4.7.2.5.  <get-data>



   Performing a <get-data> operation with the candidate configuration

   datastore as the datastore, the configuration from the private

   candidate will be returned.  If no private candidate configuration

   has been created, one will be created at this point.



I suggest: "... datastore as the datastore input parameter" will instead act on the private candidate datastore, and return data from that datastore.

Kind regards,
Rob


From: Kent Watsen <[e-mail]>
Date: Friday, 21 February 2025 at 17:02
To: [e-mail] <[e-mail]>
Subject: [netconf] Re: WGLC for privcand-06

No comments have been received in two weeks.

The chairs are extending the WGLC for another two weeks.  Now ending Mar 7.

Kent // as co-chair



> On Feb 7, 2025, at 1:41?PM, Kent Watsen <[e-mail]> wrote:
>
> This email begins a two-week WGLC on:
>
>        NETCONF Private Candidates
>        https://datatracker.ietf.org/doc/draft-ietf-netconf-privcand/06/
>
> Please take time to review this draft and post comments by February 21.  Favorable comments are especially welcomed.
>
> FWIW, none of the authors or contributors declared IPR:
>
>        https://mailarchive.ietf.org/arch/msg/netconf/YwHX_dcztrH_vxhKzrgcNf1AjXA/
>
>
> Kent  // co-chair
>
>
>
>
>
>
> _______________________________________________
> netconf mailing list -- [e-mail]
> To unsubscribe send an email to [e-mail]

_______________________________________________
netconf mailing list -- [e-mail]
To unsubscribe send an email to [e-mail]