Re: [Idr] 2 Week WG last call for draft-ietf-idr-bgp-enhanced-route-refresh

["John G. Scudder" <[e-mail]>]

<co-chair hat = off>

Summary: based on the below review, I think the document is not yet done.

- The document needs an "Updates: RFC2918" header

- "   A BGP speaker that support the message subtypes for the ROUTE-REFRESH"
  s/support/supports/
  This was previously noted by Randy Bush, IIRC. BTW, I don't claim to 
  be an expert grammarian but other than what I've noted in this review 
  I don't see any problems.

- "   Before the speaker starts a route refresh that is either initiated
   locally, or in response to a "normal route refresh request" from the
   peer, the speaker MUST send a ROUTE-REFRESH message with the
   specified message subtype to mark the beginning of the route refresh.
   After the speaker completes the re-advertisement of the entire Adj-
   RIB-Out to the peer, it MUST send a ROUTE-REFRESH message with the
   specified message subtype to mark the ending of the route refresh."

  Presumably in the first case, the "specified message subtype" is 
  the "Demarcation of the beginning of a route refresh" subtype 1, and
  in the second case, the "specified message subtype" is the 
  "Demarcation of the ending of a route refresh" subtype 2. Say that 
  explicitly (either by the name, the type code, or both). See also 
  the second-last point in this review, about short symbolic names.

- "   Conceptually the "entire ADJ-RIB-Out" for a peer in this section
   refers to all the route entries in the "ADJ-RIB-Out" for the peer at
   the start of the route refresh.  When a route entry in the "ADJ-RIB-
   Out" changes, the advertisement of the modified route entry (instead
   of the snapshot entry) would suffice."

  (By the way, s/ADJ/Adj/g)

  As Qin Wu's message suggests, this text isn't clear enough. Part of
  the problem is that you haven't defined "snapshot" but I think it
  goes beyond that. One way I can think of to fix it would be to
  simply remove the paragraph, on the assumption that "advertise the
  entire Adj-RIB-Out" is sufficiently obvious to an implementor, and
  that's all you're really trying to specify here. Otherwise, I think
  you're going to have to specify more rigorously what you mean by
  "advertise the entire Adj-RIB-Out". In particular, I suspect you're
  trying to bound the time you have to spend before you send the
  subcode 2 message to be O(size of Adj-RIB-Out when you started), so
  that you're not tripped up by pathological cases?

  I don't have text to offer for this case, but I do think the authors
  and WG need to consider it.

- I don't think this text is right:

  "   In processing a ROUTE-REFRESH message from a peer, the BGP speaker
   MUST examine the "message subtype" field of the message and take the
   appropriate actions."

  I mean, it's OK, except it basically says "do the right thing" ("take
  the appropriate actions"). The spec either needs to say unambiguously 
  what the right thing is (if it's a MUST) or adopt some more relaxed 
  tone. Likewise,

  "The BGP speaker SHALL use the demarcations of
   the beginning and the ending of a route refresh to perform
   consistency validations of the updates received from the peer."

  At most, the speaker MAY do that. If we wanted to mandate it (and I
  don't think we do) we would need to say what "perform consistency
  validations" means. I would be OK with either striking the text, or
  changing the SHALL to a MAY.

  Here is a possible rewrite of the paragraph:

   When a BGP speaker receives a Route Refresh message from a peer
   with subtype 1 it MUST mark all routes with the given AFI/SAFI 
   from that peer as stale. As it receives routes from its peer's 
   subsequent Adj-RIB-Out readvertisement, these replace any
   corresponding stale routes. When the BGP speaker receives a 
   Route Refresh message from the peer with subtype 2, it MUST 
   immediately remove any routes from the peer that are still marked 
   as stale for that address family. Such routes MAY be logged for
   further analysis. (It can be observed that this procedure is 
   substantially the same as for the Restarting Speaker in Section 
   4.2 of [RFC4724].)

- RFC 4724 has a timer to deal with the case where the Restarting
  Speaker forgets to send the EoR. Subcode 2 is basically an EoR, but
  we have no timer. Is this a problem? Do we need some other kind of
  safety mechanism? Or are we happy to just rely on the correctness
  of the implementation (ahem).

- During the initial Adj-RIB-Out advertisement, can normal withdraws
  be sent? I assume so. Not sure if anything need be said about this.

- The Error Handling section of the draft assumes traditional RFC 4271
  style error handling, namely if you see something wrong, reset the
  session. Is this the right thing to do in light of the other work the 
  WG has been doing with draft-ietf-idr-error-handling? For that 
  matter, is this the right thing to do in light of the fact that 
  regular route-refresh messages aren't specified to behave in this 
  way? I don't *necessarily* propose that malformed refresh messages 
  should be ignored, but it seems worth discussing. If this stricter
  approach is adopted, should it be considered for regular Route 
  Refresh too, and if so, should that go in this document?

  Also, assuming the current error approach is kept, why not go whole 
  hog and finish enumerating the possible error conditions? I think 
  there's only one other: Invalid Subtype. (See also my final point
  below.)

- What is the expected behavior if the peer sends another Route Refresh
  request when the first request is still in progress (subtype 2 has
  not yet been sent)? One option is that you could cancel the old
  refresh, send another subtype 1, then the new refresh, then close it
  with a subtype 2. But this implies there would be two subtype 1's
  followed by only one subtype 2. Another way of asking this question
  is whether there are any restrictions on the interleaving of subtype
  1 and 2, and what error handling (if any) is to be applied if the
  restrictions (if any) are violated.  

- In the Acks, you need to capitalize Jeff Hass's last name.

- In writing this review I've had to choose several times between 
  referring to "subcode 1" and "subcode 2" (not very descriptive) and
  "Demarcation of the beginning of a route refresh" and "Demarcation 
  of the ending of a route refresh" (verbose). This leads me to think
  it might be handy for the document to provide short symbolic names
  for the subtypes, such as BoRR and EoRR.

- By the way, now that you've turned the "reserved" field into a 
  "subtype" field, you need to decide what to do with the rest of the
  values. Right now we have 0 which means either route-refresh or
  ORF (disambiguated by length) and 1 and 2 defined in this document.
  The obvious thing to do would be to make this a registry, though
  then you have to say what to do with unrecognized values. Come to
  think of it, you have to say that anyway, right now the Error 
  Handling section doesn't cover it. 

</hat>

Thanks,

--John

On Sep 13, 2013, at 3:31 PM, Susan Hares <[e-mail]> wrote:

> This begins a 2 week working group last call for (ending 9/27)
> Enhanced Route Refresh Capability for BGP-4
> http://tools.ietf.org/html/draft-ietf-idr-bgp-enhanced-route-refresh-04
> 
> This WG last call is in parallel with the 2 week 
> implementation report adoption call. 
> 
> The implementation report is at: 
> http://datatracker.ietf.org/doc/draft-keyupate-idr-enhanced-refresh-impl/
> 
> We will read both threads in considering the last call.  If the
> implementation report is accepted, we will do a last call on the
> implementation report. 
> 
> Sue Hares and John Scudder
> Idr co-chairs 
> 
> 
> 
> 
>