Early Review of draft-ietf-netmod-rfc8022bis-05
review-ietf-netmod-rfc8022bis-05-yangdoctors-early-bjorklund-2017-12-21-00

Request Review of draft-ietf-netmod-rfc8022bis-04
Requested rev. 04 (document currently at 11)
Type Early Review
Team YANG Doctors (yangdoctors)
Deadline 2018-01-12
Requested 2017-12-18
Requested by Mehmet Ersue
Other Reviews Secdir Telechat review of -10 by Carl Wallace (diff)
Genart Telechat review of -06 by Francis Dupont (diff)
Opsdir Telechat review of -07 by Joe Clarke (diff)
Rtgdir Telechat review of -08 by He Jia (diff)
Comments
Requested by YANG Doctors secretary on behalf of the WG chairs.
Review State Completed
Reviewer Martin Björklund
Review review-ietf-netmod-rfc8022bis-05-yangdoctors-early-bjorklund-2017-12-21
Posted at https://mailarchive.ietf.org/arch/msg/yang-doctors/rBFR-Pj6pCcnnUVfY6DfBR0dmOQ
Reviewed rev. 05 (document currently at 11)
Review result Ready with Nits
Draft last updated 2017-12-21
Review completed: 2017-12-21

Review
review-ietf-netmod-rfc8022bis-05-yangdoctors-early-bjorklund-2017-12-21

Hi,

I have reviewed this document mainly from a NMDA perspective.  As
expected, the YANG model follows all conventions and looks good in
general.  Most of my comments below relate to NMDA terminology.



o  Abstract and Introdcution

Both these has:

   The main difference from the first version is that this version fully
   conforms to the Network Management Datastore Architecture (NMDA).
   Consequently, this document obsoletes RFC 8022.

But this is a bit w/o context - no "first version" is mentioned.
Maybe:

   The YANG modules in this document conform to the Network
   Management Datastore Architecture (NMDA).  This document obsoletes
   RFC 8022.


o  Terminology

The term "state data" is imported from 7950, but this is not correct,
it is actually defined in 6241.  But I suggest you remove this term,
and instead use the terms defined in the NMDA document.  Actually,
most of the rest of my comments below is about this terminology usage.


o  Section 2.1

OLD:

   user-controlled entry:  An entry of a list in operational state data

NEW:

   user-controlled entry:  An entry of a list in operational state



o  Section 4.1

Use the imported terms "intended configuration" and "operational
state" consistently:


OLD:

   In such a list, the server creates the required item as a so-called
   system-controlled entry in state data in the operational state
   datastore [I-D.ietf-netmod-revised-datastores], i.e., inside read-
   only lists in the "routing" container.

NEW:

   In such a list, the server creates the required item as a so-called
   system-controlled entry in the operational state,
   i.e., inside read-only lists
   in the "routing" container.

(the reference isn't needed here, the term is imported in the
Terminology section)


OLD:

   Additional entries may be created in the configuration by a client,
   e.g., via the NETCONF protocol.  These are so-called
   user-controlled entries.  If the server accepts a configured
   user-controlled entry, then this entry also appears in the state
   data version of the list.

NEW:

   Additional entries may be created in the configuration by a client,
   e.g., via the NETCONF protocol.  These are so-called
   user-controlled entries.  If the server accepts a configured
   user-controlled entry, then this entry also appears in the
   operaitonal state version of the list.


OLD:

   Corresponding entries in both versions of the list (in operational
   state datastore and intended datastore

NEW:

   Corresponding entries in both versions of the list (in the intended
   configuration and the operational state)


OLD:

   In order to bind this entry to the corresponding entry in the state
   data list in the operational state datastore, the key of the
   configuration entry has to be set to the same value as the key of
   the state entry.

NEW:

   In order to bind this entry to the corresponding entry in the
   operational state, the key of the configuration entry has to be set
   to the same value as the key of the operational state entry.


OLD:

   Deleting a user-controlled entry from the configuration list results
   in the removal of the corresponding entry in the state data list.  In
   contrast, if client delets a system-controlled entry from the
   configuration list in the intended datastore, only the extra
   configuration specified in that entry is removed but the
   corresponding state data entry remains in the list in the operational
   datastore.

NEW:

   Deleting a user-controlled entry from the intended configuration
   results in the removal of the corresponding entry in the
   operational state list.  In contrast, if client deletes a
   system-controlled entry from the intended configuration, only the
   extra configuration specified in that entry is removed but the
   corresponding operational state entry is not removed.


o  Section 5.2

OLD:

   Routes are primarily state data that appear as entries of RIBs

NEW:

   Routes are primarily system state that appear as entries of RIBs


OLD:

   In the core routing data model, RIBs are state data represented as
   entries of the list "/routing/ribs/rib" in the operational state
   datastore [I-D.ietf-netmod-revised-datastores].

NEW:

   In the core routing data model, RIBs are represented as entries of
   the list "/routing/ribs/rib" in the operational state.


o  Section 5.3.2

OLD:

   It is expected that future YANG modules will create data models for
   additional control-plane protocol types.  Such a new module has to
   define the protocol-specific configuration and state data, and it has
   to integrate it into the core routing framework in the following way:

NEW:

   It is expected that future YANG modules will create data models for
   additional control-plane protocol types.  Such a new module has to
   define the protocol-specific data nodes, and it has
   to integrate it into the core routing framework in the following way:


OLD:

   o  Additional route attributes MAY be defined, preferably in one
      place by means of defining a YANG grouping.  The new attributes
      have to be inserted by augmenting the definitions of the node

       /rt:routing/rt:ribs/rt:rib/rt:routes/rt:route

      and possibly other places in the configuration, state data,
      notifications, and input/output parameters of actions or RPC
      operations.

   o  Configuration parameters and/or state data for the new protocol
      can be defined by augmenting the "control-plane-protocol" data
      node under "/routing".

   By using a "when" statement, the augmented configuration parameters
   and state data specific to the new protocol SHOULD be made
   conditional and valid only if the value of "rt:type" or
   "rt:source-protocol" is equal to (or derived from) the new protocol's
   identity.

NEW:

   o  Additional route attributes MAY be defined, preferably in one
      place by means of defining a YANG grouping.  The new attributes
      have to be inserted by augmenting the definitions of the node

       /rt:routing/rt:ribs/rt:rib/rt:routes/rt:route

      and possibly other places in the schema tree.

   o  Data nodes for the new protocol
      can be defined by augmenting the "control-plane-protocol" data
      node under "/routing".

   By using a "when" statement, the augmented data nodes
   specific to the new protocol SHOULD be made
   conditional and valid only if the value of "rt:type" or
   "rt:source-protocol" is equal to (or derived from) the new protocol's
   identity.


o  Section 5.4

OLD:

   YANG module "ietf-ipv6-router-advertisements" (Section 9.1), which is
   a submodule of the "ietf-ipv6-unicast-routing" module, augments the
   configuration and state data of IPv6 interfaces with definitions of
   the following variables as required by Section 6.2.1 of [RFC4861]:

NEW:

   YANG module "ietf-ipv6-router-advertisements" (Section 9.1), which is
   a submodule of the "ietf-ipv6-unicast-routing" module, augments the
   schema tree of IPv6 interfaces with definitions of
   the following variables as required by Section 6.2.1 of [RFC4861]:


o  In ietf-routing

  feature router-id {
    description
      "This feature indicates that the server supports configuration
       of an explicit 32-bit router ID that is used by some routing
       protocols.

and

  container routing {
    description
      "Configuration parameters for the routing subsystem.";
    uses router-id {
      if-feature "router-id";
      description
        "Configuration of the global router ID.  Routing protocols
         that use router ID can use this parameter or override it
         with another value.";

This design actually works:

If a server supports configuration of router-id, it would advertise
the feature "router-id" in the schema for the conventional
configuration datastores, and in the schema for the operational state
datastore.

If a server does not support the configuration of the router-id, it
would have to advertise this feature in the schema for the operational
datastore (but not for conventional).

But if you want to keep this design, the description of the feature
has to change - it is no longer about "configuration", but rather just
"support".


o  In ietf-routing

The description of several nodes says: "Configuration of" or
"Configuration parameters for".  This needs to change; it is not just
about configuration.  This applies to several nodes in the modules.


o  In ietf-routing

OLD:

  grouping next-hop-state-content {
    description
      "Generic parameters of next hops in state data.";
    choice next-hop-options {
      mandatory "true";
      description
        "Options for next hops in state data.

NEW:

  grouping next-hop-state-content {
    description
      "Generic state parameters of next hops.";
    choice next-hop-options {
      mandatory "true";
      description
        "Options for next hops.

OLD:

          description
            "The name of the RIB.

             For system-controlled entries, the value of this leaf
             must be the same as the name of the corresponding entry
             in state data.

NEW:

          description
            "The name of the RIB.

             For system-controlled entries, the value of this leaf
             must be the same as the name of the corresponding entry
             in operational state.


o  In all YANG modules except ietf-routing

In the module description:

OLD:

     This YANG module augments the 'ietf-routing' module with basic
     configuration and state data for IPv4 unicast routing.

NEW:

     This YANG module augments the 'ietf-routing' module with basic
     parameters for IPv4 unicast routing.
     
(or maybe s/parameters/data nodes/)

There's a similar statement in all modules.


o  In all the YANG modules

This is minor, but it sticks out:

OLD:

    description "A Network Management Datastore Architecture (NDMA)
                 compatible version of the ietf-interfaces module
                 is required.";


NEW:

    description
      "A Network Management Datastore Architecture (NDMA)
       compatible version of the ietf-interfaces module
       is required.";

There are similar statements in all modules.


o  Section 11

OLD:

   The YANG module specified in this document defines a schedma for data

NEW:

   The YANG modules specified in this document define a schema for data



o  Appendix D

OLD:

   This section contains an example of an instance data tree in the JSON
   encoding [RFC7951], containing both configuration and state data.

NEW:

   This section contains an example of an instance data tree from the
   operational state, in the JSON encoding [RFC7951]



/martin