[netconf] Rob Wilton's review of draft-ietf-netconf-privcand-09

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

Hi James, Rob, WG,

Sorry that I didn't get a chance to review the changes to the private-candidate draft sooner.  Because of the length of time, I just did a clean review of the latest version of the document (draft-ietf-netconf-privcand-09) without looking at the comments that I raised previously.

I believe that this document still is not ready and I have flagged quite a few comments below, some of which I think are the same as I raised previously.  Quite a few of the ones below are editorial, but some others are more technical in nature and I think that they need more clarity and discussion to reach consensus so that they can be closed.


1. I still find the doc flow to be out of order around the introduction where the private candidate datastore is defined before it is used.  I.e., too much text is 2.3 before the Introduction in section 3.  I would propose that you keep the first sentence of section 2.3, with a forward reference to section 3.1.  Move the rest of section 2.3 to 3.1.


Sec 3.4: This rebase is conceptually described as replacing the candidate configuration datastore with the current running configuration datastore at that point in time and replaying the configuration changes from the candidate configuration datastore against it, identifying any conflicts as this process progresses.

2. Does this have to replay a list of commits in private-candidate (i.e., like a git branch rebase operation), or it is allowed to just apply the resultant combined diff from the last update/branch point?  I think that this might need to be more carefully worded since different implementations (e.g., GitHub rebase like operation or GitHub merge like operation) may end up with different behaviour and semantics.  Or possibly, they end up being the same.  I've not thought deeply about it ;-)


Sec 3.4.  A server may choose to automatically issue an update when any client updates the running configuration datastore. Triggering an update in this manner must be specifically chosen and signalled by a server using an optional parameter to the :private-candidate NETCONF capability (see Section 3.5<https://www.ietf.org/archive/id/draft-ietf-netconf-privcand-09.html#how_to_signal_privcand>)

3. The document has to more clearly indicate that servers work in two modes, e.g., manual user initiated update operations, or automatic server initiated update operations.  The server indicates which mode it is running in through the NETCONF capability exchange.  Note, I also think that it would be useful to indicate this in the Capabilities YANG model as well.


Sec 3.5.1.  If a server chooses to execute an update operation automatically when any client (not just the local client) performs an update to the running configuration datastore it must advertise this behaviour using the following optional parameter to the capability: trigger=all-updates.

4.  This could be clearer.  I would suggest writing the full capability with the optional parameter, or perhaps you have an example that illustrates this that could be referenced.


Sec 3.5.2.  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.

5. There is no capability exchange to force the client to acknowledge that the server is running in auto-update mode.  I think that the client should also have to explicit acknowledge this since it changes the API contract between the client and server.


Sec 3.5.3.  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<https://rfc-editor.org/rfc/rfc8040#section-3.3.1> of [RFC8040<https://www.ietf.org/archive/id/draft-ietf-netconf-privcand-09.html#RFC8040>] All edits are made to the client's private candidate, and the private candidate is automatically committed.

This ensures backwards compatibility with RESTCONF clients that are not aware of private candidates, because those clients will expect their changes to be committed immediately.

6. I don't think that this section is sufficiently clear about whether it is using automatic update mode, or the RESTCONF client must use explicit <update> operations.  Am I right in thinking that it must effectively operate in auto-update mode to not break existing clients?  Also, do you need to consider what the update conflict semantics will be, because if you fail the operation due to a merge conflict that would that not also break existing RESTCONF clients?


7. Sec 3.6.  I suggest putting a '|','*' or other symbol on the branch line to indicate where the edits have occurred (i.e., above the ^ symbols).  If the key was explained then you could then use these to show edits in other examples.


Sect 3.6.  As described earlier, the client MUST be aware of changes to its private candidate configuration that are different from the running configuration datastore so it can be assured that it is only committing its own modifications.

8. Does this statement hold true for the auto-update case?  Either way, I don't think that this should use RFC 2119 language (e.g., "must be made aware").



Sec 3.6.  Each private candidate is treated as a separate branch and changes made to the running configuration are not placed into a private candidate datastore except in one of the following situations:

  *   The client requests that the private candidate be refreshed using a new <update> operation
  *   <commit> is issued from the private candidate (which MUST automatically issue an <update> operation for that private candidate immediately prior to committing the configuration)
  *   An implementation chooses to perform an <update> operation after a change to the running configuration by any other client.

9. I would suggest changing this to a numbered list.  The third sentence should be reworded to something like:
"The server runs in auto-update mode and so it performs an automatic <update> operation after any change to the running configuration has been committed by any other clients."


Sec 3.6.  To provide visibility into the differences between the private candidate configuration datastore a <compare><https://www.ietf.org/archive/id/draft-ietf-netconf-privcand-09.html#RFC9144> [RFC9144<https://www.ietf.org/archive/id/draft-ietf-netconf-privcand-09.html#RFC9144>] operation MAY be performed against: ...

10. I would move this text about the compare operation into a 3.6.1 sub-section to keep it a bit more distinct.



Sec 3,.7. When this happens a conflict occurs for each node modified in the running configuration that is also modified in the private candidate configuration.

A node is considered modified if:

  *
There is a change of any value
  *
...

11.  So, for clarity, I want to check that my reading of the text is what you mean:  If a leaf started with value 10, and a private candidate changed it to 20, and it was changed to 20 in running (from another commit) then when attempting to commit the configuration in private-candidate to write the same value as is already in the running datastore this would be regarded as a conflict and an update would need to be performed?  I would have thought that many clients might regard that as a null merge and not a conflict.

 you would regard this as a conflict?


Sec 3.7.  A server MAY choose to add extra checks in addition to the list above.

12. It is unclear to me what these extra checks might be, or why a server may add them.  Can you give examples of what these additional checks might look like.


Sec 3.7.2 The mechanism by which a server marks a node as "in conflict" is outside the scope of this draft.

13. I think that this sentence can just be deleted since it is talking about internal implementation details which normally would always be out of scope.


Sec 3.7.2 The client MUST be given the opportunity to re-evaluate its intent based on the new information.

14. As per above, I don't think that this is a MUST and I think that this statement conflicts with automatic update and resolution since In that scenario the client is not informed about the conflict.


Sec  3.7.2  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).

15.  The nuances of this statement when a server is running in auto-update mode are very subtle and it needs to be spelled out more clearly, since if a server is running in auto-update mode this basically doesn't happen because the update will already have previously been performed when the other commit occurred and hence this would always be a no-op.


Sec 3.7.2 The location of the conflict(s) should be reported as a list of xpaths and values.

16. This appears to be underspecified to me.  What is the error type, tag, etc.  I think that this draft definitely needs an example of a error conflict being returned.


Sec 3.7.3 When a conflict is detected in any client-triggered activity, the client MUST be informed.

17. How is the client informed.  Is this as an error in whatever operation is being performed, or via another unspecified mechanism.  I think that more clarity is required here please.

Sec 3.7.3 Conflict resolution<https://www.ietf.org/archive/id/draft-ietf-netconf-privcand-09.html#name-conflict-resolution>
18. Is it ever possible for automatic conflict resolution (in whichever direction) to result in an invalid configuration?  E.g., there a dependency between two leaves and one is modified and the other is not?  How is this handled?  Would this just end up being a config error at commit time (or if the validate command was run on the private candidate datastore)?


Sec 3.7.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.

19. This MUST statement doesn't make sense for a server that performed automatic conflict resolution.  I don't think that you should have this constraint.  Possibly the default-conflict mode should be advertised during the capabilities exchange so that the client knows what it is.


Sec 3.7.3.3 Revert-on-conflict

This resolution method MUST be supported by a server.

20.  This doesn't make sense for servers operating in an automatic update mode, or makes it harder to use.  Another solution here could be to allow or force the default update mode for the session to be expressed and determined during NETCONF capability exchange, although a RESTCONF solution would also be needed.  I think that part of the issue here is that you mix the description of the resolution with the assumption that the resolution is performed manually rather than automatically.


Section 3.7.3 A conflict is detected, the update fails with an <rpc-error> and no merges/overwrite operations happen.

21.  I think that it would be helpful to give the example of what the returned rpc-error may look like.

Section 3.7.4. System resolution mode and advertisement of this mode<https://www.ietf.org/archive/id/draft-ietf-netconf-privcand-09.html#name-system-resolution-mode-and->
22.  This sort of resolves some of concerns about issue 19 and 20, but makes the document internally inconsistent.  These inconsistencies need to be resolved, which would be helped by making the distinction between the manual updates and automatically updates more clear in the various sections.  E.g., a cleaned split between (i) this is the semantics, (ii) this is how it works with manual updates, (iii) this is how it works with automatic updates.  I also think that there is needs to be a single section that covers all of the capabilities exchange/options rather it is being split through the document.

3.8.1.1. <https://www.ietf.org/archive/id/draft-ietf-netconf-privcand-09.html#section-3.8.1.1> <update><https://www.ietf.org/archive/id/draft-ietf-netconf-privcand-09.html#name-update>

The <update> operation MUST be implicitly triggered by a specific NETCONF session issuing a <commit> operation when using private candidates. This implicit <update> operation always has a resolution mode of revert-on-conflict. The actual order of operations in the server MUST be to issue the implicit <update> operation first and then the <commit> operation.

23. Same issue/comment as per 15 applies here.  I also think that it is better to try and only have the RFC 2119 MUST text in one place when referring to the same thing.


8.3.2.1.1 Nothing in this document alters the behaviour of the <confirmed>, <persist> or <persist-id> parameters and these MUST work when using the a private candidate configuration if the :confirmed-commit capability is advertised.

24. I would reword since you are changing the behaviour of these RPCs to work with private-candidate datastores, so their behaviour is being changed.  Also, if nothing is changed, then I'm not sure why you need or want those next 3 paragraphs, can't you just rely on the definition in the other RFCs?


3.8.2.5. <https://www.ietf.org/archive/id/draft-ietf-netconf-privcand-09.html#section-3.8.2.5> <get-data><https://www.ietf.org/archive/id/draft-ietf-netconf-privcand-09.html#name-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.

25. I would expand the text in section 3.2., to also cover this as an example because it may be non-obvious.


26. IANA section, I think that this is incomplete on what needs to be specified for modules.  I would check other recent RFCs and follow one of those as an example.

Sec  A.1, A.2, YANG Modules
27. (i) I think that you should delete the private-candidate feature.  Advertising the module is sufficient.
      (ii) Reformat descriptions to use 69 characters width.
      (iii) Add a note to RFC editor to replace reference to the draft with the RFC on publication.
      (iv) Delete duplicate revision statements, just keep the latest.
      (v) I would also suggest adding YANG Semver statements (e.g., 0.1.0), and ask RFC editor to update them to 1.0.0 on publication.

28. Is the comparison the creation point actually needed?  I suspect that we would never support that option and would probably prefer that it is removed.
29. The last-update operation ties to the update or commit RPCs, but I think that it is whenever the update occurs, even if not by an RPC, e.g., due to an auto-update.

Kind regards,
Rob