Re: [Lsr] AD review of draft-ietf-lsr-flex-algo-20

["Acee Lindem (acee)" <[e-mail]>]

Speaking as Document Shepherd:

Hi John,

Thanks much for your review and suggested text. I think these will improve the both the precision of the specification and the readability. I read though the comments and I don’t see any show stoppers although the additional operational considerations may take some thinking. Note that the main editor just went on PTO after you completed you review and it will be at least a 3 weeks before you receive a response (and maybe more).

See one comment inline.





From: Lsr <[e-mail]> on behalf of John Scudder <jgs=[e-mail]>
Date: Tuesday, August 16, 2022 at 5:46 PM
To: "[e-mail]" <[e-mail]>, "[e-mail]" <[e-mail]>
Subject: [Lsr] AD review of draft-ietf-lsr-flex-algo-20

Dear Authors,

Thanks for you patience as this document sat in my queue for too long. :-( Here’s my review.

I’ve supplied my comments in the form of an edited copy of the draft. Minor editorial suggestions I’ve made in place without further comment, more substantive comments are done in-line and prefixed with “jgs:”. You can use your favorite diff tool to review them; I’ve attached a PDF of 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-lsr-flex-algo-20.txt     2022-08-16 11:12:37.000000000 -0400
+++ draft-ietf-lsr-flex-algo-20-jgs-comments.txt        2022-08-16 17:37:22.000000000 -0400
@@ -24,7 +24,7 @@
    on the IGP metric assigned to the links.  Many network deployments
    use RSVP-TE based or Segment Routing based Traffic Engineering to
    steer traffic over a path that is computed using different metrics or
-   constraints than the shortest IGP path.  This document proposes a
+   constraints than the shortest IGP path.  This document specifies a
    solution that allows IGPs themselves to compute constraint-based
    paths over the network.  This document also specifies a way of using
    Segment Routing (SR) Prefix-SIDs and SRv6 locators to steer packets
@@ -155,12 +155,12 @@

 1.  Introduction

-   An IGP-computed path based on the shortest IGP metric is often be
-   replaced by a traffic-engineered path due to the traffic requirements
+   An IGP-computed path based on the shortest IGP metric is often
+   replaced by a traffic-engineered path due to requirements
    which are not reflected by the IGP metric.  Some networks engineer
    the IGP metric assignments in a way that the IGP metric reflects the
-   link bandwidth or delay.  If, for example, the IGP metric is
-   reflecting the bandwidth on the link and the user traffic is delay
+   link bandwidth or delay.  If, for example, the IGP metric
+   reflects the bandwidth on the link and user traffic is delay



@@ -171,7 +171,10 @@


    sensitive, the best IGP path may not reflect the best path from such
-   users' perspective.
+   a user's perspective.
+
+jgs: or "from such users' perspectives" but as it was written, there was a
+disagreement in number, so I made it singular 'user'.

    To overcome this limitation, various sorts of traffic engineering
    have been deployed, including RSVP-TE and SR-TE, in which case the TE
@@ -179,7 +182,7 @@
    metrics and/or constraints.  Such paths need to be installed in the
    forwarding tables in addition to, or as a replacement for, the
    original paths computed by IGPs.  Tunnels are often used to represent
-   the engineered paths and mechanisms like one described in [RFC3906]
+   the engineered paths and mechanisms like the one described in [RFC3906]
    are used to replace the native IGP paths with such tunnel paths.

    This document specifies a set of extensions to IS-IS, OSPFv2, and
@@ -193,10 +196,29 @@
    of calculation-type, metric-type, and constraints.

    This document also specifies a way for a router to use IGPs to
-   associate one or more SR Prefix-SIDs or SRv6 locators with a
+   associate one or more Segment Routing (SR) Prefix-SIDs or Segment Routing over IPv6 (SRv6) locators with a
    particular Flex-Algorithm.  Each such Prefix-SID or SRv6 locator then
    represents a path that is computed according to the identified Flex-
    Algorithm.
+
+jgs: "SR" is in the well-known abbreviations list
+(https://www.rfc-editor.org/materials/abbrev.expansion.txt), but "SRv6"
+isn't. We could suggest the RFC Editor add "SRv6", but really it seems
+reader-friendly to expand these on first use anyway, as I've done with the
+text above.
+
+jgs: Making this edit also led me to wonder -- does "SR" above really mean
+"SR-MPLS"? Because "SR" as such is an architecture, right, not an
+instantiation? (SR-MPLS is also not in the well-known abbreviations list...)
+
+jgs: Please provide references for Prefix-SID and SRv6 locator?
+
+jgs: Finally, it seems to me as though a useful piece of context for the
+new reader is something like what I'm suggesting below:
+
+   Not all routers in a given network need to participate in a given
+   Flexible Algorithm. The Flexible Algorithm(s) a given router
+   participates in is determined by configuration.

 2.  Requirements Language

@@ -238,7 +260,7 @@

    IGP Algorithm - value from the the "IGP Algorithm Types" registry
    defined under "Interior Gateway Protocol (IGP) Parameters" IANA
-   registries.  IGP Algorithms represents the triplet (Calculation Type,
+   registry grouping.  IGP Algorithms represents the triplet (Calculation Type,
    Metric, Constraints), where the second and third elements of the
    triple MAY be unspecified.

@@ -258,7 +280,7 @@
    also possible.  Combinations of these are also possible.

    To provide maximum flexibility, we want to provide a mechanism that
-   allows a router to (a) identify a particular calculation-type, (b)
+   allows a router to (a) identify a particular calculation-type and (b)
    metric-type, (c) describe a particular set of constraints, and (d)
    assign a numeric identifier, referred to as Flex-Algorithm, to the
    combination of that calculation-type, metric-type, and those
@@ -283,22 +305,26 @@


    Flexible-Algorithm is a numeric identifier in the range 128-255 that
-   is associated via configuratin with the Flexible-Algorithm
+   is associated via configuration with the Flexible-Algorithm
    Definition.

-   IANA "IGP Algorithm Types" registry defines the set of values for IGP
-   Algorithms.  We propose to allocate the following values for Flex-
-   Algorithms from this registry:
+   The IANA "IGP Algorithm Types" registry defines the set of values for IGP
+   Algorithms.  The following values area allocated from this registry for Flex-
+   Algorithms:

       128-255 - Flex-Algorithms

 5.  Flexible Algorithm Definition Advertisement

-   To guarantee the loop-free forwarding for paths computed for a
+   To guarantee loop-free forwarding for paths computed for a
    particular Flex-Algorithm, all routers that (a) are configured to
    participate in a particular Flex-Algorithm, and (b) are in the same
    Flex-Algorithm definition advertisement scope MUST agree on the
    definition of the Flex-Algorithm.
+
+jgs: It would be correct -- wouldn't it? -- to add "The following procedures
+ensure this condition is fulfilled." If that's not wrong, I think it would
+help the new reader.

 5.1.  IS-IS Flexible Algorithm Definition Sub-TLV

@@ -341,7 +367,7 @@
       Flex-Algorithm: Single octet value between 128 and 255 inclusive.

       Metric-Type: Type of metric to be used during the calculation.
-      Following values are defined:
+      The following values are defined:

          0: IGP Metric

@@ -362,9 +388,16 @@
       The Metric/Constraints MUST NOT be inherited.  If the required
       calculation type is Shortest Path First, the value 0 SHOULD appear
       in this field.
+
+jgs: Why isn't the SHOULD a MUST? If SHOULD is appropriate, it would be
+desirable to explain the anticipated exception cases here.

       Priority: Value between 0 and 255 inclusive that specifies the
-      priority of the advertisement.
+      priority of the advertisement.  Numerically greater values are
+      preferred.
+
+jgs: I see that in 5.3 (1) you tell us that this is a higher-better value.
+It might be nice to mention that here. I've suggested text above.

       Sub-TLVs - optional sub-TLVs.

@@ -375,15 +408,15 @@
    The IS-IS FAD Sub-TLV has an area scope.  The Router Capability TLV
    in which the FAD Sub-TLV is present MUST have the S-bit clear.

-   IS-IS L1/L2 router MAY be configured to re-generate the winning FAD
-   from level 2, without any modification to it, to level 1 area.  The
+   An IS-IS L1/L2 router MAY be configured to re-generate the winning FAD
+   from level 2, without any modification to it, to the level 1 area.  The
    re-generation of the FAD Sub-TLV from level 2 to level 1 is
    determined by the L1/L2 router, not by the originator of the FAD
-   advertisement in the level 2.  In such case, the re-generated FAD
+   advertisement in the level 2.  In such a case, the re-generated FAD
    Sub-TLV will be advertised in the level 1 Router Capability TLV
    originated by the L1/L2 router.

-   L1/L2 router MUST NOT re-generate any FAD Sub-TLV from level 1 to
+   An L1/L2 router MUST NOT re-generate any FAD Sub-TLV from level 1 to
    level 2.


@@ -396,10 +429,10 @@

 5.2.  OSPF Flexible Algorithm Definition TLV

-   OSPF FAD TLV is advertised as a top-level TLV of the RI LSA that is
+   The OSPF FAD TLV is advertised as a top-level TLV of the Router Information (RI) LSA that is
    defined in [RFC7770].

-   OSPF FAD TLV has the following format:
+   The OSPF FAD TLV has the following format:


       0                   1                   2                   3
@@ -426,7 +459,7 @@
       inclusive.

       Metric-Type: Type of metric to be used during the calculation.
-      Following values are defined:
+      The following values are defined:

          0: IGP Metric

@@ -467,7 +500,7 @@
    The RI LSA can be advertised at any of the defined opaque flooding
    scopes (link, area, or Autonomous System (AS)).  For the purpose of
    OSPF FAD TLV advertisement, area-scoped flooding is REQUIRED.  The
-   Autonomous System flooding scope SHOULD NOT be used by default unless
+   Autonomous System flooding scope SHOULD NOT be used unless
    local configuration policy on the originating router indicates domain
    wide flooding.

@@ -485,17 +518,17 @@

    Every router, that is configured to participate in a particular Flex-
    Algorithm, MUST select the Flex-Algorithm definition based on the
-   following ordered rules.  This allows for the consistent Flex-
+   following ordered rules.  This allows for consistent Flex-
    Algorithm definition selection in cases where different routers
    advertise different definitions for a given Flex-Algorithm:

       1.  From the advertisements of the FAD in the area (including both
       locally generated advertisements and received advertisements)
-      select the one(s) with the highest priority value.
+      select the one(s) with the numerically greatest priority value.

       2.  If there are multiple advertisements of the FAD with the same
-      highest priority, select the one that is originated from the
-      router with the highest System-ID, in the case of IS-IS, or Router
+      numerically greatest priority, select the one that is originated from the
+      router with the numerically greatest System-ID, in the case of IS-IS, or Router
       ID, in the case of OSPFv2 and OSPFv3.  For IS-IS, the System-ID is


@@ -508,6 +541,12 @@

       described in [ISO10589].  For OSPFv2 and OSPFv3, standard Router
       ID is described in [RFC2328] and [RFC5340] respectively.
+
+   The FAD selected according to these rules is also known as the
+   "winning FAD".
+
+jgs: I suggested the above text because you refer to the "winning FAD" multiple
+places but don't formally define that term.

    A router that is not configured to participate in a particular Flex-
    Algorithm MUST ignore FAD Sub-TLVs advertisements for such Flex-
@@ -515,13 +554,17 @@

    A router that is not participating in a particular Flex-Algorithm is
    allowed to advertise FAD for such Flex-Algorithm.  Receiving routers
-   MUST consider FAD advertisement regardless of the Flex-Algorithm
-   participation of the FAD originator.
+   MUST consider a received FAD advertisement regardless of the Flex-Algorithm
+   participation of that FAD advertisement's originator.

    Any change in the Flex-Algorithm definition may result in temporary
    disruption of traffic that is forwarded based on such Flex-Algorithm
    paths.  The impact is similar to any other event that requires
    network-wide convergence.
+
+jgs: IMO this would merit discussion in the Operational Considerations
+section.  In particular, is there any advice regarding how to
+stage/sequence FAD config changes in order to minimize disruption?

    If a node is configured to participate in a particular Flexible-
    Algorithm, but there is no valid Flex-Algorithm definition available
@@ -545,14 +588,14 @@
    maximum length supported by a single FAD sub-TLV.  In such cases, the
    FAD may be split into multiple such sub-TLVs and the content of the
    multiple FAD sub-TLVs combined to provide a complete FAD for the
-   Flex-Algorithm.  In such case, the fixed portion of the FAD (see
+   Flex-Algorithm.  In such a case, the fixed portion of the FAD (see
    Section 5.1) MUST be identical in all FAD sub-TLVs for a given Flex-
    Algorithm from a given IS.  In case the fixed portion of such FAD
    Sub-TLVs differ, the values in the fixed portion in the FAD sub-TLV
    in the first occurrence in the lowest numbered LSP from a given IS
    MUST be used.

-   Any specification that introduces a new ISIS FAD sub-sub-TLV MUST
+   Any specification that introduces a new IS-IS FAD sub-sub-TLV MUST
    specify whether the FAD sub-TLV may appear multiple times in the set


@@ -604,7 +647,7 @@

    The IS-IS FAEAG Sub-TLV MUST NOT appear more than once in the set of
    FAD sub-TLVs for a given Flex-Algorithm from a given IS.  If it
-   appears more than once in such set, the IS-IS FAEAG Sub-TLV in the
+   appears more than once in such a set, the IS-IS FAEAG Sub-TLV in the
    first occurrence in the lowest numbered LSP from a given IS MUST be
    used and any other occurrences MUST be ignored.

@@ -625,7 +668,7 @@
    computation.

    The IS-IS Flexible Algorithm Include-Any Admin Group Sub-TLV is used
-   to advertise include-any rule that is used during the Flex-Algorithm
+   to advertise the include-any rule that is used during the Flex-Algorithm
    path calculation as specified in Section 13.

    The format of the IS-IS Flexible Algorithm Include-Any Admin Group
@@ -642,7 +685,7 @@

    The IS-IS Flexible Algorithm Include-Any Admin Group Sub-TLV MUST NOT
    appear more than once in the set of FAD sub-TLVs for a given Flex-
-   Algorithm from a given IS.  If it appears more than once in such set,
+   Algorithm from a given IS.  If it appears more than once in such a set,
    the IS-IS Flexible Algorithm Include-Any Admin Group Sub-TLV in the
    first occurrence in the lowest numbered LSP from a given IS MUST be
    used and any other occurrences MUST be ignored.
@@ -650,11 +693,11 @@
 6.3.  IS-IS Flexible Algorithm Include-All Admin Group Sub-TLV

    The Flexible Algorithm definition can specify 'colors' that are used
-   by the operator to include link during the Flex-Algorithm path
+   by the operator to include links during the Flex-Algorithm path
    computation.

    The IS-IS Flexible Algorithm Include-All Admin Group Sub-TLV is used
-   to advertise include-all rule that is used during the Flex-Algorithm
+   to advertise the include-all rule that is used during the Flex-Algorithm
    path calculation as specified in Section 13.

    The format of the IS-IS Flexible Algorithm Include-All Admin Group
@@ -679,7 +722,7 @@

    The IS-IS Flexible Algorithm Include-All Admin Group Sub-TLV MUST NOT
    appear more than once in the set of FAD sub-TLVs for a given Flex-
-   Algorithm from a given IS.  If it appears more than once in such set,
+   Algorithm from a given IS.  If it appears more than once in such a set,
    the IS-IS Flexible Algorithm Include-All Admin Group Sub-TLV in the
    first occurrence in the lowest numbered LSP from a given IS MUST be
    used and any other occurrences MUST be ignored.
@@ -703,6 +746,13 @@
       Type: 4

       Length: variable, non-zero number of octets of the Flags field
+
+jgs: Is it fine if the length is not a multiple of four? If so, why the
+difference from the way you define length in §6.1?
+
+Also, this is the only Length field in this document that specifies
+it must be non-zero. That makes me think all the other Length fields
+MAY be zero. Is that right? Or, should they also say "non-zero"?

       Flags:

@@ -739,7 +789,7 @@

    The IS-IS FADF Sub-TLV MUST NOT appear more than once in the set of
    FAD sub-TLVs for a given Flex-Algorithm from a given IS.  If it
-   appears more than once in such set, the IS-IS FADF Sub-TLV in the
+   appears more than once in such a set, the IS-IS FADF Sub-TLV in the
    first occurrence in the lowest numbered LSP from a given IS MUST be
    used and any other occurrences MUST be ignored.

@@ -801,7 +851,7 @@
    sub-TLVs for a given Flex-Algorithm from a given IS.  This may be
    necessary in cases where the total number of SRLG values which are
    specified cause the FAD sub-TLV to exceed the maximum length of a
-   single FAD sub-TLV.  In such case the receiver MUST use the union of
+   single FAD sub-TLV.  In such a case the receiver MUST use the union of
    all values across all IS-IS FAESRLG Sub-TLVs from such set.

 7.  Sub-TLVs of OSPF FAD TLV
@@ -809,7 +859,7 @@
 7.1.  OSPF Flexible Algorithm Exclude Admin Group Sub-TLV

    The Flexible Algorithm Exclude Admin Group Sub-TLV (FAEAG Sub-TLV) is
-   a Sub-TLV of the OSPF FAD TLV.  It's usage is described in
+   a Sub-TLV of the OSPF FAD TLV.  Its usage is described in
    Section 6.1.  It has the following format:

     0                   1                   2                   3
@@ -1082,9 +1132,9 @@

       Length: 8 octets

-      Flex-Algorithm: Single octet value between 128 and 255 inclusive.
+      Flex-Algorithm: Single octet, value between 128 and 255 inclusive.

-      Flags: single octet value
+      Flags: One octet

                  0 1 2 3 4 5 6 7
                 +-+-+-+-+-+-+-+-+
@@ -1094,12 +1144,26 @@
          E bit : position 0: The type of external metric.  If bit is
          set, the metric specified is a Type 2 external metric.  This
          bit is applicable only to OSPF External and NSSA external
-         prefixes.  This is semantically the same as E bit in section
+         prefixes.  This is semantically the same as the E bit in section
          A.4.5 of [RFC2328] and section A.4.7 of [RFC5340] for OSPFv2
          and OSPFv3 respectively.

          Bits 1 through 7: MUST be cleared by sender and ignored by
          receiver.
+
+jgs: Are "sender" and "receiver" sufficiently clear to OSPF practitioners
+that there would be no ambiguity? I can think of two different ways
+to read them -- one is that the "sender" is the router that
+originates the LSA, and the "receiver" is any router that processes
+the LSA. I think that's what you mean. The other, pedantic, reading,
+is the "sender" is any router that puts the LSA on the wire, and the
+"receiver" is any router that takes the LSA from the wire, so anyone
+participating in flooding would be both a "sender" and a "receiver"
+at times.
+
+If this is how people write OSPF specs and talk about OSPF, fine.
+But if there are more precise terms than "sender" and "receiver" in
+use, it would be nice to use them.

       Reserved: Must be set to 0, ignored at reception.

@@ -1140,7 +1204,7 @@
    LSA is equivalent to the fixed format Type 4 Summary LSA [RFC2328].
    Unlike the Type 4 Summary LSA, the LSID of the EIA-ASBR LSA does not
    carry the ASBR Router-ID - the ASBR Router-ID is carried in the body
-   of the LSA.  OSPFv2 EIA-ASBR LSA is advertised by an OSPFv2 ABR and
+   of the LSA.  The OSPFv2 EIA-ASBR LSA is advertised by an OSPFv2 ABR and
    its flooding is defined to be area-scoped only.

    An OSPFv2 ABR generates the EIA-ASBR LSA for an ASBR when it is
@@ -1194,15 +1258,36 @@
      |                                                               |
      +-                            TLVs                             -+
      |                             ...                               |
+
+jgs: Maybe add something like

+   Other than where specified below, these fields' definitions are as
+   given in [RFC2328] Section A.4.1.

-   The Opaque Type used by the OSPFv2 EIA-ASBR LSA is TBD (suggested
+   The Opaque Type used by the OSPFv2 EIA-ASBR LSA is TBD1 (suggested
    value 11).  The Opaque Type is used to differentiate the various
    types of OSPFv2 Opaque LSAs and is described in Section 3 of
    [RFC5250].  The LS Type MUST be 10, indicating that the Opaque LSA
    flooding scope is area-local [RFC5250].  The LSA Length field
    [RFC2328] represents the total length (in octets) of the Opaque LSA,
    including the LSA header and all TLVs (including padding).
+
+jgs: The way you've written "The LSA Length field [RFC2328]" is a little
+confusing. The reference makes it sound as though you're simply relying
+on the RFC 2328 definition, and I guess you basically are since the
+TLVs portion is just the balance of "the LSA". But RFC 2328 doesn't
+define a Length field (so capitalized) which makes unpacking this
+harder than it needs to be.
+
+Since the Length field is just exactly what RFC 2328 says it is, the
+easiest fix would be to remove that entire sentence. No need to
+redefine something that's already defined, unless it's needed for
+clarity.
+
+If you do feel it's needed for clarity, then I suggest something like
+"The Length field is as defined in [RFC2328] Section A.4.1. It
+represents the total length (in octets) of the Opaque LSA, including
+the LSA header and all TLVs (including padding)."

    The Opaque ID field is an arbitrary value used to maintain multiple
    OSPFv2 EIA-ASBR LSAs.  For OSPFv2 EIA-ASBR LSAs, the Opaque ID has no
@@ -1220,11 +1305,28 @@
    TLV is padded to 4-octet alignment; padding is not included in the
    Length field (so a 3-octet value would have a length of 3, but the
    total size of the TLV would be 8 octets).  Nested TLVs are also
-   32-bit aligned.  For example, a 1-byte value would have the Length
+   32-bit aligned.  For example, a 1-octet value would have the Length
    field set to 1, and 3 octets of padding would be added to the end of
    the value portion of the TLV.  The padding is composed of zeros.
-
-
+
+jgs: I have mixed feelings about how you cut-and-paste the definition from
+RFC 3630 Section 2.3.2 instead of just referencing it. On one hand, the
+material starting with "for example" is new, provides more clarity, and
+the requirement for padding to be zeroes is new. On the other hand, your
+reference to the Length field, which makes sense in the original context
+of RFC 3630 §2.3.2, is confusing here -- you have a diagram above with a
+field called Length, but that is NOT what you're talking about here.
+In 3630 there's a TLV diagram that makes it clear at a glance what's
+being talked about.
+
+Again I think the easiest fix is to leave the first sentence (adding
+"Section 2.3.2" to the reference) and remove the rest, although if it's
+important to specify zero-padding then leave that sentence.
+
+On the other hand if you feel the full detail is needed for clarity,
+then go all the way and make this its own subsection and don't just
+copy-paste the definition portion from 3630, copy the TLV diagram too,
+so the reader isn't led astray.
Since the included text is only a couple sentences, I think it is clearer to include it as has been done in other OSPF documents. To avoid the ambiguity that you have pointed out, we can replace “Length field” with “TLV or Sub-TLV length”. While I don’t think replication of the TLV format in a diagram is needed, it better to have the very brief text rather than require going to RFC 3630 to learn the encoding rules. One only has to cite the infamous RFC 7752 as a document that is unwieldy, in part, due to the number of external references required for the encodings.
Thanks,
Acee


@@ -1271,7 +1373,7 @@
    other than the first one in an LSA.

    OSPFv2 EIA-ASBR TLV MUST be present inside an OSPFv2 EIA-ASBR LSA
-   with at least a single sub-TLV included, otherwise the OSPFv2 EIA-
+   and must include at least a single sub-TLV, otherwise the OSPFv2 EIA-
    ASBR LSA MUST be ignored by the receiver.

 10.2.  OSPF Flexible Algorithm ASBR Metric Sub-TLV
@@ -1308,13 +1410,13 @@

    where:

-      Type: 1 for OSPFv2, TBD (suggested value 30) for OSPFv3
+      Type: 1 for OSPFv2, TBD2 (suggested value 30) for OSPFv3

       Length: 8 octets

       Flex-Algorithm: Single octet value between 128 and 255 inclusive.

-      Reserved: Must be set to 0, ignored at reception.
+      Reserved: Three octets. Must be set to 0, ignored at reception.

       Metric: 4 octets of metric information

@@ -1324,9 +1426,9 @@
    ignored.

    The advertisement of the ASBR reachability using the OSPF FAAM Sub-
-   TLV inside the OSPFv2 EIA-ASBR LSA follows the section 12.4.3 of
-   [RFC2328] and inside the OSPFv3 E-Inter-Area-Router LSA follows the
-   section 4.8.5 of [RFC5340].  The reachability of the ASBR is
+   TLV inside the OSPFv2 EIA-ASBR LSA follows Section 12.4.3 of
+   [RFC2328] and inside the OSPFv3 E-Inter-Area-Router LSA follows
+   Section 4.8.5 of [RFC5340].  The reachability of the ASBR is
    evaluated in the context of the specific Flex-Algorithm.

    The FAAM computed by the ABR will be equal to the metric to reach the
@@ -1350,7 +1452,7 @@
    specific Flex-Algorithm.

    An OSPF ABR MUST include the OSPF FAAM Sub-TLVs as part of the ASBR
-   reachability advertisement between areas for the Flex-Algorithm for
+   reachability advertisement between areas for any Flex-Algorithm for
    which the winning FAD includes the M-flag and the ASBR is reachable
    in the context of that specific Flex-Algorithm.

@@ -1364,8 +1466,8 @@
    specified in section 16.2 of [RFC2328] and section 4.8.5 of [RFC5340]
    for OSPFv2 and OSPFv3 respectively.

-   The processing of the new or changed OSPF FAAM Sub-TLV triggers the
-   processing of the External routes similar to what is described in
+   The processing of a new or changed OSPF FAAM Sub-TLV triggers the
+   processing of External routes similar to what is described in
    section 16.5 of the [RFC2328] for OSPFv2 and section 4.8.5 of
    [RFC5340] for OSPFv3 for the specific Flex-Algorithm.  The External
    and NSSA External route calculation should be limited to Flex-
@@ -1382,6 +1484,11 @@
    presence of the base LSA is not mandatory for the usage of the
    extended LSA with the OSPF FAAM Sub-TLV.  This means that the order
    in which these LSAs are received is not significant.
+
+jgs: The "it is expected" leads me to wonder, what is the consequence if
+that expectation is violated. Should this be a mandate ("MUST advertise
+the reachability of the same ASBR in...") rather than just an
+expectation?

 11.  Advertisement of Node Participation in a Flex-Algorithm

@@ -1403,17 +1510,17 @@


    Flex-Algorithm for each data-plane.  Some data-planes may share a
-   common participation advertisement (e.g.  SR MPLS and SRv6).
+   common participation advertisement (e.g.  SR-MPLS and SRv6).

 11.1.  Advertisement of Node Participation for Segment Routing

    [RFC8667], [RFC8665], and [RFC8666] (IGP Segment Routing extensions)
    describe how the SR-Algorithm is used to compute the IGP best path.

-   Routers advertise the support for the SR-Algorithm as a node
-   capability as described in the above mentioned IGP Segment Routing
+   Routers advertise support for the SR-Algorithm as a node
+   capability as described in the above-mentioned IGP Segment Routing
    extensions.  To advertise participation for a particular Flex-
-   Algorithm for Segment Routing, including both SR MPLS and SRv6, the
+   Algorithm for Segment Routing, including both SR-MPLS and SRv6, the
    Flex-Algorithm value MUST be advertised in the SR-Algorithm TLV
    (OSPF) or sub-TLV (IS-IS).

@@ -1477,10 +1584,46 @@
    Administrative Group encodings.  If the Administrative Group encoding
    is used, then the first 32 bits of the corresponding FAD sub-TLVs are
    mapped to the link attribute advertisements as specified in RFC 7308.
+
+jgs: I think "the first 32 bits of the corresponding FAD sub-TLVs" can't
+possibly be strictly the correct wording. But before you read the rest
+of this long (ish) comment, keep in mind that my preferred solution is
+to drop the final sentence -- if you agree then we're done. If not, read
+on.
+
+First, I think you must be talking about the various admin group
+sub-TLVs (so, exclude admin group, include-any admin group, include-all
+admin group) and this is what you mean by "the corresponding FAD
+sub-TLVs"? If so, please be more specific, for example by listing the
+sub-TLV types explicitly.
+
+Second, I guess you don't mean strictly the first 32 bits, but rather
+the first 32 bits of the value portion?
+
+Finally, when you say "mapped... as specified in RFC 7308" I'm more
+confused than if you'd said nothing (or I think I am!). As I read 7308
+Section 3.2.1, we have these options:
+
+What's present  How to interpret
+--------------  ----------------
+AG              Bits 0-31 from AG
+EAG             Bits 0-N from EAG
+AG + EAG        Bits 0-31 from AG
+                Bits 32-N from EAG (ignoring bits 0-31 other than
+                                    possibly logging mismatches)
+
+Since the value portion of the various FAD admin group sub-TLVs
+doesn't have variant encodings, I wouldn't think there is a need to
+talk about how to map those bits (which are unambiguously 0-N).
+And RFC 7308 is already clear about how to different combinations
+of AG and EAG are mapped.
+
+If you mean something different from the above, let's please
+discuss.

    A receiver supporting this specification MUST accept both ASLA
    Administrative Group and Extended Administrative Group TLVs as
-   defined in [RFC8919] or [RFC8920].  In the case of ISIS, if the
+   defined in [RFC8919] or [RFC8920].  In the case of IS-IS, if the
    L-Flag is set in ASLA advertisement, as defined in [RFC8919]
    Section 4.2, then the receiver MUST be able to accept both
    Administrative Group TLV as defined in [RFC5305] and Extended
@@ -1501,6 +1644,13 @@
    Algorithm MUST be advertised on a per data-plane basis.  Calculation
    of the paths for any particular Flex-Algorithm MUST be data-plane
    specific.
+
+jgs: I don't understand your use of MUST above, regarding calculation of
+paths. Computing paths on a data-plane basis seems like it's fundamental
+to this spec -- is there some other choice you think an implementor
+might even make? I would think changing the MUST to (for example) "is"
+would keep the sense of what's being said without causing surprise to
+the reader.

    Multiple data-planes MAY use the same Flex-Algorithm value at the
    same time, and and as such, share the FAD for it.  Traffic for each
@@ -1523,9 +1673,13 @@
    calculation, then when computing paths for a given Flex-Algorithm,
    all nodes that do not advertise participation for that Flex-Algorithm
    in their data-plane specific advertisements MUST be pruned from the
-   topology.  Segment Routing, including both SR MPLS and SRv6, are
+   topology.  Segment Routing, including both SR-MPLS and SRv6, are
    data-planes that MUST use such pruning when computing Flex-Algorithm
    paths.
+
+jgs: As I mentioned much earlier in this review, Segment Routing references
+seem to be in order. The fact you're making quite specific requirements
+on those data planes re-emphasizes that need.

    When computing the path for a given Flex-Algorithm, the metric-type
    that is part of the Flex-Algorithm definition (Section 5) MUST be
@@ -1538,11 +1692,26 @@
    Various link include or exclude rules can be part of the Flex-
    Algorithm definition.  To refer to a particular bit within an AG or
    EAG we use the term 'color'.
+
+jgs: Please expand AG and EAG on first use, or place in the terminology section.

    Rules, in the order as specified below, MUST be used to prune links
    from the topology during the Flex-Algorithm computation.
+
+jgs: Since the rules are a sieve (each rule reduces the size of the set left
+by the previous rule) the order of applying them appears to be irrelevant.

    For all links in the topology:
+
+jgs: So, perhaps the previous two paragraphs could be condensed something like,
+
+   Prior to running the Flex-Algorithm computation, the topology is
+   pruned as follows:
+
+(You could write "MUST be pruned" if you feel the need for an RFC 2119
+keyword, although honestly I don't think it's crucial -- similar to my
+earlier comment, it's hard to imagine an implementor making a different
+choice.)

       1.  Check if any exclude AG rule is part of the Flex-Algorithm
       definition.  If such exclude rule exists, check if any color that
@@ -1580,7 +1749,7 @@
       (Section 5), and such metric is not advertised for the particular
       link in a topology for which the computation is done, such link
       MUST be pruned from the computation.  A metric of value 0 MUST NOT
-      be assumed in such case.
+      be assumed in such a case.

 13.1.  Multi-area and Multi-domain Considerations

@@ -1647,6 +1816,9 @@
    Algorithm (with FAD selected includes the M-Flag) where the
    advertising ASBR is in a remote area, the metric will be the sum of
    the following:
+
+jgs: I don't understand what the words in parentheses are trying to say, can
+you explain?

    o  the FAPM for that Flex-Algorithm advertised with the external
       route by the ASBR
@@ -1684,8 +1856,14 @@

    conclude whether the ABR or ASBR has reachability to the inter-area
    or inter-domain prefix for a given Flex-Algorithm in the next area or
-   domain.  Sending the Flex-Algoritm traffic for such prefix towards
+   domain.  Sending the Flex-Algoritm traffic for such a prefix towards
    the ABR or ASBR may result in traffic looping or black-holing.
+
+jgs: I think we've discussed in the context of a different draft recently,
+that there's some desire to consider terms other than "black-holing".
+One reason is that although it's a familiar term to many of us, it
+still may be an obstacle to understanding for people unfamiliar with
+the idiom. Perhaps consider "persistent traffic loss"?

    During the route computation, it is possible for the Flex-Algorithm
    specific metric to exceed the maximum value that can be stored in an
@@ -1698,7 +1876,7 @@
    advertised for these route-types, it MUST be ignored during the
    prefix reachability calculation.

-   The M-flag in FAD is not applicable to prefixes advertised as SRv6
+   The M-flag in the FAD is not applicable to prefixes advertised as SRv6
    locators.  The IS-IS SRv6 Locator TLV
    [I-D.ietf-lsr-isis-srv6-extensions] includes the Algorithm and Metric
    fields.  When the SRv6 Locator is advertised between areas or
@@ -1740,6 +1918,12 @@

    to avoid such partitioning by providing enough redundancy inside the
    area for each Flex-Algorithm being used.
+
+jgs: Instead of "the algorithm 0", how about "the base algorithm"?
+
+The warning seems on point, but the recommendation at the end made me a
+little sad, since it amounts to "don't build bad networks". At minimum,
+maybe change "avoid" to "minimize the risk of"?

 14.  Flex-Algorithm and Forwarding Plane

@@ -1748,7 +1932,7 @@

 14.1.  Segment Routing MPLS Forwarding for Flex-Algorithm

-   This section describes how Flex-Algorithm paths are used with SR MPLS
+   This section describes how Flex-Algorithm paths are used with SR-MPLS
    forwarding.

    Prefix SID advertisements include an SR-Algorithm value and, as such,
@@ -1774,9 +1958,11 @@
    advertised specifically for the given algorithm.  LFA paths MUST NOT
    use an Adjacency-SID that belongs to a link that has been pruned from
    the Flex-Algorithm computation.
+
+jgs: Maybe supply a reference for LFA.

    If LFA protection is being used to protect a given Flex-Algorithm
-   paths, all routers in the area participating in the given Flex-
+   path, all routers in the area participating in the given Flex-
    Algorithm SHOULD advertise at least one Flex-Algorithm specific Node-
    SID.  These Node-SIDs are used to steer traffic over the LFA computed
    backup path.
@@ -1794,15 +1980,21 @@
 Internet-Draft           IGP Flexible Algorithm                 May 2022


-   In SRv6 a node is provisioned with topology/algorithm specific
-   locators for each of the topology/algorithm pairs supported by that
+   In SRv6 a node is provisioned with a topology/algorithm specific
+   locator for each topology/algorithm pair supported by that
    node.  Each locator is an aggregate prefix for all SIDs provisioned
    on that node which have the matching topology/algorithm.
+
+jgs: If the usage "topology/algorithm" isn't already widespread, may I
+suggest "(topology, algorithm)" instead? I tend to read the slash
+as either/or, whereas what you mean is a pair.
+
+It's not a big deal and if the ship has sailed already, fine.

    The SRv6 locator advertisement in IS-IS
    [I-D.ietf-lsr-isis-srv6-extensions] includes the MTID value that
    associates the locator with a specific topology.  SRv6 locator
-   advertisements also includes an Algorithm value that explicitly
+   advertisements also include an Algorithm value that explicitly
    associates the locator with a specific algorithm.  When the algorithm
    value advertised with a locator represents a Flex-Algorithm, the
    paths to the locator prefix MUST be calculated using the specified
@@ -1857,7 +2049,7 @@
    The scope of the Flex-Algorithm computation is an area, so is the
    scope of the FAD.  In IS-IS, the Router Capability TLV in which the
    FAD Sub-TLV is advertised MUST have the S-bit clear, which prevents
-   it to be flooded outside of the level in which it was originated.
+   it from being flooded outside of the level in which it was originated.
    Even though in OSPF the FAD Sub-TLV can be flooded in an RI LSA that
    has AS flooding scope, the FAD selection is performed for each
    individual area in which it is being used.
@@ -1879,7 +2071,7 @@
    RI LSA in which the FAD Sub-TLV for the particular Flex-Algoritm is
    advertised.

-   Re-generation of FAD from a level 1 area to the level 2 area is not
+   Re-generation of the FAD from a level 1 area to the level 2 area is not
    supported in IS-IS, so if the intent is to regenerate the FAD between
    IS-IS levels, the FAD MUST be defined on router(s) that are in level
    2.  In OSPF, the FAD definition can be done in any area and be
@@ -1947,7 +2139,7 @@

    This extension brings no new backward compatibility issues.  IS-IS,
    OSPFv2 and OSPFv3 all have well defined handling of unrecognized TLVs
-   and sub-TLVs that allows the introduction of the new extensions,
+   and sub-TLVs that allows the introduction of new extensions,
    similar to those defined here, without introducing any
    interoperability issues.

@@ -1996,8 +2188,8 @@
 18.1.2.  IGP Metric-Type Registry

    IANA is requested to set up a registry called "IGP Metric-Type
-   Registry" under an "Interior Gateway Protocol (IGP) Parameters" IANA
-   registries.  The registration policy for this registry is "Standards
+   Registry" under the "Interior Gateway Protocol (IGP) Parameters" IANA
+   grouping.  The registration policy for this registry is "Standards
    Action" ([RFC8126] and [RFC7120]).

    Values in this registry come from the range 0-255.
@@ -2036,9 +2228,12 @@
 18.2.  Flexible Algorithm Definition Flags Registry

    IANA is requested to set up a registry called "IS-IS Flexible
-   Algorithm Definition Flags Registry" under an "Interior Gateway
-   Protocol (IGP) Parameters" IANA registries.  The registration policy
+   Algorithm Definition Flags Registry" under the "Interior Gateway
+   Protocol (IGP) Parameters" IANA grouping.  The registration policy
    for this registry is "Standards Action" ([RFC8126] and [RFC7120]).
+
+   New registrations should be assigned in ascending bit order (see
+   Section 6.4).

    This document defines the following single bit in Flexible Algorithm
    Definition Flags registry:
@@ -2052,14 +2247,18 @@

 18.3.  IS-IS IANA Considerations

-18.3.1.  Sub TLVs for Type 242
+18.3.1.  IS-IS Sub-TLVs for IS-IS Router CAPABILITY TLV

-   This document makes the following registrations in the "sub-TLVs for
-   TLV 242" registry.
+   This document makes the following registrations in the "IS-IS Sub-TLVs
+   for IS-IS Router CAPABILITY TLV" registry.

       Type: 26.

-      Description: Flexible Algorithm Definition.
+      Description: Flexible Algorithm Definition (FAD).
+
+jgs: Since this is frequently referred to in this spec as "FAD" I've suggested
+updating its description to include that. I've done similarly below for
+OSPF and for a few other acronyms.

       Reference: This document (Section 5.1).

@@ -2074,24 +2273,33 @@
 Internet-Draft           IGP Flexible Algorithm                 May 2022


-18.3.2.  Sub TLVs for for TLVs 135, 235, 236, and 237
+18.3.2.  IS-IS Sub-TLVs for TLVs Advertising Prefix Reachability

-   This document makes the following registrations in the "Sub-TLVs for
-   for TLVs 135, 235, 236, and 237" registry.
+   This document makes the following registrations in the "IS-IS Sub-TLVs for
+   TLVs Advertising Prefix Reachability" registry.

       Type: 6

-      Description: Flexible Algorithm Prefix Metric.
+      Description: Flexible Algorithm Prefix Metric (FAPM).

       Reference: This document (Section 8).

 18.3.3.  Sub-Sub-TLVs for Flexible Algorithm Definition Sub-TLV

-   This document creates the following Sub-Sub-TLV Registry:
+   This document creates the following Sub-Sub-TLV Registry, under the
+   IS-IS TLV Codepoints grouping.

       Registry: Sub-Sub-TLVs for Flexible Algorithm Definition Sub-TLV

       Registration Procedure: Expert review
+
+jgs: You might want to preemptively mention here that the IS-IS grouping
+already contains expert review guidance for all Expert Review registries
+under it. Your choice, might save you some review queries though. Text
+if you want it:
+
+         (Note that the IS-IS TLV Codepoints grouping includes Expert
+         Review guidance that applies to all registries thereunder.)

       Reference: This document (Section 5.1)

@@ -2140,12 +2348,13 @@

 18.4.1.  OSPF Router Information (RI) TLVs Registry

-   This specification updates the OSPF Router Information (RI) TLVs
+   This specification makes the following registration in
+   the OSPF Router Information (RI) TLVs
    Registry.

       Type: 16

-      Description: Flexible Algorithm Definition TLV.
+      Description: Flexible Algorithm Definition (FAD) TLV.

       Reference: This document (Section 5.2).

@@ -2156,7 +2365,7 @@

       Type: 3

-      Description: Flexible Algorithm Prefix Metric.
+      Description: Flexible Algorithm Prefix Metric (FAPM).

       Reference: This document (Section 9).

@@ -2167,11 +2376,11 @@

       Type: 26

-      Description: Flexible Algorithm Prefix Metric.
+      Description: Flexible Algorithm Prefix Metric (FAPM).

       Reference: This document (Section 9).

-      Type: TBD (suggested value 30)
+      Type: TBD2 (suggested value 30)

       Description: OSPF Flexible Algorithm ASBR Metric Sub-TLV

@@ -2188,8 +2397,9 @@

 18.4.4.  OSPF Flex-Algorithm Prefix Metric Bits

-   This specification requests creation of "OSPF Flex-Algorithm Prefix
-   Metric Bits" registry under the OSPF Parameters Registry with the
+   This specification requests creation of the "OSPF Flex-Algorithm Prefix
+   Metric Bits" registry under the "Open Shortest Path First (OSPF) Parameters"
+   grouping, with the
    following initial values.

       Bit Number: 0
@@ -2203,16 +2413,17 @@

 18.4.5.  OSPF Opaque LSA Option Types

-   This document makes the following registrations in the "OSPF Opaque
-   LSA Option Types" registry.
+   This document makes the following registrations in the "Opaque Link-State
+   Advertisements (LSA) Option Types" registry under the "Open Shortest Path
+   First (OSPF) Opaque Link-State Advertisements (LSA) Option Types" grouping.

-      Value: TBD (suggested value 11)
+      Value: TBD1 (suggested value 11)

-      Description: OSPFv2 Extended Inter-Area ASBR LSA
+      Description: OSPFv2 Extended Inter-Area ASBR (EIA-ASBR) LSA

       Reference: This document (Section 10.1).

-18.4.6.  OSPFv2 Externded Inter-Area ASBR TLVs
+18.4.6.  OSPFv2 Extended Inter-Area ASBR TLVs

    This specification requests creation of "OSPFv2 Extended Inter-Area
    ASBR TLVs" registry under the OSPFv2 Parameters Registry with the
@@ -2231,8 +2442,9 @@

 18.4.7.  OSPFv2 Inter-Area ASBR Sub-TLVs

-   This specification requests creation of "OSPFv2 Extended Inter-Area
-   ASBR Sub-TLVs" registry under the OSPFv2 Parameters Registry with the
+   This specification requests creation of the "OSPFv2 Extended Inter-Area
+   ASBR Sub-TLVs" registry under the "Open Shortest Path First v2 (OSPFv2)
+   Parameters" grouping, with the
    following initial values.


@@ -2255,7 +2467,8 @@

 18.4.8.  OSPF Flexible Algorithm Definition TLV Sub-TLV Registry

-   This document creates the following registry:
+   This document creates the following registry under the "Open Shortest
+   Path First (OSPF) Parameters" grouping.

       Registry: OSPF Flexible Algorithm Definition TLV sub-TLV

@@ -2265,12 +2478,26 @@

    The "OSPF Flexible Algorithm Definition TLV sub-TLV" registry will
    define sub-TLVs at any level of nesting for the Flexible Algorithm
-   TLV and should be added to the "Open Shortest Path First (OSPF)
-   Parameters" registries group.  New values can be allocated via IETF
+   TLV. New values can be allocated via IETF
    Review or IESG Approval.
+
+jgs: I made the minor edit of moving the grouping request for consistency
+with the other requests.
+
+Less trivially, you have a conflict -- the "Registration Procedure"
+line says "Expert review", but then the paragraph right above says "IETF
+Review or IESG Approval". What do you want the registration policy to be?
+Also, if the policy really is Expert Review, you'll need to provide some
+guidance for the expert, since the OSPF grouping doesn't include blanket
+guidance. See RFC 8126 §4.5 for details.

-   This document registers following Sub-TLVs in the "TLVs for Flexible
+   This document registers the following Sub-TLVs in the "TLVs for Flexible
    Algorithm Definition TLV" registry:
+
+jgs: What is the real name of the registry? Is it "OSPF Flexible Algorithm
+Definition TLV sub-TLV" which is what you mention first, or is it "TLVs
+for Flexible Algorithm Definition TLV" as immediately above? Probably
+the former but you make the call and do the edit, please.

       Type: 1

@@ -2312,6 +2539,11 @@

    Types in the range 32768-33023 are for experimental use; these will
    not be registered with IANA, and MUST NOT be mentioned by RFCs.
+
+jgs: I see what you're trying to do here but I don't think it's really suitable to
+put directions to RFC authors in the IANA Considerations section. This
+restriction is kind of implicit in the Experimental status anyway. You might
+also want to take a look at RFC 8126 §4.2.

    Types in the range 33024-65535 are not to be assigned at this time.
    Before any assignments can be made in the 33024-65535 range, there