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

[Andy Bierman <[e-mail]>]

Hi,

I think you have to use 2 different objects with different names,
1 config=true and the other config=false.
If the config=false node exists then the config=true node is ignored.
The description-stmt defines how they are related.

Seems like you are describing the ephemeral datastore.
You should wait for the datastores RFC that will hopefully be
discussed in Berlin.


Andy


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

> Hi,
>
>
>
> what is the suggested course of action then?
>
>
>
> There will be some instances that will be populated by a server, others by
> a client app.  We do want client configured instances to reference
> instances that are server-provided (e.g. for scenarios in which an overlay
> topology is configured on top of another topology provided by the
> network).  So, we do need them to be part of the same datastore, as I don’t
> think it is possible for objects in one datastore to reference objects in
> another datastore.
>
>
>
> Perhaps that would indeed make the case to define a YANG extension for
> server provided, that defines the additional semantics that config with
> that tag is subject to getting undone by the server. The model would
> essentially be the same, but the semantics would be more explicit than just
> putting this into documentation.
>
>
>
> Of course, at the end of the day the situation is not so much different
> from other cases where multiple clients want to modify the same
> configuration.  Ultimately, clients should be “well behaved” to only touch
> what they should own, respectively authorization rules / NACM should be
> defined to establish clear ownership as needed.
>
>
>
> What would be your preference to do here – server-provided extension, or
> just keep server-provided leaf?  (or another suggestion?)
>
>
>
> Thanks
>
> --- Alex
>
>
>
> *From:* Andy Bierman [mailto:[e-mail]]
> *Sent:* Thursday, July 07, 2016 8:04 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,
>
>
>
> 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
>
>
>
>
>