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

Benjamin Kaduk <kaduk@mit.edu> Thu, 11 April 2019 19:50 UTC

Return-Path: <kaduk@mit.edu>
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 0A1ED120603; Thu, 11 Apr 2019 12:50:50 -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, RCVD_IN_DNSWL_LOW=-0.7, 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 EeEw6jGhCmQN; Thu, 11 Apr 2019 12:50:47 -0700 (PDT)
Received: from outgoing.mit.edu (outgoing-auth-1.mit.edu [18.9.28.11]) (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 1748B120183; Thu, 11 Apr 2019 12:50:46 -0700 (PDT)
Received: from kduck.mit.edu (24-107-191-124.dhcp.stls.mo.charter.com [24.107.191.124]) (authenticated bits=56) (User authenticated as kaduk@ATHENA.MIT.EDU) by outgoing.mit.edu (8.14.7/8.12.4) with ESMTP id x3BJobMY008534 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 11 Apr 2019 15:50:39 -0400
Date: Thu, 11 Apr 2019 14:50:36 -0500
From: Benjamin Kaduk <kaduk@mit.edu>
To: Dhruv Dhody <dhruv.dhody@huawei.com>
Cc: The IESG <iesg@ietf.org>, "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>
Message-ID: <20190411195036.GU18549@kduck.mit.edu>
References: <155486089608.19649.9617345506233285248.idtracker@ietfa.amsl.com> <23CE718903A838468A8B325B80962F9B8DA0F235@BLREML503-MBS.china.huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <23CE718903A838468A8B325B80962F9B8DA0F235@BLREML503-MBS.china.huawei.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Archived-At: <https://mailarchive.ietf.org/arch/msg/pce/z16KJCAOLboGIt9Bf-A6vACQ6Nc>
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: Thu, 11 Apr 2019 19:50:50 -0000

On Wed, Apr 10, 2019 at 06:16:17AM +0000, Dhruv Dhody wrote:
> 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 - 

I read the linked diff; these changes are very helpful and will resolve my
DISCUSS point -- thank you!

> - 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. 

Maybe I am misunderstanding something, but the "are grouped together"
implied to me that it was always the case.  Is this just an optional thing
for efficiency, in which case "can be grouped together" might be more
appropriate?

> <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). 

Okay, thanks.  (I think I had not quite internalized everything by this
point in the document, when I was reading.)

> <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.

Sorry, I don't think I conveyed my point very well.  I was suggesting that
the examples would look like:

              Common Header
              LSP with P2MP flag set
              END-POINTS for leaf type 1 (add)
                S2LS (O=DOWN)
                ERO list (empty)

But maybe everyone who is going to read this already knows that and it's
not worth the bother of changing.

> > 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).

Okay.  With the clarifying text you added there is much less chance of
confusion.

> > 
> >    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.

That's a big help, yes.  I'm still not sure whether there's any practical
impact (other than message size!) of using an all-zeroes IPv4 TLV vs. an
all-zeros IPv6 TLV, but that may be too nit-picky to worry about.

Thanks again,

Ben

> <snip> 
> 
> Thanks again for your detailed review. 
> 
> Thanks! 
> Dhruv
>