Subject: [netmod] review of draft-ietf-netmod-rfc6020bis-07 (except 9. built-in types)

From: Juergen Schoenwaelder <[email-masked]>
Date: Mon, 12 Oct 2015 16:44:31 +0200

Hi,

I have read through draft-ietf-netmod-rfc6020bis-07 (except section 9.
Built-In Types). Overall, the document is in a good shape. I spotted a
number of editorial issues or a number of issues where we could try to
seek a clearer separation of NETCONF specifics. (I plan to read
section 9 as well but I assume this is less important since there
likely are not many changes from RFC 6020 in this part.)

I am indicating the paper number below for each comment/suggestion.

* p8:

  s/XML has been/XML have been/

  s/how the data model/how a data model/

  s/represented in/encoded in/

* p9:

  s/names means/names mean/

* p11:

  s/the module faithfully/a module faithfully/

  I am also wondering why we use device and server. It seems we use
  these terms interchangeably. If so, for clarity, I would suggest to
  use a single term, that is s/device/server/ and perhaps explicitly
  state that unless stated otherwise server means a server providing
  access to a YANG defined data tree.

* p12/p13

  We import 7 terms from RFC 6241. Would it make sense to copy the
  necessary text in order to avoid a too strict binding to RFC 6241?
  In particular, 'client' and 'server' means NETCONF client and server
  if we import from RFC 6241 but this may be a bit narrow given that
  we have RESTCONF as well. In an ideal world, we would factor out
  core architectural concepts but the best we can do is likely to
  define core concepts inline, pointing out where they are the same.
  The idea is to loosen the coupling to RFC 6241. Example:

  OLD

   o  datastore: an instantiated data tree

  NEW

   o  datastore: A conceptual place to store and access information.
      A datastore might be implemented, for example, using files, a
      database, flash memory locations, or combinations thereof.
      [Matches the definition in RFC 6241.]

* p13

  Does the term 'mandatory node' really deserve its own subsection?  I
  understand that there are references to section 3.1 but would
  reference to section 3 not work as well?

* p13

  s/used for other/used with other/

* p14

  s/encoded in NETCONF operations/encoded on-the-wire/

* p14

  s/of NETCONF data models/of data models for network management
  protocols such as NETCONF,/

  And I suggest to remove the following sentence since it is not
  really needed.

      The data models described by YANG are designed to be easily
      operated upon by NETCONF operations.

* p15

  s/read-only access/read-only access [RFC6643]/

* p15

  I am not sure why the last paragraph in section 4.1 is needed. In
  the first setence, I would remove 'Like NETCONF' and in the second
  sentence I am not sure what we are saying given that there is NACM.
  I would definitely remove the second sentence and if the first
  should be kept, I would likely move it up since it is a fairly
  general statement. But I think removing the whole paragraph is the
  simplest solution since it is not essential.

* p13

  Would it make sense to add a sentence right at the beginning of
  section 4 stating that section 4 is intended to provide orientation
  for the first time reader but that section 4 is not normative?

* p15

  With the previous in mind, I suggest to remove "This progressive
  approach handles the inter-related nature of YANG concepts and
  statements.  A detailed description of YANG statements and syntax
  begins in Section 7." from the text right below 4.2. Note that
  Section 7 is actually Section 6 (but Section 5 has also important
  normative content).

* p16

  s/four types of nodes/four types of data nodes/

* p16

  Perhaps simplify:

  OLD

    A leaf instance contains simple data like an integer or a string.  It
    has exactly one value of a particular type and no child nodes.

  NEW

    A leaf data node has exactly one value of a particular type and no
    child nodes.

* p16-p25

  Should this be 'XML Encoding Example' instead "NETCONF XML Example'?
  This does not seem to be NETCONF specific.

* p25

  s/XML representation/XML encoding/

* p25

  s/operations' names/operations' name/

* p25

  s/node in the data hierarchy/data node/

* p28

  Start section 5 by saying that this section defined language
  concepts and that it is normative, e.g.

    This normative section defines core language concepts.

* p29

  s/its contents are unchanged/its content is unchanged/

* p37

  s/following shows valid data/following XML encoding shows valid data/

* p35

  I think there should be a citation of I-D.ietf-netconf-yang-library
  where this module first appears.

* p46

  The text concerning module names is a repetition from section 5.1.
  To avoid introducing inconsistencies, perhaps replace the repeated
  text with a reference to section 5.1?

* p49

  Is the text in section 7.1.3 correct when it says all identifiers
  defined by the module? I mean, schema node identifiers are
  namespaced by module names and their prefixes. And data node
  identifiers are using the namespace in the XML encoding. Here is
  an attempt of a rewrite:

     The "namespace" statement defines the XML namespace that all data
     node identifiers defined by the module are qualified by in the
     XML encoding. Data node identifiers defined inside a grouping are
     an exception q(see Section 7.13 for details).  The argument to
     the "namespace" statement is the URI of the namespace.

* p53

  Another repetition of the module/submodule name requirements, does
  it make sense to avoid repetition?

* p61

  Should the text in 7.5.4.1 and 7.5.4.2 say explicitly that we talk
  about NETCONF when we refer to <rpc-error>? Or should the text try
  to be more generic, saying that the respective fields will be
  carried in error message in protocols that use YANG? It is actually
  somewhat opaque what an error-app-tag is or how it should be used.

* p63

  Should the last paragraph in 7.5.7 be factored out into its own
  subsection titled "NETCONF <get> and <get-config> Operations"?  The
  text is not really anymore about XML encoding (which may be used
  with RESTCONF). Or the text is actually about the encoding but
  should be written to simply state that non-presence containers can
  be pruned in encodings (without restricting this to NETCONF).

* p67

  Similar to the comment for p63. Perhaps the right way is to phrase
  this in such a way that is simply says leaf nodes containing a
  default value may not be present in the XML encoding. Simply remove
  NETCONF <get> or <get-config> specifics.

* p72

  s/an unspecified order/an order determined by the system/

  Note that a description clause may actually define how the system
  has to order elements and hence 'unspecified order' seems wrong.

* p97

  The text in 7.14 talks about NETCONF specifics, namely the <rpc>
  element and the "rpcOperation" substitution group. Perhaps move this
  down into a specific section defining the mapping of YANG RPCs to
  NETCONF <rpc>s.

* p100

  The XML encoding text starts with detailing NETCONF specifics.
  Would it make sense to separate the XML encoding of the rpc and its
  input/output from the specifics how the RPCs fit into NETCONF's
  <rpc> system?

* p102

  The XML encoding section again mixes XML encoding of the node
  hierarchy with NETCONF specifics how an action is invoked using
  NETCONF's <rpc> system. I suggest to factor this into two different
  sections.

* p105

  Again, the proposal is to separate the XML encoding of notifications
  from the details how notifications work with RFC 5277.

* p120

  The text in section 7.21.1 talks about NETCONF specific operations.
  Is this actually necessary? Can the purpose of the config statement
  not be described by referring to general concepts such as
  configuration datastores instead of using protocol specific
  operations?

* p123

  Is there a special reason why the text uses 'notification instance
  data tree' instead of just 'notification data tree'?

* p123

  "All leaf data values MUST match the type constraints..." may be
  read as 'all data nodes that contain values' or 'all instantiations
  of nodes defined by the leaf statement'.

* p192

  The Acknowledgements section may need some updates.

/js

-- 
Juergen Schoenwaelder           Jacobs University Bremen gGmbH
Phone: +49 421 200 3587         Campus Ring 1 | 28759 Bremen | Germany
Fax:   +49 421 200 3103         <http://www.jacobs-university.de/>

---

From: Ladislav Lhotka <[email-masked]>
Date: Tue, 13 Oct 2015 16:15:30 +0200

>   s/XML has been/XML have been/

And right after that: s/propsed/proposed/

Lada

> _______________________________________________
> netmod mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/netmod

-- 
Ladislav Lhotka, CZ.NIC Labs
PGP Key ID: E74E8C0C

---

From: Martin Bjorklund <[email-masked]>
Date: Thu, 15 Oct 2015 22:37:49 +0200 (CEST)

Hi,

Thanks for this detailed review, it is much appreciated!  Comments
inline.

>   s/represented in/encoded in/

Fixed.

>   s/names means/names mean/

Fixed.

>   s/the module faithfully/a module faithfully/

Fixed.

>   use a single term, that is s/device/server

Ok, fixed.

>  / and perhaps explicitly

Yes this makes sense.  But then I guess we shouldn't import client and
server from 6241.  (And most other documents (restconf etc) should
probably import these terms from ths document).  See also below.

>       [Matches the definition in RFC 6241.]

To start with, I think we should define client and server more
generically than just NETCONF:

  server: An entity that provides access to YANG-defined data to a
          client, over some network management protocol.

  client: An entity that can access YANG-defined data on a server,
          over some network management protocol.

?

>   reference to section 3 not work as well?

Sure.  The thing is that I have toolchain problems with nested
lists...  [hacking away] ... Fixed.

>   s/used for other/used with other/

Fixed.

>   s/encoded in NETCONF operations/encoded on-the-wire/

Fixed.

>   protocols such as NETCONF,/

Fixed.

>       operated upon by NETCONF operations.

Fixed.

>   s/read-only access/read-only access [RFC6643]/

Fixed.

>   simplest solution since it is not essential.

Agree, removed.

>   for the first time reader but that section 4 is not normative?

How about:

  This non-normative section is intended to give a high-level
  overview of YANG to first-time readers.

>   Section 7 is actually Section 6 (but Section 5 has also important

I agree; removed.

>   s/four types of nodes/four types of data nodes/

Fixed.

>     A leaf data node has exactly one value of a particular type and no

We changed this to "leaf instance" after discussion with Lada.  I
think it makes sense to mention that it is supposed to contain data
like an integer or string, so I think I prefer to keep this text as
is.

>   This does not seem to be NETCONF specific.

Fixed.

>   s/XML representation/XML encoding/

Fixed.

>   s/operations' names/operations' name/

Fixed.

>   s/node in the data hierarchy/data node/

Fixed.

>     This normative section defines core language concepts.

But aren't all sections normative unless we say so?  Otherwise it
seems we should start every section by saying that it is either
normative or not.

>   s/its contents are unchanged/its content is unchanged/

I am not sure that this is correct.  The text is present in RFC 6020,
and has been reviewd by the RFC editor; they usually capture things
like this.

>   s/following shows valid data/following XML encoding shows valid data/

Fixed (I did "following XML encoding example...")

>   where this module first appears.

Fixed.

>   text with a reference to section 5.1?

Ok, fixed.

>      the "namespace" statement is the URI of the namespace.

It's actually not only data nodes, it is also rpc, actions,
notifications, identities.  How about:

  The "namespace" statement defines the XML namespace that all
  identifiers defined by the module are qualified by in the XML
  encoding, with the exception of identifiers for data nodes, action
  nodes, and notification nodes defined inside a grouping (see ^uses^
  for details).  The argument to the "namespace" statement is the URI
  of the namespace.

>   it make sense to avoid repetition?

Yes, fixed.

>   somewhat opaque what an error-app-tag is or how it should be used.

I think we can say "If the constraint evaluates to false, the string is
passed as <error-message> in the <rpc-error> in NETCONF."  Other
protocols have to define how these fields are used.

>   be pruned in encodings (without restricting this to NETCONF).

This makes sense.  I think the last paragraph can be replaced with:

  If a non-presence container does not have any child nodes, the
  container may or may not be present in the XML encoding.

>   NETCONF <get> or <get-config> specifics.

Or maybe we should simply remove the last paragraph completely?  For
NETCONF, RFC 6243 details how leafs with defaults are handled.

>   has to order elements and hence 'unspecified order' seems wrong.

Fixed.

>   down into a specific section defining the mapping of YANG RPCs to

Yes, fixed, but see below.

>   input/output from the specifics how the RPCs fit into NETCONF's

Hmm.  NETCONF and RESTCONF use quite different encodings of rpcs and
their output.

NETCONF:

  <rpc message-id="101"
       xmlns="urn:ietf:params:xml:ns:netconf:base:1.0">
    <rock-the-house xmlns="http://example.net/rock">
      <zip-code>27606-0100</zip-code>
    </rock-the-house>
  </rpc>

  <rpc-reply message-id="101"
             xmlns="urn:ietf:params:xml:ns:netconf:base:1.0">
    <ok/>
  </rpc-reply>

RESTCONF:

   POST to  .../rock-the-house

   <input xmlns="http://example.net/rock">
    <zip-code>27606-0100</zip-code>
   </input>
    
   result: 204 no content

Maybe it would have been better to have a common encoding, like:

    <rock-the-house xmlns="http://example.net/rock">
      <input>...</input>
    </rock-the-house>  

   and

    <rock-the-house xmlns="http://example.net/rock">
      <output>...</output>
    </rock-the-house>  

but this cannot be done now.

So, maybe the simplest solution would be to rename the section to
"NETCONF XML Encoding"?

>   NETCONF's <rpc> system. I suggest to factor this into two different

See above, I suggest we simply rename the section to "NETCONF XML
Encoding".

>   from the details how notifications work with RFC 5277.

Also notifications are encoded differently in RESTCONF and NETCONF.

>   configuration datastores instead of using protocol specific

Yes, maybe we can just say:

  Data nodes representing configuration are part of configuration
  datastores.

>   data tree' instead of just 'notification data tree'?

No, fixed.

>   of nodes defined by the leaf statement'.

I don't really see what you mean.  Can you suggest new text?

>   The Acknowledgements section may need some updates.

Ok.

/martin

---

From: Juergen Schoenwaelder <[email-masked]>
Date: Fri, 16 Oct 2015 00:58:46 +0200

>           over some network management protocol.

Yes, we should definately open things up so that RESTCONF clients and
servers are covered.

>   overview of YANG to first-time readers.

OK

> like an integer or string, so I think I prefer to keep this text as

I think the document uses 'leaf' in different meanings; sometimes a
leaf is a 'leaf schema node instance' and sometimes a leaf is 'any leaf
of the tree'. This is what got me started here but perhaps the context
is clear enough (hence the 'perhaps simplify').

> seems we should start every section by saying that it is either

OK

>   for details).  The argument to the "namespace" statement is the URI

OK

> protocols have to define how these fields are used.

OK

>   container may or may not be present in the XML encoding.

OK

> NETCONF, RFC 6243 details how leafs with defaults are handled.

Well, yes, but then readers may not know about RFC 6243. Perhaps state
that leaf nodes containing a default value may not be present in the
XML encoding and point to RFC 6243 for further details how NETCONF
handles leafs with defaults (and I think the reference to RFC 6243
would be informative).

> So, maybe the simplest solution would be to rename the section to

You mean section 7.14.4? Well, OK... Too bad these RPC encodings are
actually different. Out of curiosity, was there a specific reason not
to use

in the RESTCONF POST or did this "just happen"?

> Also notifications are encoded differently in RESTCONF and NETCONF.

Hm.
 
>   Data nodes representing configuration are part of configuration

Yes.
 
> I don't really see what you mean.  Can you suggest new text?

See above, I think 'leaf' sometimes means an instance of a leaf schema
node and sometimes it means all leaf instances of the data tree. Or I
am confused. I think here you mean 'all leaf instances of the data
tree' and not the subset of all 'instances of a leaf schema nodes'.

/js

---

From: Martin Bjorklund <[email-masked]>
Date: Fri, 16 Oct 2015 09:25:00 +0200 (CEST)

> handles leafs with defaults (and I think the reference to RFC 6243

In some sense I think it is weird to talk about default values here.
This text describes how leafs and their values are encoded.

The default value is defined as the value being used if the leaf
doesn't exist in the data tree, so why should we mention defaults
here?

> You mean section 7.14.4? Well, OK...

Yes.

> Too bad these RPC encodings are

The reason is that we can't do output in the same way as NETCONF, and
the current RESTCONF encoding of input is consistent with how we do
output.

But it is not too late to change this.  This should be discussed in
netconf though.

> tree' and not the subset of all 'instances of a leaf schema nodes'.

Right; this is what the current text says:

   The following properties are true in all data trees:

   o  All leaf data values MUST match the type constraints for the leaf,
      including those defined in the type's "range", "length", and
      "pattern" properties.

Note the sentence before the bullet where it says that this is for the
data tree.

/martin

---

From: Ladislav Lhotka <[email-masked]>
Date: Fri, 16 Oct 2015 11:19:34 +0200

Martin Bjorklund <[email-masked]> writes:

...

>           client, over some network management protocol.

But then perhaps "device" is a broader term in the sense that the server is just
a software component running in a device.

For example, it is the device that is required to operationally use
default values of parameters that are not present in the
configuration. Replacing "device" with "server" here IMO means something
different.

Lada

---

From: Martin Bjorklund <[email-masked]>
Date: Fri, 16 Oct 2015 13:17:08 +0200 (CEST)

> a software component running in a device.

So how do you define "device"?

> configuration. Replacing "device" with "server" here IMO means something

The text was actually already in RFC 6020:

   When the default value is in use, the server MUST operationally
   behave as if the leaf was present in the data tree with the default
   value as its value.

/martin

---

From: Ladislav Lhotka <[email-masked]>
Date: Fri, 16 Oct 2015 13:32:23 +0200

> On 16 Oct 2015, at 13:17, Martin Bjorklund <[email-masked]> wrote:
> 

An entity that's being managed and runs the server.

>   behave as if the leaf was present in the data tree with the default

Right, but with your definition of "server" there is no guarantee that, say, if "mtu" on an interface has a default value and no value is configured, then the default value is really used - MTU has nothing to do with server operation.

Lada

---

From: Juergen Schoenwaelder <[email-masked]>
Date: Sun, 18 Oct 2015 10:42:09 +0200

> configuration. Replacing "device" with "server" here IMO means something

I prefer server. At the end it is an implementation matter whether the
server fills in a default used by the device or whether the device
uses a default which happens to be the default that the data model
used by the server defines.

/js