Re: YANG doctor review of draft-ietf-rtgwg-ni-model-02

[Lou Berger <[e-mail]>]

Martin,

    sorry about the slow response, but as you know we've been waiting to
update this document until schema-mount was updated/finalized. 


On 4/23/2017 5:48 AM, Martin Bjorklund wrote:
> 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";
>      }
okay.

>
> 2) IETF boilerplate
>
>   Use IETF boilerplate with contact, description w/ copyright etc.
will do.
>
> 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?
 this is vestigial and can be deleted

>
> 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.
>
same issue.

> 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.
agreed.

>
> 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? 
yes

>  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.
agreed. Hope to have this done in the rev we're working now.

> 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?
how about:

       "Add a node for the identification of the network instance
        (which is within the interface's identified physical or
        virtual device) associated with the information
        configured on an interface";

>
>     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?
How about:

       "Add a node for the identification of the network instance
        (which is within the interface's identified physical or
        virtual device) associated with the IP information
        configured on an interface";

>
> 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";
>       }
>       ...
>     }
good catch, this was left over form earlier version.
>
> 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.
>
will do

>
> 11)  section 2
>
>   Align with section 2 in the LNE document.
>
will do

> 12)  section 3
>
>   Can the same interface be bound to both an LNE and an NI?  If not,
>   this needs to be explained.
>
okay.

> 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)
>
I personally like the tree representation to show instance data find it
as much more readable for informative text, but agree that this isn't a
'standard' convention so perhaps we need to move way from it.  The next
rev will use JSON representation for example instance data.

>
>
> /martin

Thank you for the comments!
> _______________________________________________
> rtgwg mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/rtgwg
>