Hi Loa,
thank you for your thorough review and constructive comments; greatly
appreciated. My apologies for the belated response. Please find my notes
below tagged GIM>>. Attached is the new working version of the draft and
the diff to highlight updates noted below and those that are intended to
address the Alvaro's WGLC comments.
Regards,
Greg
On Fri, Dec 20, 2024 at 3:46 AM Loa Andersson <loa@xxxxx
<mailto:loa@xxxxx>> wrote:
All,
Sorry I forgot to clean up the template at one point, the Summary
should
say:
Summary:
I have some minor concerns about this document that I think should be
resolved before publication.
Overview:
The document describes the use of > BFD for monitoring individual
segment lists of candidate paths of an SR Policy. It documents the use
of various BFD modes and features such as BFD Demand mode, Seamless
BFD,
and BFD Echo function with the BFD Control packet payload in an SR-MPLS
domain. The document also defines the use of LSP Ping for Segmen
Routing
networks over the MPLS data plane [RFC8287] to bootstrap and control
path of a BFD session from the egress LER to the ingress LER using
Segment Routing segment list with MPLS data plane (SR-MPLS).
Then it should be followed by "Minor Issues" as below.
/Loa
Den 20/12/2024 kl. 12:31, skrev Loa Andersson via Datatracker:
> Reviewer: Loa Andersson
> Review result: Has Issues
>
> RTG-DIR LC review of draft-ietf-spring-bfd
> RTG-DIR Last Call review of draft-ietf-spring-bfd-12
>
> Routing Directorate Last Call Review Template
> To:
>
> rtg-ads@xxxxxxxx <mailto:rtg-ads@xxxxxxxx>
> Cc:
>
> rtg-dir@xxxxxxxx <mailto:rtg-dir@xxxxxxxx>;
> draft-ietf-spring-bfd.all@xxxxxxxx <mailto:draft-ietf-spring-
bfd.all@xxxxxxxx>;
> spring@xxxxxxxx <mailto:spring@xxxxxxxx>;
> last-call@xxxxxxxx <mailto:last-call@xxxxxxxx>;
> Subject:
>
> RtgDir Last Call review: draft-ietf-spring-bfd-12
> Hello,
>
> I have been selected as the Routing Directorate reviewer for this
draft. The
> Routing Directorate seeks to review all routing or routing-
related drafts as
> they pass through IETF last call and IESG review, and sometimes
on special
> request. The purpose of the review is to provide assistance to
the Routing ADs.
> For more information about the Routing Directorate, please see
> https://wiki.ietf.org/en/group/rtg/RtgDir <https://wiki.ietf.org/
en/group/rtg/RtgDir>
>
> Although these comments are primarily for the use of the Routing
ADs, it would
> be helpful if you could consider them along with any other IETF
Last Call
> comments that you receive, and strive to resolve them through
discussion or by
> updating the draft.
>
> Document: draft-ietf-spring-bfd-12
> Reviewer: Loa Andersson
> Review Date: date
> IETF LC End Date: 2024-12-13
> Intended Status: Experimental
>
> Summary:
> Choose from this list...
>
> No issues found. This document is ready for publication.
> This document is basically ready for publication but has nits
that should be
> considered prior to publication. I have some minor concerns about
this document
> that I think should be resolved before publication. I have
significant concerns
> about this document and recommend that the Routing ADs discuss
these issues
> further with the authors. Comments: Please supply an overview of
the draft
> quality and readability. Include anything else that you think
will be helpful
> toward understanding your review. Overview The document describes
the use of
> BFD for monitoring individual segment lists of candidate paths of
an SR Policy.
> It documents the use of various BFD modes and features such as
BFD Demand mode,
> Seamless BFD, and BFD Echo function with the BFD Control packet
payload. in the
> SR-MPLS domain. The document also defines the use of LSP Ping for
Segment
> Routing networks over the MPLS data plane [RFC8287] to bootstrap
and control
> path of a BFD session from the egress LER to the ingress LER
using Segment
> Routing segment list with MPLS data plane (SR-MPLS).
>
> Mailing list discussion
> The SPRING mailining list has been actvie discussing this WGLC. I
had not read
> all the mails when I started my review, but has done so prior to
finish it.
> There are some overlaps, and some differences between my comments
and thise on
> the mailing list, for example ALvaro and I both have a comment on
the use of
> the "expected" in the Abstract, I suggest a change and Alvaro to
drop the enire
> sentence. While I prefer what I proposed, I have no problem
living with what
> Alvaro proposes. This is true for almost all overlapping
comments, I have made
> a note if I strongly prefer something I suggested.
>
Minor issues
IANA Considerations
I have a some concerns about the IANA
> Considerations. Ketan had almost the same concerns in a mail to
the list, but
> the document has changed, so I go through.
>
> It is OK to allocate the new "Non-FEC Path TLV" the
> way you do.
>
> However you assign it from a range that require a response
> if the the TLV is not recognized, which is fine if that is
> what you want. If that is the case this need to be
> described in section 3.1.
GIM>> You are correct. The expectation (and the running experiment) is
that the addressee that does not recognize Non-FEC Path TLV will respond
with the "One or more of the TLVs was not understood " Return Code.
Added the following text in Section 3.1:
NEW TEXT:
If the receiver of the
echo request doesn't recognize Non-FEC Path TLV, it MUST set the
Return Code in an echo reply to 2 ("One or more of the TLVs was not
understood").
>
> If youy think that it can be silently dropped if not
> recognized you should use range 49162-64507. Again you
> need to describe this in section 3.1.
> Table 1 should have a column listing if there are sub-TLV
registries or not.
>
> +=======+==================+===============+============+
> | Value | TLV Name | Reference | Sub-TLV |
> | | | |registry |
> +=======+==================+===============+============|
> | TBD1 | Non-FEC Path TLV | This document | Non-FEC |
> | | | |Path sub-TLV|
> +-------+------------------+---------------+------------|
>
> Table 1: New Non-FEC Path TLV
> Since you assign a sub-TLV I prefer that you list it.
GIM>> Please let me know if I correctly followed you suggestion:
+=======+==================+===============+==================+
| Value | TLV Name | Reference | Sub-TLV Registry |
+=======+==================+===============+==================+
| TBD1 | Non-FEC Path TLV | This document | Path sub-TLV |
+-------+------------------+---------------+------------------+
Table 1: New Non-FEC Path TLV
>
> You create the Non-FEC Path sub-TLV registry almost correctly,
but I think
> Table 2 should look like this:
>
> +=============+==============+===========================+
> | 0-16383 | Standards | This range is for sub-TLVs|
> | | Action | that require an error |
> | | | message if notrecognized. |
> | | | [RFC9041, Section 3.1 |
> +=============+==============+===========================+
> | 16384-31739 | RFC | This range is for sub-TLVs|
> | | Required | that require an error |
> | | | message if notrecognized. | |
|
> | [RFC9041, Section 3.1 |
> +=============+==============+===========================+ |
31740-31743 |
> Experimental | Reserved, not to be | | | Use
|
> assigned. | | | | This
range is for
> sub-TLVs| | | | that require an error
| |
> | | message if notrecognized. | | |
> | [RFC9041, Section 3.1 |
> +=============+==============+===========================+ |
31744-32767 |
> First Come | This range is for sub-TLVs| | | Use
| that
> require an error | | | | message if
notrecognized.
> | | | | [RFC9041, Section 3.1 |
> +=============+==============+===========================+ |
32768-49161 |
> Standards | This range is for sub-TLVs| | |
Action | that
> that can be silently | | | | dropped if
notrecognized.
> | +=============+==============+===========================+ |
49162-64507 |
> RFC | This range is for sub-TLVs| | |
Required | that
> that can be silently | | | | dropped if
notrecognized.
> | +=============+==============+===========================+
| 64508-64511
> | Experimental | Reserved, not to be | | | Use
|
> assigned. | | | | This
range is for
> sub-TLVs| | | | that that can be
silently | |
> | | dropped if notrecognized. |
> +=============+==============+===========================+ |
64512-65535 |
> First Come | This range is for sub-TLVs| | | Use
| that
> that can be silently | | | | dropped if
notrecognized.
> | +=============+==============+===========================+ I.e.
do it exactly
> as for other sub-TLV registries. If not you'll have to motivate this.
GIM>> Please let me know if updates of Table 2 capture your suggestions.
I noticed that RFC 8126 <https://datatracker.ietf.org/doc/rfc8126/
> among the allocation policies lists "First Come First Served" , and I
used that. also, I noticed that for the Experimental Use you suggest a
Reserved, not to be assigned. Is that because Section 4.2 of RFC 8126
notes that IANA does not tracks the use of values from the experimental
range? Should I reference Section 4.2 in the Notes?
>
> Some questions:
>
> Experimental RFC needed
> In the "Note column" of the "Non-FEC Path sub-TLV registry" you
"Specification
> Required", that registration policy is the widest we have, almost
anything that
> we can imagine to be a "specification" is allowed. All documents
from any SDO
> is liokely to pass that var. You scribble something on a napkin,
take a photo
> of it and store somewhere where it is publicly retrievable, and
you can make a
> case for "specification". Then we go to the "Note" field and
there you say
> "Experimental RFC needed", so you limit our widest category down
to a single
> type of document. Please not that "Experimental RFC needed" is not a
> registrtation policy. If you want it you have to describe it,
>
> Why is that? Isn't RFC do it like this? Isn't RFC Required"
sufficicient?
GIM>> As noted above, Table 2 was updated according to your
recommendation, including the for Experimental Use ranges. I couldn't
figure out whether "Reserved, Not to be assigned" belongs - Registration
Procedures column of Note.
>
> Populate new registries
> When you create a new registry you can populate if, the "TBDs"
are not needed,
> there are no conflicts. IANA will review and do what you say. We
let INA pick
> the values when there are a risk for conflict. Add a value
instead of "TBD2".
GIM>> Thank you for the suggestion. Done.
>
> First Come, First Served
> Why did you remove FCFS? We had a long discussion on including it
when we wrote
> RFC 9041.
GIM>> Thank you for pointing that out to me. The new Non-FEC Path sub-
TLV registry now uses FCFS policy.
>
> Assignement conflicts
> For "Non-FEC Path sub-TLV registry" you first say that we have a
small problem.
> You want to Reserve to values "0" and "65535". The glitch is that
you first say
> that for "0" the registration policy is "Standard Tracks", and
then yo try to
> "Reserve" it from and Experimental RFC. It shold not work.
>
> For "65535" you first say it "First Come, First Served" and
then"Reserve", it
> can't be both. I think you can reserve "0" and "65535" and make
the first
> Standard track range 1-16383, and the Private Use range
64512-65534. But I
> think yuo should get an opinion from IANA before you commit.
>
> Alternatively you could do as all other sub-TLV registries does,
skip reserving
> 65535.
GIM>> I agree with the partition of the values you suggested for
the Non-FEC Path sub-TLV registry.
>
> Assigment from the Return Code registry
> You are requesting that a Standard Track code point from the
"Return Codes"
> registry. You can't do that from an Experimental RFC. I suggest
that you ask
> for a value from the RFC Required range instead.
GIM>> Updated according to your recommendation.
>
> Nits:
>
> Nits are editorial or layout items. They are things that would
ideally be
> resolved before publication to make the document more readable
and may be
> raised now to save the RFC Editor's work. Usually a reviewer will
not be
> looking for this type of issue but may find some in the course of
their review.
> Please try to avoid raising esoteric questions about English
usage. The RFC
> Editor will spot these, and it is not a wise use of time to
discuss these
> things. If you find no nits, please leave this section out.
Abstract SR-MPLS is
> not a wellknow abbreviation, so it need to expanded at first use.
If it is used
> both in the Abstract and the document text I think it should be
expanded twice.
> The Abstract is treated as something stand-alone.
>
> Terminology
> Consistency
> This is is inconsistence, sometimes there is a ":" between the
abbreviation and
> expansion, sometime not. I prefer with the ":".
>
> SR-MPLS
> In the terminology section you expand SR-MPLS as:
>
> SR-MPLS Segment Routing with MPLS data plane
> The working group have told the RFC Editor to use:
>
> SR-MPLS Segment Routing over MPLS
> in the Abbreviation list. We should follow the abbreviation list.
GIM>> Updated per your suggestion.
>
> The terminology section kisses:
>
> LSR Label Switching Router
> which is used in setion 11.
GIM>> Changed to the expanded form.
>
> Section 2
> You correctly have a reference to RFC 8660, but the forwarding
plane operation
> "NEXT" shows up a bit abrupt and if I undedrstand correctly it is
explained in
> RFC 8402, could please add some text on what "NEXT" is and where
to find the
> definition.
>
> In section 2 you have this paragraph:
>
> When LSP Ping is used to bootstrapping a BFD session for
> SR-MPLS segment list the FEC corresponding to the last
> segment to be associated with the BFD session MUST be
> as the very last sub-TLV in the Target FEC TLV.
> First, a "Target FEC" TLV does not exist. There is a "Target FEC
Stack" TLV
> (TLV # 1)is that the TLV you mean?
>
> Second, the "Target FEC Stack" TLV shares sub-TLVs with the
"Reverse-path
> Target FEC Stack" TLV and the "Reply Path" TLV, together there
are in the order
> of 50 sub-TLVs defined. Then you say "last must be last", is that
among all
> sub-TLVs, or just among those defined in this document.
GIM>> As a result of addressing WG LC Chair's comments <https://
mailarchive.ietf.org/arch/browse/spring/?
gbt=1&index=J1bVdJsKfxTRhHOSau02xnNe-wQ>, Section 2 changed. I hope that
you agree with the updates and that they address your concerns.
>
> You also have this paragraph:
>
> Encapsulation of a BFD Control packet in Segment Routing
> network with MPLS data plane MUST follow Section 7
> [RFC5884] when the IP/UDP header used and MUST follow
> Section 3.4 [RFC6428] without IP/UDP header being used.
> I think this would be better:
>
> Encapsulation of a BFD Control packet in Segment Routing
> networks over a MPLS data plane MUST follow Section 7 of
> [RFC5884] when the IP/UDP is header used. The
> encapsulation MUST follow Section 3.4 of [RFC6428] when
> an IP/UDP header is not used.
GIM>> Thank you for the recommendation that improves the readability.
Applied in the working version.
> Section 3
> I found the text hard to read, I think it could be improved. If
you want I can
> do an attempt off-line.
GIM>> Thank you for your kind offer. Section 3 was updated to address
Alavor's comments. I greatly appreciate your help in improving the new
version in the attached copy of the working version of the draft.
>
> Section 3.1
> RFC 8029 gives the names of the LÖSP Ping messages as:
>
> MPLS echo request; and
> MPLS echo reply
> In section 3.1 you use "echo request" and "Echo Request", please
align with RFC
> 8029.
GIM>> Thank you. Done throughout the document.
>
> Section 3.2
> Section 3.2 is unclear. You say:
>
> "...BFD Reverse Path TLV MAY use Target FEC sub-TLVs
> defined in [RFC8287]."
> Do you mean that it can use ALL the Target FEC Stack TLVs, or
just the three
> sub-TLVs defined in RFC 8287? If the latter I think we should say:
>
> "...the BFD Reverse Path TLV MAY use the three Target FEC
> sub-TLVs defined in [RFC8287]."
> I also wonder about the "MAY", is there an alternative. maybe the
is "may"?
GIM>> As the result of addressing WG Chair's comments Section 3.2 now
reads as:
Target FEC sub-TLVs defined in [RFC8287] are applicable in SR domains
that are in the scope of [RFC8287].
Does the new text resolve your concern?
>
> Section 4
> s/can be following in SR-MPLS/can be followed in SR-MPLS
GIM>> Done.
>
> You say:
>
> "an ingress SR node bootstraps BFD session over SR-MPLS in Async
BFD mode"
>
> RFC 5880 does not use "Async BFD mode", but uses "Asynchronous
mode", I think
> we should follow RFC 5880.
GIM>> Fixed in two remaining occurences.
>
> You say:
>
> "it sends its BFD Control packet to the ingress SR node over the
IP network
> with a Poll sequence"
>
> You might want to add a reference to RFC 5880 section 6.5.
GIM>> Thank you for the suggestion. Done.
>
> Section 5
> In your text is p2mp and multicast synoumous? I have a tendency
to think of
> multicast as a application offered to "users" that they can join
or leave,
> while p2mp is a service that a lower layer supply. I'm not
dogmnatic about just
> wanna know how to understand your text.
GIM>> Thank you for another great question, Loa. I agree with your view
of p2mp vs. multicast. The text refers to multicat in connection with a
service and a distribution tree. In that section, p2mp is used as a
synonym of Multipoint, and in P2MP SR Policy. Would you suggest an
editorial update or a clarification?
>
> Section 6
> In section 6 you use "Echo-BFD", "Echo BFD". and "Echo BFD
packets", RFC 5880
> talks about "BFD Echo packets", please align.
GIM>> Done.
>
> Section 10
> Security considerations are normally outside the scope of my
competence, I'll
> be waiting for the SEC-DIR review of the document.
>
> I know from experience that the SEC-DIR does not like "no
additional security
> risks".
>
> Grammar concerns
> I have a couple of comments that are grammatical in nature.
Please take care
> with these comments. English is not my mother tongue, but I'm
happy if you read
> and consider (even if you decide not to take what I suggest).
>
> MPLS Data Plane
> In the Abstract you talk about the MPLS Data Plane and say:
>
> Inm the 2nd sentence "It can be realized in the Multiprotocol
Label Switching
> (MPLS) network without changing the data plane."
>
> I have the feeling that "without changing the data plane" can be
understood
> that the entire data plane is swapped, maybe:
>
> s/without changing the data plane/without any changes to the data
plane/
GIM>> I agree and updated accordingly.
>
> "Expected to"
> The second sentence in the Abstract says:
>
> "Bidirectional Forwarding Detection (BFD) is expected to monitor
a segment
> list..."
>
> I have the feeling that "expectations" leave quite a bit of
> uncertainty. What about:
> s/(BFD) is expected to/(BFD) is used to/
GIM>> Abstract got significant re-work, and that wording now reads as:
This
document describes using Bidirectional Forwarding Detection (BFD) for
monitoring individual segment lists of candidate paths of an SR
Policy.
Please let me know if the readability improved.
>
> Language concerns
> I think there is a need to clean up the language in this
document, but for
> easily understandle reasons I'm not the person to do this.
>
> I can point at some problems, for example I find that articles
(the, a and an)
> are often missing. However, my English is not good enough.
>
>
>
--
Loa Andersson
Senior MPLS Expert
Bronze Dragon Consulting
loa@xxxxx <mailto:loa@xxxxx>
loa.pi.nu.@xxxxxxxxx <mailto:loa.pi.nu.@xxxxxxxxx>
