Re: [Pce] Shepherd review of draft-ietf-pce-stateful-pce-p2mp-08

Hi Adrian, 

Thanks for the clear suggested text to improve the document, much appreciated. 

All editorial comments are accepted and updated in the working copy. Please let me know if there is any issue. 

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

<Snip to a few issues that require some discussion>

> ---
> 
> 6.1
> 
> The following bit of RBNF echoes something we did in the base stateful
> draft and which has caused confusion / errata to be raised because it
> looks so wrong (actual and intended all mixed up).  Do we absolutely need
> to do this just to have something that looks like the base draft (which
> looks weird)?  If so, please give an explicit explanation in the text,
> otherwise we will just get the errata raised again.
> 
>    <state-report> ::= [<SRP>]
>                        <LSP>
>                        <end-point-intended-path-pair-list>
>                        [<actual-attribute-list>
>                        <end-point-actual-path-pair-list>]
>                        <intended-attribute-list>
> 
[[Dhruv Dhody]] So far we have tried to maintain compatibility between P2P and P2MP versions of the PCEP messages. Additionally we state - 

   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.

I have also added this now - 

   Note that the ordering of <end-point-intended-path-pair-list>,
   <actual-attribute-list>, <end-point-actual-path-pair-list>, and
   <intended-attribute-list> is done to retain compatibility with state
   reports for the P2P LSPs as per [RFC8231].

> ---
> 
> 6.1 (and same question for 6.2)
> 
> In the following bit of RBNF, what is the use of having more than 1
> [intended | actual] path?
> 
>    <intended-path> ::= (<ERO>|<SERO>)
>               [<intended-path>]
> 
>    <actual-path> ::= (<RRO>|<SRRO>)
>               [<actual-path>]
> 
> 
[[Dhruv Dhody]] The Endpoint Object for P2MP has multiple destinations, and for each destination there would be a corresponding ERO/SERO or RRO/SRRO. That is why it is a list matched to the order of the destinations encoded in the corresponding END-POINTS object. 

> ---
> 
<snip>
> 
> 6.2
> 
>    The PCC SHOULD use the make-before-break or sub-group-based
>    procedures described in [RFC4875] based on a local policy decision.
> 
> That's fine, but if you have "SHOULD" you need to describe the variant
> behavior as well.
> 
[[Dhruv Dhody]] RFC8231 also uses a SHOULD with no mention of any other behavior.  

   Any changes to LSP parameters
   SHOULD be done in a make-before-break fashion.

Any suggestions on how would you prefer this to be handled? 

<snip>
> ---
> 
> 6.6.2
> 
> s/LSP State Report message/The LSP State Report message/
> 
> In the second example, which prunes some leaves, why does the PCC need to
> supply the EROs of the pruned leaves?
> 
[[Dhruv Dhody]] Marked that this could be empty.

> Reading the fourth example, I wondered if it would be possible in
> principle to delegate some leaves in a tree and not others?  Or is it only
> possible to delegate the whole tree?
> 
[[Dhruv Dhody]] yes, currently we delegate the whole tree. One could play around with leaf-type in future, if there is a requirement to support those 'per-leaf'. 

<snip> 
> -
> 
> Shouldn't the description of the N bit should also mention PCReq and PCRep?
> 
[[Dhruv Dhody]] I have added this text - 

   The N flag is used in a PCRpt, PCUpd, or PCInitiate message to
   indicate a P2MP TE LSP.  In case of PCReq and PCRep, the N flag in
   the RP (Request Parameters) object ([RFC8231]) is used to indicate
   P2MP path computation.  The N flag in the LSP object MUST also be set
   if the N flag in the RP object is set.  If there is mis-match
   between the N flag in the RP and the LSP object in PCReq or PCRep
   message, the PCEP speaker MUST generate an error with error-type 10
   ("Reception of an invalid object") and error-value TBD1 (to be
   allocated by IANA) ("Mis-match of N flag in RP and LSP object").
<snip>
> ---
> 
> 7.2
> 
>    The P2MP LSP Identifier TLV MUST be included in the LSP object in
>    PCRpt message for RSVP-TE signaled P2MP TE LSPs.
> 
> But 7.1 says it MUST be present if the N bit is set.  Isn't RSVP-TE a
> subset of all cases where the path may be P2MP?
> 
[[Dhruv Dhody]] I think we can remove any mention of RSVP-TE here. 
> -
> 
>    If the TLV is
>    missing, the PCE will generate an error with error-type 6 (mandatory
>    object missing) and error-value 14 (early allocated by IANA) (P2MP-
>    LSP-IDENTIFIER TLV missing) and close the PCEP session.
> 
> Presumably, this is actually...
> 
>    If the N bit is set on a PCRpt 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 Identifier TLV MAY be included in the LSP object in
>    PCUpd message for RSVP-TE signaled P2MP TE LSPs.
> 
> A double issue as not only is there the question about limiting to RSVP-TE,
> but this "MAY" contradicts the "MUST" in 7.1.
> 
[[Dhruv Dhody]] As per RFC 8231, the LSP-IDENTIFIERS TLV is MUST for PCRpt, but optional for PCUpd. 
This borrow the same idea. The text now says - 

   If the N bit is set on a PCRpt 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
   this TLV is used to refer to all paths pertaining to a particular
   PLSP-ID.

> -
> 
>    There are two P2MP LSP Identifier TLVs, one for IPv4 and one for
>    IPv6.
> 
> Presumably only one can be present?
> 
[[Dhruv Dhody]] Though RFC8231 does not say anything more, do you suggest I add some text for P2MP?

<snip>
> ---
> 
> 7.3
> 
> You need to define how the other Flags should be set? Can they be
> allocated by a future draft? Do you need a registry for them? Or should
> you mark the field as "Reserved". (But see also 11.7)
> 
[[Dhruv Dhody]] Updated to - 

   Flags(32-bits):  the following flags are currently defined -

   O(Operational - 3 bits)  the O Field represents the operational
      status of the group of destinations.  The values are as per
      Operational field in LSP object defined in Section 7.3 of
      [RFC8231].

   Unassigned bits are reserved for future uses.  They MUST be set to 0
   on transmission and MUST be ignored on receipt.

> Can the O field in the LSP object be down and the O field in the S2LS
> object be up?  What would that mean?
[[Dhruv Dhody]] The state of the P2MP tree would be considered up, if the path to at least one of the leaf is up. So I don't see a case where LSP object says down while S2LS says up.
Added an error case - 

   If there is a
   conflict between the O field in the LSP and the S2LS object (for
   example, O field in LSP corresponds to down whereas the O field in
   S2LS is up), the PCEP speaker MUST generate an error with error-type
   10 ("Reception of an invalid object") and error-value TBD2 (to be
   allocated by IANA) ("Mis-match of O field in S2LS and LSP object").

> 
> "Future documents MAY define optional TLVs..." - RFC 2119 language is not
> appropriate here.  s/MAY/might/.
> 
[[Dhruv Dhody]] Ack

<snip>
> ---
> 
> 8.1, 8.2, and 8.3
> 
> 2nd paragraph does not scan - a reword is needed.
> Possibly...
> OLD
>    To indicate P2MP message fragmentation errors associated with a P2MP
>    Report, a Error-Type (18) for "P2MP Fragmentation Error" and a new
>    error-value 2 (early allocated by IANA) is used if a PCE has not
>    received the last piece of the fragmented message, it should send an
>    error message to the PCC to signal that it has received an incomplete
>    message (i.e., "Fragmented Report failure").
> NEW
>    The Error-Type value 18 ("P2MP Fragmentation Error") is used to
>    report any error associated with the fragmentation of a P2MP PCEP
>    message.  A new error-value 2 (early allocated by IANA) indicates
>    "Fragmented report failure" and is used if a PCE does not receive the
>    any part of the fragmented message.
> END (and similar for 8.2 and 8.3)
> 
[[Dhruv Dhody]] One change to your suggested text, instead of 'any part', I think we should use 'last part' as done before (as well as in RFC8306). 

I have updated the definition of F bit as well to - 

   F (Fragmentation flag - 1 bit):  If the F flag is unset (0), it
      indicates that the LSP is not fragmented or it is the last piece
      of the fragmented LSP.  If the F flag is set to 1, it indicates
      that the LSP is fragmented and this is not the last piece of the
      fragmented LSP.  The receiver needs to wait for additional
      fragments until it receives an LSP with the same PLSP-ID and with
      the F-bit set to 0.  See Section 8 for further details.

This should make it clear that the error would be not receiving the last fragment of report with F bit unset. 

<snip>
> 12.
> 
>    The stateful operations on P2MP TE LSP are more CPU-intensive and
>    also utilize more bandwidth on wire.
> 
> More intensive than what?
> 
[[Dhruv Dhody]] Added - 

   The stateful operations on P2MP TE LSPs are more CPU-intensive and
   also utilize more bandwidth on wire (in comparison to P2P TE LSPs).

<snip>
> 
> 12.
> 
> The second paragraph in this section is unclear in the requirements that
> it is placing on implementations.  What security behaviour is it
> specifying?
> 
[[Dhruv Dhody]] Updated to - 

   Securing the PCEP session using Transport Layer
   Security (TLS) [RFC8253], as per the recommendations and best current
   practices in [RFC7525], is RECOMMENDED.

Thanks! 
Dhruv (on behalf of co-authors)

> _______________________________________________
> Pce mailing list
> Pce@ietf.org
> https://www.ietf.org/mailman/listinfo/pce