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

Andy Bierman <andy@yumaworks.com> Fri, 08 July 2016 01:25 UTC

Return-Path: <andy@yumaworks.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 108DB12D77C for <yang-doctors@ietfa.amsl.com>; Thu, 7 Jul 2016 18:25:29 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -2.6
X-Spam-Level:
X-Spam-Status: No, score=-2.6 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_LOW=-0.7, SPF_PASS=-0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (2048-bit key) header.d=yumaworks-com.20150623.gappssmtp.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 RLCZw2CfoPk1 for <yang-doctors@ietfa.amsl.com>; Thu, 7 Jul 2016 18:25:25 -0700 (PDT)
Received: from mail-vk0-x236.google.com (mail-vk0-x236.google.com [IPv6:2607:f8b0:400c:c05::236]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id B8B3012D522 for <yang-doctors@ietf.org>; Thu, 7 Jul 2016 18:25:24 -0700 (PDT)
Received: by mail-vk0-x236.google.com with SMTP id d67so43158851vkh.1 for <yang-doctors@ietf.org>; Thu, 07 Jul 2016 18:25:24 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yumaworks-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=z5Q5YXFu0X8/4w6gFxzABp3roLrv0jL35EFA2j1TftA=; b=wXAorqIo0OYHSIkDOInrFOEbVp1t/k11oiQuEUZPcux+3qK58GCjG//KT3ntltdbiY glrVvVB8tIvcyHlHYnbvEvh/CdmAzPBrbwTxauyCQeceaNWiupHKtlEt95HKHpfOgBd0 k+mT4tHon8lj+z9jGuMZBKYnmPaY0GEpgPM8dhbsq0kOwEEbME9VHSaZ3++oKd5ink8F 1SI25+PQbEcw8lp2U1yFOO7Mmn5lEr/Fwmdvb+nGesbVsxjkU9seeff3Qnq2NV+sbsqC ptpjlPIep5rFrC0euS/u8mtUo7yqWOnwKY6NGy/GJhpR8qAsd1mrzbgZNviev6rCqbwn p/BQ==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=z5Q5YXFu0X8/4w6gFxzABp3roLrv0jL35EFA2j1TftA=; b=ROV6S9Nh5cTwpmifUMNmlp0KaOO7gsfKv0WB4C/Apbk0SNxozvcTpvyQA7zzqFdt/C GK3V3uZrt6u9S0ZJKJwqCLkKEBz8/8bMwzsnYSpFH//AMYcYQ7QPTdSfdfEaHwVj3gAb wz/770wGV4aawt/smb01TB+oKNN7rhXr4GosAK3mV0eYCiVlJC2a04Kv7/iOwMXSCuaS rhvZKDrchfFfF9ZPYdKTnIFoXAUc973gUsbjkIrewlPx3YUgRuWdu2sYE3YxyZxnx2XI j7KRl8Lq+MwFC0HrhwEp14++Rb8ZcbI8m9fx/V+MAzcu/10RAkMGvaCnV+65nUu4fzel Hp6Q==
X-Gm-Message-State: ALyK8tJoaJuokrdOb/INtydDKssJKUf/uxwPrUaCWyPWxiOfkhjTiFEA6KIWcIMNYgLUkt+oTqD7RwAeGqraLg==
X-Received: by 10.31.9.65 with SMTP id 62mr1535314vkj.89.1467941123815; Thu, 07 Jul 2016 18:25:23 -0700 (PDT)
MIME-Version: 1.0
Received: by 10.103.20.2 with HTTP; Thu, 7 Jul 2016 18:25:23 -0700 (PDT)
In-Reply-To: <806ae2b72ceb450b903e547f83267d6f@XCH-RTP-001.cisco.com>
References: <330E8448-FBA4-44C4-BD99-CBE9E50D6E33@juniper.net> <047001d1d75a$df327730$9d976590$@ndzh.com> <806ae2b72ceb450b903e547f83267d6f@XCH-RTP-001.cisco.com>
From: Andy Bierman <andy@yumaworks.com>
Date: Thu, 07 Jul 2016 18:25:23 -0700
Message-ID: <CABCOCHQP=ctsutiA+3Hc_36Hdcvr=9ow3kPYHz4CeeO32E9=Rg@mail.gmail.com>
To: "Alexander Clemm (alex)" <alex@cisco.com>
Content-Type: multipart/alternative; boundary="001a11440b643c916a053715ad3a"
Archived-At: <https://mailarchive.ietf.org/arch/msg/yang-doctors/w-8Sc66UUBiXZG4FYqXabI7ivA8>
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 01:25:29 -0000

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>
wrote:

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