[netmod] Re: WGLC on system-config-05

["Jason Sterne (Nokia)" <[e-mail]>]

Thx Qiufang. Please see inline.
Jason

From: maqiufang (A) <[e-mail]>
Sent: Monday, May 13, 2024 3:47 AM
To: Jason Sterne (Nokia) <[e-mail]>; Rob Wilton (rwilton) <[e-mail]>; [e-mail]; NETMOD Group <[e-mail]>; [e-mail]
Subject: RE: [netmod] Re: WGLC on system-config-05


Hi, Jason and Rob,



Thanks you both for your valuable comments, all good points, much appreciated. Please also find my reply below inline...



-----Original Message-----
From: Jason Sterne (Nokia) [mailto:[e-mail]]
Sent: Saturday, May 11, 2024 12:29 AM
To: Rob Wilton (rwilton) <rwilton=[e-mail]<mailto:rwilton=[e-mail]>>; Kent Watsen <[e-mail]<mailto:[e-mail]>>; [e-mail]<mailto:[e-mail]>; [e-mail]<mailto:[e-mail]>
Subject: RE: [netmod] Re: WGLC on system-config-05



Please see inline for comments on the "moderate level comments".  I'll try to reply later with more feedback on further items below.

Jason



From: Rob Wilton (rwilton) <rwilton=[e-mail]<mailto:rwilton=[e-mail]>>

Sent: Thursday, May 9, 2024 9:55 AM

To: Kent Watsen <[e-mail]<mailto:[e-mail]>>; [e-mail]<mailto:[e-mail]>; [e-mail]<mailto:[e-mail]>

Subject: [netmod] Re: WGLC on system-config-05



CAUTION: This is an external email. Please be very careful when clicking links or opening attachments. See the URL nok.it/ext for additional information.



[Resending due to mailer issues.]

[Qiufang] Thanks, I see it's been accurately archived now.



Hi authors, chairs, WG,



I'm generally supportive of this work, but I think that there are still some potential corner cases that are not covered, or it isn't entirely obvious how they are handled.



Comments below.



Moderate level comments:



(1) p 7, sec 2.3.  Inactive-Until-Referenced



   There are some system configuration predefined (e.g., application



   ids, anti-x signatures, trust anchor certs, etc.) as a convenience



   for the clients, which must be referenced to be active.  The clients



   can also define their own configurations for their unique



   requirements.  Inactive-until-referenced system configuration is



   generated in <system> immediately when the device is powered on, but



   it is not active until being referenced.



I'm not sure whether Inactive-Until-Referenced actually needs to be defined, or to put it another way, I'm not sure whether this type of configuration is special to system datastores at all.  If a configuration (either explicitly in <running> or implicitly from <system>) defines a QoS policy that is not referenced from anywhere, (e.g., not applied to any interfaces) then I think that it up to the server to decide whether that unreferenced QoS policy is reported in operational or not, depending on server implementation.



[>>JTS:] I agree. I think section 2 may be mixing up two concepts:



  1.  Data dynamically being populated/removed from the system datastore, and

  2.  For data that is in the system datastore, whether it is "active" and present in the operational datastore

[Qiufang] Yes, it's actually from 2 dimensions to differentiate different kinds of system config: 1. Time of being generated; 2. Time of being applied.



For #2 I don't think there should be anything special. We could say something like: As with the running datastore, data present in the system datastore may or may not be present in the operational datastore depending on whether it is considered as active configuration or not by the server.

[Qiufang] I am personally okay to remove the third kind of definition to keep the definition dimension consistent, and maybe for system configuration being generated at both different times (immediately vs. conditional), they may be either applied by the server immediately or only after being referenced. I also think it's worth adding some text as Jason suggested, the client would benefit from the knowledge that there might be some system configuration that is defined there but not actually in use.

[>>JTS:] I think it is more than just removing the 3rd type defined in section 2. We probably need to rework to just have two types (for the one dimension):

  1.  Immediately-present
  2.  Conditionally-present

They would both related purely to presence of config in <system> (and not say anything about whether they are applied or active).



For #1 there is a bit of repeat with section 4.2. We should probably just talk about system config coming and going (and changing) in one place.

[Qiufang] I think you are referring to the QoS examples in the first paragraph of sec.4.2, right? We will move this to sec.2 in the new revision.

[>>JTS:] I wasn't just referring to the QoS example. It was the entire concept that the contents of the <system> datastore can change. But maybe it is OK if that concept is mentioned in the 2 places. Perhaps we should just refer back to section 2 from 4.2 when we mention things dynamically showing up in <system>.  I would not necessarily move your QoS example out of 4.2 - it seems useful there (as well as useful in section 2).



We'd have to update other parts of the doc (e.g. 5.1) where these 3 types of data are mentioned.

[Qiufang]Yes, I agree.



(2) p 9, sec 5.1.  Conceptual Model of Datastores



   When the device is powered on, immediately-active system



   configuration is generated in <system> and active immediately, but



   inactive-until-referenced system configuration only becomes active if



   referenced by client-defined configuration.  However, conditionally-



   active system configuration will only be created and active when



   specific conditions on system resources are met.



I think that it should be "merged with system" not "merged into system" since the running configuration never ends up in the system datastore.



[>>JTS:] Yes. Jan mentioned the same.

[Qiufang] This will be fixed in the new revision, thanks!



(3) p 9, sec 5.1.  Conceptual Model of Datastores



                  additional nodes to a list entry or new list/leaf-



   list entries appearing in <running> extends the list entry or the



   whole list/leaf-list defined in <system> if the server allows the



   list/leaf-list to be updated.



How is this achieved?  This appears to suggest that there are two different merging behaviours (one choice is to be additive, the other is to replace), and it seems to be down to the server to choose what to do on a case-by-case basis.  I think that it would be cleaner to define a single merge behaviour if that is feasible (even if it is slightly less flexible).  Also, potentially it is appropriate for the merge behaviour to be different for list vs leaf-list (e.g., always merge list entries, but do a simple replace on leaf-lists).



[>>JTS:] I think part of the complication is that we want to allow the possibility that there is a list that contains entries in <system>, and it is a completely non-modifiable list (no new entries allowed). I think maybe that's what the "if the server allows" part is about.



I agree though that it should be a merge for lists (and the server can just error if that merge makes the list invalid, i.e. no additional entries allowed on top of what's in system).



For leaf-lists that a tough one (merge vs replace). I'm not sure what to do there (and can imagine use cases for both merge and replace).

[Qiufang] I think for both leaf-list and list cases, additive should be the right answer when merging(note that client has no way to remove system config, its lifecycle is beyond client control). But then ordering is another question if it is "ordered-by user".

I am not sure this draft is right place to define how the merge should happen, merge operation is there as early as 6241. Is there any difference when it comes to <running> being merged with <system>, no? I expect the merge behavior in this document be consistent with what's defined elsewhere.

Just noticed that Kent has already raised a netconf-next issue for this: https://github.com/netconf-wg/netconf-next/issues/19.

Maybe the right thing to do is to remove any text related to the merge behavior which also related to this sentence?

[>>JTS:] You raise a good point about ordered-by user. That's going to make the merge problematic. I don't think currently existing definitions really address it for this draft. I'm doubtful we should actually define how merge of ordered-by-user lists should work for system->running. We may need to leave that undefined (system specific) or disallow it (error, or make it a replace). I'd lean towards leaving it undefined.



(4) p 9, sec 5.1.  Conceptual Model of Datastores

                                      If a server implements



   <intended>, <system> MUST be merged into <intended>.



This sentence is just repetition and can be deleted.  The text above is still normative without the RFC 2119 MUST.

[Qiufang] Yes! It is always the case that <system> is merged into <intended>. Even though the server doesn't implement an explicit <intended>, there could also be a conceptual one. Will remove it in the new revision.



(5) p 13, sec 5.4.  Modifying (Overriding) System Configuration



   For instance, descendant nodes in a system-defined list entry may be



   modifiable or not, even if some system configuration has been copied



   into <running> earlier.  If a system node is non-modifiable, then



   writing a different value for that node MUST return an error.  The



   immutability of system configuration is defined in



   [I-D.ma-netmod-immutable-flag].



I think that some care is needed here.  E.g., if the modification was being done to <candidate>, then it isn't writing a different value to <candidate> that would return an error, but instead the <validate> or <commit> operation that would fail.



[>>JTS:] Agree. Maybe we can just add this?



If a system node is non-modifiable, then writing a different value for that node MUST return an error during a validate or commit operation.

[Qiufang]"A validate operation" is easy to be confused with the <validate> RPC operation, I think you're referring to the server's validation process, not just <validate> RPC operation, right?

Or we can just state writing a different value into <running> MUST return an error. I am okay with either.

[>>JTS:] I was talking about the <validate> operation. We should probably clarify this in the text and not just keep the current sentence.



(6) p 13, sec 5.4.  Modifying (Overriding) System Configuration



   A server may also allow a client to add data nodes to a list entry in



   <system> by writing those additional nodes in <running>.  Those



   additional data nodes may not exist in <system> (i.e., an *addition*



   rather than an override).



Earlier, the text in 5.1 seems to suggest that a list-entry could be overwritten.  Is the intention that this is always a merge?  I.e., it is possible to override entries, but there is no way that running can remove a list entry that is defined in <system>.

[Qiufang] I think it is an overriding case (e.g., clients overwrites a list entry with the same key value), instead of different list/leaf-list entries being merged together.

[>>JTS:] I don't think there is a way to remove an entry in <system>.  You can configure the *same* entry (key) in <running>, and then potentially modify child nodes if the entry is modifiable.

[Qiufang] Agree.



This section, 5.4., seems somewhat of a repeat of what is specified in section 5.1, and arguably it would be nice if this text could be co-located and only specified once (for brevity and to avoid ambiguity).  I'm wondering if the merge behaviour generally needs to be specified more explicitly.

[Qiufang] will remove the duplicate text in the new revision. See comments above, I am unsure to what extent the merge behavior should be specified in this draft.



Minor level comments:



(7) p 0, sec



   This document defines how a management client and server handle YANG-



   modeled configuration data that is defined by the server itself.  The



   system-defined configuration can be referenced (e.g. leafref) by



   configuration explicitly created by a client.



Perhaps 'instantiated' by the server itself rather than 'defined' by the server.

[Qiufang] Okay, will fix this in the new revision, thanks.



(8) p 0, sec



   The Network Management Datastore Architecture (NMDA) defined in RFC



   8342 is updated with a read-only conventional configuration datastore



   called "system" to hold system-defined configuration.



Perhaps 'expose system-defined configuration' to clients rather than 'hold'

[Qiufang] Okay, will fix this in the new revision, thanks.

(9) p 0, sec

   As an alternative to clients explicitly copying referenced system-

   defined configuration into the target configuration datastore (e.g.,

   <running>) so that the datastore is valid, a "resolve-system"

   parameter is defined to allow the server acting as a "system client"

   to copy referenced system nodes automatically.  This solution enables

   clients manipulating the target configuration datastore (e.g.,

   <running>) to reference nodes defined in <system>, override system-

   provided values, and configure descendant nodes of system-defined

   configuration.

I think that this paragraph is too detailed to be in the abstract and should be removed from the abstract.
[Qiufang] Will remove this paragraph in the new revision, thanks.

(10) p 4, sec 1.1.  Terminology

   The following terms are defined in this document:

   System configuration:  Configuration that is provided by the system

      itself.  System configuration is present in the system

      configuration datastore (regardless of whether it is applied or

      referenced) and appears in <intended> unless explicitly

      overridden.  System configuration that is considered active

      appears in <operational> with origin="system".  It is a different

      and separate concept from factory default configuration defined in

      RFC 8808 (which represents a preset initial configuration that is

      used to initialize the configuration of a server).

RFC 8808 should turn into a proper reference, it looks like it is just text here.
[Qiufang] Agree, will fix this.

(11) p 5, sec 1.4.  Updates to RFC 6241 and RFC 8526

   This document defines a NETCONF protocol capability to indicate

   support for this parameter.  NETCONF server that supports "resolve-

   system" parameter MUST advertise the following capability identifier:

Are we ambiguous as to whether this must be supported, or is optional to implement?  Ah, I see that this is specified later in the document (which is arguably the right place).  Is the capability really an update to RFC 6241 and 8526?  I wonder whether this last paragraph (i.e., the capability definition) would be better under section 5.3.
[Qiufang] You're right, I'll move the capability definition to sec.5.3, but note that this is only specific to NETCONF protocol and sec.5.3 generally is intends to be protocol-independent.

(12) p 5, sec 1.5.  Updates to RFC 8040

   This document extends Sections 4.8 and 9.1.1 of [RFC8040] to add a

   new query parameter "resolve-system" and corresponding query

   parameter capability URI.

Again, I think that possibly sections 1.5.1 and 1.5.2 would be better outside of the introduction, perhaps as subsections of 5.3.  Then section 1.5, could then forward reference to those sections.
[Qiufang]Sure, and this is the part that is only specific to RESTCONF protocol.

(13) p 6, sec 2.  Kinds of System Configuration

   Active system configuration refers to system configuration that is

   currently in use.  As per definition of the operational state

   datastore in [RFC8342], if system configuration is inactive, it does

   not appear in <operational>.  However, system configuration is

   present in <system> once it is generated, regardless of whether it is

   active or not.

I'm not sure that calling this "active configuration" is a great choice, because it seems to be a slightly different concept to inactive configuration defined in RFC 8342.  Specifically, I thought that the inactive configuration in RFC 8342 controlled whether or not it would appear in <intended>, but in this case, presumably it always turns up in <intended> if it is in <system> and instead doesn't appear in <operational>?
[Qiufang] You are right, and maybe it also includes the naming of immediately-active vs. conditionally-active? I agree it would be good to make a distinction here, maybe use "applied" instead of "active"?

(15) p 7, sec 3.  The System Configuration Datastore (<system>)

   *  Management operations: The content of the datastore is set by the

      server in an implementation dependent manner.  The content can not

      be changed by management operations via protocols such as NETCONF,

      RESTCONF, but may change itself by license change, device upgrade

      and/or system-controlled resources change.  The datastore can be

      read using the standard network management protocols such as

      NETCONF and RESCTCONF.

Rather than saying that the contents can change itself, I think that it would be better to say that the server may change the contents under various conditions, such as ...
[Qiufang]Yes, it should be fixed. Thanks for pointing this out.

(16) p 7, sec 3.  The System Configuration Datastore (<system>)

   *  Origin: This document does not define any new origin identity when

      it interacts with <intended> and flows into <operational>.  The

      "system" origin Metadata Annotation [RFC7952] is used to indicate

      the origin of a data item is system, which is achieved by updating

      the definition of "intended" origin metadata annotation in

      [RFC8342].

If a different value is configured in <running> that overrides a value in <system> then it is clear that the origin should be <intended>.  Do we specify what the origin should be if the same value exists in both <running> and <system> (which could be a very common occurrence if resolve-system is used)?
[Qiufang] Yes, it is still "intended" for system config copied into <running>, being specified in sec.5.1. Do you expect it to be specified here?

(17) p 8, sec 3.  The System Configuration Datastore (<system>)

   The system datastore is defined as a conventional configuration

   datastore and shares a common datastore schema with other

   conventional datastores.

This paragraph should probably move up to "YANG modules".
[Qiufang] Sure, will move to sec.6.1.

(18) p 8, sec 4.2.  May Change via Software Upgrades or Resource Changes

   *  Servers rejects the operation to change system configuration

      (e.g., device upgrade fails) and needs the client to correct the

      configuration in <running> as a prerequisite to ensure validity

Should we add a recommendation for servers to document how they handle these issues?
[Qiufang] Okay, so how about adding the following text:
Servers are recommended to include some hints in error responses to help clients understand how <running> should be updated.

(19) p 10, sec 5.1.  Conceptual Model of Datastores

    ct = config true; cf = config false

    rw = read-write; ro = read-only

    boxes denote named datastores

In this diagram, (1) please move the system box 1 line to the left to keep is more cleanly separate from the arrow into running.
[Qiufang] That is easy to fix.
(2) I think that we should discuss whether the running and system arrows should merge at a common point rather than running flowing into the side.
[Qiufang] By "running flowing into the side", are you referring to running directly flowing into intended?
In the previous version (https://datatracker.ietf.org/doc/html/draft-ietf-netmod-system-config-04#section-5.1) we do have both running and system (equally) flowing into intended. But I feel that way we cannot emphasize running takes precedence over system, and now it is updated like, to have system as the "overlay", and running is merged into system to create intended. But I am unsure if that makes sense, e.g., to have one flow arrow points to another one. Thoughts?

(20) p 10, sec 5.1.  Conceptual Model of Datastores

    ct = config true; cf = config false

    rw = read-write; ro = read-only

    boxes denote named datastores

I know that it isn't directly related to this work, but I wonder whether the "default configuration" arrow is really in the right place, and whether that shouldn't also be feeding this arrow into <intended>, since validation would surely take default values into account.  But this is perhaps a question for another day ...
[Qiufang] I agree that validation should take default configuration into account.  Should we allow the default configuration to be present in <system>?
Jan raised a comment which is about the interplay between system config and default configuration, which might not tightly relate to your comment, I am not sure that discussion might end up with, but maybe it is worth documenting some outcome and also pointing this out?

(21) p 11, sec 5.1.  Conceptual Model of Datastores

   Any deletable system-provided configuration that is populated as part

   of <running> by the system at boot up, without being part of the

   contents of a <startup> datastore, must be defined in <factory-

   default> [RFC8808], which is used to initialize <running> when the

   device is first-time powered on or reset to its factory default

   condition.

I agree with the sentiment of what is written here, but I'm not sure that it is wise to restate it in this document, or whether it would be better to delete this paragraph and just reference RFC 8808.

E.g., maybe something like ..

<factory-default> [RFC8808] defines a mechanism for populating <running> at system boot up with regular configuration data nodes, that hence can be deleted.
[Qiufang] I think there might be some slight difference, the intent here is not to highlight contents in <factory-default> is deletable, but deletable system config must be defined in <factory-default> and outside the scope of this draft. Would it be okay for you if we remove the part starting from "which is used to initialize <running> ..."?

(22) p 12, sec 5.3.  Servers Auto-configuring Referenced System Configuration
      ("resolve-system" parameter)

   The "resolve-system" parameter is optional and has no value.  If it

   is present, and the server supports this capability, the server MUST

   copy referenced system nodes into the target datastore (e.g.,

   <running>) without the client doing the copy/paste explicitly, to

   resolve any references not resolved by the client.  The server acting

   as a "system client" like any other remote clients copies the

   referenced system-defined nodes when triggered by the "resolve-

   system" parameter.  Legacy clients interacting with servers that

   support this parameter don't see any changes in <edit- config>/<edit-

   data> and <copy-config> behaviors.

How does resolve-system interplay with the candidate configuration datastore?  E.g., should it also be listed in the examples of datastores.  What about the <validate> or <commit> operations?  Is there any impact of private-candidate datastores, and if so, where should that be documented?
[Qiufang] I assume the resolve-system parameter can be used for any r-w config datastore, including private-candidate (will list them all in the examples). Given the auto-copy happens during validation time, for candidate/private-candidate, the <edit-config> does not necessarily cause the server to perform the auto-copy, e.g., if the test-option value in <edit-config> is "set". And in another mail response to the comments from Jan, some question in mind is whether we should also augment the  <commit> and <validate> RPCs to support this parameter.
Regarding the impact of introducing this parameter to private-candidate datastores, generally, once the config is copied into private-candidate, it's no different from the configuration explicitly configured by the client, and thus IMO needs no specific handling. For example:

  1.  If <system> contains an interface named "loopback" with "enabled" value "true" which is not in <running> at t0 time;
  2.  Client1 issues an <edit-config> to create priv-candidate #1 and leafrefs the loopback interface without specifying the "resolve-system" at t1 time;
  3.  Client2 issues an <edit-config> to set the loopback interface "enable" value to "false" towards <running> at t2;
  4.  Client1 issues the <commit> (assume "resolve-system" is carried) which will automatically issue the <update> as per priv-candidate draft, and thus what is set in <running> (step3) will be synced into priv-candidate #1, no extra copy for the server needs to be done; and if we don't have step 3, the server just copies config of loopback interface from <system>;
Make sense?

(23) p 12, sec 5.3.  Servers Auto-configuring Referenced System Configuration
      ("resolve-system" parameter)

   The server's copy referenced nodes from <system> to the target

   datastore MUST be enforced at the end of the <edit-config>/<edit-

   data> or <copy-config> operations during the validation processing,

   regardless of which target datastore it is.

This probably means that it isn't a separate "system client" because I would expect that to turn in the commit history as a separate commit, but instead, the update to running via resolve-system is exactly the same as if the client had made the modification directly as part of an edit-data (or similar) operation.
[Qiufang] The idea is that the server writing some configuration into <running> when triggered by the "resolve-system" behaves just like a client writing into it, because the server won't make a distinction in <running> then, origin value for both type is reported as "intended". But you interpretation makes sense to me, I guess the "system client" expression should be removed in the next revision to avoid any potential confusion.

(24) p 13, sec 5.3.  Servers Auto-configuring Referenced System Configuration
      ("resolve-system" parameter)

   If the "resolve-system" parameter is not given by the client, the

   server should not modify <running> in any way otherwise not specified

   by the client.  Not using capitalized "SHOULD NOT" in the previous

   sentence is intentional.  The intention is to bring awareness to the

   general need to not surprise clients with unexpected changes.  It is

   desirable for clients to always opt into using mechanisms having

   server-side changes.  This document enables a client to opt into this

   behavior using the "resolve-system" parameter.  An example of this

   type of opt-in behavior can also be found in RFC 7317, which enables

   a client to opt into its behavior using a "$0$" prefix (see

   ianach:crypt-hash type defined in [RFC7317]).

Arguably, I don't think that above paragraph is needed at all and can just be removed.  Otherwise, you could argue that it perhaps conflicts with the text in 4.2?
[Qiufang]As the co-author, I want to keep this paragraph as it reflects what the WG has been expecting for <running>: to have control over it. The initial proposal for this draft is to allow the server to populate system config into <running> when the device is powered on, but it has evolved into what it is now, I think this general principle of client-control is important. Or would it work for you if we only keep the first sentence, i.e., "If the "resolve-system" parameter is not given by the client, the server should not modify <running> in any way otherwise not specified by the client. "?
But you are right that this seems to contradict the text in 4.2. For sec.4.2, the case is (or should be, will make it clear in sec.4.2) locked down to software upgrades. And above text is applied to the scenarios other than software upgrades.

(25) p 13, sec 5.3.  Servers Auto-configuring Referenced System Configuration
      ("resolve-system" parameter)

   Implementation specifics are beyond the scope of this document,

   however, due to the extra complexity brought by the "resolve-system"

   parameter, clients should be aware that it would cost a reasonable

   amount of time for the server to resolve reference, retrieve and copy

   the referenced system configuration from <system>, which could take

   multiple rounds since some errors may depend on the resolution of

   previous ones.

Suggest changing "it would cost" to "it may take".  But I'm also not really sure that this paragraph should be in the document (e.g., what is a reasonable amount of time?  Is it 1 second, or a minute, or a few minutes).
[Qiufang] This is about some implementation consideration for "resolve-system" parameter, the authors added this due to one comment received from the WG that  this parameter might be expensive to implement properly because of the reason illustrated above. So do you think the extra complexity introduced by this parameter, and the delayed response time of operation deserved to be mentioned here? Or any suggestion to improve the text? I am really open to this.

(26) p 24, sec 6.2.  Example Usage

   The local port and remote port are used when the BGP peer connection

   is established.  Since both are not supplied explicitly in <running>

   and <intended>, the default value for "bgp/peer/remote-port" is used,

   and there is no default statement for "bgp/peer/local-port", the

   system will select a value for it.  So the contents of <system> are

   shown as follows:

There is some level of interplay here between YANG default values and the information present in system.  E.g., depending on how the YANG data model is written (i.e., sometimes complex default values are specifying in description statements rather than as formal YANG defaults), then the choice as to whether to report a value in system vs a default value in the configuration may be a bit ambiguous.
[Qiufang] Agree, note that the "default" origin value defined in RFC8342 is specified as follows:
default: represents configuration using a default value specified
      in the data model, using either values in the "default" statement
      or any values described in the "description" statement.  The
      default origin is only used when the configuration has not been
      provided by any other source.
Currently <system> doesn't contain any default configuration, and mainly to seek consistence with the origin definition in 8342, i.e., only the configuration with origin value "system" is treated as system config and thus defined in <system>.
Is the consistence necessary and worth maintaining?
E.g., this example of using the local-port as an example of system configuration potentially feels like the weakest of the alternative justifications that have been provided.
[Qiufang] Are you suggesting to document this explicitly? I am wondering whether we need a brand-new subsection to talk about the interplay between default vs. system, once agreement is reached, maybe this is worth documenting.

(27) p 29, sec 7.3.  YANG Module

         description

           "When present, the server is allowed to automatically

            configure referenced system configuration into the

            target configuration datastore.";

Should this be "is allowed to automatically configure", or should it be "must automatically configure"?
[Qiufang] Yes, you are right, the description needs to be updated to be consistent with the related description in sec.5.3. Thanks for pointing it out. I will also check other description statements in the yang module.

(28) p 31, sec 9.2.  Regarding the "ietf-netconf-resolve-system" YANG Module

   The security considerations for the base NETCONF protocol operations

   (see Section 9 of [RFC6241] apply to the new extended RPC operations

   defined in this document.

Possibly, this section should say a bit more about the security impacts of supporting the resolve-system option, i.e., that there aren't any beyond the potential performance impacts of implementing resolve-system, which may mean that employing some form of rate limiting of requests specifying this option might be a good idea to avoid DoS attacks.
[Qiufang]Having to say I don't have much security knowledge, but as far as I know, some systems uses rate limiting already, but given the extra performance impacts introduced by resolve-system parameter, we might ask implementations to adjust the limited rate threshold for better protection. Would the following tweaking work for you:
There is not any beyond the potential performance impacts of implementing the "resolve-system" parameter, which may mean employing some form of rate limiting or adapting the rate threshold might be a good idea to avoid DoS attacks.

(29) p 35, sec Appendix A.  Key Use Cases

A.1.  Device Powers On

Please provide a short prose description of what the example illustrates.
[Qiufang]Yes, I agree this needs to be provided.

(30) p 35, sec Appendix A.  Key Use Cases

   <running>:

Please expand these, e.g.  The <running> datastore contains:
[Qiufang] Will expand these in the new revision, thanks.

(31) p 35, sec Appendix A.  Key Use Cases

   <system>:

Please expand these, e.g.  The <system> datastore contains:
[Qiufang] Will expand these in the new revision, thanks.

(32) p 35, sec Appendix A.  Key Use Cases

   <intended>:

Please expand these, e.g.  After merging, the <intended> datastore contains:
[Qiufang] Will expand these in the new revision, thanks.

(33) p 35, sec Appendix A.  Key Use Cases

   <operational>:

Please expand these, e.g.  Once the configuration is applied, the <operational> datastore contains:
[Qiufang] Will expand these in the new revision, thanks.

(34) p 36, sec Appendix A.  Key Use Cases

   <running>:

Please expand these simiarly to above for the other examples, A.2 and A.3.
[Qiufang] Sure, will fix them all.

(35) p 36, sec Appendix A.  Key Use Cases

   <interfaces xmlns:or="urn:ietf:params:xml:ns:yang:ietf-origin"

               or:origin="or:intended">

As per a previous comment, I wonder whether the origin of the 'interfaces' container itself should be 'intended' or 'system' (given than loopback always exists and hence it can never be removed).
[Qiufang] Yes, config in <system> always exists and can never be removed.
I failed to mention in response to your above comments that this is also the very first issue that we discussed at the interim we had in January (what's the origin value for config copied from <system> into <running>), and the consensus of the participants was that system configuration copied from <system> into <running> should have origin value "intended" when flowing into <operational>.
So configuration once written into <running>, it reflects the intent of operators and thus always takes precedence over what's in <system>, this is independent of whether it's an overridden or copy of system config. Does this make sense?

(36) p 37, sec Appendix A.  Key Use Cases

     <interface or:origin="or:system">

       <name>lo0</name>

       <ip-address>127.0.0.1</ip-address>

       <ip-address>::1</ip-address>

     </interface>

   </interfaces>

A.3.  Operator Installs Card into a Chassis

Please provide a short prose description of what the example illustrates.
[Qiufang]Sure, will do.


Nit level comments:

(37) p 5, sec 1.3.  Updates to RFC 8342

   Configuration in <running> is merged into <system> to create the

   contents of <intended> after the configuration transformations to

   <running> (e.g., template expansion, removal of inactive

   configuration defined in [RFC8342]) have been performed.  This

   document updates the definition of "intended" origin metadata

   annotation identity to allow a subset of configuration provided by

   <intended> to use "system" as origin value as it flows into

   <operational>.  Applied system configuration appears in <operational>

   with origin value being reported as "system" (Section 5.1).

I think that "<running> is merged into <system>" is confusing.  I would say that <running> is merged with the contents of <system> and how that merge is performed must be specified.
[Qiufang] Will update it to "<running> is merged with the contents of <system>", and let's discuss the merge behavior in your comments #3?

(38) p 8, sec 3.  The System Configuration Datastore (<system>)

   *  Defining YANG module: "ietf-system-datastore".

   The datastore's content is defined by the server and read-only to

   clients.  Upon the content is created or changed, it will be merged

   into <intended>.  Unlike <factory-default> [RFC8808], it MAY change

   dynamically, e.g., depending on factors like license change, device

   upgrade or system-controlled resources change (e.g., HW available).

   The system configuration datastore doesn't persist across reboots;

   <factory-reset> RPC operation defined in [RFC8808] can reset it to

   its factory default configuration without including configuration

   generated due to the system update or client-enabled functionality.


Upon the content => When the content.  Some of the content here seems to repeat the text in "Management operations", I think the examples would be better in only a single place.
[Qiufang]You're right, will remove the example description. Thanks.


(39) p 8, sec 4.2.  May Change via Software Upgrades or Resource Changes

   If system configuration changes (e.g., due to device upgrade),

   <running> MAY become invalid.  The server behaviors of migrating

   updated system data into <running> is beyond the scope of this

   document.  That said, the following gives a list of examples of

   server implementations that might be possible:


Suggest rewording to: "That said, here are some examples of how a server might handle this scenario:"
[Qiufang]Sure, will update this in the next version.


(40) p 9, sec 4.3.  No Impact to <operational>

   This work intends to have no impact to <operational>.  System

   configuration appears in <operational> with origin value being

   reported as "system" if not configured or overridden explicitly in

   <running>.  This document enables a subset of those system generated

   nodes to be defined like configuration, i.e., made visible to clients

   in order for being referenced or configurable prior to present in

   <operational>.  "Config false" nodes are out of scope, hence existing

   "config false" nodes are not impacted by this work.

As per above, does "Overridden explicitly" mean "has a different value" in running?
[Qiufang]Yes,  having a different value can be interpreted as "overridden", and there are other cases, e.g., clients expanding a list entry to add new descendant nodes which are not in <system> is overriding the system list entry.
maybe also refer to sec.5.4. Generally, configuration in <system> with its origin being reported as <system> unless the system configuration is copied or overridden in <running>. Is the existing text clear enough? Or please feel free to propose text.



Note sec.5.1 also specifies that "Configuration copied from <system> into <running> has its origin value reported as "intended" when it flows into <operational>." Maybe things related to the origin should be specified in one place, e.g., adding a new subsection to talk about origin value?

(41) p 12, sec 5.3.  Servers Auto-configuring Referenced System Configuration

      ("resolve-system" parameter)

   Note that even an auto-configured node is allowed to be deleted from

   the target datastore by the client, the system may automatically

   configure the deleted node again to make configuration valid, when a

   "resolve-system" parameter is carried.  It is also possible that the

   operation request (e.g., <edit-config>) may not succeed due to

   incomplete referential integrity.


Perhaps "recreate the deleted node" rather than "configure the deleted node".
[Qiufang] Yes, your proposal is better.

(42) p 12, sec 5.3.  Servers Auto-configuring Referenced System Configuration
      ("resolve-system" parameter)


   Support for the "resolve-system" parameter is OPTIONAL.  Servers not

   supporting NMDA [RFC8342] MAY also implement this parameter without

   implementing the system configuration datastore, which would only

   eliminate the ability to expose the system configuration via protocol

   operations.  If a server implements <system>, referenced system

   configuration is copied from <system> into the target datastore

   (e.g., <running>) when the "resolve-system" parameter is used;

   otherwise it is an implementation decision where to copy referenced

   system configuration into the target datastore (e.g., <running>).


Perhaps 'examine' rather than 'expose'.
[Qiufang] I am not sure if word "examine" is proper here. By using "expose", it means that to enable the client to read the system config via standard network management protocols (e.g., netconf and restconf).
Could you please explain the intent to use "examine" here? Maybe "retrieve"?

(43) p 21, sec 5.5.3.  Modifying a System-instantiated Leaf's Value

   <interfaces xmlns="urn:example:interface">
     <interface>
       <name>lo0</name>
       <mtu>65536</mtu>
       <ip-address>127.0.0.1</ip-address>
       <ip-address>::1</ip-address>
     </interface>
   </interfaces>

   A client modifies the value of MTU to 65535 and adds the following

   configuration into <running>:

I initially hadn't spotted the subtle change, perhaps use an MTU value that is more obviosuly different from the value in system.  E.g., perhaps 9216.

[Qiufang] Sure, will fix this in the new revision!


Thanks a lot taking the time for reviewing this document! All excellent comments!

Regards,

Rob



Best Regards,

Qiufang