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

[Andy Bierman <[e-mail]>]

Hi,

2 concerns:

1) separate config and state as best practice.
IMO this is not correct.  If the state has no meaning unless
the config exists, then it is fine to put the config=false nodes
under the config=true nodes (sharing instances)

2) server-defined extension
I keep hearing about this but have never seen a reasonable definition.
Does this mean the server MUST, SHOULD, or MAY provide a value?
(pick 1)

Isn't this situation already covered by YANG since it never says
creating or modifying data can only be done by a client?


Andy


On Thu, Jul 7, 2016 at 6:05 PM, Alexander Clemm (alex) <[e-mail]>
wrote:

> 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] <[e-mail]>]
> *Sent:* Tuesday, July 5, 2016 2:55 PM
> *To:* [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
>
>
>
>
>
>
>
> _______________________________________________
> yang-doctors mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/yang-doctors
>
>