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