> On Oct 17, 2018, at 8:59 AM, Stewart Bryant 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 > > . > > 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 > > > 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. 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. Well, this ‘reverse metric’ is just like a normal IS-IS metric change. and microloops is implied. This document does not create new condition or trying to address the issue. It assumes final convergence 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. 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 This sentence is talking about it does not deal with the case of one side of the link uses IS-IS wide metric, but the other side of the link uses a narrow metric. It is saying it’s broken and we 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. Yes, added a sentence in the TLV definition part to describe them 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. this document does not support that. The main reason is that the pnode LSP is shared by all the topologies, and it’s not per topology pnode definition. For backwards competibility of this draft, it follows the same logic. Otherwise we need a flag day in the network to use ‘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. 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, ‘link of last resort’ is only for certain use cases, but not all. added 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. 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. As also replied also to Alvaro’s AD review comments, one of the main use case is for operational maintenance window to divert the traffic into certain links by using the ‘reverse-metric’ mechanism. If an operator has to track down all the nodes on the LAN side of the links, and to manually enable the feature one by one correctly, then it defeats the 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 changed to lower case. > > ========= > >> From the IANA section > > SB> Why is 18 chosen in an otherwise empty registry? Originally, it inherits the same sub-TLV from IS-IS TE sub-TLV RFC RFC 5305, and the Traffic Engineering Metric sub-TLV is 18. now this ‘reverse-metric’ TLV has it’s own registry, but we keep the 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. > Good catch. corrected.