Re: [Pce] Yangdoctors early review of draft-ietf-pce-pcep-yang-08

Dhruv Dhody <dhruv.ietf@gmail.com> Tue, 26 March 2019 15:50 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 B4D3C12047C; Tue, 26 Mar 2019 08:50:39 -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 2qHHmA5-AMgu; Tue, 26 Mar 2019 08:50:36 -0700 (PDT)
Received: from mail-io1-xd2b.google.com (mail-io1-xd2b.google.com [IPv6:2607:f8b0:4864:20::d2b]) (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 E024B12048C; Tue, 26 Mar 2019 08:50:35 -0700 (PDT)
Received: by mail-io1-xd2b.google.com with SMTP id e13so11204403ioq.6; Tue, 26 Mar 2019 08:50:35 -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=SfyTTCxDiiMixb/8mBVnyK8yWgpQCkzkpiSBXuc0X4c=; b=W9V5HwmSfhHKcCoFx4l7BliUdhXPZA4WRMWxXcIjDm9DBuGSL+8JAoynHeRLrA8zIE PTcMecPWlhm9ApjMWzGprhyTC3Rm06ahM/YAgXAiefShQyrQM6Swz7dwkEfSND/54/bk xJFebR5dlh5YQcygphJNKykZGHB3LWErhFkXOUPO9U3PXtAz74y0i8y2S/n5amUwqiKj HrhyXrn0T/3Ls0t1fOh+qwKCZwwXshGNEraS0yV+BHtkM3GjQMtKuK3qrhpYXCmmTyBo v0y3OFvXXZF28713lIhRgIq29m/jZx8aFHk31CouPlc6Yve3zZOy7b1a5/97WMqbEoa/ VY1A==
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=SfyTTCxDiiMixb/8mBVnyK8yWgpQCkzkpiSBXuc0X4c=; b=VE831o+GLvKfe2UwALL8CaahYDlk0/Wpc0vO0InaN9AsQZst3IYF0XhDtiTd2j1dwg TumQ4MyvjtN5fZAIDGdgWzempC19Yi0tVN30Vo8U4phQJbde2uEK0a2hxJDy2R18R0sg r7JRN0mxykdPZSxBwO+oIm63X1BewMyvJ16kwRwqvU6df423OaOtHO8zQ9xnS9ewO7lP RldC2ZrM1zZlJVD9zDfGFEppXLZ/xVie883Glv3TfHe11BdfdrJvUJMm+RF1o8FO32CJ p79xuHB4ymbyqz6i2RQWX+fvcy5Zs13/GNH7DwJA6l6+k5+vuMazBOt5hfVnxYMuNEJx nMhQ==
X-Gm-Message-State: APjAAAXOp0WNzWM/PSJl7fKN1HHqDpXfZrZs4+E0AdFaQKigABlGjLQh SGhaBI9eUoWj5CEu+aGI1D41N4jpZUMsooNWMqc=
X-Google-Smtp-Source: APXvYqzYh6k7opQ5pJq9tBmrAXOft8EWr6IjQunKyh1mdg1HIirxwrQsVJbVm/uoxT3sMeem3ZWzlDhmLZ1rxm5DXWc=
X-Received: by 2002:a6b:7804:: with SMTP id j4mr2141760iom.171.1553615434923; Tue, 26 Mar 2019 08:50:34 -0700 (PDT)
MIME-Version: 1.0
References: <154328751678.23848.17617047655308813382@ietfa.amsl.com> <CAB75xn4NYtECK_EuBELRYxDaDg_n9EFZHwAX2EpT=AJUdCwa8w@mail.gmail.com> <00d801d4e3d2$0408a620$4001a8c0@gateway.2wire.net>
In-Reply-To: <00d801d4e3d2$0408a620$4001a8c0@gateway.2wire.net>
From: Dhruv Dhody <dhruv.ietf@gmail.com>
Date: Tue, 26 Mar 2019 16:49:58 +0100
Message-ID: <CAB75xn6HAuxvm1R-3hFqinXHvbnjVcrX-+LaMG5PLnKCEsSuRw@mail.gmail.com>
To: tom petch <ietfc@btconnect.com>
Cc: Mahesh Jethanandani <mjethanandani@gmail.com>, "draft-ietf-pce-pcep-yang.all@ietf.org" <draft-ietf-pce-pcep-yang.all@ietf.org>, "yang-doctors@ietf.org" <yang-doctors@ietf.org>, "pce@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/nrE-dIbhOEtZRRSr0xniNJeflF8>
Subject: Re: [Pce] 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 15:50:40 -0000

Hi Tom,

I just had a chat with RFC Editor and they say that they are happy
with the draft names in the references instead of RFC YYYY/ RFC ZZZZ
for other documents being referenced.

Thanks again for your reviews, I will fix the others and plan to make
an update today.

Regards,
Dhruv

On Tue, Mar 26, 2019 at 1:49 PM 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.
>
> 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
> >
>