Re: [yang-doctors] YANG Doctor Review of draft-ietf-i2rs-yang-network-topo-03

["Alexander Clemm (alex)" <[e-mail]>]

Hi Kent,

thank you for your review!

Please find replies inline, <ALEX>.

--- Alex

From: Susan Hares [mailto:[e-mail]]
Sent: Wednesday, July 06, 2016 12:49 AM
To: 'Kent Watsen' <[e-mail]>; [e-mail]
Cc: 'YANG Doctors' <[e-mail]>; Alexander Clemm (alex) <[e-mail]>
Subject: RE: YANG Doctor Review of draft-ietf-i2rs-yang-network-topo-03

Kent:

Thank you for the Yang Doctor reviews.  The authors will respond to your comments shortly.

Sue

From: Kent Watsen [mailto:[e-mail]]
Sent: Tuesday, July 5, 2016 2:55 PM
To: [e-mail]<mailto:[e-mail]>
Cc: YANG Doctors
Subject: YANG Doctor Review of draft-ietf-i2rs-yang-network-topo-03

Hi,

I am the assigned YANG doctor for this individual submission document.  This review regards the YANG modules alone.  While I did read the draft, I only did so to better understand the modules.   Note: I was hoping the draft would contain examples, but alas it does not.

This draft contains two yang modules: ietf-network and ietf-network-topology
    - ietf-network-topology imports and augments ietf-network
    - both yang modules pass `pyang --ietf` testing.

These modules are relatively small and straightforward, which makes reviewing them easy.  Below are some detailed comments on these two modules.

ietf-network
==========

•         grouping “node-ref” is defined but not used in this module, though it is used by the ietf-network-topology module.  Should the “node-ref” grouping should be moved to the ietf-network-topology module?

<ALEX> You are correct that the grouping is not actually used in this module.  The reason we had it in ietf-network was basically as a convenience, expecting that other modules will need to import the same module.  For that reason, unless you tell us there are strong reasons not to, we would actually prefer to keep it.</ALEX>

•         Why have top-level container “networks-state”?   Why not have the “server-provided” config false leaf in /nd:networks/nd:network/ instead?

<ALEX> The server-provided leaf is an item that has gone back and forth.  We are actually not entirely happy with the way it is currently in the model either; in fact in the ODL implementation it is part of /nd:networks/nd:network, just as you indicate and we had some controversial discussions surrounding it.  The reason for the separate container was the “best practice” of separating config branches and state branches.  While we have only a single config=false leaf, we thought that because of that “best practice” the branch would be needed.



Based on your comments, we will strike the branch then.  We will move the leaf back into the main branch, respectively introduce instead a YANG extension to tag whether an instance is server provided or not.  We will run this by you in the next revision.

</ALEX>



•         Is there a need to define any notifications in this module?  - or is the plan to leverage yang-push?

<ALEX> We did not really foresee the need for notifications that would be specific to networks and network topologies.  Updates on topologies, including topology changes, could be sent using yang-push, as you indicate.   If topology changes would have to be always notified, I guess there is a case that could be made to introduce custom notifications that would be available independent of YANG-push.   However, that would impose the requirement to implement additional “stuff”.



That said, there is one type of notifications that might be worth considering, namely cases in which something in an underlay topology changes, that impacts something in an overlay topology.  In the event that you have e.g. a termination point that maps to a physical port, and the physical port is deleted, it is possible for the overlay to refer to a node that no longer exists (this is permissible with YANG 1.1 – we have “require-instance false” for the corresponding leafrefs in the model).  In such a case, one might arguably build a case to request a notification when a change in a server-provided instance causes a configured instance to no longer be in a state where has the intended “support”.



There are reasons to not introduce a notification for this case: for one, it again imposes the requirement to implement additional “stuff” – perhaps something that might be considered for an additional “topology monitoring model”, but not a base topology model.  Also, it might result in redundant notificaitons from multiple servers with a view of the same network/topology that all detect the same condition, and possibly a large volume of notifications under certain conditions – may not be worth the while given that these will in general be only very transient conditions and applications “owning” the overlay will apply prompt remappings.



For those reasons, my preference would be to not introduce notifications at this point.  Please let us know if you think we should reconsider.

</ALEX>

ietf-network-topology
==================

•         The description statements for “link-id” and “tp-id” were not helpful.  The node is a URI, so I expect the description statement to be mostly about setting the URI value.  Example URI values would be helpful.



<ALEX> I will add some more text.  That said, the precise structure will generally be picked by an implementation.

</ALEX>

•         Watch capitalization on the “link” description statement: “A Network Link connects a by Local (Source) node and a Remote (Destination) Network Nodes.”

<ALEX> Will fix the description.

</ALEX>

•         The sentence structure of the first sentence in the “link” description statement needs fixing.

<ALEX> Will fix the description.

</ALEX>



•         Using uncommon acronyms is discouraged.  Recommend replacing “tp” with “termination-point” throughout.

<ALEX> Will check to make sure we expand it where possible in the descriptions.  In the names, we do not plan to replace it due to length considertations.

</ALEX>



•         Is the description statement for the grouping link-ref missing the word “to”?

<ALEX> Will fix the description.

</ALEX>



•         The description for “supporting-termination-point” says “leaf list”, though it’s actually just a list...

<ALEX> Will fix the description.

</ALEX>

•         I notice that ‘require-instance false’ is used on all leafrefs except source/dest-node and source/desy-tp, is this intentional?
<ALEX> Good point.  The reason we don’t have it in the source/destination leafrefs is that those represent “horizontal” relationships in the same network/topoogy, not “vertical” relationships.  The scenario we were concerned about is the one in which a (server-provided) resource in an underly undergoes some change, affecting a (configured) resource in an overlay.  Within a topology, everything is either server provided or configured; we do not have a mix of the two; therefore the scenario of a leafref without valid instance is something that really is not applicable.  That said, I don’t see a reaosn why we should not allow to relax this, so let’s add “require-instance false” also to those nodes.
</ALEX>

Kent