[yang-doctors] YANG doctor review of draft-ietf-rtgwg-lne-model-02

[Martin Bjorklund <[e-mail]>]

Hi,

I am the assigned YANG doctor for draft-ietf-rtgwg-lne-model, and
here are my review comments, based on -02:


1) Add reference to import statements.

     import ietf-interfaces {
       prefix if;
       reference
         "RFC 7223: A YANG Data Model for Interface Management";
     }


2) IETF boilerplate

  Use IETF boilerplate with contact, description w/ copyright etc.


3) feature bind-lne-name

     feature bind-lne-name {
       description
         "Logical network element to which an interface is bound";
     }

  This description doesn't make much sense.

  Also, this feature is not used in the model.  Should the feature be
  removed or used?


4) leaf managed

      leaf managed {
        type boolean;
        description
          "True if the host can manage the LNE using the root mount
           point";
      }

  This description needs to be expanded.  This is a config leaf, so
  what happens if a client sets this to 'true' for a certain LNE?

  The text in the document says:

    In addition to standard management interfaces, a host device
    implementation may support accessing LNE configuration and
    operational YANG models directly from the host system.  When
    supported, such access is accomplished through a yang-schema-mount
    mount point

  So are there three cases involved?

     1.  The host device does NOT support accessing LNE data from the
         host system => in this case no modules will be mounted.

     2.  The host device supports reading LNE data from the host
         system => in this case the modules are mounted read-only.

     3.  The host device supports reading/writing LNE data from
         the host system => in this case the modules are mounted
         read-write.

  I _think_ the idea is that if the server supports 2 or 3, a client
  can choose to set 'managed' to "false", in which case it turns into
  case 1.  Is this correct?  If so, would it be useful to allow a
  client to turn case 3 into 2?

  The text in the document in section 3 has more details on the
  "managed" leaf.  I suggest you add text from the document to the
  YANG module.


5) leaf bind-lne-name

  This leaf is a string.  Shouldn't it be a leafref?

    leaf bind-lne-name {
      type leafref {
        path "/logical-network-elements/logical-network-element/name";
      }
      ...
    }


6) inconsistent formatting

  I suggest you run pyang -f yang [--keep-comments] and possibly edit
  the result add/remove comments.

  The following comment doesn't add much and should be removed:

   // namespace
   namespace "urn:ietf:params:xml:ns:yang:ietf-logical-network-element";

  Also remove the comments about rpcs and notifications.

  Also add period ('.') at the end of all sentences in the
  descriptions.


7) YANG tree

  The document should explain the tree diagram syntax.


  Section 2 has this tree diagram:

             +--rw interfaces
             |  +--rw interface* [name]
             |     +--rw name                       string
             |     +--rw lne:bind-lne-name?         string
             |     +--rw ethernet
             |     |  +--rw ni:bind-network-instance-name? string
             |     |  +--rw aggregates
             |     |  +--rw rstp
             |     |  +--rw lldp
             |     |  +--rw ptp
             |     +--rw vlans
             |     +--rw tunnels
             |     +--rw ipv4
             |     |  +--rw ni:bind-network-instance-name? string
             |     |  +--rw arp
             |     |  +--rw icmp
             |     |  +--rw vrrp
             |     |  +--rw dhcp-client
             |     +--rw ipv6
             |        +--rw ni:bind-network-instance-name? string
             |        +--rw vrrp
             |        +--rw icmpv6
             |        +--rw nd
             |        +--rw dhcpv6-client


  This diagram is not correct; it seems to indicate that there are
  nodes 'ethernet', 'vlans', 'ipv4', etc in the ietf-interfaces
  module.  I suggest you remove the nodes that do not exist, and
  change 'ipv4' to 'ip:ipv4' (and add a reference to RFC 7277).


8)  YANG tree (2)

  Section 3 has this diagram:

       module: ietf-logical-network-element
          +--rw logical-network-inventory
             +--rw logical-network-element* [name]
                +--rw name?   string
                +--rw description? string
                +--rw managed?     boolean
                +--rw root?        yang-schema-mount
       augment /if:interfaces/if:interface:
          +--rw bind-lne-name?     string


  It needs to be updated to match the YANG model (rename
  logical-network-inventory), and the 'root' should not be shown as a
  leaf.  (Yes I know that there currently is no syntax defined for a
  mount point in a tree)


9)  YANG tree (3)

  Section 3.1 uses a tree diagram to show instance data.  I think this
  is confusing, and you should use XML or JSON instead.


10)  typo

   s/The interface management model [RFC7223] is and existing model/
     The interface management model [RFC7223] is an existing model/



/martin