Re: YANG doctor review of draft-ietf-rtgwg-lne-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. I think
it's close enough now and certainly we have  an update for this upcoming
meeting, links to formatted text can be found in the repo
https://github.com/ietf-rtg-area-yang-arch-dt/meta-model . See below for
specific responses:


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

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

> 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";
>       }
>       ...
>     }

agreed. Thanks!

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

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

this is fully replaced by an examples appendix in the forthcoming version.

>
> 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)

The document is now aligned with draft-ietf-netmod-yang-tree-diagrams-01

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

Same comment as NI:The next rev will use JSON representation for example
instance data.


> 10)  typo
>
>    s/The interface management model [RFC7223] is and existing model/
>      The interface management model [RFC7223] is an existing model/
>

Thanks.

Lou

PS I failed to hit send at the same time as the NI response, so this sat
around in my drafts folder for a while.  But this allowed me to update
the above based on the forthcoming rev, so all the better.

>
> /martin
>
> _______________________________________________
> rtgwg mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/rtgwg
>