Hi Joel, Thanks for the review. > On Dec 5, 2024, at 14:09, Joel Halpern via Datatracker <noreply@xxxxxxxx> wrote: > > Reviewer: Joel Halpern > 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://wiki.ietf.org/en/group/gen/GenArtFAQ>. > > Document: draft-ietf-lsvr-bgp-spf-39 > Reviewer: Joel Halpern > Review Date: 2024-12-05 > IETF LC End Date: 2024-12-10 > IESG Telechat date: Not scheduled for a telechat > > Summary: > > Major issues: > > Minor issues: > In section 5.2.1.1 on the SPF Status TLV there is an odd phrasing that I > think I understand, and if so should be clarified. In discussing unknown > status value handling the text reads "However, a BGP SPF speaker MUST NOT > use the Status TLV in its SPF computation." I presume that the NLRI and all > its TLVs should not be used, as the Status is unkown, rather than just > ignoring the Status? If the intention is that the NLRI is used, and the > status is assumed to make this node available for SPF, please reword to make > clear that the Status TLV is ignore and the node information IS USED in the > SPF. (I tend to assume one errs on the side of avoiding nodes that may not > be usable. Reading section 6.3 I think your intention is the obverse. > Which is your call. In any case, we want everyone to understand this the > same way so clarification is a good idea.) This applies to the other Status > TLV descriptions as well. I’ve updated these. with a non-reserved value (2-254) that is unknown SHOULD be advertised to other BGP SPF speakers and MUST ignore the Status TLV with an unknown value in the SPF computation. > > In section 5.2.2.1 on BGP-LS-LINK NLRI Address Family Descriptor Link TLV, > it is unclear if the intention is that unnumbered links must support only > one of IPv4 or IPv6, or if they are permitted to support both. I would > guess that the intention is to permit both, and that the advertiser simply > includes two such TLVs. It would help to be clear about the case. I added text for this. I have a second comment on this as well. Note that an unnumbered link can be used for both the IPv4 and IPv6 SPF computation by advertising separate Address Family Link Descriptor TLVs for IPv4 and IPv6. > > In section 6.1 on BGP SPF NLRI Selection, I missed something in the > recovvery from complete state loss. I understand how the preference for > self-originated information from direct neighbors helps direct neighbors, > and why that is specified. The text seems to say that takes care of all > problems. I don't see how it fixes remote stale information. Suppose we > have routers A and B adjacent, and Router C further away. Router A goes > down so hard it loses the upper part of its sequence number, and therefore > comes up with lower sequence numbers than it was using before. Router B > will as we want prefer these new advertisements anyway. But now consider > router C. It has in its store old advertisements from router A. The new > advertisements will have lower sequence numbers, so, if I read this > correctly, router C will prefer the old (stale) information. What drives > convergence so C eventually prefers new information from A? (As far as I > can tell, because B will prefer the new information, it may well not send A > the old information, thus preventing A from updating its sequence numbers.) You’d have to rely on 6.1.1 for the convergence on non-directly connected routers. > > Nits/editorial comments: > I found the first paragraph of the introduction jarring. It seemed to mix > propaganda for this work with a description of something (unclear what) > about current practices in MSDCs. I think it would benefit from rephrasing. > I suggest a single sentence. "This document describes a technique to use > BGP, BGP-LS, and the SPF technology from IGPs to provide routing for > Massively Scaled Data Centers (MSDCs)." This is what the WG set out to do. > > Is it my imagination or do section 5.1.1 and the first part of section > 5.1.2 say exactly the same thing? I’ve removed the second one since it is tangential. > > I would suggest slightly adjusting the initial text in section 5.2.4 on the > BGP_LS Attribute Sequence NUmber TLV. The text currently reads "A BGP-LS > Attribute TLV of the BGP-LS-SPF NLRI" which caused me to go check what 1181 > was defined to be. Could it instead read "A BGP-LS Attribute Sequence > Number TLV of the BGP-LS-SPF NLRI" (e.g. add "Sequence Number".) I changed three instances where BGP-LS Attribute TLV is used generically to reference the specific TLV. > > Several short paragraphs in the middle of section 7.1 on processing of > BGP-LS-SPF TLVs begin with the TLV name and then say " TLVs and their error > handling rules are either defined in section 5.2.1 of [RFC9552]. ". What is > the "either"? There appears to only be the reference to section 5.2.1 of > RFC 9552? I’ve fixed the instances where there isn’t an alternative. Thanks, Acee > > -- last-call mailing list -- last-call@xxxxxxxx To unsubscribe send an email to last-call-leave@xxxxxxxx
