Hi John, Thanks for your detailed review and comments/suggestions. We've accepted your editorial changes and please check inline for responses to your comments.
We have also posted the updated version with these changes: https://datatracker.ietf.org/doc/html/draft-ietf-lsr-ospf-bfd-strict-mode-06 On Tue, Aug 30, 2022 at 11:53 PM John Scudder <[email protected]> wrote: > Dear Authors, > > Thanks for you patience. Here’s my review. > > I’ve supplied my comments in the form of an edited copy of the draft. > Minor editorial suggestions I’ve made in place without further comment, > more substantive comments are done in-line and prefixed with “jgs:”. You > can use your favorite diff tool to review them; I’ve attached a PDF of the > iddiff output for your convenience if you’d like to use it. I’ve also > pasted a traditional diff below in case you want to use it for in-line > reply. > > Thanks, > > —John > > > --- draft-ietf-lsr-ospf-bfd-strict-mode-05.txt 2022-08-30 > 10:55:03.000000000 -0400 > +++ draft-ietf-lsr-ospf-bfd-strict-mode-05-jgs-comments.txt 2022-08-30 > 14:18:06.000000000 -0400 > @@ -22,7 +22,7 @@ > Detection (BFD) session prior to adjacency formation. Link-Local > Signaling (LLS) is used to advertise the requirement for strict-mode > BFD session establishment for an OSPF adjacency. If both OSPF > - neighbors advertise the BFD strict-mode, adjacency formation will be > + neighbors advertise BFD strict-mode, adjacency formation will be > blocked until a BFD session has been successfully established. > > Status of This Memo > @@ -92,10 +92,19 @@ > protocols like OSPFv2 [RFC2328] and OSPFv3 [RFC5340] to detect > connectivity failures for established adjacencies faster than the > OSPF hello dead timer detection and trigger rerouting of traffic > - around the failure. The use of BFD for monitoring routing protocols > + around the failure. The use of BFD for monitoring routing protocol > adjacencies is described in [RFC5882]. > > - When BFD monitoring is enabled for OSPF adjacencies, the BFD session > +jgs: In the paragraph below, I found the flow of reading disrupted by > +the dawning realization that it's describing the status quo prior to > +introduction of this spec, i.e. the shortcomings that drove development > +of the spec. I've proposed an additional first clause that might help > +mitigate that, feel free to use it if you like. (It's a little > +inelegant, you might want to do a more intrusive edit instead, up to > +you.) > KT> Ack. I've modified the text a little differently than your suggestion. Please let me know if it works. > + > + In implementations prior to introduction of this specification, > + when BFD monitoring is enabled for OSPF adjacencies, the BFD session > is bootstrapped based on the neighbor address information discovered > by the exchange of OSPF Hello packets. Faults in the bidirectional > forwarding detected via BFD then result in the OSPF adjacency being > @@ -192,7 +201,7 @@ > > Type: 21 > > - Length: 4 octet > + Length: 4 octets > > Local Interface IPv4 Address: The primary IPv4 address of the > local interface. > @@ -238,6 +247,32 @@ > the interface if they either have a corresponding BFD session > established or have not advertised OSPF BFD strict-mode in the > Hello packet LLS Extended Options and Flags. > + > +jgs: I expect that taken as a whole, this document is clear enough to > +enable an implementor to do the right thing, and that's the main goal. > +It does seem to me however that "updating the state machine" by means > +of putting a few notes into one of the states isn't the most thorough > +way of doing it -- probably if you were doing this from the get-go you'd > +introduce a new substate called "waiting for BFD establishment" or > +something like that, and then transition to it from Init at the > appropriate > +time. Or something like that, I'm just making things up on the fly here. > +Likewise you'd introduce a new "BFD comes up" event that gets you out of > +your new state and back to the main flow of the state machine. Instead > +that's encapsulated in the note you've added to Init. > + > +I don't want a return to the Bad Old Days where we spent months of our > +lives wrangling over state machines in the Routing Area (if you missed > +that time count yourself lucky). However, it would make me feel better to > +know that the WG has at least considered the question and decided to move > +ahead without doing a full update to the state machine. So, I'd appreciate > +a bit of discussion on this point, thanks. > KT> I don't remember this discussion on the mailer. I do remember some discussion during an initial presentation of this draft to the WG though not sure if it was during the session or offline. The introduction of a new state would result in far more intrusive changes to implementations. We would have this state whether or not the strict-mode is in use (BFD itself is optional). It was simpler to update the INIT state as proposed by the draft instead. We've added text in the Operations section so implementations show this additional BFD wait information along with the adjacency state and transitions. That should help in troubleshooting and operations. > + > +Also: when you write "This document updates ... [RFC2328]" it seems like > +there's a decent chance that someone's going to ask if the document should > +formally Update RFC 2328. If you're not going to use the updates header > +(and I don't insist you do) you might at least consider the reasons such > +an update isn't called for. It might be helpful too, to update the > +shepherd write-up with some text saying the WG considered the question. > KT> This is a good point and at least I was not sure if doing the formal update to RFC2328 was the right way since this is an optional feature. That said, we can do the formal updates if that is the right way to go about this. Looking for feedback if this change is required. > > Whenever the neighbor state transitions to Down state, the removal of > the BFD session associated with that neighbor SHOULD be requested by > @@ -246,6 +281,14 @@ > result in the deletion and creation of the BFD session respectively > when OSPF is the only client interested in the BFD session with the > neighbor address. > + > +jgs: Please do a global search for the RFC 2119 keyword SHOULD and in > +each instance, consider whether it can be a MUST, or the normal English > +word "should" (in lower or mixed case). If SHOULD is the appropriate > +keyword, please supply some detail about when it would be appropriate > +for an implementor to disregard the SHOULD. (The one place where I > +thought "should" might be the appropriate choice is in the Operations & > +Management Considerations section.) > KT> In this specific case, we are discussing implementation specifics that are also applicable without the strict-mode, and hence introducing a MUST here was not appropriate. We could change it to "should" if that would be more appropriate. Looking for feedback if this change is required. In some other places, the behavior may be applicable and shared with "base" BFD and hence we felt not appropriate to make it a MUST. However, we have taken the opportunity to suggest a desirable behavior. We look for feedback if any specific instance needs change or clarification. > > An implementation MUST NOT wait for BFD session establishment in Init > state unless OSPF BFD strict-mode is enabled on the interface and the > @@ -268,6 +311,12 @@ > BFD sessions. Disabling BFD would result in the BFD session being > brought down due to Admin reason [RFC5882] and hence would not bring > down the OSPF adjacency. > + > +jgs: Regarding the last sentence above, suppose the router is configured > +as you permit in Section 6, such that strict mode is required in order > +to establish an adjacency. Would it then be appropriate to bring > +down the adjacency if the B-bit is cleared? If the B-bit is cleared and > +the BFD session moves to admin down? > KT> No. The OSPF adjacency would not be brought down. The checking for B-bit and BFD session UP dependency check is only while in the INIT state. > > When BFD is enabled on an interface over which we already have an > existing OSPF adjacency, it would result in the router setting the > @@ -307,7 +356,7 @@ > state of adjacency formation (ideally when it receives the first > hello). The use of the Local Interface IPv4 Address TLV (as defined > in Section 3) in the LLS block of OSPFv3 Hello packets for IPv4 AF > - instances makes this possible. Implementations that support for OSPF > + instances makes this possible. Implementations that support OSPF > BFD strict-mode for OSPFv3 IPv4 AF instances MUST include the Local > Interface IPv4 Address TLV in the LLS block of their Hello packets > whenever the B-bit is also set in the LLS Options and Flags field. A > @@ -319,13 +368,16 @@ > > An implementation needs to handle scenarios where both graceful > restart (GR) and the OSPF BFD strict-mode are deployed together. The > - GR aspects discussed in [RFC5882] also apply with OSPF BFD strict- > + GR aspects discussed in [RFC5882] Section 3.3 also apply with OSPF BFD > strict- > mode. Additionally, in OSPF BFD strict-mode, since the OSPF > adjacency formation is delayed until the BFD session establishment, > the resultant delay in adjacency formation may affect or break the > GR-based recovery. In such cases, it is RECOMMENDED that the GR > timers are set such that they provide sufficient time to allow for > normal BFD session establishment delays. > + > +jgs: I suggested adding the section number to the reference above, because > +RFC 5882 doesn't use the term "Graceful Restart". > KT> Ack Thanks, Ketan > > > > @@ -363,8 +415,8 @@ > BFD strict-mode capability - both when that neighbor router does not > support BFD and when it does support BFD but does not signal the OSPF > BFD strict-mode as described in this document. Implementations MAY > - provide a local configuration option to specifically enable BFD > - operation in OSPF BFD strict-mode only. In this case, an OSPF > + provide a local configuration option to require BFD strict-mode. > + In this case, an OSPF > adjacency with a neighbor that does not support OSPF BFD strict-mode > would not be established successfully. Implementations MAY provide a > local configuration option to enable BFD without the strict-mode > @@ -396,13 +448,18 @@ > > 7. IANA Considerations > > - This specification updates Link Local Signaling TLV Identifiers > - registry. > + This specification makes the following updates under the > + "Open Shortest Path First (OSPF) Link Local Signalling (LLS) - > + Type/Length/Value Identifiers (TLV)" grouping. > > The following values have been assigned via early allocation: > + > + In the "LLS Type 1 Extended Options and Flags" registry, > > o B-bit from "LLS Type 1 Extended Options and Flags" registry at bit > position 0x00000010. > + > + In the "Link Local Signalling TLV Identifiers (LLS Types)" registry, > > o Type 21 - Local Interface IPv4 Address TLV > > >
_______________________________________________ Lsr mailing list [email protected] https://www.ietf.org/mailman/listinfo/lsr
