[Pce] AD review of draft-ietf-pce-lsp-extended-flags-04

[John Scudder <[e-mail]>]

Dear Quan,

Thanks for your patience. Here’s my review of your document. For the most part I have just made small style and grammar suggestions. I do have one larger substantive comment that may need further discussion, as well as a change to the IANA section I’ve flagged but which I’m guessing won’t need further discussion.

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. I’d appreciate feedback regarding whether you found this a useful way to receive my comments as compared to a more traditional numbered list of comments with selective quotation from the draft.

Thanks,

—John

--- draft-ietf-pce-lsp-extended-flags-04.txt	2022-09-20 18:56:37.000000000 -0400
+++ draft-ietf-pce-lsp-extended-flags-04-jgs-comments.txt	2022-09-23 17:16:30.000000000 -0400
@@ -8,7 +8,7 @@
 Expires: 18 March 2023
 
 
-    Label Switched Path (LSP) Object Flag Extension of Stateful PCE
+    Label Switched Path (LSP) Object Flag Extension for Stateful PCE
                   draft-ietf-pce-lsp-extended-flags-04
 
 Abstract
@@ -16,7 +16,7 @@
    RFC 8231 describes a set of extensions to Path Computation Element
    Communication Protocol (PCEP) to enable stateful control of MPLS-TE
    and GMPLS Label Switched Paths (LSPs) via PCEP.  One of the
-   extensions is the LSP object which includes a Flag field of the
+   extensions is the LSP object which includes a Flag field with a
    length of 12 bits.  However, all bits of the Flag field have already
    been assigned in RFC 8231, RFC 8281, RFC 8623 and I-D.ietf-pce-
    binding-label-sid.
@@ -97,13 +97,13 @@
    [RFC5440] describes the Path Computation Element (PCE) Communication
    Protocol (PCEP) which is used between a PCE and a Path Computation
    Client (PCC) (or other PCE) to enable computation of Multi-protocol
-   Label Switching (MPLS) for Traffic Engineering Label Switched Path
-   (TE LSP).
+   Label Switching for Traffic Engineering (MPLS-TE) Label Switched Path
+   (LSP).
 
    PCEP Extensions for the Stateful PCE Model [RFC8231] describes a set
    of extensions to PCEP to enable active control of MPLS-TE and
    Generalized MPLS (GMPLS) tunnels.  One of the extensions is the LSP
-   object which contains a flag field; bits in the flag field are used
+   object, which contains a flag field; bits in the flag field are used
    to indicate delegation, synchronization, removal, etc.
 
 
@@ -115,15 +115,15 @@
 
 
    As defined in [RFC8231], the length of the flag field is 12 bits and
-   the value from bit 5 to bit 11 is used for operational,
+   the values from bit 5 to bit 11 are used for operational,
    administrative, remove, synchronize and delegate bits respectively.
-   The bit value 4 is assigned in [RFC8281] for create for PCE-Initiated
-   LSPs.  The bits from 1 to 3 is assigned in [RFC8623] for Explicit
+   The bit value 4 is assigned in [RFC8281] for create PCE-Initiated
+   LSPs.  The bits from 1 to 3 are assigned in [RFC8623] for Explicit
    Route Object (ERO)-compression, fragmentation and Point-to-Multipoint
    (P2MP) respectively.  The bit 0 is assigned in
    [I-D.ietf-pce-binding-label-sid] to PCE-allocation.  All bits of the
-   Flag field has been assigned already.  Thus, it is required to extend
-   the flag field for the LSP Object for future use.
+   Flag field have been assigned already.  Thus, it is required to extend
+   the flag field of the LSP Object for future use.
 
    This document proposes to define a new LSP-EXTENDED-FLAG TLV for an
    extended flag field in the LSP object.
@@ -151,7 +151,7 @@
 3.1.  The LSP-EXTENDED-FLAG TLV
 
    The format of the LSP-EXTENDED-FLAG TLV follows the format of all
-   PCEP TLVs as defined in [RFC5440] and is shown in the Figure 1.
+   PCEP TLVs as defined in [RFC5440] and is shown in Figure 1.
 
 
 
@@ -184,7 +184,7 @@
               Figure 1: Figure 1: LSP-EXTENDED-FLAG TLV Format
 
 
-   Type (16 bits): the value is TBD1 by IANA.
+   Type (16 bits): TBD1
 
    Length (16 bits): multiple of 4 octets.
 
@@ -200,7 +200,7 @@
 
 3.2.  Processing
 
-   The LSP Extended Flags field is an array of units of 32 flags and to
+   The LSP Extended Flags field is an array of units of 32 flags, to
    be allocated starting from the most significant bit.  The bits of the
    LSP Extended Flags field will be assigned by future documents.  This
    document does not define any flags.  Unassigned flags MUST be set to
@@ -208,7 +208,55 @@
    that do not understand any particular flag MUST ignore the flag.
    This flags should follow the specification as per [RFC8786].
 
-   Note that, PCEP peers MAY encounter different length of the LSP-
+---   
+jgs: RFC 8786 is primarily a patch document, that makes corrections
+specifically to the use of Request Parameters Flags. So, I think that
+strictly speaking, it doesn't make sense to say that "This flags should
+follow the specification as per [RFC8786]", first of all, because 8786
+isn't a standalone specification, and second, because it doesn't relate
+to the LSP object. 
+
+I agree that it's fairly obvious what you mean to say, which is something
+like "RFC 8786 has some rules, they are good rules, please consider them
+to apply to this TLV of this object also". It would be my preference, 
+though, that you just state the rule for your TLV here instead of 
+incorporating them by reference. 
+
+Based on a quick review of RFC 8786 it looks like you've already 
+incorporated most of it in your own text. The only relevant part that
+I think remains is this part of Section 3.1:
+
+                                                   each new
+      specification that defines additional flags is expected to
+      describe the interaction between these new flags and any existing
+      flags.  In particular, new specifications are expected to explain
+      how to handle the cases when both new and pre-existing flags are
+      set.
+
+Is there anything else you think is needed, that your document doesn't
+already cover? If not, I would favor:
+
+- Delete the sentence above ("This flags should follow the specification as 
+  per [RFC8786].")
+- Add a section to this document called "Advice for Specification of New 
+  Flags", that contains the needed text. Maybe something like:
+  
+4. Advice for Specification of New Flags
+
+   Following the model provided in [RFC 8786] Section 3.1, we provide
+   the following advice for new specifications that define additional
+   flags. Each such specification is expected to describe the
+   interaction between these new flags and any existing flags.  In
+   particular, new specifications are expected to explain how to handle
+   the cases when both new and pre-existing flags are set.
+   
+RFC 8786 would be an Informative reference, then.
+
+But that is just a suggestion, if you'd like to fix this some other way
+that's OK, let's discuss.
+---
+
+   Note that PCEP peers MAY encounter varying lengths of the LSP-
    EXTENDED-FLAG TLV.
 
    If a PCEP speaker receives the LSP-EXTENDED-FLAG TLV of a length more
@@ -233,8 +281,8 @@
 
    A router that does not understand or support the LSP-EXTENDED-FLAG
    TLV will silently ignore the TLV as per [RFC5440].  It is expected
-   that future document that define bits in the LSP-EXTENDED-FLAG TLV
-   would also define the error case handling required for missing LSP-
+   that future documents that define bits in the LSP-EXTENDED-FLAG TLV
+   will also define the error case handling required for missing LSP-
    EXTENDED-FLAG TLV if it MUST be present.
 
 5.  IANA Considerations
@@ -271,8 +319,13 @@
    *  Defining RFC
 
 
-   No values are currently defined.
-
+   No values are currently defined. Bits 0-31 should initially be marked 
+   as "Unassigned". Bits with a higher ordinal than 31 will be added to the 
+   registry in future documents if necessary
+   
+jgs: The above addition is per Jon Hardwick's rtgdir review, I assume 
+it was an oversight and not omitted deliberately, but if you actually
+don't want the text we should discuss further.
 
 
 
@@ -309,7 +362,7 @@
 
    At the time of posting this version of this document, there are no
    known implementations of this TLV.  It is believed that this would be
-   implemented along side the documents that allocate flags in the TLV.
+   implemented alongside the documents that allocate flags in the TLV.
 
 7.  Management Considerations