Subject: [netmod] js review of yang-09

From: Juergen Schoenwaelder <[email-masked]>
Date: Tue, 5 Jan 2010 13:27:37 +0100

Hi,

here is my WG last call review of draft-ietf-netmod-yang-09.txt. I am
a few hours past the deadline - I apologize and hope the chairs and
the WG find my review still useful.

Overall, I found the document to be in a pretty good shape. Martin has
done an excellent job in editing this memo. I have read the whole
document, skipping the ABNF section though.

Below are my comments, some are more substantial than others, some are
probably just nice to have edits and I leave it to the discretion of
the editor to address them or not. The comments come in no particular
order.

* JS-01:

In the terminology section, I think it would help if the text would
spell out 'data node' or 'schema node' where it currently just says
'node' since data node and schema node are defined terms and node is
not defined and also more ambiguous. This affects multiple

* JS-02:

OLD:

   o  conformance: A measure of how accurately a device follows the
      model.

NEW

   o  conformance: A measure of how accurately a device implements a
      data model.

* JS-03:

RPC vs. method vs. RPC method vs. operation: All these terms seem to
mean the same and I would prefer to have less of these terms around.
My preference would be to eliminate the usage of "method" or "RPC
method" in favour of "operation" throughout the YANG spec. Personally,
I would even go as far as eliminating RPC in most cases in the text.

* JS-04:

The third sentence of the following text is ambiguous:

   A list defines a sequence of list entries.  Each entry is like a
   structure or a record instance, and is uniquely identified by the
   values of its key leafs.  A list can define multiple keys and may
   contain any number of child nodes of any type (including leafs,
   lists, containers etc.).

I think it should be "A list can define multiple key leafs ...".
There is only one key but the key may be made up of multiple key
leafs. Perhaps we need a definition of the terms "key" and "key leaf".

* JS-05:

I do not understand the column "Type" in the table on page 20 nor do I
see that it is needed since the "Description" column is sufficient. I
suggest to get rid of the "Type" column.

* JS-06:

OLD

   using the "grouping" statement. "grouping" defines a set of nodes

NEW

   using the "grouping" statement. A "grouping" defines a set of nodes

* JS-07:

The separator of the revision-data in the file name (section 5.2)
should change, see also the mailing list discussion.

* JS-08:

OLD

   reasons.  Many errors and warnings that cannot be detected during

NEW

   reasons.  Errors and warnings that cannot be detected during

* JS-09:

There are multiple places where the ID talks about RFC streams. Shall
we add a reference to RFC 4844 in those places?

* JS-10:

OLD

   and semantics behind the modeled data.  The strength of YANG lies in
   the strength of this contract and the mindless devotion with which
   implementers follow it.

NEW

   and semantics behind the modeled data.  The strength of YANG lies in
   the strength of this contract.

* JS-11:

OLD:

   In many models, the modeler will allow sections of the model to be
   conditional, based on the device. 

NEW

   In many models, the modeler will allow sections of the model to be
   conditional.

* JS-12:

I fail to understand the purpose of section 5.7. This either needs to
be rewritten or it should be removed. Here is the section:

5.7.  Data Store Modification

   Data models may allow the server to alter the configuration data
   store.  A formal mechanism for specifying the circumstances where
   these changes are allowed is out of scope for this specification.

* JS-13:

Quoting:

   The following sections describe all of the YANG core statements.

This raises the question what are core statements and what are
non-core statements. And given the organization of the document, type
restrictions (defined in section 9) suddenly are not YANG core
statements anymore. I believe the wording "core statements" creates
more trouble than it is adding value and I thus prefer to avoid using
this term. If the term, however, is considered essential, define it
properly in the terminology section. 

* JS-14:

Quoting:

   Note that even a statement which does not have any substatements
   defined in core YANG can have vendor-specific extensions as
   substatements.  For example, the "description" statement does not
   have any substatements defined in core YANG, but the following is
   legal:

This now talks about core YANG. Similar to JS-13, I believe we are
better served to avoid this terminology.

* JS-15:

OLD

   the module, including the initial revision.  A series of revisions

NEW

   the module, including the initial revision.  A series of revision

* JS-16:

OLD

   The "container" statement is used to define an interior node in the
   schema tree.

NEW

   The "container" statement is used to define an interior data node in the
   schema tree.

* JS-17:

It seems the examples are pretty much chosen randomly. We have
examples related to DNS resolvers, interfaces, ssh services, users,
sonet, gps, and rock-the-house (perhaps even stuff I do not recall).
Perhaps we can replace some of the more esoteric examples with more
down to earth stuff easily understood by most readers.

* JS-18:

The examples come with varying level of completeness. I would prefer
to make several examples more complete. A good example is section
9.2.5 and 9.4.5.

* JS-19:

This text is somewhat unclear:

   When a datastore is validated, all "must" constraints are
   conceptually evaluated once for each corresponding instance in the
   data tree

What is a 'corresponding instance'? I guess I know what the intent is
behind the text but I see why someone new might get confused. Perhaps
this is what was intended:

   When a datastore is validated, all "must" constraints are
   conceptually evaluated for each data node in the data tree
   matching a schema node holding a "must" statement.

* JS-20:

There are some "elements of procedure" for the NETCONF <edit-config>
operation and it seems they over simplify things. An example is
7.6.7:

      If the operation is "merge", the node is created if it does not
      exist, and its value is set to the value found in the XML RPC
      data.

      If the operation is "replace", the node is created if it does not
      exist, and its value is set to the value found in the XML RPC
      data.

      If the operation is "create" the node is created if it does not
      exist.

I assume the value is assigned in a create as well. This means, the
text for all three (merge, replace, create) then is the same but the
NETCONF processing is not since the create fails if the node exists.
I think we serve the reader by spelling out this in more detail. And
is it really true that "merge" and "replace" are interchangeable or
does "replace" require that the data mode exists? Note that this
comment affects multiple places in the document.

* JS-21:

A usage examples that "creates a leaf" by using a "merge" may be
confusing since a "merge" is not the same as a "create". (This affects
section 7.6.8. and 7.7.8, perhaps other examples as well.)

* JS-22:

OLD

   Where the "leaf" statement is used to define a simple scalar variable
   of a particular type, the "leaf-list" statement is used to define an
   array of a particular type.

NEW

   Where the "leaf" statement is used to define a simple scalar variable
   of a particular type, the "leaf-list" statement is used to define an
   ordered set of values of a particular type.

The word "array" may be potentially confusing and is never used
elsewhere in the document.

* JS-23:

The ordered-by statement takes as argument "user" or "system". The
text in section 7.7.1 talks about user and the device. NETCONF itself
has clients and servers. There might be meaning behind the decision of
using all these different terms. But then the terminology section does
not help with understanding the differences. Perhaps this really
should have been 'ordered-by device'? Or declare that device and
system are the same?

* JS-24:

This text perhaps needs some revision:

   A leaf-list node is encoded as a series of XML elements.  Each
   element's name is the leaf-list's identifier, and its XML namespace
   is the module's XML namespace.

   The value of the leaf-list node is encoded to XML according to the
   type, and sent as character data in the element.

The point here is that the schema tree leaf-list node leads to
multiple data tree nodes. If we only talk about "node", the text
becomes somewhat confusing I think.

* JS-25:

The insert behaviour is not clear to me. Suppose I have this slight
variation of what is in 7.7.8:

     <rpc message-id="101"
          xmlns="urn:ietf:params:xml:ns:netconf:base:1.0"
          xmlns:nc="urn:ietf:params:xml:ns:netconf:base:1.0"
          xmlns:yang="urn:ietf:params:xml:ns:yang:1">
       <edit-config>
         <target>
           <running/>
         </target>
         <config>
           <system xmlns="http://example.com/schema/config">
             <services>
               <ssh>
                 <cipher nc:operation="create"
                         yang:insert="after"
                         yang:value="3des-cbc">blowfish-cbc</cipher>
	         <cipher nc:operation="create">aaa</cipher>
               </ssh>
             </services>
           </system>
         </config>
       </edit-config>
     </rpc>

Where does aaa go? Do the yang:insert and yang:value attributes have
impact of the subsequent create?

* JS-26:

The move feature defined by way of example in section 7.8.7. probably
requires some mentioning in the section 7.8.6. as well. It came to me
as a surprise after having read 7.8.6.

* JS-27:

OLD

   the subelements are be encoded in any order.

NEW

   the subelements are encoded in any order.

* JS-28:

This might be wrong:

   The grouping statement is not a data definition statement and, as
   such, does not define any nodes in the schema tree.

Perhaps you meant the data tree and not the schema tree.

* JS-29:

OLD

   with the "refine" statement.  The argument is a a string which

NEW

   with the "refine" statement.  The argument is a string which

* JS-30:

   If a "config" statement is present for any node in the output tree,
   it is ignored.

What is being ignored, the config statement of the node?

* JS-31:

What is the rationale behind this:

   If a container in the notification tree has a "presence" statement,
   the container need not be present in a NETCONF notification.

* JS-32:

   If a "config" statement is present for any node in the notification
   tree, it is ignored.

Again, it is unclear what is ignored (the config statement of the node).

* JS-33:

I suggest to replace the very abstract even notification with
something more down to earth. This is pretty much the high-level
opaque stuff we prefer not to see in YANG data models. (At least
this is my opinion. ;-)

* JS-34:

OLD

   name is created by combining the the prefix of the module in which

NEW

   name is created by combining the prefix of the module in which

* JS-35:

OLD

   The deviation statement defines a hierarchy of the module which the

NEW

   The deviation statement defines a hierarchy of a module which the

* JS-36:

OLD

   identifies the node in the schema tree where a deviation from the

NEW

   identifies the node in the schema tree where a deviation from a

* JS-37:

OLD

   the standard.  This means that deviations MUST never be part of a

NEW

   a standard.  This means that deviations MUST never be part of a

* JS-38:

OLD

   Instead, YANG allows devices to document portions of the base module

NEW

   Instead, YANG allows devices to document portions of a base module

* JS-39:

OLD

   properties to delete are identified by substatement to "delete".  The

NEW

   properties to delete are identified by a substatement of the "delete" statement.  The

* JS-40:

Section 9.4 contains this:

     // any Unicode character, excluding the surrogate blocks,
     // FFFE, and FFFF.
     string = *char
     char = %x9 / %xA / %xD / %x20-DFFF / %xE000-FFFD /
            %x10000-10FFFF

It is unclear what this is. Looks like ABNF but then the comment
character would be wrong. 

* JS-41:

   as defined in [XSD-TYPES].  It is used to restrict the built-in type
   "string", or types derived from "string", to values that completely
   matches the pattern.

Not sure what "completely" adds here. The pattern matches or it does
not - I think there is no notion of a partial match.

* JS-42:

OLD

   restricted, based on the rules defined in Section 9 chapter.

NEW

   restricted, based on the rules defined in Section 9.

* JS-43:

Section 11 says:

    A YANG module can be specified in an alternative XML-based syntax
    called YIN.  This section describes symmetric mapping rules between
    the two formats.

I believe we need to separate the concept of a YANG module from that
of a YIN module. I would prefer this instead:

    A YANG module can be translated into an alternative XML-based
    syntax called YIN. The translated module is called a YIN
    module. This section describes symmetric mapping rules between the
    two formats.

This makes it clear that a YANG parser is parsing YANG modules and a
YIN parser is parsing YIN modules.

* JS-44:

I suggest to use this YIN URI

   "urn:ietf:params:xml:ns:yin:1"

instead of 

   "urn:ietf:params:xml:ns:yang:yin:1"

since there is little value of nesting this. But I do not care
strongly about this.

/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: Martin Bjorklund <[email-masked]>
Date: Tue, 05 Jan 2010 23:47:47 +0100 (CET)

Hi,

Thank you Juergen for the detailed review!

> not defined and also more ambiguous. This affects multiple

I will get back on this one.

>    o  conformance: A measure of how accurately a device implements a

Fixed.

> I would even go as far as eliminating RPC in most cases in the text.

Ok.  I replaced "method" with "operation".  (Maybe this should be
fixed in 4741bis as well.)

> I think it should be "A list can define multiple key leafs ...".

Fixed.

> suggest to get rid of the "Type" column.

Ok, the Type column is removed.

>    using the "grouping" statement. A "grouping" defines a set of nodes

NEWER:

   using the "grouping" statement. A grouping defines a set of nodes

> should change, see also the mailing list discussion.

Ok.  Changed to '@'.

>    reasons.  Errors and warnings that cannot be detected during

Fixed.

> we add a reference to RFC 4844 in those places?

I think that makes sense.

>    the strength of this contract.

Fixed.

>    In many models, the modeler will allow sections of the model to be

Fixed.

>    these changes are allowed is out of scope for this specification.

This was suggested in the thread "resolution of system-creatable
discussion".

> properly in the terminology section. 

I agree, I have removed 'core' from the document.

> better served to avoid this terminology.

Yes.  removed.

>    the module, including the initial revision.  A series of revision

Fixed.

>    The "container" statement is used to define an interior data node in the

See JS-1.

> down to earth stuff easily understood by most readers.

I think the examples are ok, but feel free to suggest improvements!

> to make several examples more complete. A good example is section

Ok, I have changed some examples in section 9 to be more complete.

>    matching a schema node holding a "must" statement.

Hmm... do we need to explain what "matching" a schema node means?  I
will think some more about this one.

> comment affects multiple places in the document.

Agreed.  See my email "YANG WGLC: edit-config clarifications" from 22
dec. with suggested changes to this text.

> section 7.6.8. and 7.7.8, perhaps other examples as well.)

There are also some examples with operation create.  I think it is
good to show both methods.

> elsewhere in the document.

"array" is used to distinguish it from "simple scalar varaible".  I
think it is ok to use.

> should have been 'ordered-by device'? Or declare that device and

I think 7.7.1 does that:

   In many lists, the order of list entries does not impact
   the implementation of the list's configuration, and the device is
   free to sort the list entries in any reasonable order. [...]  YANG calls
   this style of list "system ordered" and they are indicated with the
   statement "ordered-by system".

> becomes somewhat confusing I think.

NEW:

  A leaf-list node is encoded as a series of XML elements.  Each
  element's local name is the leaf-list's identifier, and its
  namespace name is the module's XML namespace (see ^namespace^).

  The value of each leaf-list entry is encoded to XML according to the
  type, and sent as character data in the element.

> impact of the subsequent create?

No; aaa will be added last in the list.

> as a surprise after having read 7.8.6.

7.8.6 mentions 'move':

   These can be used during "create"
   operations to insert a new list entry, or during "merge" or "replace"
   operations to insert a new list entry or move an existing one.

>    the subelements are encoded in any order.

Fixed.

> Perhaps you meant the data tree and not the schema tree.

No it is correct - a grouping does not define a node in the schema
tree.

Maybe the sentence should be removed.  We should not list everything
we *don't* do.

>    with the "refine" statement.  The argument is a string which

Fixed.

> What is being ignored, the config statement of the node?

NEW:

  If a "config" statement is present for any node in the input tree,
  the "config" statement is ignored.

>    the container need not be present in a NETCONF notification.

I agree that it doesn't really say anything.  I will remove this
sentence.

> Again, it is unclear what is ignored (the config statement of the node).

Fixed.

> opaque stuff we prefer not to see in YANG data models. (At least

The example is from rfc 5277; it is just a YANG version of an example
there.  But I am open to suggestions!   We also have some other
examples in the document.

>    name is created by combining the prefix of the module in which

Fixed.

>    The deviation statement defines a hierarchy of a module which the

Fixed.

>    a standard.  This means that deviations MUST never be part of a

Fixed.

>    Instead, YANG allows devices to document portions of a base module

Fixed.

>    properties to delete are identified by a substatement of the "delete" statement.  The

Fixed.

> character would be wrong. 

Ok, changed comment character to ;

> not - I think there is no notion of a partial match.

Agreed.  I removed 'completely'.

>    restricted, based on the rules defined in Section 9.

Fixed.

> YIN parser is parsing YIN modules.

Ok, fixed.

> since there is little value of nesting this. But I do not care

I prefer to keep the current one.

/martin

---

From: Martin Bjorklund <[email-masked]>
Date: Thu, 07 Jan 2010 11:26:20 +0100 (CET)

Hi,

> I will get back on this one.

These are the changes for this one:

-- anyxml: A node which can contain an unknown chunk of XML data.
+- anyxml: A data node which can contain an unknown chunk of XML data.
 
-- augment: Adds new nodes to a previously defined node.
+- augment: Adds new schema nodes to a previously defined schema node.
 
-- choice: A node where only one of a number of identified
-  alternative nodes is valid.
+- choice: A schema node where only one of a number of identified
+  alternatives is valid.
 
-- container: An interior node in the data tree which exist in at most
-  one instance.  A container node has no value, but rather a set of
-  child nodes.
+- container: An interior data node which exists in at most one
+  instance in the data tree.  A container has no value, but rather a
+  set of child nodes.
 
-- extension: An extension attaches non-YANG semantics to nodes.  The
+- extension: An extension attaches non-YANG semantics to statements.  The
   extension statement defines new statements to express these
   semantics.
 
-- grouping: A reusable set of nodes, which may be used locally
+- grouping: A reusable set of schema nodes, which may be used locally
   in the module, in modules which include it, and by other
   modules which import from it.  The grouping statement is not a data
   definition statement and, as such, does not define any nodes in the

-- leaf: A node in the data tree with a value but no child
-  nodes.
+- leaf: A data node which exists in at most one instance in the data
+  tree.  A leaf has a value but no child nodes.
 
-- list: Interior nodes in the data tree which may exist in multiple
-  instances.  A list node has no value, but rather a set of child
+- list: An interior data node which may exist in multiple instances in
+  the data tree.  A list has no value, but rather a set of child
   nodes.
 
-- uses: The "uses" statement is used to instantiate the set of nodes
-  defined in a grouping statement.  The instantiated nodes may be
-  refined and augmented to tailor them to any specific needs.
+- uses: The "uses" statement is used to instantiate the set of schema
+  nodes defined in a grouping statement.  The instantiated nodes may
+  be refined and augmented to tailor them to any specific needs.

> will think some more about this one.

How about this:

  When a datastore is validated, all "must" constraints are
  conceptually evaluated once for each data node in the data tree, and
  for all leafs with default values in use (see ^leaf-default-value^).
  If an data node does not exist in the data tree, and it does not
  have a default value, its "must" statements are not evaluated.

> Maybe the sentence should be removed.  We should not list everything

Actually, the sentence was added because someone wanted this
clarification.  I think we should keep it.

/martin

---

From: Juergen Schoenwaelder <[email-masked]>
Date: Thu, 7 Jan 2010 14:23:32 +0100

> This was suggested in the thread "resolution of system-creatable

Ah, now I understand what this is trying to tell me. What about this
rewrite:

    Data models may allow the server to alter the configuration data
    store while not processing NETCONF protocol messages affecting the
    data store.  A formal mechanism for specifying the circumstances
    where these such changes to a configuration data store are allowed
    is out of scope for this specification.

Perhaps this wording can be further improved. I was missing in the
first sentence the hint that we talk about changes not initiated by
NETCONF message processing.

> good to show both methods.

Thats fine but then the text should in my view explain that such an
example is a merge and not a create.

> Maybe the sentence should be removed.  We should not list everything

Or reduce the sentence to "The grouping statement is not a data
definition statement."

> examples in the document.

But RFC 5277 is written in caml style. :) I would prefer a basic
link-down notification example.

/js

---

From: Phil Shafer <[email-masked]>
Date: Thu, 7 Jan 2010 08:29:42 -0500

>NETCONF message processing.

The changes may be initiated by NETCONF message processing, but not
be the exact change requested.  Setting <foo> to 5 may set <bar>
to 10.  Or creating <user> may create a <uid>.  How about:

    Data models may allow the server to alter the configuration
    data store in ways not explicitly directed via NETCONF protocol
    messages.  For example, a data model may define leafs that are
    assigned system-generated values when the client does not provide
    one.  A formal mechanism for specifying the circumstances where
    these changes are allowed is out of scope for this specification.

Thanks,
 Phil

---

From: Juergen Schoenwaelder <[email-masked]>
Date: Thu, 7 Jan 2010 14:58:13 +0100

> to 10.  Or creating <user> may create a <uid>.  How about:

Good point.
 
>     these changes are allowed is out of scope for this specification.

Fine with me.

/js

---

From: Martin Bjorklund <[email-masked]>
Date: Thu, 07 Jan 2010 15:11:07 +0100 (CET)

> >     these changes are allowed is out of scope for this specification.

+1.

/martin

---

From: Martin Bjorklund <[email-masked]>
Date: Thu, 07 Jan 2010 15:28:02 +0100 (CET)

> example is a merge and not a create.

Ok.  In 7.6.8:

OLD:

  To create a leaf with an <edit-config>:

NEW:

  To set the value of a leaf with an <edit-config>:

And 7.7.8:

OLD:

   To create a new element in the list:

NEW:

  To create a new element in this list, using the default

> Or reduce the sentence to "The grouping statement is not a data

I would prefer to keep the current text.  IIRC it was requested by
someone who reviewed the document.

> link-down notification example.

We already have one 'link-failure' in the document.  I don't want to
copy&paste the same example twice.

/martin

---

From: Ladislav Lhotka <[email-masked]>
Date: Thu, 07 Jan 2010 17:30:24 +0100

> "array" is used to distinguish it from "simple scalar varaible".  I

I think "set" it more precise here because all values must be unique,
which is normally not assumed for arrays. (But in fact also not for
lists, so strictly speaking "leaf-set" would be a more appropriate
name.)

>    statement "ordered-by system".

I also tend to think that "ordered-by server" and "ordered-by client"
would be clearer.

Lada

-- 
Ladislav Lhotka, CESNET
PGP Key ID: E74E8C0C

---

From: Andy Bierman <[email-masked]>
Date: Thu, 07 Jan 2010 09:34:20 -0800

> I also tend to think that "ordered-by server" and "ordered-by client"

If this was a review of -00, then I would agree that
tweaking the terminology would be a good idea, even
though it is a subjective process.  IMO, 'system' is better
because it suggests that the database order may indeed
come from the HW within the system.  The NETCONF server
is just one component within the managed system.

> Lada

Andy

---

From: Juergen Schoenwaelder <[email-masked]>
Date: Thu, 7 Jan 2010 18:59:34 +0100

> +  be refined and augmented to tailor them to any specific needs.

Looks OK to me.
 
>   have a default value, its "must" statements are not evaluated.

OK

Fine with me.

/js