Re: [Pce] [yang-doctors] Yangdoctors early review of draft-ietf-pce-pcep-yang-08
Dhruv Dhody <dhruv.ietf@gmail.com> Tue, 26 March 2019 14:39 UTC
Return-Path: <dhruv.ietf@gmail.com>
X-Original-To: pce@ietfa.amsl.com
Delivered-To: pce@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id D52721202CC; Tue, 26 Mar 2019 07:39:25 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.999
X-Spam-Level:
X-Spam-Status: No, score=-1.999 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, FREEMAIL_FROM=0.001, RCVD_IN_DNSWL_NONE=-0.0001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (2048-bit key) header.d=gmail.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 YLDh6SoKtt2L; Tue, 26 Mar 2019 07:39:23 -0700 (PDT)
Received: from mail-it1-x12b.google.com (mail-it1-x12b.google.com [IPv6:2607:f8b0:4864:20::12b]) (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 042B81202C6; Tue, 26 Mar 2019 07:39:23 -0700 (PDT)
Received: by mail-it1-x12b.google.com with SMTP id h9so20336094itl.1; Tue, 26 Mar 2019 07:39:22 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=sNKglfs2axVgUHi98+9aO6tNuet8oWune4L8TJoU/OA=; b=d3q2vvqm6RLFKatgi50ZaInzHDDaUd7xLqclxnQBRWtOYPfCf3QijMsooYW09DVXy9 kPQYNy4XthKWZ3U52zbqhxYeHwOWrpch40vxBqZuSWpFGJTI7FO/jl4LLrcsRZ/8EBhW EbBE3xKLgTYQPxm4gF9sFysIpedH5spmjjUh6zeVFkXi+TTvfEd8rNBnlwwnjNQ1Ga+H 7J0kGvSsahPpDc1+J0WlbBYmGVnAVNqMEa+LS+/dGLxJyAwgfx5aLcupMb18+1YQLfhf hp/wo4PpF1Pch+WZRIRMcVQDqN6eD2umziWVkxFyPdsmh9Px1PIlfdjNIAkGEol1rQ3z QT/w==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=sNKglfs2axVgUHi98+9aO6tNuet8oWune4L8TJoU/OA=; b=F9YBqGL3QRnu88/1yVkjH978qyJPjDHDMJTTfg7bxVCcxEcaNNbGKNxEgLiSNtUcj6 VaiY88Bv04XCLrUVtrpevh2wUPr+CHlduGADqBjEnbJOWbOB3cZ59bh7OV+yLKaM3JVR cx8NA+yReO8awEDnW1uHF4yqL2f8RTKOI8mI2kjUGlzdfeMnPcqInwW0fpgijS1Wuf6B 66oHJmmyc2s2OOSyNplIe28lv36BbTTywVO8mZQxB3qcpigewmLrqatmmL0qji7F6Cgv d0F9Rywk5jcKFSWwKQCkYz3GhCNhEeux0X/hew/w4OpcMCCu8OhBOFGXUTMrF966O0KO SkZg==
X-Gm-Message-State: APjAAAW2xJa+Wd7OI/TQJKF0Z1O65BXYShZcjfU0MJ9GEUemDUjJ8yBE PkMuqFNgJNh8LUVZlTpYdv5saQQKaQayOjOguoc=
X-Google-Smtp-Source: APXvYqxuZE/cBOpqDkeQRQ2tWxCt6gNBAtW0LYJ+zu9F81qoee2Bi6KeNBbHiKwMAbY8+JBEeaEvpKj7qDO1k/qBZ1k=
X-Received: by 2002:a24:420a:: with SMTP id i10mr457539itb.164.1553611162014; Tue, 26 Mar 2019 07:39:22 -0700 (PDT)
MIME-Version: 1.0
References: <00d801d4e3d2$0408a620$4001a8c0@gateway.2wire.net> <20190326.141608.1265169009050218055.mbj@tail-f.com> <CAB75xn5d=FTid6uCM8byFPGWUj+gQK4HC942DZf273K-LyhSig@mail.gmail.com> <20190326.150053.1710533731743618728.mbj@tail-f.com>
In-Reply-To: <20190326.150053.1710533731743618728.mbj@tail-f.com>
From: Dhruv Dhody <dhruv.ietf@gmail.com>
Date: Tue, 26 Mar 2019 15:38:45 +0100
Message-ID: <CAB75xn7z8KMVTtBqDHQwp6nx92xrHaJBeCgC=RH+hkEg_qZx8A@mail.gmail.com>
To: Martin Bjorklund <mbj@tail-f.com>
Cc: tom petch <ietfc@btconnect.com>, Mahesh Jethanandani <mjethanandani@gmail.com>, draft-ietf-pce-pcep-yang.all@ietf.org, yang-doctors@ietf.org, pce@ietf.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://mailarchive.ietf.org/arch/msg/pce/5GAyjLvz_j8kI0Du_ww8MwzoymA>
Subject: Re: [Pce] [yang-doctors] Yangdoctors early review of draft-ietf-pce-pcep-yang-08
X-BeenThere: pce@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Path Computation Element <pce.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/pce>, <mailto:pce-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/pce/>
List-Post: <mailto:pce@ietf.org>
List-Help: <mailto:pce-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/pce>, <mailto:pce-request@ietf.org?subject=subscribe>
X-List-Received-Date: Tue, 26 Mar 2019 14:39:26 -0000
Hi Martin, The newer version of pyang worked! Thanks for your help! I found the full tree useful when I am searching for a leaf in the yang models and understand how it fits in the overall tree. Thus I see value in both. We can also consider if we should also update 5.2-5.6 additionally. Thanks! Dhruv On Tue, Mar 26, 2019 at 3:00 PM Martin Bjorklund <mbj@tail-f.com> wrote: > > Hi, > > Dhruv Dhody <dhruv.ietf@gmail.com> wrote: > > Hi Mahesh, Tom, > > > > Got it, will make the necessary change soon. > > > > Where I need help is the tree creation, even though I use > > '--tree-line-length' I faced the issue with overrunning the 80 > > characters. > > > > pyang --ietf -f tree --tree-line-length=68 --tree-depth=10 > > ietf-pcep@2019-03-24.yang --ietf >ietf-pcep.tree > > Have you tried using pyang 1.7.8? When I run that the tree seems to > fit the line lengths. > > > That made me pick a shorter prefix, but happy to learn if there is a > > better way out there! > > Personally, I'm not too fond of very large tree diagrams. I prefer to > split them into smaller diagrams. So I like your overview diagram in > section 5.1. I would then probably add a small diagram in each of the > section 5.2-5.6, and remove secion 5.7 completely. But this is just > my personal preference! > > > > /martin > > > > > > > > Thanks! > > Dhruv > > > > > > > > On Tue, Mar 26, 2019 at 2:16 PM Martin Bjorklund <mbj@tail-f.com> wrote: > > > > > > Hi, > > > > > > tom petch <ietfc@btconnect.com> wrote: > > > > On the question of prefix, where I an interested in the opinion of a > > > > YANG > > > > Doctor, you use the single letter 'p' and say that a longer prefix gives > > > > you line length problems. YANG does allow statements to span lines, as > > > > happens in almost every TEAS module so for me that is not a very good > > > > reason; I would prefer something of two characters or more. > > > > > > > > I note that IANA Considerations says > > > > Prefix: pcep > > > > which would be my first choice even if I then have to span lines. > > > > > > I strongly agree. Since the prefix is actually part of the IANA > > > registry and needs to be unique, I think you should use a longer > > > prefix. "pcep" seems reasonable. If you run into line length > > > problems, I'll be glad to help you fix them. > > > > > > Before this document goes to the RFC editor, I suggest you run the > > > tool: > > > > > > pyang -f yang --keep-comments --yang-line-length 69 <FILE> > > > > > > on these modules, in order to get them formatted consistently with the > > > rest of the IETF modules. > > > > > > > > > > > > > > > > > You import the module key-chain but you do not use the prefix that it > > > > defines, namely key-chain; not forbidden but not recommended practice > > > > > > > > Likewise tls-client should be tlsc and tls-server tlss. > > > > > > > > Security and IANA Considerations deal with > > > > Name: ietf-pcep > > > > What about > > > > module ietf-pcep-stats { > > > > which I think needs separate coverage, a separate section, in Security > > > > and must be covered in IANA Considerations. > > > > > > > > The problem with > > > > "I-D.ietf-pce-association-group: PCEP Extensions for ... > > > > as a reference is that when it appears in the text of the I-D, then it > > > > is as > > > > [I-D.ietf-pce-association-group] > > > > i.e. a XML/HTML type anchor which is picked up by tools so the RFC > > > > Editor cannot miss it. > > > > > > > > When it appears in the YANG module, it must be plain text as in > > > > "I-D.ietf-pce-association-group: PCEP Extensions for .... > > > > so the tools cannot pick it up, it must be spotted by eye and so might > > > > be missed. Hence I suggest using > > > > > > > > "RFC YYYY - PCEP Extensions for > > > > Establishing Relationships Between Sets of LSPs"; > > > > > > > > with a note to the RFC Editor asking them to replace YYYY with the RFC > > > > number assigned to I-D.ietf-pce-association-group > > > > > > > > Likewise RFC ZZZZ for > > > > "I-D.ietf-pce-segment-routing: PCEP Extensions for Segment > > > > and so on for the others (of which there are several) > > > > > > > > The RFC Editor is ok, likes even, all the notes thereon to appear once > > > > at the start of the I-D. > > > > > > > > So my previous comment was that using XXXX for multiple I-Ds was > > > > confusing but I meant to use YYYY ZZZZ, with an RFC Editor Note for > > > > each, and not to use the I-D name. > > > > > > > > HTH > > > > > > > > Tom Petch > > > > > > > > ----- Original Message ----- > > > > From: "Dhruv Dhody" <dhruv.ietf@gmail.com> > > > > To: "Mahesh Jethanandani" <mjethanandani@gmail.com> > > > > Cc: <draft-ietf-pce-pcep-yang.all@ietf.org>; <yang-doctors@ietf.org>; > > > > <pce@ietf.org> > > > > Sent: Sunday, March 24, 2019 9:07 PM > > > > Subject: Re: [Pce] Yangdoctors early review of > > > > draft-ietf-pce-pcep-yang-08 > > > > > > > > > > > > Hi Mahesh, > > > > > > > > Apologies for a late reply to your review. Being stuck in a long flight > > > > finally gave me enough time to fix up the indentation in the model :) > > > > > > > > An update (-10) has been posted. > > > > > > > > More details inline... > > > > > > > > On Tue, Nov 27, 2018 at 8:28 AM Mahesh Jethanandani > > > > <mjethanandani@gmail.com> > > > > wrote: > > > > > > > > > Reviewer: Mahesh Jethanandani > > > > > Review result: On the Right Track > > > > > > > > > > Document reviewed: draft-ietf-pce-pcep-yang-08 > > > > > > > > > > I am not an expert in PCEP. This review is looking at the draft from a > > > > YANG > > > > > perspective. With that said, I have marked it as “On the Right Track” > > > > > because > > > > > of some of the points discussed below. > > > > > > > > > > Summary: > > > > > > > > > > This document defines a YANG data model for the management of Path > > > > > Computation > > > > > Element communications Protocol (PCEP) for communications between a > > > > Path > > > > > Computation Client (PCC) and a Path Computation Element (PCE), or > > > > between > > > > > two > > > > > PCEs. The data model includes configuration data and state data > > > > (status > > > > > information and counters for the collection of statistics). > > > > > > > > > > Comments: > > > > > > > > > > General > > > > > > > > > > - The module uses indentation that varies all over the module, from 2 > > > > > spaces to > > > > > 5. Please fix the module to have consistent indentation. > > > > > > > > > > > > > Used 2 spaces now. > > > > > > > > > > > > > > - The module makes heavy use of groupings. They are great if they are > > > > being > > > > > used in multiple places. But I seem to see single usage of groupings, > > > > which > > > > > makes the model hard to read. Please collapse all groupings that are > > > > used > > > > > only > > > > > once into the module. > > > > > > > > > > > > > > All groupings that were used only once are now removed. > > > > > > > > > > > > > > > > > Abstract: > > > > > > > > > > It is best not to try to redefine terms, specially if they have > > > > already > > > > > been > > > > > defined already in another RFC. Case in point, "state data". This term > > > > has > > > > > been > > > > > defined in RFC6241, and it would be best to list it in the Terminology > > > > and > > > > > Notation section, as has been done with other definitions. > > > > > > > > > > The following terms are defined in [RFC6241]: > > > > > > > > > > o configuration data > > > > > > > > > > o state data > > > > > > > > > > > > > Done. > > > > > > > > > > > > > > > > > > > > > > Introduction: > > > > > > > > > > Please update reference of YANG to RFC7950. These are YANG 1.1 modules > > > > > after > > > > > all. > > > > > > > > > > > > > Done. > > > > > > > > > > > > > > > > > > > > > > Section 5. The Design of the PCEP Data Model. > > > > > > > > > > Thank you for first of all for creating a abridged version of the tree > > > > > diagram. > > > > > What would really help to understand the design of the model would be > > > > to > > > > > place > > > > > the full tree diagram at the end of the section, and move sections 5.3 > > > > to > > > > > 5.7. > > > > > directly under 5.1. Scrolling through pages of the full diagram to get > > > > to > > > > > the > > > > > design sections is painful to read. > > > > > > > > > > > > > Done. > > > > > > > > > > > > > > > > > > > > > > Section 10. PCEP YANG Modules > > > > > > > > > > - Please list all RFCs and I-D that are referenced in the model, so > > > > there > > > > > is a > > > > > normative reference to them in the draft. > > > > > > > > > > > > > Done. > > > > > > > > > > > > > > > > > > > > > > - Please expand the reference to different RFCs to include the title > > > > of the > > > > > RFC, and not just the number. > > > > > > > > > > > > > Done. > > > > > > > > > > > > > > > > > > > > > > - The reference to tls-server and tls-client should be to > > > > > I-D.ietf-netconf-tls-client-server, as it is not an RFC as yet. Also, > > > > the > > > > > document refers to all other RFCs as RFC XXXX. What is the RFC editor > > > > > supposed > > > > > to replace XXXX with? With the RFC number assigned to this draft?? I > > > > think > > > > > you > > > > > want to refer to I-D that contain those modules. > > > > > > > > > > > > > Done. > > > > > > > > > > > > > > > > > > > > > > - What is "PCEP common"? That term has not been defined anywhere in > > > > the > > > > > document, but is used in the YANG model. > > > > > > > > > > > > > > Removed. > > > > > > > > > > > > > > > > > - What is the 'identify pcep' for? > > > > > > > > > > > > > Removed. > > > > > > > > > > > > > > > > > > > > > > - Why is pcep-admin-status a enum and not a boolean? Since YANG nodes > > > > are > > > > > hierarchical, there should be no reason to repeat prefixes like > > > > > 'admin-status' > > > > > in node names such as 'admin-status-up', both where it is defined and > > > > > where it > > > > > is used (under admin-status). > > > > > > > > > > > > > Changed. > > > > > > > > > > > > > > > > > > > > > > - Where are the different operational status definitions defined? Can > > > > that > > > > > RFC > > > > > be referenced? Same for Session state, Association Type, Objective > > > > > Function. > > > > > > > > > > > > > > References added. > > > > > > > > > > > > > > > > > - Could the YANG module use existing definitions? For example could > > > > the > > > > > module > > > > > use ospf-area as defined in I-D.ietf-ospf-yang or use isis-area > > > > defined in > > > > > the > > > > > ISIS YANG Module. > > > > > > > > > > > > > > Updated. > > > > > > > > > > > > > > > > > - Can the document use more descriptive names for features such as > > > > 'gco'. > > > > > > > > > > > > > Updated. > > > > > > > > > > > > > > > > > > > > > > - If the range of the timer is 1..65535, why does it need to be a > > > > uint32? > > > > > Same > > > > > for the range of 0..255. > > > > > > > > > > > > > Corrected. > > > > > > > > > > > > > > > > > > > > > > - RFC 5440 makes no reference to 'max-keep-alive-timer' or > > > > > 'max-dead-timer'. If > > > > > they are max value, can they not be expressed as part of the range for > > > > > 'keep-alive-timer' or 'dead-timer'? Same for 'min-keep-alive-timer' > > > > and > > > > > 'min-dead-timer'. > > > > > > > > > > > > > > You are right that these are not explicitly stated in 5440, but are > > > > needed > > > > to set what is the acceptable range of these values as received in the > > > > open > > > > message from a peer. These are different from the max value as part of > > > > the > > > > range allowed by the protocol. You would also find these in our PCEP MIB > > > > RFCs. > > > > > > > > > > > > > > > > > - What is the default value for 'admin-status'? > > > > > > > > > > > > > set now to enabled (true). > > > > > > > > > > > > > > > > > > > > > > - The grouping pce-scope seems to be defining a header with each of > > > > the > > > > > leafs > > > > > as bits in the header. In that case, it would be better if this was > > > > > defined as > > > > > a bits/bit field, rather than leafs that are of type uint8 and > > > > boolean. > > > > > Same > > > > > for the grouping called 'capability' > > > > > > > > > > > > > > Updated. > > > > But the priority fields are kept outside of bits/bit. > > > > Also in case of capability, fields that are not part of RFC5088/RFC5089 > > > > capability bit fields are kept outside. > > > > > > > > > > > > > > > > > - The description "LSP is PCE-initiated or not" is hardly a > > > > description > > > > > for the > > > > > leaf 'enabled'. It might be more a description of the feature > > > > > 'pce-initiated'. > > > > > > > > > > > > > > Updated. > > > > > > > > > > > > > > > > > - Could description "Valid at PCC" be improved upon? > > > > > > > > > > > > > > Updated. > > > > > > > > > > > > > > > > > - Most keys are defined as 'type binary'. Why is key-string defined as > > > > > 'type > > > > > string' or 'type hex-string', and not 'type binary'? Is it possible to > > > > > reuse > > > > > definitions from draft-ietf-netconf-crypto-types? > > > > > > > > > > > > > > Updated according to the Key chain RFC that allows both ASCII and Hex > > > > (instead of binary), i think this is better aligned to other related > > > > work. > > > > > > > > > > > > > - I am not an expert in this protocol, but a lot of the nodes defined > > > > are > > > > > generated by the system. Yet, they are defined as rw. For example, the > > > > list > > > > > 'path-keys' carries a description "The list of path-keys generated by > > > > the > > > > > PCE". > > > > > If so, should this not be marked 'config false'. I would suggest > > > > authors > > > > > take a > > > > > more concerted look and see what nodes are indeed rw and which ones > > > > are ro. > > > > > Other examples include 'req-id' and 'retrieved'. > > > > > > > > > > > > > > The examples you cited are already 'ro'. I did a check throughout the > > > > document as well. > > > > > > > > > > > > > > > > > - Can this error-message and description be reconciled? > > > > > > > > > > error-message > > > > > "The Path-key should be retreived"; > > > > > description > > > > > "When Path-Key has been retreived"; > > > > > > > > > > > > > > Updated. > > > > > > > > > > > > > > > > > - It is great to see that extensive amount of statistics are required > > > > to be > > > > > implemented by the model. How many implementations actually support > > > > all > > > > > these > > > > > statistics? What would happen if implementations support a small > > > > number of > > > > > these statistics? In other words, are all these statistics required to > > > > be > > > > > maintained/implemented? > > > > > > > > > > > > > > We have kept most of these as optional and not mandated it, these are > > > > also > > > > aligned to stats in PCEP MIB RFC. > > > > > > > > > > > > > - In addition, a lot of the statistics have when statements. Since > > > > these > > > > > are > > > > > statistics maintained by the system, why the when statement? Does it > > > > mean > > > > > that > > > > > even if the statistics are written by the system, they are not valid > > > > (for > > > > > reading) under certain scenarios. Or is it more likely that they are > > > > only > > > > > written when the role is ether of a 'pce' or 'pcc-and-pce', in which > > > > case > > > > > reading for other roles would return 0 values. > > > > > > > > > > > > > > It is the latter case, where some statistics are written based on the > > > > role. > > > > Do you think this usage of 'when' is incorrect and needs changing? > > > > > > > > Thanks again for your detailed review. > > > > > > > > Regards, > > > > Dhruv > > > > > > > > > > > > > > > > ------------------------------------------------------------------------ > > > > -------- > > > > > > > > > > > > > _______________________________________________ > > > > > Pce mailing list > > > > > Pce@ietf.org > > > > > https://www.ietf.org/mailman/listinfo/pce > > > > > > > > > > > > > _______________________________________________ > > > > yang-doctors mailing list > > > > yang-doctors@ietf.org > > > > https://www.ietf.org/mailman/listinfo/yang-doctors > >
- Re: [Pce] [yang-doctors] Yangdoctors early review… Dhruv Dhody
- [Pce] Yangdoctors early review of draft-ietf-pce-… Mahesh Jethanandani
- Re: [Pce] Yangdoctors early review of draft-ietf-… Dhruv Dhody
- Re: [Pce] Yangdoctors early review of draft-ietf-… Dhruv Dhody
- Re: [Pce] Yangdoctors early review of draft-ietf-… tom petch
- Re: [Pce] [yang-doctors] Yangdoctors early review… Martin Bjorklund
- Re: [Pce] [yang-doctors] Yangdoctors early review… Dhruv Dhody
- Re: [Pce] [yang-doctors] Yangdoctors early review… Martin Bjorklund
- Re: [Pce] [yang-doctors] Yangdoctors early review… Dhruv Dhody
- Re: [Pce] Yangdoctors early review of draft-ietf-… Dhruv Dhody
- Re: [Pce] [yang-doctors] Yangdoctors early review… tom petch
- Re: [Pce] [yang-doctors] Yangdoctors early review… Dhruv Dhody
- Re: [Pce] [yang-doctors] Yangdoctors early review… tom petch