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

["Adrian Farrel" <[e-mail]>]

Hi,

I have picked up this document as Shepherd and I have done a "final"
review before the document is passed on to the AD with a publication
request.  Jon had started work on this, and I have folded in his
comments.

Pretty much everything we have here is editorial in nature, but there
are quite a lot of comments in this email, so I think it would be
worthwhile to tidy the document in a new revision before we take it to
the AD.

Thanks,
Adrian

=======

Sections 1 and 14.2

s/RFC4857/RFC4875/   (!)

---

Section 1

OLD
   Stateful PCEs are shown to be helpful in many application scenarios,
   in both MPLS and GMPLS networks, as illustrated in [RFC8051].  These
   scenarios apply equally to P2P and P2MP TE LSPs.  [RFC8231] provides
   the fundamental extensions needed for stateful PCE to support general
   functionality for P2P TE LSP.  [RFC8281] provides the an extensions
   needed for stateful PCE-initiated P2P TE LSP.  Complementarily, this
   document focuses on the extensions that are necessary in order for
   the deployment of stateful PCEs to support P2MP TE LSPs.  This
   document describes the setup, maintenance and teardown of PCE-
   initiated P2MP LSPs under the stateful PCE model.
NEW
   Stateful PCEs are shown to be helpful in many application scenarios,
   in both MPLS and GMPLS networks, as illustrated in [RFC8051].  These
   scenarios apply equally to P2P and P2MP TE LSPs.  [RFC8231] provides
   the fundamental extensions to PCEP needed for stateful PCE to support
   general functionality for P2P TE LSP.  [RFC8281] provides extensions
   to PCEP needed for stateful PCE-initiated P2P TE LSP.  This document
   complements that work by focusing on PCEP extensions that are
   necessary in order for the deployment of stateful PCEs to support
   P2MP TE LSPs.  This document describes the setup, maintenance, and
   teardown of PCE-initiated P2MP LSPs under the stateful PCE model.
END

---

3.1

s/etc/etc./

---

3.1

OLD
   Complementarily, this document focuses on the extensions that are
   necessary in order for the deployment of stateful PCEs to support
   P2MP TE LSPs.
NEW
   This document complements the previous work by focusing on
   extensions that are necessary in order for the deployment of stateful
   PCEs to support P2MP TE LSPs.
END

---

3.1

s/via PLSP-ID/via a PCEP-specific LSP identifier (PLSP-ID)/

---

3.1

OLD
   In case of
   stateless PCE, a modification of P2MP tree requires encoding of all
   leaves along with the paths in PCEP message, but using a stateful PCE
   with P2MP capability, the PCEP message can be used to convey only the
   modifications (the other information can be retrieved from the P2MP
   LSP identifier in the LSP database (LSPDB)).
NEW
   When using a
   stateless PCE, a request to modify an existing P2MP tree requires
   that all the leaves are presented in the PCEP messages along with all
   the path information.  But when using a stateful PCE, the PCEP
   messages can use a PLSP-ID to represent all information about the LSP
   that has previously been exchanged in PCEP messages, and it is only
   necessary to encode the modifications (such as new or removed leaf
   nodes).  The PLSP-ID provides an index into the LSP-DB at the PCE,
   and identifies the LSP at the PCC.
END

---

5.1 (and all of the document)

Please check and fix the capitalisation of PCEP messages.  For example,

OLD
   The path computation request (PCReq) and path computation reply
   (PCRep)
NEW
   The Path Computation Request (PCReq) and Path Computation Reply
   (PCRep)

END

---

5.2 (and all of document)

Use consistent names such as "Stateful PCE Capability TLV" versus
"STATEFUL-PCE-CAPABILITY TLV"

Be consistent with "flag" versus "bit"

s/PCEP speakers advertises/PCEP speakers advertise/

s/changes.;/changes;/

s/an P2MP LSP/a P2MP LSP/

---

5.6.1 (and elsewhere)

s/a LSP/an LSP/

---

5.6.3

s/same as P2P/same as for P2P/

---

5.6.3.1

s/P2MP TE LSP/P2MP TE LSPs/

---

Section 6 needs a reference to RFC 5511 to explain the notation.

---

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>

---

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


---

6.1

In this paragraph:

   When reporting the status of a P2MP TE LSP, the destinations are
   grouped in END-POINTS object based on the operational status (O field
   in S2LS object) and leaf type (in END-POINTS).  This way the leaves
   that share the same operational status are grouped together.  For
   reporting the status of delegated P2MP TE LSP leaf-type = 3 is used,
   where as for non-delegated P2MP TE LSP, leaf-type = 4 is used.

I think the first sentence should use normative language: "the
destinations MUST be grouped..."

You probably mean "This way, leaves of the same type that share the same
operational status are grouped together."

- s/where as/whereas/

---

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.

---

6.6

Not mandatory, but how about adding another example showing a PCInitiate?

---

6.6.1

s/LSP Update Request/An LSP Update Request/

OLD
   request updation of path taken by
   some of the leaves in a P2MP tree.
NEW
   requests an update of the path taken to
   some of the leaves in a P2MP tree.
END

---

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?

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?

---

6.7

This subsection seems misplaced after the Examples. Maybe move it up within
Section 6?

But maybe also rename it because it seems actually to be about error
handling.

---

7.

s/The PCEP TLV defined/The new PCEP TLVs defined/

---

7.1

s/LSP Object is defined/The LSP Object is defined/
s/PLSP-ID to uniquely/the PLSP-ID to uniquely/
s/for P2MP tunnel, PLSP-ID/for a P2MP tunnel, the PLSP-ID/

-

The text for the N bit uses a mix of descriptive and 2119 language. I
don't think you need the MUST. So...
OLD
   N (P2MP bit):  If the bit is set to 1, it specifies the message is
      for P2MP TE LSP which MUST be set in PCRpt, PCUpd, or PCInitiate
      message for a P2MP TE LSP.
NEW
   N (P2MP bit):  If the N bit is set to 1, it indicates that the
      message is for a P2MP TE LSP.  It can be used on a PCRpt, PCUpd,
      or PCInitiate message.
END

-

Shouldn't the description of the N bit should also mention PCReq and
PCRep?

-

OLD
   If P2MP bit is set, the following P2MP-LSP-IDENTIFIER TLV MUST be
   present in LSP object.
NEW
   If the N bit is set, the P2MP-LSP-IDENTIFIER TLV defined in Section
   7.2 MUST be present in LSP object.
END

---

7.1

   F (Fragmentation bit):  If the bit is set to 1, it specifies the
      message is fragmented.

A forward reference to Section 8 would be useful.

---

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?

-

   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.

-

   There are two P2MP LSP Identifier TLVs, one for IPv4 and one for
   IPv6.

Presumably only one can be present?

-

s/the following figure:/Figure 6./

...and then...

s/the following figure:/Figure 7./

---

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)

Can the O field in the LSP object be down and the O field in the S2LS
object be up?  What would that mean?

"Future documents MAY define optional TLVs..." - RFC 2119 language is
not appropriate here.  s/MAY/might/.

---

8.

"The total PCEP message length, including the common header, is 16
bytes" - can't be right.  Not sure what you intended here.  

This seems to have been copied from RFC 8306. But I don't know what was
meant there, either.

Maybe (2^16)-1  ?

---

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)

---

9.

It's normal to put the names of the error-values in double quotes.

---

9. 

You have that error-value 11 is followed by closing the session.  What 
about error-values 12 and 13?

---

10.

s/PCEP protocol/PCEP/

---

10.2

"The PCEP YANG model SHOULD be extended" Probably no need for RFC 2119
language. Maybe "could".

---

11.7

s/object.New/object.  New/

Tell IANA how many bits are in the registry and what to do for bits 0...28.

---

12.

   The stateful operations on P2MP TE LSP are more CPU-intensive and
   also utilize more bandwidth on wire.

More intensive than what?

---

12.

I find this a bit vague:

   In the event of an unauthorized
   stateful P2MP operations, or a denial of service attack, the
   subsequent PCEP operations may be disruptive to the network.

How about this instead:
   If a rogue PCC were able to request unauthorize stateful PCE
   operations then it may be able to mount a DoS attack against
   a PCE, which would disrupt the network and deny service to
   other PCCs.

---

12.

The second paragraph in this section is unclear in the requirements
that it is placing on implementations.  What security behaviour is 
it specifying?