Subject: Re: [Gen-art] Genart last call review of draft-ietf-isis-reverse-metric-15

From: Alissa Cooper <[email-masked]>
Date: Tue, 20 Nov 2018 15:41:57 -0500

Stewart, thanks for your review. Naiming, thanks for your responses. I have entered a No Objection ballot.

Alissa

> On Oct 21, 2018, at 5:21 PM, Naiming Shen (naiming) <[email-masked]> wrote:
> 
> 
> Hi Stewart,
> 
> Thanks for detailed review and comments, please see some
> replies inline. the modified version diff is attached, please reivew.
> 
> thanks.
> - Naiming
> 
> <draft-ietf-isis-reverse-metric-15-rev-from-.diff.html>
> 
>> On Oct 17, 2018, at 8:59 AM, Stewart Bryant <[email-masked]> wrote:
>> 
>> Reviewer: Stewart Bryant
>> Review result: Ready with Issues
>> 
>> I am the assigned Gen-ART reviewer for this draft. The General Area
>> Review Team (Gen-ART) reviews all IETF documents being processed
>> by the IESG for the IETF Chair.  Please treat these comments just
>> like any other last call comments.
>> 
>> For more information, please see the FAQ at
>> 
>> <https://trac.ietf.org/trac/gen/wiki/GenArtfaq>.
>> 
>> Document: draft-ietf-isis-reverse-metric-15
>> Reviewer: Stewart Bryant
>> Review Date: 2018-10-17
>> IETF LC End Date: 2018-10-17
>> IESG Telechat date: 2018-11-21
>> 
>> Summary: Generally a well written document, but some earlier text on what a
>> reverse metric is and what it does would be very helpful to the reader. Also
>> the reader is left with the impression that the use of this gives disruption
>> free network changes, and yet it does not discuss microloops.
>> 
>> Major issues: None
>> 
>> Minor issues:
>> 1.2.  Distributed Forwarding Planes
>> 
>> <snip>
>> Temporarily signaling
>>  the 'Reverse Metric' over this link to discourage the traffic via the
>> 
>> SB> I know it's always chicken and egg, but it would be useful if a
>> SB> clearer definition of reverse metric were provided before you
>> SB> explained its use.
> 
> <NS> I have added a paragraph at the beginning of the Introduction section
> 
>> 
>>  corresponding line-card will help to reduce the traffic loss in the
>>  network.  In the meantime, the remote PE routers will select a
>>  different set of PE routers for the BGP best path calculation or use
>>  a different link towards the same PE router on which a line-card is
>>  resetting.
>> 
>> SB> Remember that when you change paths you have to deal with
>> SB> microloops.
> 
> <NS> Well, this ‘reverse metric’ is just like a normal IS-IS metric change.
> <NS> and microloops is implied. This document does not create new
> <NS> condition or trying to address the issue. It assumes final convergence
> <NS> state will be reached for the mentioned use cases.
> 
>> 
>> =======
>> 
>> 1.5.  IS-IS Reverse Metric
>> 
>>  This document uses the routing protocol itself as the transport
>>  mechanism to allow one IS-IS router to advertise a "reverse metric"
>>  in an IS-IS Hello (IIH) PDU to an adjacent node on a point-to-point
>>  or multi-access LAN link.  This would allow the provisioning to be
>>  performed only on a single node, setting a "reverse metric" on a link
>>  and have traffic bidirectionally shift away from that link gracefully
>>  to alternate, viable paths.
>> 
>> SB> Again you need to be much clearer what a reverse metric is before
>> SB> you cite the use cases and advantages.
> 
> <NS> added a new paragraph.
> 
>> 
>> ===========
>> 
>> 3.1.  Processing Changes to Default Metric
>> 
>>  It is important to use the same IS-IS metric type on both ends of the
>>  link and in the entire IS-IS area or level.
>> 
>> SB> Isn't the point about links redundant given that it needs to be the
>> SB> same in the area/the level
> 
> <NS>  This sentence is talking about it does not deal with the case of
> <NS> one side of the link uses IS-IS wide metric, but the other side
> <NS> of the link uses a narrow metric. It is saying it’s broken and we
> <NS> don’t support such a mixup, thus ‘reverse-metric’ also does not handle.
> 
>> 
>>  On the receiving side of
>>  the 'reverse-metric' TLV, the accumulated value of configured metric
>>  and the reverse-metric needs to be limited to 63 in "narrow" metric
>>  mode and to (2^24 - 2) in "wide" metric mode.
>>  This applies to both
>>  the Default Metric of Extended IS Reachability TLV and the Traffic
>>  Engineering Default Metric sub-TLV in LSP or Pseudonode LSP for the
>>  "wide" metric mode case.  If the "U" bit is present in the flags, the
>>  accumulated metric value is to be limited to (2^24 - 1) for both the
>>  normal link metric and Traffic Engineering metric in IS-IS "wide"
>>  metric mode.
>> 
>> SB> A clarifying note explaining the different usage of 2^24 - 1 and
>> SB> 2^24 - 2 would be helpful to the reader.
> 
> <NS> Yes, added a sentence in the TLV definition part to describe them
> <NS> in some detail.
> 
>> 
>> =========
>> 3.2.  Multi-Topology IS-IS Support on Point-to-point links
>> 
>>  The Reverse Metric TLV is applicable to Multi-Topology IS-IS (M-ISIS)
>>  [RFC5120].  On point-to-point links, if an IS-IS router is configured
>>  for M-ISIS, it MUST send only a single Reverse Metric TLV in IIH PDUs
>>  toward its neighbor(s) on the designated link.
>> 
>> SB> Might you not want to use this on a topology by topology basis?
>> SB> For example you might want to bring up important typologies first.
> 
> <NS> this document does not support that. The main reason is that the
> <NS> pnode LSP is shared by all the topologies, and it’s not per topology
> <NS> pnode definition. For backwards competibility of this draft, it follows
> <NS> the same logic. Otherwise we need a flag day in the network to use
> <NS> ‘reverse-metric’.
> 
>> 
>> =========
>> 
>>  its
>>  inbound metric value to be the maximum and this puts the link of this
>>  new node in the last resort position without impacting the other IS-
>>  IS nodes on the same LAN.
>> 
>> SB> It is only down in S3.4 that you provide this simple definition of
>> SB> reverse metric as the "inbound metric". It would be helpful to have this
>> SB> earlier in the text.
> 
> <NS> yes, indeed. a new paragraph is added.
> 
>> 
>> =========
>> 
>> It is RECOMMENDED also that the CSPF does the immediate CSPF
>>  re-calculation when the Traffic Engineering metric is raised to (2^24
>>  - 2) to be the last resort link.
>> 
>> SB> Again it would help the reader if "link of last resort" was earlier
>> SB> in the text,
> 
> <NS> ‘link of last resort’ is only for certain use cases, but not all. added
> <NS> also in the TLV definition section.
> 
>> =========
>>  It is RECOMMENDED that implementations provide a capability to
>>  disable any changes by Reverse Metric mechanism through neighbor's
>>  Hello PDUs.
>> 
>> SB> Changes of what? That sentence does not seem to read very well.
> 
> <NS> added the “IS-IS metric changes”.
> 
>> 
>> ==========
>> 
>>  If an implementation enables this mechanism by default, it is
>>  RECOMMENDED that it be disabled by the operators when not explicitly
>>  using it.
>> 
>> SB> Why not RECOMMEND that it be disabled by default, or perhaps
>> SB> strengthen that to MUST be disabled by default.
> 
> <NS> As also replied also to Alvaro’s AD review comments, one of the main
> <NS> use case is for operational maintenance window to divert the traffic
> <NS> into certain links by using the ‘reverse-metric’ mechanism. If an operator
> <NS> has to track down all the nodes on the LAN side of the links, and to
> <NS> manually enable the feature one by one correctly, then it defeats the
> <NS> purpose of this to simplify the operational use case here.
> 
>> =========
>> 
>> it is highly RECOMMENDED that operators configure
>> authentication of IS-IS PDUs to mitigate use of the Reverse Metric
>> TLV as a potential attack vector.
>> 
>> SB> Not sure that you can qualify RFC2119 RECOMMENDED
> 
> <NS> changed to lower case.
> 
>> 
>> =========
>> 
>>> From the IANA section
>> 
>> SB> Why is 18 chosen in an otherwise empty registry?
> 
> <NS> Originally, it inherits the same sub-TLV from IS-IS TE sub-TLV RFC
> <NS> RFC 5305, and the Traffic Engineering Metric sub-TLV is 18.
> <NS> now this ‘reverse-metric’ TLV has it’s own registry, but we keep the
> <NS> number the same.
> 
>> 
>> =========
>> Regarding Appendix A and I think Appendix B
>> 
>> SB> As noted earlier you really need to talk about microloops. There
>> SB> is no disruption free lunch available.
>> 
>> ========
>> 
>> Nits/editorial comments:
>> 
>>> From ID-nits
>> ** Downref: Normative reference to an Informational RFC: RFC 5443
>> This is correctly dealt with in the LC
>> 
>> == Outdated reference: A later version (-07) exists of
>>    draft-shen-isis-spine-leaf-ext-03
>> I am sure the RFC Editor will address, but could usefully be fixed in any
>> respin.
>> 
> <NS> Good catch. corrected.
> 
> 
> 
> _______________________________________________
> Gen-art mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/gen-art