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

[Peter Psenak <[e-mail]>]

Hi John,

thanks a lot for the thorough review.

I incorporated all your edits and almost all of your comments.

For the few that I have not, please see inline (loop for ##PP):


On 16/08/2022 23:45, John Scudder wrote:
> 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 
> <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.

##PP
we can certainly add this to the document, but I don't think 
Introduction is the right place. I added your proposed text to the 
Terminology section where the "Flexible Algorithm Participation" is 
described.

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

##PP
I don't really see much to add here. FAD changes the constraints used 
during the algo specific SPF and as such any change in it requires all 
routers to recompute the entire topology. In terms of the order of 
changes, I don't see why that would be significant and why someone would 
not want to advertise all changes at once if there are any multiple 
changes in the FAD.

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

##PP
yes, the length is not required to be number of four (similar to rfc7794).
For EAG, we are following rfc7308 that mandates the multiple of 4 bytes 
for EAG advertisement.


> +
> +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"?

##PP
I removed the "non-zero" part from the above.
If the length of the TLV in ISIS is 0, there will be no flags advertised 
in it and as such will be useless. I looked at the rfc7794 that 
specifies the Prefix Attribute Flags and it does not talk about the 0 
length sub-TLV treatment specifically.


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

##PP
send/receive is the standard term used, e.g 
https://datatracker.ietf.org/doc/html/rfc8665#section-5

I can replace sender with originator, if you prefer, but receiver would 
remain.

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

##PP
RFC2328 does not use TLVs, so that would not be correct.

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

##PP
I replaced “Length field” with “TLV length field”, as suggested by Acee. 
That should clarify it, I believe.
> 
> 
> 
> @@ -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?
> 
##PP
I removed that sentence that includes "expected" as well as the last 
sentence of that paragraph.

>   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.)

##PP
Though I agree that the order is not important for now, one can imagine 
that in the future there could be rules added for which the order would 
be important. I feel numbering these rules and keep them in the strict 
order would help in such case. And mandating the order from the 
beginning does not make any harm. So I would prefer to keep it as it is.


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

##PP
it means that the "winning" FAD includes the M-bit.

thanks,
Peter

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