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

[Andy Bierman <[e-mail]>]

Hi,

I think that config that immediately gets undone by the server stretches
the definition of config=true a bit too far.


The so-called ephemeral datastore would be where you put config that
is not subject to backup/restore.


Andy


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

> Hi Andy,
>
>
>
> ok. So, with regards to 1, then we really don’t need the separate
> networks-state container / branch.  Will remove this.  This is much
> preferred actually.
>
>
>
> With regards to 2, this means that we should be fine with having the model
> all config, even in cases where the server creates / modifies data.  The
> “server-provided” is essentially intended as a “hint” as to who populated
> the data, and as a hint that the attempt to reconfigure this data by
> another client – assuming it had the privileges to do so - will be
> immediately “undone”.  This is something we can add to the text, in fact,
> to the effect that regular clients should not be authorized to modify
> network/topology instances that are server-provided.  There was also a
> concern earlier that with config data, all data would be subjected to
> backup and restore operations, which in the case of data that is populate
> by a server would not be required, just like state data would not be
> subjected to backup/restore.  I think you are implying this should not be
> an issue – even if server-provided data were restored, the server could
> essentially “override” to reflect the actual.  So I guess this means we can
> put this concern to rest.  Then I think there is only one potential issue
> that remains, concerning when someone locks all configuration data.  This
> would imply that in such situations, the “server-provided” topology
> information cannot be updated even by the server until the lock is
> released.  Presumably even this should be acceptable.
>
>
>
> I think this simplifies things.  I will basically return the model to the
> earlier version with the server-provided leaf as part of the rest of the
> topology information, adding the points from the description from this
> discussion. In that case, we would not even need a YANG extension (which
> would be an alternative to the server-provided leaf: instead of the leaf,
> we would introduce an extension “server-provided” with a boolean
> argument).  Does this sound acceptable to everyone?
>
>
>
> Thanks
>
> --- Alex
>
>
>
> *From:* Andy Bierman [mailto:[e-mail]]
> *Sent:* Thursday, July 07, 2016 6:25 PM
> *To:* Alexander Clemm (alex) <[e-mail]>
> *Cc:* Susan Hares <[e-mail]>; Kent Watsen <[e-mail]>;
> [e-mail]; YANG Doctors <
> [e-mail]>
> *Subject:* Re: [yang-doctors] YANG Doctor Review of
> draft-ietf-i2rs-yang-network-topo-03
>
>
>
> 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
>
>
>