Re: [Pce] Benjamin Kaduk's Discuss on draft-ietf-pce-stateful-pce-p2mp-12: (with DISCUSS and COMMENT)

Dhruv Dhody <dhruv.dhody@huawei.com> Wed, 10 April 2019 06:16 UTC

Return-Path: <dhruv.dhody@huawei.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 D02BF120149; Tue, 9 Apr 2019 23:16:34 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -4.2
X-Spam-Level:
X-Spam-Status: No, score=-4.2 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, RCVD_IN_DNSWL_MED=-2.3, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
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 KCcJXyT8iwFy; Tue, 9 Apr 2019 23:16:32 -0700 (PDT)
Received: from huawei.com (lhrrgout.huawei.com [185.176.76.210]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id CCF9C120048; Tue, 9 Apr 2019 23:16:31 -0700 (PDT)
Received: from lhreml701-cah.china.huawei.com (unknown [172.18.7.108]) by Forcepoint Email with ESMTP id C53F5A95A4600ADA3CB5; Wed, 10 Apr 2019 07:16:29 +0100 (IST)
Received: from BLREML407-HUB.china.huawei.com (10.20.4.45) by lhreml701-cah.china.huawei.com (10.201.108.42) with Microsoft SMTP Server (TLS) id 14.3.408.0; Wed, 10 Apr 2019 07:16:29 +0100
Received: from BLREML503-MBS.china.huawei.com ([169.254.12.125]) by BLREML407-HUB.china.huawei.com ([10.20.4.45]) with mapi id 14.03.0439.000; Wed, 10 Apr 2019 11:46:17 +0530
From: Dhruv Dhody <dhruv.dhody@huawei.com>
To: Benjamin Kaduk <kaduk@mit.edu>, The IESG <iesg@ietf.org>
CC: "draft-ietf-pce-stateful-pce-p2mp@ietf.org" <draft-ietf-pce-stateful-pce-p2mp@ietf.org>, "pce@ietf.org" <pce@ietf.org>, "pce-chairs@ietf.org" <pce-chairs@ietf.org>
Thread-Topic: [Pce] Benjamin Kaduk's Discuss on draft-ietf-pce-stateful-pce-p2mp-12: (with DISCUSS and COMMENT)
Thread-Index: AQHU7z+M60F99Nqc4U2m/TB9hOh6m6Y03ROw
Date: Wed, 10 Apr 2019 06:16:17 +0000
Message-ID: <23CE718903A838468A8B325B80962F9B8DA0F235@BLREML503-MBS.china.huawei.com>
References: <155486089608.19649.9617345506233285248.idtracker@ietfa.amsl.com>
In-Reply-To: <155486089608.19649.9617345506233285248.idtracker@ietfa.amsl.com>
Accept-Language: en-GB, zh-CN, en-US
Content-Language: en-US
X-MS-Has-Attach:
X-MS-TNEF-Correlator:
x-originating-ip: [10.18.149.39]
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-CFilter-Loop: Reflected
Archived-At: <https://mailarchive.ietf.org/arch/msg/pce/BstcmX315lWFLNrq31v4gZhdIb0>
Subject: Re: [Pce] Benjamin Kaduk's Discuss on draft-ietf-pce-stateful-pce-p2mp-12: (with DISCUSS and COMMENT)
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: Wed, 10 Apr 2019 06:16:35 -0000

Hi Benjamin, 

Thank you for your review. 

Working Copy: https://raw.githubusercontent.com/dhruvdhody-huawei/ietf/master/draft-ietf-pce-stateful-pce-p2mp-13.txt
Diff: https://tools.ietf.org/rfcdiff?url1=draft-ietf-pce-stateful-pce-p2mp-12&url2=https://raw.githubusercontent.com/dhruvdhody-huawei/ietf/master/draft-ietf-pce-stateful-pce-p2mp-13.txt

More inline... 

> -----Original Message-----
> From: Pce [mailto:pce-bounces@ietf.org] On Behalf Of Benjamin Kaduk via
> Datatracker
> Sent: 10 April 2019 07:18
> To: The IESG <iesg@ietf.org>
> Cc: draft-ietf-pce-stateful-pce-p2mp@ietf.org; pce@ietf.org; pce-
> chairs@ietf.org
> Subject: [Pce] Benjamin Kaduk's Discuss on draft-ietf-pce-stateful-pce-
> p2mp-12: (with DISCUSS and COMMENT)
> 
> Benjamin Kaduk has entered the following ballot position for
> draft-ietf-pce-stateful-pce-p2mp-12: Discuss
> 
> When responding, please keep the subject line intact and reply to all
> email addresses included in the To and CC lines. (Feel free to cut this
> introductory paragraph, however.)
> 
> 
> Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
> for more information about IESG DISCUSS and COMMENT positions.
> 
> 
> The document, along with other ballot positions, can be found here:
> https://datatracker.ietf.org/doc/draft-ietf-pce-stateful-pce-p2mp/
> 
> 
> 
> ----------------------------------------------------------------------
> DISCUSS:
> ----------------------------------------------------------------------
> 
> This is a comparatively minor point, but I don't think some of the message
> layout is quite fully specified.  In particular, Section 7.2 discusses
> some new TLVs (in a confusing way, referring to the class of two TLVs as a
> single nonexistent TLV) but does not always say which object the TLVs are
> allowed to appear within.  (The first paragraph seems to talk of the TLV
> appearing in a PCRpt, which is a message and as such would contain only
> objects and not TLVs directly.)  The second paragraph does mention the LSP
> object, which leads the reader to infer that this TLV is only intended to
> appear in the LSP object, and only in the PCRpt and PCUpd messages
> explicitly mentioned, but we probably shouldn't force readers to make that
> leap.
> 
> 
[[Dhruv Dhody]] I have made following changes - 

- Made 7.2 (P2MP-LSP-IDENTIFIERS TLV) as a sub-section of 7.1, making it clear that it is part of LSP object
- Added this text at the start 

   [RFC8231] specify the LSP-IDENTIFIERS TLVs to be included in the LSP
   object.  For P2MP TE LSP, this document defines P2MP-LSP-IDENTIFIERS
   TLVs for the LSP object.  There are two P2MP-LSP-IDENTIFIERS TLVs,
   one for IPv4 and one for IPv6.

- I further clarified 
                                    The P2MP-LSP-IDENTIFIERS TLV MUST be
   included in the LSP object in PCRpt message for P2MP TE LSPs.  If the
   N bit is set in the LSP object in the PCRpt message but the P2MP-LSP-
   IDENTIFIER TLV is absent, the PCE MUST respond with a PCErr message
   carrying error-type 6 ("mandatory object missing") and error-value 14
   (early allocated by IANA) ("P2MP-LSP-IDENTIFIER TLV missing") and
   close the PCEP session.

   The P2MP-LSP-IDENTIFIERS TLV MAY be included in the LSP object in the
   PCUpd message for P2MP TE LSPs.  The special value of all zeros for
   all the fields in the value portion of the TLV is used to refer to
   all paths pertaining to a particular PLSP-ID.  The length of the TLV
   remains fixed based on the IP version. 

   The P2MP-LSP-IDENTIFIERS TLV SHOULD NOT be used in PCInitiate (see
   Section 5.6.3.1) and MAY optionally be included in the LSP object in
   the PCReq and the PCRep message for P2MP TE LSP.

> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
> 
> I've made a lot of comments about grammar nits (tagged as such), but there
> are also some more substantial comments to look for.
> 
> Section 1
> 
>    [RFC4875] describes how to set up point-to-multipoint (P2MP) Traffic
>    Engineering Label Switched Paths (TE LSPs) for use in Multiprotocol
>    Label Switching (MPLS) and Generalized MPLS (GMPLS) networks.  The
>    PCE has been identified as a suitable application for the computation
>    of paths for P2MP TE LSPs ([RFC5671]).
> 
> nit: some readers might wonder who has done this identification.
> 
[[Dhruv Dhody]] Reworded 

   [RFC5671] examines the applicability of PCE for the path computation
   for P2MP TE LSPs.

<snip>
> 
> Section 6.1
> 
> Was it a conscious decision to depart from RFC 8231 and inline objects in
> the definition of <state-report> (as opposed to keeping the <path>
> intermediate construction)?
> 

[[Dhruv Dhody]] Fixed
<snip>
Section 6.1
>    When reporting the status of a P2MP TE LSP, the destinations MUST be
>    grouped in END-POINTS object based on the operational status (O field
>    in S2LS object) and leaf type (in END-POINTS).  This way, leaves of
>    the same type that share the same operational status are grouped
>    together.  [...]
> 
> Does this mean that it's an error for a PCRpt message to include more than
> one END-POINTS object with a given value of leaf-type?  If so, it may be
> worth stating that explicitly.

[[Dhruv Dhody]] No, one can have more than one END-POINTS object with the same leaf-type. There is no such restriction. 

<snip>
> 
>    Note that the compatibility with the [RFC8231] definition of <state-
>    report> is preserved.  At least one instance of <END-POINTS> MUST be
>    present in this message for P2MP LSP.
> 
> Interestingly, RFC 8231 does not spell the structure of the PCRpt message
> using an <END-POINTS> object.
> 
> Section 6.2
> 
>    Note that the compatibility with the [RFC8231] definition of <update-
>    request> is preserved.
> 
> If I'm reading this right, compatibility is only preserved when <END-
> POINTS> is omitted (and the list only has a single endpoint/path pair).
> Perhaps some more text could be added about the nature of the provided
> (and needed) compatibility?
> 
[[Dhruv Dhody]] The compatibility is in the fact that a P2P TE LSP encoded as per RFC8231, would pass the above RBNF check. The same wording is used in RFC 8306 (section 3.4). 

<snip>
> Section 6.6.2
> 
>    The LSP State Report message is sent by a PCC to report or delegate
>    the P2MP TE LSPs.  An example of a PCRpt message for a delegated P2MP
>    TE LSPs is described below to add new leaves to an existing P2MP TE
> 
> nit: "TE LSP"
> 
> It might be handy to mention inline "for leaf type 1 (add)" and "leaf type
> 2 (remove)" just to refresh the reader's memory.
> 
[[Dhruv Dhody]] I have added this instead - 

   The leaves of the P2MP TE LSP are grouped in the
   END-POINTS object based on the operational status and the leaf-type.

> Section 7.1
> 
>    The flags defined in this section (N, F, E flags) are used in PCRpt,
>    PCUpd, or PCInitiate message.  In case of PCReq and PCRep message,
>    these flags have no meaning and thus MUST be ignored.  The
>    corresponding flags in the RP (Request Parameters) object are used as
>    described in [RFC8231].
> 
> I'm not really finding much discussion of RP flags in RFC 8231 (as opposed
> to SRP, in Section 7.2 thereof).  RFC 5440 does define a few flags for the
> RP object, though.
> 
[[Dhruv Dhody]] It should be RFC8306, thanks for spotting this mixup. 

> Section 7.2
> 
> I strongly suggest changing the initial language to make it clear that
> there is not a single P2MP-LSP-IDENTIFIER TLV, but rather a class of two
> related TLVs that serve to identify P2MP LSPs, e.g., by referring to "P2MP
> LSP Identification TLVs" (without hyphenation or all-caps).
> 
[[Dhruv Dhody]] I want to avoid making this change to remain consistent with RFC8231 (Section 7.3.1. LSP-IDENTIFIERS TLVs) but I have added clarifying text (see DISCUSS).

> 
>    The P2MP-LSP-IDENTIFIERS TLV MAY be included in the LSP object in the
>    PCUpd message for P2MP TE LSPs.  The special value of all zeros for
>    this TLV is used to refer to all paths pertaining to a particular
>    PLSP-ID.
> 
> Given that we haven't specialized into the IP version-specific TLVs yet, I
> agree with the previous comments that we need greater clarity on what the
> "value of all zeros" means, here.  Additionally, is that special value
> supposed to be restricted to the given IP version or literally all paths,
> so that there are two functionally equivalent encodings for these
> semantics?
> 
[[Dhruv Dhody]] Updated text is - 

7.1.1.  P2MP-LSP-IDENTIFIERS TLV

   [RFC8231] specify the LSP-IDENTIFIERS TLVs to be included in the LSP
   object.  For P2MP TE LSP, this document defines P2MP-LSP-IDENTIFIERS
   TLVs for the LSP object.  There are two P2MP-LSP-IDENTIFIERS TLVs,
   one for IPv4 and one for IPv6.  The P2MP-LSP-IDENTIFIERS TLV MUST be
   included in the LSP object in PCRpt message for P2MP TE LSPs.  If the
   N bit is set in the LSP object in the PCRpt message but the P2MP-LSP-
   IDENTIFIER TLV is absent, the PCE MUST respond with a PCErr message
   carrying error-type 6 ("mandatory object missing") and error-value 14
   (early allocated by IANA) ("P2MP-LSP-IDENTIFIER TLV missing") and
   close the PCEP session.

   The P2MP-LSP-IDENTIFIERS TLV MAY be included in the LSP object in the
   PCUpd message for P2MP TE LSPs.  The special value of all zeros for
   all the fields in the value portion of the TLV is used to refer to
   all paths pertaining to a particular PLSP-ID.  The length of the TLV
   remains fixed based on the IP version.

Since I have moved the text introducing the two TLVs to the first para, I hope this is enough to clear up any confusion.

<snip> 

Thanks again for your detailed review. 

Thanks! 
Dhruv