[Pce] AD review of draft-ietf-pce-pcep-stateful-pce-gmpls-20

[John Scudder <[e-mail]>]

Hi Authors, WG,

Thanks for this spec. I appreciated the care that went into organizing and presenting it, it made my review quite easy. 

I’ve supplied my questions and comments in the form of an edited copy of the draft. Minor editorial suggestions I’ve made in place without further comment, more substantive questions and comments are done in-line and prefixed with “jgs:”. You can use your favorite diff tool to review them; I’ve attached the iddiff output for your convenience if you’d like to use it. I’ve also pasted a traditional diff below in case you want to use it for in-line reply. 

Thanks,

—John

--- draft-ietf-pce-pcep-stateful-pce-gmpls-20.txt	2022-09-26 10:23:51.000000000 -0400
+++ draft-ietf-pce-pcep-stateful-pce-gmpls-20-jgs-comments.txt	2023-04-19 14:23:59.000000000 -0400
@@ -176,8 +176,8 @@
    over them could be delegated to the PCE. Furthermore, [RFC8281] 
    describes the setup and teardown of PCE-initiated LSPs under the 
    active stateful PCE model, without the need for local configuration 
-   on the PCC. However, both the documents left out the specification 
-   for technology-specific objects/TLVs, and do not cover the GMPLS-
+   on the PCC. However, both documents omit the specification 
+   for technology-specific objects/TLVs, and do not cover GMPLS-
    controlled networks (e.g., Wavelength Switched Optical Network 
    (WSON), Optical Transport Network (OTN), Synchronous Optical Network 
    (SONET)/Synchronous Digital Hierarchy (SDH), etc. technologies).  
@@ -188,7 +188,7 @@
    general context of the usage of Stateful PCE and PCEP for GMPLS.  
    The various requirements for stateful GMPLS, including PCE-
    initiation for GMPLS LSPs, are provided in Section 4. An overview of 
-   the PCEP extensions are specified in Section 5, as a solution to 
+   the PCEP extensions are specified in Section 5, and a solution to 
    address such requirements with PCEP object extensions in Section 6. 
 
 1.1. Conventions used in this document 
@@ -233,7 +233,11 @@
    PCUpd messages. 
 
    For passive stateful PCEs, PCReq/PCRep messages are used to request 
-   for path computation.  GMPLS-technology specific Objects and TLVs 
+---
+jgs: can you please expand the symbolic names of PCReq and PCRep as you've
+done for the other messages? Thanks.
+---
+   path computation.  GMPLS-technology specific Objects and TLVs 
    are defined in [RFC8779], this document builds on it and adds the 
    stateful PCE aspects where applicable. Passive Stateful PCE makes 
    use of PCRpt messages when reporting LSP State changes sent by PCCs 
@@ -247,7 +251,7 @@
    used to trigger the end node to set up the LSP. Any modifications to 
    the Objects/TLVs that are identified in this document to support 
    GMPLS technology-specific attributes will be carried in the 
-   PCInitiate messages. 
+   PCInitiate message. 
 
    [RFC8779] defines GMPLS-technology specific Objects/TLVs in 
    stateless PCEP, and this document makes use of these Objects/TLVs 
@@ -388,8 +392,8 @@
 
 6.1. Existing Extensions used for Stateful GMPLS 
 
-   Existing extensions defined in [RFC8779] can be used in the Stateful 
-   PCEP with no changes or slightly changes for GMPLS network control, 
+   Existing extensions defined in [RFC8779] can be used in Stateful 
+   PCEP with no or slight changes for GMPLS network control, 
    including the following:  
 
    o END-POINTS: Generalized END-POINTS was specified in [RFC8779] to 
@@ -398,8 +402,8 @@
    LABEL-REQUEST TLV.  Further note that  
 
     *  As per [RFC8779] for stateless GMPLS path computation, the 
-      Generalized END-POINTS object may contain a LABEL-REQUEST TLV 
-      and/or LABEL-SET. In this document, only the LABEL-REQUEST TLV is 
+      Generalized END-POINTS object may contain a LABEL-REQUEST  
+      and/or LABEL-SET TLV. In this document, only the LABEL-REQUEST TLV is 
       used to specify the switching type, encoding type and G-PID of the 
       LSP.  
 
@@ -429,7 +433,7 @@
    7.2.2 of this document.  
 
    o ERO: The Explicit Route Object (ERO) was not extended in 
-   [RFC8779], and nor in this document.  
+   [RFC8779], nor is it in this document.  
 
 
  
@@ -453,7 +457,7 @@
 
    R (LSP-REPORT-CAPABILITY(TBDa) -- 1 bit): if set to 1 by a PCC, the 
    R flag indicates that the PCC is capable of reporting the current 
-   state of an GMPLS LSP, whenever there's a change to the parameters 
+   state of a GMPLS LSP, whenever there's a change to the parameters 
    or operational status of the GMPLS LSP; if set to 1 by a PCE, the R 
    Flag indicates that the PCE is interested in receiving GMPLS LSP 
    State Reports whenever there is a parameter or operational status 
@@ -480,7 +484,7 @@
    [RFC5521] defines a mechanism for a PCC to request or demand that    
    specific nodes, links, or other network resources are excluded from   
    paths computed by a PCE.  A PCC may wish to request the computation    
-   of a path that avoids all link and nodes traversed by some other LSP. 
+   of a path that avoids all links and nodes traversed by some other LSP. 
 
    To this end this document defines a new sub-object for use with 
    route exclusion defined in [RFC5521].  The LSP exclusion sub-object 
@@ -506,8 +510,38 @@
     
                Figure 1: New LSP Exclusion Sub-object Format 
     
-     X bit: Same as the X bit defined in XRO sub-objects by [RFC5521] 
-     (i.e., mandatory vs. desired).  
+     X: Same as the X-bit defined in XRO sub-objects by [RFC5521] 
+     (i.e., mandatory vs. desired). 
+---
+jgs: You might at least cite "Section 2.1.1 of [RFC5521]" instead of
+just the RFC number. I also edited in a hyphen, for consistency with
+how RFC5521 spells "X-bit". I'm generally sympathetic with not repeating
+a definition that occurs in a base specification, although in this
+instance I think there's a case to be made for quoting RFC5521's 
+definition, for the convenience of the reader. If you did that it would
+look something like:
+
+     X: Section 2.1.1 of [RFC5521] defines the X field as follows: 
+     "The X-bit indicates whether the exclusion is mandatory or desired.
+     0 indicates that the resource specified MUST be excluded from the
+     path computed by the PCE.  1 indicates that the resource specified
+     SHOULD be excluded from the path computed by the PCE, but MAY be
+     included subject to PCE policy and the absence of a viable path
+     that meets the other constraints and excludes the resource."
+     
+It's up to you whether to adopt that approach or keep to the more
+minimal one you've used, but if you keep to the current text, perhaps
+delete "(i.e., mandatory vs. desired)" which appears to be wrong, or at
+minimum quite confusing; I guess the correct understanding of 
+"mandatory" is "exclusion is mandatory" and "desired" means "exclusion
+is desired", but this is not at all obvious to the casual reader.
+
+In short, please either make the text even more terse (sending the 
+reader to the RFC 5221 if they want to know what the X-bit semantics
+are, with no hints), or make the definition comprehensive by quoting
+RFC 5221 verbatim, but don't try to paraphrase it, you risk muddying
+the water by doing that.
+---
  
      Type: Sub-object Type for an LSP exclusion sub-object. Value of 
      TBD3. To be assigned by IANA.  
@@ -523,6 +557,26 @@
     
      Symbolic Path Name: This is the identifier given to an LSP and is 
      unique in the context of the PCC address as defined in [RFC8231].  
+---
+jgs: The way you've written this would lead a reader to imagine that 
+RFC 8231 only defines uniqueness, but in fact I think you must be 
+relying on 8231 for the full definition of the field, because
+the text here leaves out important details such as the alphabet over
+which the field is defined. Perhaps replace with something like,
+
+NEW:
+     Symbolic Path Name: This is the identifier given to an LSP. Its
+     syntax and semantics are identical to those of the Symbolic Path
+     Name field defined in Section 7.3.2 of [RFC8231]: "symbolic name 
+     for the LSP, unique in the PCC.  It SHOULD be a string of printable 
+     ASCII characters, without a NULL terminator."
+
+(and then continue with the "it is worth noting" text.) 
+
+If I were reviewing RFC 8231, I would question the use of "SHOULD" in the
+field definition, but since it's a quote, there's not another viable
+option.
+---
      It is worth noting that given that the Symbolic Path Name is 
      unique in the context of the headnode, only LSPs that share the 
      same headnode/PCC could be excluded. 
@@ -535,13 +589,13 @@
    LSP. 
 
    Note that this XRO Sub-object could also be used by non-GMPLS LSPs.  
-   The description of usage of non-GMPLS LSPs is not in the scope of 
+   The description by usage of non-GMPLS LSPs is not in the scope of 
    this document.  
 
  6.2.3. New flags in the LSP-EXTENDED-FLAG TLV in LSP Object 
 
    The LSP Object is defined in Section 7.3 of [RFC8231], and the new 
-   extended flags TLV is defined in [I-D.ietf-pce-lsp-extended-flags].  
+   extended flags TLV is defined in [RFC9357].  
  
  
 Lee & Zheng             Expires December 2022                [Page 10] 
@@ -583,12 +637,39 @@
    The PCEP extensions described in this document for stateful PCEs 
    with GMPLS capability MUST NOT be used if the PCE has not advertised 
    its stateful capability with GMPLS as per Section 5.2.  If the PCC 
+---
+jgs: two comments here. First, I think your reference should be to 
+Section 6.2, not Section 5.2? (As an aside, if you're authoring in 
+XML, you can use the "anchor" attribute and make it the target of an
+<xref/> and then you avoid this kind of misdirected reference, which
+I'm guessing is literal text in your source.)
+
+Second, I wonder what exactly it means to have "advertised its 
+stateful capability"? Section 6.2 defines three bits, none of which
+is individually or collectively its "stateful capability", or at least
+this isn't self-evident. Can you re-word "stateful capability" to be
+more explicit, or help me understand what's going on here? "Stateful
+capability" also occurs a few paragraphs down, also flagged.
+---
    understands the U flag that indicates the stateful LSP-UPDATE-
    CAPABILITY but did not advertise this capability, then upon receipt 
    of a PCUpd message for GMPLS LSP from the PCE, it SHOULD generate a 
    PCErr with error-type 19 ("Invalid Operation"), error-value TBDx 
    ("Attempted LSP Update Request for GMPLS if stateful PCE capability 
    for GMPLS was not advertised"), and terminate the PCEP session.   
+---
+jgs: There are several places in Section 7 (and its subsections) where
+you specify that an error SHOULD be generated. Can you explain why 
+these aren't MUST? Generally, if you use a SHOULD like this you can
+expect to be asked to provide the reader with some idea about under
+what circumstances it is OK for an implementation to ignore the SHOULD,
+and what the implementation would be expected to do instead. Please
+consider this question for each of the SHOULD in Section 7.x.
+
+I note that two of your SHOULD are qualified by a "MAY suppress", which
+addresses this point in those cases, although see my further question
+for those MAY.
+---
 
    If the PCE understands the R flag that indicates the stateful LSP-
    REPORT-CAPABILITY but did not advertise this capability, then upon 
@@ -607,6 +688,9 @@
    The PCEP extensions described in this document for PCC or PCE with 
    the PCE-Initiation capability for GMPLS LSPs MUST NOT be used if the 
    PCC or PCE has not advertised its stateful capability with 
+---
+jgs: Second instance of "stateful capability".
+---
    Instantiation and GMPLS-CAPABILITY as per [RFC8779].  If the PCC  
    understands the I flag that indicates LSP-INSTANTIATION-CAPABILITY 
    but did not advertise this capability, then upon receipt of a 
@@ -626,10 +710,15 @@
    value= TBD6).  Note that this error message could also be used by 
    non-GMPLS LSPs.  The PCE MAY suppress this error message by a 
    configurable threshold. 
+---
+jgs: I don't understand what the final sentence above means. What kind
+of threshold are we talking about? Messages per second? Some other 
+metric?
+---
     
 7.3. Error Handling in Route Exclusion   
 
-   The LSP exclusion sub-object in XRO is defined in section 6.2.2 of 
+   The LSP exclusion sub-object in XRO defined in section 6.2.2 of 
    this document MAY be present multiple times.  When a stateful PCE 
    receives a PCEP message carrying this sub-object, it searches for 
    the identified LSP in its LSP-DB and then excludes from the new path 
@@ -642,10 +731,18 @@
    name information to the requesting PCC using the error reporting 
    techniques described in [RFC5440].  However, the PCE MAY suppress 
    this error message by a configurable threshold. 
+---
+jgs: Same question about threshold.
+---
     
 7.4. Error Handling for generalized END-POINTS   
 
    Note that the ENDPOINT object in the Stateful PCEP messages was 
+---
+jgs: I don't see an ENDPOINT object defined in any of the obvious
+citations (including in RFC 8623). Is the above a typo and you really
+meant END-POINTS?
+---
    introduced for P2MP [RFC8623]. Similarly, the END-POINTS object MUST 
    be carried for the GMPLS LSP.  If the END-POINTS object is missing 
  
@@ -745,6 +842,17 @@
    IANA is requested to create a registry to manage the Flag field of 
    the LSP Exclusion sub-object in the XRO.  No Flag is currently 
    defined for this flag field in this document. 
+---
+jgs: I'm guessing this should be a sub-registry within the PCEP Numbers
+group? If so, perhaps,
+
+NEW:
+   IANA is requested to create a registry named "LSP Exclusion
+   Sub-Object Flag Field", within the "Path Computation Element Protocol
+   (PCEP) Numbers" group, to manage the Flag field of the LSP Exclusion
+   sub-object in the XRO. No Flag is currently defined for this flag
+   field in this document.
+---
 
       Codespace of the Flag field (LSP Exclusion sub-object) 
         Bit      Description             Reference 
@@ -763,14 +871,14 @@
 Internet-Draft         Stateful PCEP for GMPLS               June 2022 
     
 
-9.4. New  Flags in the LSP-EXTENDED-FLAGS TLV 
+9.4. New Flags in the LSP-EXTENDED-FLAGS TLV 
 
    [I-D.ietf-pce-lsp-extended-flags] requested IANA to create a 
    subregistry, named the "LSP-EXTENDED-FLAG TLV Flag Field", within 
    the "Path Computation Element Protocol (PCEP) Numbers" registry, to 
    manage the Flag field of the LSP-EXTENDED-FLAG TLV. 
 
-   IANA is requested to make following assignment from this registry as 
+   IANA is requested to make assignments from this registry as 
    follows: 
 
        Bit      Description                        Reference 
@@ -779,7 +887,7 @@
        TBD4     Bi-directional co-routed LSP (B)   [This.I-D] 
        TBDc*    Routing Granularity Flag (RG)      [This.I-D] 
     
-   * - 2 bits needs to be allocated 
+   * - 2 bits need to be allocated 
     
 9.5. New PCEP Error Codes 
 
@@ -841,6 +949,12 @@
    In addition to the parameters already listed in Section 8.1 of 
    [RFC5440], a PCEP implementation SHOULD allow configuration of the 
    following PCEP session parameters on a PCC: 
+---
+jgs: I'm less bothered by SHOULD in a section about configurability, 
+but you might want to consider whether this could be a MUST, or 
+whether you could further qualify it. Please also consider this for
+the second SHOULD in the section.
+---
 
    o  The ability to send stateful PCEP messages for GMPLS LSPs. 
 
@@ -850,10 +964,10 @@
    [RFC5440], a PCEP implementation SHOULD allow configuration of the 
    following PCEP session parameters on a PCE: 
 
-   o  The ability to compute path in a stateful manner in GMPLS 
+   o  The ability to compute paths in a stateful manner in GMPLS 
    networks. 
 
-   o  A set of GMPLS-specific constraint. 
+   o  A set of GMPLS-specific constraints. 
 
    These parameters may be configured as default parameters for any 
    PCEP session the PCEP speaker participates in, or they may apply to 
@@ -898,7 +1012,7 @@
 
    When the detailed route information is included for LSP state 
    synchronization (either at the initial stage or during LSP state 
-   report process), this requires the ingress node of an LSP carry the 
+   report process), this requires the ingress node of an LSP to carry the 
    RRO object in order to enable the collection of such information.  
 
 10.6. Impact on Network Operation 
@@ -907,6 +1021,12 @@
    the considerations described in [RFC5440], Section 8.6.  In addition 
    to the limit on the rate of messages sent by a PCEP speaker, a limit 
    MAY be placed on the size of the PCEP messages. 
+---
+jgs: Can you tell me a little more about this size limit? Does there need
+to be any consideration given to the consequences of such a limit? One 
+can imagine a set of semantics that require N bytes to express, but the
+message size is limited to M < N bytes, that would appear problematic.
+---
 
 11. Security Considerations 
 
@@ -928,11 +1048,11 @@
 Internet-Draft         Stateful PCEP for GMPLS               June 2022 
     
 
-   This draft provides additional extensions to PCEP so as to 
+   This document provides additional extensions to PCEP so as to 
    facilitate stateful PCE usage in GMPLS-controlled networks, on top 
    of [RFC8231] and [RFC8281].  Security issues caused by the extension 
    in [RFC8231] and RFC8281] are not altered by the additions in this 
-   draft.  The security considerations in [RFC8231] and [RFC8281], 
+   document.  The security considerations in [RFC8231] and [RFC8281], 
    including both issues and solutions, apply to this document as well. 
 
 12. Acknowledgement