Last Call Review of draft-ietf-lsr-igp-flex-algo-reverse-affinity-04
review-ietf-lsr-igp-flex-algo-reverse-affinity-04-rtgdir-lc-li-2025-01-30-01

Hi Peter,

I suddenly find out that, I started the review before the latest revision, and
turned to other business, and then came back to this draft after the revision
update. This makes me miss the new revision. Thanks for reminding. Please see
my reply below after reading the latest revision, with [Cheng2].

Thanks,
Cheng

-----Original Message-----
From: Peter Psenak <ppsenak@cisco.com>
Sent: Tuesday, February 4, 2025 12:39 PM
To: Cheng Li <c.l@huawei.com>; rtg-dir@ietf.org
Cc: draft-ietf-lsr-igp-flex-algo-reverse-affinity.all@ietf.org;
last-call@ietf.org; lsr@ietf.org Subject: Re: Rtgdir last call review of
draft-ietf-lsr-igp-flex-algo-reverse-affinity-04

Hi Cheng,

thanks for the review, please see inline (##PP):

On 30/01/2025 17:50, Cheng Li via Datatracker wrote:

> Reviewer: Cheng Li
> Review result: Has Nits
>
> I am not an expert of IGP flexalgo and this is my first official
> review. Though I understand what is the Flexalgo and how it can be
> used, as a routing area engineer. As a new reader of this document,
> the comments may be more helpful to reflect the true feeling of most
> readers. But I may make some mistakes, so some comments below are
clarification questions. > > 1. Abstract: > [Cheng] Seems too short and simple,
recommend to add more text to > explain clearer. > > 2. Introduction, > [Cheng]
Also, too short. Suggest to add more text to describe what is > flexalgo,
motivation of why we need this extension.  Some text similar > to the
introduction section in RFC9350 is ok.

##PP
ok, will add some more text to both Abstract and Introduction.

[Cheng2] Thanks.

> 3. use case example
> 3.1
> "The Flexible Algorithm definition can specify 'colors' that are used
> by the operator to include or exclude links during the Flex-Algorithm
> path computation." [Cheng]suggest to have definition of "color" in
> term.  or, a reference also helps.
> 3.2
>
> "These link 'colors' are checked in the forwarding direction of the
> SPF computation - e.g. in the direction from the parent to the child."
>
> [Cheng]I can understand the meaning. But what is 'checked' in details?
> may be good to have some specific words to describe the action?  from
> the parent to the child, these terms come from graph/tree/SPF. No sure
> if we can find better way to describe the direction. It looks normal
> to use 'check' in Flex-algo RFCs, like RFC9350 :)

##PP
Above text in section 3 is not from the last version. The text has been
updated. Can you please look at the latest version of the draft. [Cheng2] ok,
thanks, it addresses my comments. Anyone else raise the same comment regarding
'color'? good to hear the same comment.

> 3.3
>
> "the input errors can only be detected at node B."
>
> [Cheng] I may ask, why the input errors can ONLY be detected at node
> B? as a reader who is new to this area(Flexalgo in IGP). better to
> have some explaination?

##PP
Input error for traffic A->B can be detected at node B only on Rx side.
I'm not sure what else to say.

[Cheng2]All right, then ignore it.

> 3.4
>
> "An operator may measure the rate of such input errors and set certain 'color'
> on a link locally on node B when the input error rate crosses a
> certain threshold."
>
> [Cheng] Sorry I do not understand this fully.
> Clarification question: what system you are talking about when you say
> measure the rate of such input errors? It seems that we have a system
> to input something. Based on the results, the system set certain
> 'color' on a link? what input errors will replect on a 'color'? what this
color means?

##PP
We are talking about nodes running the IS-IS protocol, which would typically be
a L3 device. Such device routes a L3 traffic and as such receives L3 traffic
from other L3 devices. It can detect input errors, CRC errors, etc. Operator
can set a threshold on these errors over a certain period of time.

We do not use a color in the latest version, we say "set certain Admin Groups",
which is a link attribute that is advertised by ISIS protocol about the link.

[Cheng2] Good to learn that. If you can add some text to explain, that will be
great.

> Section 4.
>
> [Cheng] The name of the TLV is a little bit long, even when it becomes
> abbreviation, FAERAG, suggest to make it short, or make it readable.

##PP
I feel it is proper and the abbreviation makes it short enough. We use such
naming for other TLVs.

Example:
https://www.rfc-editor.org/rfc/rfc9350.html#section-6.1
[Cheng2]All right, just an suggestion. You can ignore.

> 4.1
>
> [Cheng]Type = 10, do we already got the IANA allocation? if not?
> suggest to use TBA for now. similar comment for all the type value in other
TLVs.

##PP
Again, you are not looking at the latest version:

Current text is:

"Type (1 octet): An 8-bit field assigned by IANA to uniquely identify the ISIS
FAERAG Sub-TLV. Value 10 has been assigned by IANA."

[Cheng2]LGTM

> 4.2
>
> [Cheng] in RFC7308, it looks like the name of the TLV is Extended
> Administrative Groups, but in this document, it is Extended Administrative
> Group. Am I missing something?

We are consistent with the base flex-algo spec:

https://www.rfc-editor.org/rfc/rfc9350.html#section-6.1

> [Cheng]The rules of ignore more than one FAERAG sub-TLV is clear to me,
thanks! > > Section 5 > > "The format of the IS-IS Flexible Algorithm
Include-Any Reverse Admin Group > Sub-TLV is identical to the format of the
FAERAG Sub-TLV in Section 4." > > [Cheng] what do you mean identical? even for
the EAG part? meaning there will > be some EAG info in FAIRAG sub-TLV?

##PP:
The two TLVs differ only in the Type value.
[Cheng2]OK

> Section 6,
>
> [Cheng]Same comments like section 5
>
> Section 7,8,9, similar comments like the IS-IS ones.

##PP

Same response as above.
[Cheng2]OK

> Section 12 Security Considerations.
>
> [Cheng] I like to use simple sentence in security section as well. But
> reviewers always require me to add more text to explain. LOL. Therefore, I
will > suggest the authors to add more text to in this section, since the
security > area reviewers may require to do so as well. > ##PP

I feel the security section is exactly saying the truth. There's nothing
more to add. Do you want me to copy the text from the RFC9350? Looks
redundant, but doable.

[Cheng2]Honestly, if this is a PCE WG document, I know I need to add more text
in this section. But this is a LSR WG document, so I am not sure about the
normal practice. We may leave it to security experts. I am ok with this now.

thanks,
Peter

>
>