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

[Acee Lindem (acee)]

Thanks John - I the changes in -21 and -22 improve the specification. 
Acee

On 9/12/22, 8:41 AM, "Lsr on behalf of John Scudder"  wrote:

Hi Peter,

Thanks. I’ve requested IETF LC.

—John

> On Sep 12, 2022, at 7:36 AM, Peter Psenak  wrote:
> 
> 
> Hi John,
> 
> please see inline (##PP2)
> 
> On 09/09/2022 17:29, John Scudder wrote:
>> Hi Peter,
>> 
>> Thanks for your reply and for the ping.
>> 
>> Where necessary I’ve replied in line below. I’ve snipped any points that
>> are agreed and don’t need further discussion. I’ve also reviewed the -21
>> diffs, basically LGTM. It looks like you missed a few of the nits, I
>> don’t know if this was by choice or oversight. I’ve attached an edited
>> version of -21 that captures the remaining ones, plus a few new ones I
>> noticed. You can diff if you want to pick those up for inclusion in -22.
> 
> ##PP2
> I fixed all nits, hopefully.
> 
>> 
>>> On Aug 31, 2022, at 10:23 AM, Peter Psenak 
 wrote:
>>> 
>>> [External Email. Be cautious of content]
>>> 
>>> 
>>> 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):
>> 
>> [snip]
>> 
  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.
>> 
>> I take your point, thanks. I guess about the most that you could say in
>> Operational Considerations would be something like
>> 
>> ---
>> 15.X  FAD Changes
>> 
>> As [Section 5.3] notes, a change in the Flex-Algorithm definition may
>> require network-wide SPF recomputation and network reconvergence. This
>> potential for disruption should be taken into consideration when
>> planning and making changes to the FAD.
> 
> ##PP2
> Added above to the Operational Consideration section.
> 
>> ---
>> 
>> Obvs, re-word as appropriate. IMHO it's worth elevating in the O.C.
>> section even if only briefly, but I don’t insist.
>> 
>> [snip]
>> 
 +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://urldefense.com/v3/__https://datatracker.ietf.org/doc/html/rfc8665*section-5__;Iw!!NEt6yMaO-gk!HAdyT2s3c5I_fktGSY3YPuQdVeF95m5Yb-utZWsGn9214bNEm1SkQ1dDgtbTL2tEJz4kJBwXudGrWyHF3P9zB8IEVqDz$
>> 

>>> 
>>> I can replace sender with originator, if you prefer, but receiver would
>>> remain.
>> 
>> As you prefer. It’s not a big deal. I agree, by the way, that receiver
>> is unambiguous.
> 
> ##PP
> replaced sender with originator.
> 
>> 
>> [snip]
>> 
 
 @@ -1194,15 +1258,36 @@
|   
|
+-TLVs 
-+
| 

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

[John Scudder]

Hi Peter,

Thanks. I’ve requested IETF LC.

—John

> On Sep 12, 2022, at 7:36 AM, Peter Psenak  wrote:
> 
> 
> Hi John,
> 
> please see inline (##PP2)
> 
> On 09/09/2022 17:29, John Scudder wrote:
>> Hi Peter,
>> 
>> Thanks for your reply and for the ping.
>> 
>> Where necessary I’ve replied in line below. I’ve snipped any points that
>> are agreed and don’t need further discussion. I’ve also reviewed the -21
>> diffs, basically LGTM. It looks like you missed a few of the nits, I
>> don’t know if this was by choice or oversight. I’ve attached an edited
>> version of -21 that captures the remaining ones, plus a few new ones I
>> noticed. You can diff if you want to pick those up for inclusion in -22.
> 
> ##PP2
> I fixed all nits, hopefully.
> 
>> 
>>> On Aug 31, 2022, at 10:23 AM, Peter Psenak 
>>>  wrote:
>>> 
>>> [External Email. Be cautious of content]
>>> 
>>> 
>>> 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):
>> 
>> [snip]
>> 
  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.
>> 
>> I take your point, thanks. I guess about the most that you could say in
>> Operational Considerations would be something like
>> 
>> ---
>> 15.X  FAD Changes
>> 
>> As [Section 5.3] notes, a change in the Flex-Algorithm definition may
>> require network-wide SPF recomputation and network reconvergence. This
>> potential for disruption should be taken into consideration when
>> planning and making changes to the FAD.
> 
> ##PP2
> Added above to the Operational Consideration section.
> 
>> ---
>> 
>> Obvs, re-word as appropriate. IMHO it's worth elevating in the O.C.
>> section even if only briefly, but I don’t insist.
>> 
>> [snip]
>> 
 +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://urldefense.com/v3/__https://datatracker.ietf.org/doc/html/rfc8665*section-5__;Iw!!NEt6yMaO-gk!HAdyT2s3c5I_fktGSY3YPuQdVeF95m5Yb-utZWsGn9214bNEm1SkQ1dDgtbTL2tEJz4kJBwXudGrWyHF3P9zB8IEVqDz$
>> 
>>> 
>>> I can replace sender with originator, if you prefer, but receiver would
>>> remain.
>> 
>> As you prefer. It’s not a big deal. I agree, by the way, that receiver
>> is unambiguous.
> 
> ##PP
> replaced sender with originator.
> 
>> 
>> [snip]
>> 
 
 @@ -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.
>> 
>> I looked again, and the fields that are excluded from my suggested
>> statement, since they are “where specified below” are Opaque Type,
>> Opaque ID, Length, and TLVs. That leaves LS Age, Options, LS Type,
>> Advertising Router, LS sequence number, and LS checksum. But if my
>> suggested wording is wrong, t

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

[Peter Psenak]

Hi John,

please see inline (##PP2)

On 09/09/2022 17:29, John Scudder wrote:

Hi Peter,

Thanks for your reply and for the ping.

Where necessary I’ve replied in line below. I’ve snipped any points that 
are agreed and don’t need further discussion. I’ve also reviewed the -21 
diffs, basically LGTM. It looks like you missed a few of the nits, I 
don’t know if this was by choice or oversight. I’ve attached an edited 
version of -21 that captures the remaining ones, plus a few new ones I 
noticed. You can diff if you want to pick those up for inclusion in -22.


##PP2
I fixed all nits, hopefully.




On Aug 31, 2022, at 10:23 AM, Peter Psenak  
wrote:

[External Email. Be cautious of content]


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


[snip]


 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.


I take your point, thanks. I guess about the most that you could say in 
Operational Considerations would be something like


---
15.X  FAD Changes

As [Section 5.3] notes, a change in the Flex-Algorithm definition may 
require network-wide SPF recomputation and network reconvergence. This 
potential for disruption should be taken into consideration when 
planning and making changes to the FAD.


##PP2
Added above to the Operational Consideration section.


---

Obvs, re-word as appropriate. IMHO it's worth elevating in the O.C. 
section even if only briefly, but I don’t insist.


[snip]


+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://urldefense.com/v3/__https://datatracker.ietf.org/doc/html/rfc8665*section-5__;Iw!!NEt6yMaO-gk!HAdyT2s3c5I_fktGSY3YPuQdVeF95m5Yb-utZWsGn9214bNEm1SkQ1dDgtbTL2tEJz4kJBwXudGrWyHF3P9zB8IEVqDz$ 




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


As you prefer. It’s not a big deal. I agree, by the way, that receiver 
is unambiguous.


##PP
replaced sender with originator.



[snip]



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


I looked again, and the fields that are excluded from my suggested 
statement, since they are “where specified below” are Opaque Type, 
Opaque ID, Length, and TLVs. That leaves LS Age, Options, LS Type, 
Advertising Router, LS sequence number, and LS checksum. But if my 
suggested wording is wrong, that’s fine, choose your own -- the more 
general observation is that specs that provide a packet diagram usually 
tell the reader what the fields mean, either by defining them, or by 
saying what reference to look in.


##PP2
A I added reference to all fields in the Opaque LSA.



[snip]


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

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

[Peter Psenak]

Hi John,

can you please look at my responses and let me know whether you agree.

thanks,
Peter

On 31/08/2022 16:23, Peter Psenak wrote:

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.0 -0400
+++ draft-ietf-lsr-flex-algo-20-jgs-comments.txt    2022-08-16
17:37:22.0 -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 i

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

[Peter Psenak]

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.0 -0400
+++ draft-ietf-lsr-flex-algo-20-jgs-comments.txt    2022-08-16 
17:37:22.0 -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.


##PP
we can certainly add this to the document, but I don't think 
Introduction is the right place. I adde

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

[Acee Lindem (acee)]

Hi John, 

On 8/18/22, 1:29 PM, "John Scudder"  wrote:

Hi Acee,

> On Aug 18, 2022, at 1:10 PM, Acee Lindem (acee) 
 wrote:
> 
> 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).

Yes, Peter also mentioned that to me unicast, thanks.

[snip]

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

Any solution that makes it clear what’s being referred to is OK of course. 
I do think it would be a cheap and easy thing to add a section break before 
that paragraph in support of that goal, something like the below. 

Breaking it into multiple paragraphs is fine with me. I don't feel strongly on 
section break either way. 

Thanks,
Acee

--- 10.1-para   2022-08-18 13:26:20.0 -0400
+++ 10.1-para-jgs-edits 2022-08-18 13:26:07.0 -0400
@@ -1,14 +1,17 @@
+10.1.1. EIA-ASBR TLVs
+
The format of the TLVs within the body of the OSPFv2 EIA-ASBR LSA is
the same as the format used by the Traffic Engineering Extensions to
OSPFv2 [RFC3630].  The variable TLV section consists of one or more
-   nested TLV tuples.  Nested TLVs are also referred to as sub- TLVs.
-   The Length field defines the length of the value portion in octets
+   nested TLV tuples.  Nested TLVs are also referred to as sub-TLVs.
+   
+   The TLV or Sub-TLV length field defines the length of the value portion 
in octets
(thus, a TLV with no value portion would have a length of 0).  The
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 TLV or 
Sub-TLV 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.

-10.1.1.  OSPFv2 Extended Inter-Area ASBR TLV
+10.1.1.1

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

[John Scudder]

Hi Acee,

> On Aug 18, 2022, at 1:10 PM, Acee Lindem (acee) 
>  wrote:
> 
> 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).

Yes, Peter also mentioned that to me unicast, thanks.

[snip]

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

Any solution that makes it clear what’s being referred to is OK of course. I do 
think it would be a cheap and easy thing to add a section break before that 
paragraph in support of that goal, something like the below. 

--- 10.1-para   2022-08-18 13:26:20.0 -0400
+++ 10.1-para-jgs-edits 2022-08-18 13:26:07.0 -0400
@@ -1,14 +1,17 @@
+10.1.1. EIA-ASBR TLVs
+
The format of the TLVs within the body of the OSPFv2 EIA-ASBR LSA is
the same as the format used by the Traffic Engineering Extensions to
OSPFv2 [RFC3630].  The variable TLV section consists of one or more
-   nested TLV tuples.  Nested TLVs are also referred to as sub- TLVs.
-   The Length field defines the length of the value portion in octets
+   nested TLV tuples.  Nested TLVs are also referred to as sub-TLVs.
+   
+   The TLV or Sub-TLV length field defines the length of the value portion in 
octets
(thus, a TLV with no value portion would have a length of 0).  The
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 TLV or Sub-TLV 
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.
 
-10.1.1.  OSPFv2 Extended Inter-Area ASBR TLV
+10.1.1.1.  OSPFv2 Extended Inter-Area ASBR TLV

That adds one level of subsection nesting but I don’t see that as problematic.

$0.02,

—John
___
Lsr mailing list
Lsr@ietf.org
https://www.ietf.org/mailman/listinfo/lsr