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

"Alexander Clemm (alex)" <alex@cisco.com> Fri, 08 July 2016 03:46 UTC

Return-Path: <alex@cisco.com>
X-Original-To: yang-doctors@ietfa.amsl.com
Delivered-To: yang-doctors@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id B17F512B03C; Thu, 7 Jul 2016 20:46:25 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -15.946
X-Spam-Level:
X-Spam-Status: No, score=-15.946 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_HI=-5, RCVD_IN_MSPIKE_H3=-0.01, RCVD_IN_MSPIKE_WL=-0.01, RP_MATCHES_RCVD=-1.426, SPF_PASS=-0.001, USER_IN_DEF_DKIM_WL=-7.5] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (1024-bit key) header.d=cisco.com
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id c3Azgsah6Ec1; Thu, 7 Jul 2016 20:46:22 -0700 (PDT)
Received: from rcdn-iport-9.cisco.com (rcdn-iport-9.cisco.com [173.37.86.80]) (using TLSv1.2 with cipher DHE-RSA-SEED-SHA (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id DDEFF126FDC; Thu, 7 Jul 2016 20:46:21 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=cisco.com; i=@cisco.com; l=78248; q=dns/txt; s=iport; t=1467949582; x=1469159182; h=from:to:cc:subject:date:message-id:references: in-reply-to:mime-version; bh=sWz9FvTxRLLMvqyrxMXuIDof15NTjs/mqGPmsKDBf/g=; b=L6A0qoNbdgokF+9DEzh5ZDSplWQAgrItARZR9jvSEkUkGqynky6NJ1x2 xvRXqKgmzyW5RqyNNwdXRz3kksO2UF3L2XU5zqfVv63G7dOfigKDtG0Y3 2QmbeyX69/PQb7pEuE4aVGh1SwT5hM7cj2RXQzQtlkND2NrDEzqzPwvJi A=;
X-IronPort-Anti-Spam-Filtered: true
X-IronPort-Anti-Spam-Result: A0ArAgCbIH9X/5hdJa1cgnBOVnwGuRGBeyKFdgIcgRA4FAEBAQEBAQFlJ4RMAQEFAQEYCQpBBAcQAgEGAhEEAQEhAQYDAgICJQsUCQgCBA4FCIgoDpAqnR2PRwEBAQEBAQEBAQEBAQEBAQEBAQEBARcFiXGBA4QKISQJHwKCSYI9HQWTWYU6AYt3gkuBcReEQYMuhTyQCwEeNoIJHIFMbogXfwEBAQ
X-IronPort-AV: E=Sophos;i="5.28,327,1464652800"; d="scan'208,217";a="121396048"
Received: from rcdn-core-1.cisco.com ([173.37.93.152]) by rcdn-iport-9.cisco.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 08 Jul 2016 03:46:20 +0000
Received: from XCH-RTP-003.cisco.com (xch-rtp-003.cisco.com [64.101.220.143]) by rcdn-core-1.cisco.com (8.14.5/8.14.5) with ESMTP id u683kJrt020870 (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256 verify=FAIL); Fri, 8 Jul 2016 03:46:19 GMT
Received: from xch-rtp-001.cisco.com (64.101.220.141) by XCH-RTP-003.cisco.com (64.101.220.143) with Microsoft SMTP Server (TLS) id 15.0.1210.3; Thu, 7 Jul 2016 23:46:18 -0400
Received: from xch-rtp-001.cisco.com ([64.101.220.141]) by XCH-RTP-001.cisco.com ([64.101.220.141]) with mapi id 15.00.1210.000; Thu, 7 Jul 2016 23:46:18 -0400
From: "Alexander Clemm (alex)" <alex@cisco.com>
To: Andy Bierman <andy@yumaworks.com>
Thread-Topic: [yang-doctors] YANG Doctor Review of draft-ietf-i2rs-yang-network-topo-03
Thread-Index: AQHR0wjlQKb0p9i/B0yZ+nXU1OPubKALUrmAgAH4x8CAAMCugP//z4IwgABL7ID//72rQIAASiKA//++PvA=
Date: Fri, 08 Jul 2016 03:46:18 +0000
Message-ID: <cc6e9a13f17844e4adc81ef9286a8a48@XCH-RTP-001.cisco.com>
References: <330E8448-FBA4-44C4-BD99-CBE9E50D6E33@juniper.net> <047001d1d75a$df327730$9d976590$@ndzh.com> <806ae2b72ceb450b903e547f83267d6f@XCH-RTP-001.cisco.com> <CABCOCHQP=ctsutiA+3Hc_36Hdcvr=9ow3kPYHz4CeeO32E9=Rg@mail.gmail.com> <f62826fb7cd849d2a486a5eaf5218ebf@XCH-RTP-001.cisco.com> <CABCOCHSVuyH+qg_hYdvwF9M3Oq+3QVx=50wVgiEndPQjahxGjQ@mail.gmail.com> <d110a5a83055488d80932ab754c1d34b@XCH-RTP-001.cisco.com> <CABCOCHR2dqMHUzBb2pk178TqFjTv-1crx-EsEOP6kOhPcWBndw@mail.gmail.com>
In-Reply-To: <CABCOCHR2dqMHUzBb2pk178TqFjTv-1crx-EsEOP6kOhPcWBndw@mail.gmail.com>
Accept-Language: en-US
Content-Language: en-US
X-MS-Has-Attach:
X-MS-TNEF-Correlator:
x-ms-exchange-transport-fromentityheader: Hosted
x-originating-ip: [10.24.76.178]
Content-Type: multipart/alternative; boundary="_000_cc6e9a13f17844e4adc81ef9286a8a48XCHRTP001ciscocom_"
MIME-Version: 1.0
Archived-At: <https://mailarchive.ietf.org/arch/msg/yang-doctors/t_ol1XlXjSyt5S9-UkPgAPgM3tg>
Cc: YANG Doctors <yang-doctors@ietf.org>, "draft-ietf-i2rs-yang-network-topo.all@ietf.org" <draft-ietf-i2rs-yang-network-topo.all@ietf.org>, Susan Hares <shares@ndzh.com>
Subject: Re: [yang-doctors] YANG Doctor Review of draft-ietf-i2rs-yang-network-topo-03
X-BeenThere: yang-doctors@ietf.org
X-Mailman-Version: 2.1.17
Precedence: list
List-Id: email list of the yang-doctors directorate <yang-doctors.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/yang-doctors>, <mailto:yang-doctors-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/yang-doctors/>
List-Post: <mailto:yang-doctors@ietf.org>
List-Help: <mailto:yang-doctors-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/yang-doctors>, <mailto:yang-doctors-request@ietf.org?subject=subscribe>
X-List-Received-Date: Fri, 08 Jul 2016 03:46:26 -0000

Hi Andy,

not sure how the ephemeral datastores RFC would facilitate the model?  I am not sure I understand you proposal.  Are you proposing to have two “server-provided” objects, one with config true, the other config false?

Basically, I am wondering, wouldn’t the current design with server-provided leaf (config false) essentiallly accomplish the same?
If provided by the server, it will be set to true, if configured by another app, it will be set to false.  If set to true, it indicates that server app populates and maintains the data and will undo any modification made by other clients to that data.

Perhaps I can make this modification to the draft for now, also addressing the other issues, before the deadline tomorrow – if we decide further updates are needed, discuss in Berlin.  Does that sound agreeable?
--- Alex

From: Andy Bierman [mailto:andy@yumaworks.com]
Sent: Thursday, July 07, 2016 8:31 PM
To: Alexander Clemm (alex) <alex@cisco.com>
Cc: Susan Hares <shares@ndzh.com>; Kent Watsen <kwatsen@juniper.net>; draft-ietf-i2rs-yang-network-topo.all@ietf.org; YANG Doctors <yang-doctors@ietf.org>
Subject: Re: [yang-doctors] YANG Doctor Review of draft-ietf-i2rs-yang-network-topo-03

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) <alex@cisco.com<mailto:alex@cisco.com>> 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:andy@yumaworks.com<mailto:andy@yumaworks.com>]
Sent: Thursday, July 07, 2016 8:04 PM
To: Alexander Clemm (alex) <alex@cisco.com<mailto:alex@cisco.com>>
Cc: Susan Hares <shares@ndzh.com<mailto:shares@ndzh.com>>; Kent Watsen <kwatsen@juniper.net<mailto:kwatsen@juniper.net>>; draft-ietf-i2rs-yang-network-topo.all@ietf.org<mailto:draft-ietf-i2rs-yang-network-topo.all@ietf.org>; YANG Doctors <yang-doctors@ietf.org<mailto:yang-doctors@ietf.org>>
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) <alex@cisco.com<mailto:alex@cisco.com>> 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:andy@yumaworks.com<mailto:andy@yumaworks.com>]
Sent: Thursday, July 07, 2016 6:25 PM
To: Alexander Clemm (alex) <alex@cisco.com<mailto:alex@cisco.com>>
Cc: Susan Hares <shares@ndzh.com<mailto:shares@ndzh.com>>; Kent Watsen <kwatsen@juniper.net<mailto:kwatsen@juniper.net>>; draft-ietf-i2rs-yang-network-topo.all@ietf.org<mailto:draft-ietf-i2rs-yang-network-topo.all@ietf.org>; YANG Doctors <yang-doctors@ietf.org<mailto:yang-doctors@ietf.org>>
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) <alex@cisco.com<mailto:alex@cisco.com>> wrote:
Hi Kent,

thank you for your review!

Please find replies inline, <ALEX>.

--- Alex

From: Susan Hares [mailto:shares@ndzh.com<mailto:shares@ndzh.com>]
Sent: Wednesday, July 06, 2016 12:49 AM
To: 'Kent Watsen' <kwatsen@juniper.net<mailto:kwatsen@juniper.net>>; draft-ietf-i2rs-yang-network-topo.all@ietf.org<mailto:draft-ietf-i2rs-yang-network-topo.all@ietf.org>
Cc: 'YANG Doctors' <yang-doctors@ietf.org<mailto:yang-doctors@ietf.org>>; Alexander Clemm (alex) <alex@cisco.com<mailto:alex@cisco.com>>
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:kwatsen@juniper.net]
Sent: Tuesday, July 5, 2016 2:55 PM
To: draft-ietf-i2rs-yang-network-topo.all@ietf.org<mailto:draft-ietf-i2rs-yang-network-topo.all@ietf.org>
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
yang-doctors@ietf.org<mailto:yang-doctors@ietf.org>
https://www.ietf.org/mailman/listinfo/yang-doctors