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
> [email protected]
> https://www.ietf.org/mailman/listinfo/rtgwg
>
_______________________________________________
rtgwg mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/rtgwg
