Early Review of draft-ietf-rtgwg-ni-model-02
review-ietf-rtgwg-ni-model-02-yangdoctors-early-bjorklund-2017-04-26-00

Hi,

I am the assigned YANG doctor for draft-ietf-rtgwg-ni-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-network-instance-name

    feature bind-network-instance-name {
      description
        "Network Instance to which an interface instance 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) unused groupings

  The module defines 3 groupings that are not used:

    interface-ip-common
    ipv4-interface-protocols
    ipv6-interface-protocols

  Either they should be removed, or the text needs to explain how they
  are supposed to be used by other modules.


5)  network-instances

  The module has this:

    container network-instances {
        description "Network instances each of which have
                     an independent IP/IPv6 addressing space
                     and protocol instantiations. For layer 3,
                     this consistent with the routing-instance
                     definition in ietf-routing";

  There is no "routing-instance" in "ietf-routing".  This description
  needs to be updated.


6)  leaf type

          leaf type {
              type identityref {
                  base network-instance-type;
              }
              description
                  "The network instance type -- details TBD
                   Likely types include core, L3-VRF, VPLS,
                   L2-cross-connect, L2-VSI, etc.";
          }

  "details TBD" - needs to be fixed.

  Should this leaf be mandatory?  If not, it needs to be specified
  what it means if this leaf is not present.


7)  container network-instance-policy

    container network-instance-policy {
        description
          "Network Instance Policy -- details TBD,
          perhaps based on BESS model";
    }

  "details TBD" - needs to be fixed.


8)  augments

    augment "/if:interfaces/if:interface" {
      description
          "Add a node for the identification of the logical network
          instance (which is within the interface's identified logical
          network element) associated with the IP information
          configured on an interface";


  Does this mean that this model cannot be used without the LNE model?


    augment "/if:interfaces/if:interface/ip:ipv4" {
      description
          "Add a node for the identification of the logical
          network instance (which is within the interface's
          identified physical or virtual device) associated with
          the IP information configured on an interface";

  What does "which is within the interface's identified physical or
  virtual device" mean?


9) leaf bind-network-instance-name

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

    leaf bind-network-instance-name {
      type leafref {
        path "/network-instances/network-instance/name";
      }
      ...
    }


10) 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-network-instance";

  Also remove the comments about rpcs and notifications.

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



11)  section 2

  Align with section 2 in the LNE document.


12)  section 3

  Can the same interface be bound to both an LNE and an NI?  If not,
  this needs to be explained.


13) YANG tree

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

  (see my comments on YANG tree in the LNE review as well)




/martin