Subject: [netmod] AD review draft-ietf-netmod-rfc6020bis-11 (part 1)

From: Benoit Claise <[email-masked]>
Date: Tue, 19 Apr 2016 17:45:56 +0200

Dear all,

Here is part 1 of my AD review.

I found this useful: 
http://tools.ietf.org/rfcdiff/rfcdiff.pyht?url1=http://www.ietf.org/rfc/rfc6020.txt&url2=http://www.ietf.org/id/draft-ietf-netmod-rfc6020bis-11.txt

- Do we want to mention RESTCONF in the abstract? From the new charter:

    The NETMOD working group has defined the data modeling language
    YANG, which can be used to specify network management data models
    that are transported over such protocols as NETCONF and RESTCONF. 

OLD:

    YANG is a data modeling language used to model configuration data,
    state data, remote procedure calls, and notifications for network
    management protocols like the Network Configuration Protocol
    (NETCONF).

NEW:

    YANG is a data modeling language used to model configuration data,
    state data, remote procedure calls, and notifications for network
    management protocols transported over such protocols as Network
    Configuration Protocol (NETCONF) and RESTCONF. This document specifies
    the YANG mappings to NETCONF.

- Section 1.1
Can we want to stress the backwards incompatible changes from YANG version 1 in a specific paragraph?
I see
    o  Changed the rules for the interpretation of escaped characters in
       double quoted strings.  This is an backwards incompatible change
       from YANG version 1.  A module that uses a character sequence that
       is now illegal must change the string to match the new rules.  See
       Section 6.1.3 
<https://tools.ietf.org/html/draft-ietf-netmod-rfc6020bis-11#section-6.1.3>  for details.

    o  An unquoted string cannot contain any single or double quote
       characters.  This is an backwards incompatible change from YANG
       version 1.

There is section 12, but this is not exactly that.

Have we made an analysis of the 38 RFC-produced YANG modules? Any facing issues with 1.1 compilation?

- Section 1.1
Since this document introduces the NETCONF mapping, the protocol change must be included in section 1.1
Example: no NETCONF capability exchange in YANG 1.1, we use exclusively the YANG library
Any other ones?
  
- Section 3
    o  anydata: A data node that can contain an unknown set of nodes that
       can be modelled by YANG.

NEW

    o  anydata: A data node that can contain an unknown set of nodes that
       can be modelled by YANG, except anyxml, but for which the data
       model is not known at module design time.

- Terminology:
  The following terms are defined in [RFC6241 <https://tools.ietf.org/html/rfc6241>]:

    ...

    o  configuration datastore: a configuration datastore is an
       instantiated data tree with configuration data

    o  datastore: an instantiated data tree

RFC6241 has different definition for "configuration datastore" and "datastore".
I would just provide the pointer to the RFC 6241 definitions.
If you intend to provide an adapted definition for the YANG mappings, then you should say so.

- Section 4.1

    YANG models can describe constraints to be enforced on the data,
    restricting the appearance or value of nodes based on the presence or
    value of other nodes in the hierarchy.

I was looking for an example of appearance.
NEW?
    YANG models can describe constraints to be enforced on the data,
    restricting the appearance (for example, with the "when" statement)
    or value of nodes based on the presence or value of other nodes in
    the hierarchy.

- section 4.2.2.3, Container Nodes

    A container is used to group related nodes in a subtree.  A container
    has only child nodes and no value.  A container may contain any
    number of child nodes of any type (leafs, lists, containers, leaf-
    lists, and actions).
  
And notification, right? This should be added
    container-stmt      = container-keyword sep identifier-arg-str optsep
                          (";" /
                           "{" stmtsep
                               ;; these stmts can appear in any order
                               [when-stmt]
                               *if-feature-stmt
                               *must-stmt
                               [presence-stmt 
<https://tools.ietf.org/html/draft-ietf-netmod-rfc6020bis-11#ref-presence-stmt>]
                               [config-stmt]
                               [status-stmt]
                               [description-stmt]
                               [reference-stmt]
                               *(typedef-stmt / grouping-stmt)
                               *data-def-stmt
                               *action-stmt
                               *notification-stmt
                           "}") stmtsep

- Examples
I guess that no examples are supposed to compile, right?
Please add a sentence saying so.
When Andy's proposal will be ready (TAG: EXAMPLE-BEGINS => the YANG example compiles, NO TAG: => no supposed to compile), this document will already be compliant.

- Section 4.2.4

        +---------------------+-------------------------------------+
        | Name                | Description                         |
        +---------------------+-------------------------------------+
        | binary              | Any binary data                     |
        | bits                | A set of bits or flags              |
        | boolean             | "true" or "false"                   |
        | decimal64           | 64-bit signed decimal number        |
        | empty               | A leaf that does not have any value |
        | enumeration         | Enumerated strings                  |
        | identityref         | A reference to an abstract identity |
        | instance-identifier | References a data tree node         |

Editorial: What the difference between: A reference or references? Be consistent

- Section4.2.7 
<https://tools.ietf.org/html/draft-ietf-netmod-rfc6020bis-11#section-4.2.7>.  Choices

To be consistent with

    When a node from one case is created in the data tree, all nodes from
    all other cases are implicitly deleted.

This text in section 7.9 should be updated.
OLD:
    Since only one of the choice's cases can be valid at any time, the
    creation of a node from one case implicitly deletes all nodes from
    all other cases.  If a request creates a node from a case, the server
    will delete any existing nodes that are defined in other cases inside
    the choice.

NEW:
    Since only one of the choice's cases can be valid at any time_in the data tree_, the
    creation of a node from one case implicitly deletes all nodes from
    all other cases.  If a request creates a node from a case, the server
    will delete any existing nodes that are defined in other cases inside
    the choice.

- Section 4.2.9
      <rpc message-id="101"
           xmlns="urn:ietf:params:xml:ns:netconf:base:1.0">
        <activate-software-image xmlns="http://example.com/system">
          <image-name>example-fw-2.3</image-name>
       </activate-software-image>
      </rpc>

Editorial: Alignment

- Editorial: is there a logic in the numbering?
<rpc message-id="101"
<rpc message-id="102"

- section 4.2.9

    Operations can also be tied to a
    data node.  Such operations are defined with the "action" statement.

 From the definition:
    o  data node: A node in the schema tree that can be instantiated in a
       data tree.  One of container, leaf, leaf-list, list, anydata, and
       anyxml.

And I see in section 7.15:
    The "action" statement is used to define an operation connected to a
    specific container or list data node.

I believe it should be clarified in 4.2.9
NEW:
    Operations can also be tied to a
    container or list data node.  Such operations are defined with the "action" statement.

- Section 5.1
Editorial
OLD:
    o  A module or submodule belonging to that module can reference
       definitions in the module and all submodules included by the
       module.

NEW?
    o  A module, or submodule belonging to that module, can reference
       definitions in the module and all submodules included by that
       module.

- The following examples (as far as my quick check goes) are the only one that misses "yang-version 1.1"

    module example-augment {
       namespace "urn:example:augment";
       prefix mymod;
       import ietf-interfaces {
         prefix if;
       }

     module example-a {
        namespace urn:example:a;
        prefix a;

- Section 5.6.4
    If a server implements a module A that imports a module B, and A uses
    any node from B in an "augment" or "path" statement that the server
    supports, then the server MUST implement a revision of module B that
    has these nodes defined.  This is regardless of if module B is
    imported by revision or not.

"imported by revision or not" I'm, confused because I read the document in sequence.
 From 5.1 and 5.1.1, it doesn't look like we have two options (import by revision or not)
And the example shows the two possibilities:
        import b {
          revision-date 2015-01-01;
        }
        import c;

I found my answer in section 7.1.5:
    When the optional "revision-date" substatement is present, any
    typedef, grouping, extension, feature, and identity referenced by
    definitions in the local module are taken from the specified revision
    of the imported module.  It is an error if the specified revision of
    the imported module does not exist.  If no "revision-date"
    substatement is present, it is undefined from which revision of the
    module they are taken.

You should write a sentence or two (imported by revision or not) about in section 5.1 or 5.1.1

- section 5.6.4
    A server MUST NOT implement more than one revision of a module.

You should add that the server may contain more than one version for import reasons.
This would be in line with https://tools.ietf.org/html/draft-ietf-netconf-yang-library-05

        This mandatory list contains one entry for each YANG data model
        module supported by the server.  There MUST be an entry in this list
        for each revision of each YANG module that is used by the server.  It
        is possible for multiple revisions of the same module to be imported,
        in addition to an entry for the revision that is implemented by the
        server.

- section 5.6.4
ietf-yang-library comes out of the blue in section 5.6.4

    If a server implements a module A that imports a module C without
    specifying the revision date of module C, and the server does not
    implement C (e.g., if C only defines some typedefs), the server MUST
    list module C in the "/modules-state/module" list from
    "ietf-yang-library" [I-D.ietf-netconf-yang-library 
<https://tools.ietf.org/html/draft-ietf-netmod-rfc6020bis-11#ref-I-D.ietf-netconf-yang-library>], and it MUST set
    the leaf "conformance-type" to "import" for this module.

You should say a few words about it in section 4.
Alternatively, the content in 5.6.5 should come before 5.6.4

- Some statements in the ABNF section contains links. Intended? Talking with Martin, he submitted only the .txt version.
So it seems that the issues resides in the ietf submission tool chain. To be solved during AUTH48, I guess.

Example:

    container-stmt      = container-keyword sep identifier-arg-str optsep
                          (";" /
                           "{" stmtsep
                               ;; these stmts can appear in any order
                               [when-stmt]
                               *if-feature-stmt
                               *must-stmt
                               [presence-stmt 
<https://tools.ietf.org/html/draft-ietf-netmod-rfc6020bis-11#ref-presence-stmt>]
                               ...

    refine-stmt         = refine-keyword sep refine-arg-str optsep
                           "{" stmtsep
                               ;; these stmts can appear in any order
                               *if-feature-stmt
                               *must-stmt
                               [presence-stmt]
                               [default-stmt]
                               [config-stmt]
                               [mandatory-stmt]
                               [min-elements-stmt]
                               [max-elements-stmt 
<https://tools.ietf.org/html/draft-ietf-netmod-rfc6020bis-11#ref-max-elements-stmt>]
                               [description-stmt]
                               [reference-stmt]
                             "}" stmtsep

Regards, Benoit

---

From: Martin Bjorklund <[email-masked]>
Date: Thu, 21 Apr 2016 12:24:09 +0200 (CEST)

Hi Benoit,

>    the YANG mappings to NETCONF.

The first paragraph in the introduction mentions other protocols;
RESCTONF and CoMI.  My personal opinion is that this is sufficient,
but I'd like to hear what others think.

> version 1 in a specific paragraph?

Ok, this is a good idea.  I'll move them to a separate list.

> facing issues with 1.1 compilation?

I have tested to put "yang-version 1.1;" into all RFC-published
modules, and then ran pyang on them w/o any issues.

>       model is not known at module design time.

Ok, but I'd say:

NEW:

   o  anydata: A data node that can contain an unknown set of nodes that
      can be modelled by YANG, except anyxml.

This is just a summary; the details are to be found in later sections.

> If you intend to provide an adapted definition for the YANG mappings,

How about:

OLD:

NEW:

   o  configuration datastore: When modelled with YANG, a configuration
      datastore is an instantiated data tree with configuration data

   o  datastore: When modelled with YANG, an instantiated data tree

>    or value of nodes based on the presence or value of other nodes in

This is very early in the document, and the text tries to give a very
high level function overview.  I am not sure that mentioning "when" at
this time actually helps a first time reader.   I would prefer to
leave it as it is.

>  And notification, right? This should be added

Ok.

> Please add a sentence saying so.

I honestly does not think that this is an issue, but how about this:

NEW:

3.1.  A Note on Examples

   Throughout this document there are many examples of YANG statements.
   These examples are supposed to illustrate certain features, and are
   not supposed to be complete, valid YANG modules.

> When Andy's proposal will be ready (TAG: EXAMPLE-BEGINS => the YANG
> example compiles, NO TAG: => no supposed to compile), this document
> will already be compliant.
> 
> - Section 4.2.4
> 

No difference.  I'll change to A reference.

> - Section4.2.7
> - <https://tools.ietf.org/html/draft-ietf-netmod-rfc6020bis-11#section-4.2.7>.
> - Choices
> 
> To be consistent with
> 

Ok.

>       </activate-software-image>

Fixed.

> - Editorial: is there a logic in the numbering?

When there are two rpcs in the same example, I use 101, 102.

I double checked, and fixed two occurences where 101 was used instead
of 102.

>    container or list data node.  Such operations are defined with the

Ok.

>       definitions in the module and all submodules included by that

Yes, fixed.

> - The following examples (as far as my quick check goes) are the only
> - one that misses "yang-version 1.1"
> 
>    module example-augment {

Fixed.

OLD:

   Published modules evolve independently over time.  In order to allow
   for this evolution, modules need to be imported using specific
   revisions.  When a module is written, it uses the current revisions
   of other modules, based on what is available at the time. 

NEW:

   Published modules evolve independently over time.  In order to allow
   for this evolution, modules can be imported using specific revisions.
   When a module is written, it can import the current revisions of
   other modules, based on what is available at the time.

And then a new paragraph last in the same section (5.1.1):

NEW:

   If a module is not imported with a specific revision, it is undefined
   which excat revision is used.

> This would be in line with
> https://tools.ietf.org/html/draft-ietf-netconf-yang-library-05
> 

I started to write some text to address this issue, but it felt out of
place.  This section is about modules that are implemented.  Other
modules can be listed according to yang-library; but that is a
yang-library issue, not YANG 1.1.  So I prefer to leave this section
as it is.

> Alternatively, the content in 5.6.5 should come before 5.6.4

Agreed.  I switched the order of 5.6.4 and 5.6.5, and added a forward
reference:

   A NETCONF server announces the modules it implements (Section 5.6.5)

> - Some statements in the ABNF section contains links. Intended? Talking
> - with Martin, he submitted only the .txt version.
> So it seems that the issues resides in the ietf submission tool
> chain. To be solved during AUTH48, I guess.
> 
> Example:
> 

Ok.

/martin

---

From: Juergen Schoenwaelder <[email-masked]>
Date: Thu, 21 Apr 2016 15:33:05 +0200

> but I'd like to hear what others think.

I agree with less is more since we do not know how many protocols will
eventually transport YANG-defined data. What I like about Benoit's
proposal is that it clearly says that this document provides the
mappings to NETCONF. So how about this?

    YANG is a data modeling language used to model configuration data,
    state data, remote procedure calls, and notifications for network
    management protocols. This document also specifies the YANG mappings
    to the Network Configuration Protocol (NETCONF).

/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: Benoit Claise <[email-masked]>
Date: Thu, 21 Apr 2016 15:34:41 +0200

Hi Martin,

Thanks for engaging quickly.
[I removed the resolved entries]
> but I'd like to hear what others think.
The current abstract doesn't even mention the mapping to NETCONF.
> modules, and then ran pyang on them w/o any issues.
Great.
>> exclusively the YANG library
And this one?
> This is just a summary; the details are to be found in later sections.
Sure.
>     o  datastore: When modelled with YANG, an instantiated data tree
This issue is with "The following terms are defined in [RFC6241]", but 
you re-define those terms.
So give a warning about the redefinition to the readers.
> this time actually helps a first time reader.
The first time I read this, I was wondering how YANG data models can 
describe constraints on HOW data appear, while you wanted to express 
WHETHER a data appear. Maybe "when" is not the best way to help the 
first time user, but something is needed.
>   I would prefer to
Sure.
>     which excat revision is used.
Yes.
> yang-library issue, not YANG 1.1.
Not YANG 1.1 but NETCONF/YANG 1.1
>   So I prefer to leave this section
> as it is.
>
>
Regards, Benoit

---

From: Martin Bjorklund <[email-masked]>
Date: Thu, 21 Apr 2016 15:44:05 +0200 (CEST)

>     to the Network Configuration Protocol (NETCONF).

Yes, this is better!

/martin

---

From: Benoit Claise <[email-masked]>
Date: Thu, 21 Apr 2016 15:50:17 +0200

>>      to the Network Configuration Protocol (NETCONF).
Indeed.

Regards, Benoit
> .

---

From: Martin Bjorklund <[email-masked]>
Date: Thu, 21 Apr 2016 15:58:57 +0200 (CEST)

> The current abstract doesn't even mention the mapping to NETCONF.

See Juergen's proposal, I think that one is better.

NEW:

   The following changes are done to the NETCONF mapping:

   o  A server advertises support for YANG 1.1 modules by using ietf-
      yang-library [I-D.ietf-netconf-yang-library] instead of listing
      them as capabilities in the <hello> message.

> So give a warning about the redefinition to the readers.

Yes, that's what my proposed text does.  It says that "datastore" is
defined in 6241, and when YANG is used, it means the instantiated data
tree.

> first time user, but something is needed.

How about "restricting the presence or value of nodes"?

/martin

---

From: Ladislav Lhotka <[email-masked]>
Date: Fri, 22 Apr 2016 14:14:39 +0200

>    other modules, based on what is available at the time.

IMO, "published" should be removed unless its meaning is explained.

Lada

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

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

---

From: Benoit Claise <[email-masked]>
Date: Fri, 22 Apr 2016 14:26:28 +0200

Hi Martin,

Removed some extra ones on which we agree.
See in line.
OLD:

   The following terms are defined in [RFC6241
   <https://tools.ietf.org/html/rfc6241>]:

NEW:

   The following terms are defined in [RFC6241

> How about "restricting the presence or value of nodes"?
Yes, thank you.

Regards, Benoit

---

From: Martin Bjorklund <[email-masked]>
Date: Wed, 27 Apr 2016 09:55:11 +0200 (CEST)

> >> you re-define those terms.

I don't think it is correct to say that we "re-define" these terms.
It sounds like we give the terms a different meaning.  I agree that
the OLD text gave that impression, but I think the NEW proposed text
fixes this.

The terms are defined in 6241, and they keep their meaning.  We
clarify the meaning of two of the terms in a YANG context.

/martin

---

From: Benoit Claise <[email-masked]>
Date: Wed, 27 Apr 2016 12:04:03 +0200

Hi Martin,
> It sounds like we give the terms a different meaning.
Playing with words? :-)
>   I agree that
This does not work.

Reading the terminology ...

      o  configuration datastore: ...

      o  datastore: ...

... I will not even bother reading the definitions, because I know them 
from 6241.
Doing this, I will not spot the subtle "different meaning" you inserted 
in the definitions.

Regards, Benoit

---

From: Juergen Schoenwaelder <[email-masked]>
Date: Wed, 27 Apr 2016 12:27:48 +0200

> > It sounds like we give the terms a different meaning.

That's what the IETF is good for. ;-)

I think we _refine_ definitions here to make it clear what they mean
in the context of YANG. We do not _redefine_ definitions.

/js

---

From: Martin Bjorklund <[email-masked]>
Date: Wed, 27 Apr 2016 12:43:24 +0200 (CEST)

Benoit Claise <[email-masked]> wrote:
> ... I will not even bother reading the definitions, because I know
> them from 6241.
> Doing this, I will not spot the subtle "different meaning" you
> inserted in the definitions.

Ok, how about moving the specialized-meaning-text to its own paragraph:

  The following terms are defined in [RFC6241]:

   o  configuration data

   o  configuration datastore

   o  datastore

   o  running configuration datastore

   o  state data

   When modelled with YANG, a datastore is realized as an instantiated
   data tree.

   When modelled with YANG, a configuration datastore is realized as an
   instantiated data tree with configuration data.

/martin

---

From: Juergen Schoenwaelder <[email-masked]>
Date: Wed, 27 Apr 2016 13:19:23 +0200

>    instantiated data tree with configuration data.

Looks good to me.

/js

---

From: Ladislav Lhotka <[email-masked]>
Date: Wed, 27 Apr 2016 13:20:00 +0200

> Doing this, I will not spot the subtle "different meaning" you inserted 

I think it would be useful to state what "configuration data" mean in
YANG context: it is a subtree of the main data tree containing only
instances of "config true" data nodes. I don't think the definition
inherited from 6241 is useful, in fact I am not even sure it is correct.

The problem I have with the refined definitions of "datastore" and
"configuration datastore" is the cabbalistic word "instantiated". I
think it should be explained or removed. As a matter of fact, the
meaning of "instantiate" in the definition of "uses" is clearly
different.

Lada

---

From: t.petch <[email-masked]>
Date: Wed, 27 Apr 2016 12:36:44 +0100

I see that the definition of 'datastores' has cropped up in this AD
Review, as in the e-mail below.

Meanwhile, draft-ietf-i2rs-pub-sub-requirements-05.txt is in IETF Last
Call and redefines, or recreates, the term for us

   A YANG datastore is a conceptual datastore that contains hierarchical
   data defined in YANG data models.  It is what is referred in existing
   RFCs as "NETCONF datastore".  However, as the same datastore is no
   longer tied to NETCONF as a specific transport, the term "YANG
   datastore" is deemed more appropriate.

I think that the concept of datastore has been troublesome to those
coming to YANG lately, such as openconfig and I2RS, and that this will
just muddy the waters more, especially as it is tucked away in an
Informational document.  If I2RS want to define such terminology, then
it should be in the I2RS Architecture or some such; but IMHO they should
not be defining YANG datastores at all.

Tom Petch

----- Original Message -----
From: "Martin Bjorklund" <[email-masked]>
To: <[email-masked]>
Cc: <[email-masked]>
Sent: Thursday, April 21, 2016 2:58 PM
Subject: Re: [netmod] AD review draft-ietf-netmod-rfc6020bis-11 (part 1)

> > >> Here is part 1 of my AD review.
http://tools.ietf.org/rfcdiff/rfcdiff.pyht?url1=http://www.ietf.org/rfc/
rfc6020.txt&url2=http://www.ietf.org/id/draft-ietf-netmod-rfc6020bis-11.
txt
> > >> RFC6241 has different definition for "configuration datastore"
and
> > >> "datastore".
> > >> If you intend to provide an adapted definition for the YANG
mappings,
> > >> then you should say so.

---

From: Benoit Claise <[email-masked]>
Date: Wed, 27 Apr 2016 14:06:53 +0200

>     instantiated data tree with configuration data.
That works.

Regards, Benoit

---

From: Juergen Schoenwaelder <[email-masked]>
Date: Wed, 27 Apr 2016 14:15:33 +0200

Tom,

please send a comment to i2rs saying that they should import terms
rather than trying to (re-)define them. As long as people keep
(re-)defining terms, we will have confusion and in this specific case
I think it is the NETMOD or NETCONF WGs that should define the terms,
not any other WGs.

/js

---

From: Benoit Claise <[email-masked]>
Date: Wed, 27 Apr 2016 14:20:19 +0200

Tom,
> not be defining YANG datastores at all.
Agreed.
Timely review as draft-ietf-i2rs-pub-sub-requirements is on the next 
IESG telechat.

Regards, Benoit