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

From: Benjamin Kaduk via Datatracker <[email-masked]>
Date: Tue, 09 Apr 2019 18:48:16 -0700

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.

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

Section 3.1

   [RFC8051] presents several use cases, demonstrating scenarios that
   benefit from the deployment of a stateful PCE including optimization,
   recovery, etc., which are equally applicable to P2MP TE LSPs.
   [RFC8231] defines the extensions to PCEP for P2P TE LSPs.  [...]

nit: maybe "the extensions to PCEP needed for stateful operation of P2P
TE LSPs"?  Just "the extensions" is pretty generic.

Section 5.1

   Path Computation State Report (PCRpt):  Each P2MP TE LSP State Report
      in a PCRpt message can contain actual P2MP TE LSP path attributes,

Is this "can contain" or just "contains"?

Section 5.6.3.1

   The Instantiation operation of P2MP TE LSPs is same as defined in

nit: "the same as"

   section 5.3 of [RFC8281] including handling of PLSP-ID, SYMBOLIC-

nit: comma after "[RFC8281]"

   PATH-NAME TLV etc.  Rules of processing and error codes remains

nit: comma after TLV

   unchanged.  The N (P2MP) flag (Section 7.1) MUST be set in LSP object

nit: "The rules for processing and use of error codes remain unchanged."
nit: "in the LSP object"

   in PCInitiate message by PCE to specify the instantiation is for P2MP

nit: "PCInitiate messages by the PCE to specify that the instantiation"

   TE LSPs.  Like the PLSP-ID as per [RFC8281], the P2MP-LSP-IDENTIFIERS

nit: parentheses around "as per [RFC8281]"

   TLV SHOULD NOT be included in the LSP object in PCIntiitate message

nit: "PCInitiate messages"

   (as it is generated by PCC and carried in PCRpt message instead) and
   MUST be ignored on receipt.

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

   The P2MP END-POINTS object defined in [RFC8306] is mandatory for
   specifying address of P2MP leaves grouped based on leaf types.

nit: "addresses of P2MP leaves, grouped by leaf type"

   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.

   For a delegated P2MP TE LSP configuration changes are reported via
   PCRpt message.  For example, adding of new leaves END-POINTS (leaf-
   type = 1) is used where as removing of old leaves (leaf-type = 2) is
   used.

nit: "is used, and removing of old leaves (leaf-type = 2) is used".

   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?

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.

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.

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

   If the N bit is set on a PCRpt but the P2MP-LSP-IDENTIFIER TLV is

nit: the bit is set in the LSP object within the PCRpt, right?

   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?

Section 9

   The PCEP extensions described in this document for stateful PCEs with
   P2MP capability MUST NOT be used if PCE has not advertised its
   stateful capability with P2MP as per Section 5.2.  If the PCEP
   Speaker on the PCC supports the extensions of this draft (understands
   the P2MP flag in the LSP object) but did not advertise this
   capability, then upon receipt of PCUpd message from the PCE, it
   SHOULD generate a PCErr with error-type 19 ("Invalid Operation"),
   error-value 12 (early allocated by IANA) ("Attempted LSP Update
   Request for P2MP if active stateful PCE capability for P2MP was not
   advertised") and terminate the PCEP session.  If the PCEP Speaker on
   the PCE supports the extensions of this draft (understands the P2MP
   flag in the LSP object) but did not advertise this capability, then

We should probably disambiguate these two "P2MP flag"s to "M flag" and
"N flag", respectively.  ("P flags" is used below.)

   If a Stateful PCE receives a P2MP TE LSP report message and the PCE
   does not understand the P2MP flag in the LSP object, and therefore
   the PCEP extensions described in this document, then the Stateful PCE
   would act as per [RFC8231].

Without a section reference, it's a little annoying to figure out
exactly what that behavior would be.

Section 14.2

One could perhaps argue that RFCs 4875, 7525, and 8253 should be
normative references.

---

From: Dhruv Dhody <[email-masked]>
Date: Wed, 10 Apr 2019 06:16:17 +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:[email-masked]] On Behalf Of Benjamin Kaduk via
> Datatracker
> Sent: 10 April 2019 07:18
> To: The IESG <[email-masked]>
> Cc: [email-masked]; [email-masked]; pce-
> [email-masked]
> Subject: [Pce] Benjamin Kaduk's Discuss on draft-ietf-pce-stateful-pce-
> p2mp-12: (with DISCUSS and COMMENT)
> 
> 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.

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

[[Dhruv Dhody]] Fixed
<snip>
Section 6.1
> 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>
> Interestingly, RFC 8231 does not spell the structure of the PCRpt message
> using an <END-POINTS> object.
> 
> Section 6.2
> 
> 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>
> 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.

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

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

> 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

---

From: Benjamin Kaduk <[email-masked]>
Date: Thu, 11 Apr 2019 14:50:36 -0500

> [[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!

> [[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?

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

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

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

> 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

---

From: Dhruv Dhody <[email-masked]>
Date: Fri, 12 Apr 2019 06:06:11 +0000

Hi Ben, 

I think we have converged and I have posted a new version -13. 

> > > 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.
> 
[[Dhruv Dhody]] Agreed. 

> not worth the bother of changing.

[[Dhruv Dhody]] I have updated the examples. 

Thanks! 
Dhruv