Re: [Idr] RtgDir review: draft-ietf-idr-bgp-gr-notification-07

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

Hi Emmanuel,

Thanks for your review. My questions and comments are in line below.

--John

> On Sep 12, 2016, at 8:05 AM, Emmanuel Baccelli <[e-mail]> wrote:
> 
> Hello,
> 
> I have been selected as the Routing Directorate reviewer for this draft.
> The Routing Directorate seeks to review all routing or routing-related
> drafts as they pass through IETF last call and IESG review, and sometimes
> on special request. The purpose of the review is to provide assistance to
> the Routing ADs. For more information about the Routing Directorate, please
> see ​http://trac.tools.ietf.org/area/rtg/trac/wiki/RtgDir
> 
> Although these comments are primarily for the use of the Routing ADs, it
> would be helpful if you could consider them along with any other IETF Last
> Call comments that you receive, and strive to resolve them through
> discussion or by updating the draft.
> 
> Document: draft-ietf-idr-bgp-gr-notification-07
> Reviewer: Emmanuel Baccelli
> Review Date: Sept. 12th 2016
> Intended Status: Standards Track
> 
> Summary:
> 
> This document is basically ready for publication, but has nits that should
> be considered prior to publication.
> 
> Comments:
> 
> This document is clearly written and easy to understand.
> I am not a BGP specialist, and in particular I was not familiar with the
> details of BGP Graceful restart before I have reviewed, so I had to go back
> and read RFC4724.

Thanks for doing that, it is particularly helpful to have that sort of review.

> It may mean that my review is not sufficiently in-depth, or that the nits I
> point out and my editorial suggestions may be too pedantic.
> 
> 
> Major Issues:
> No major issues found.
> 
> 
> Minor Issues:
> No minor issues found.
> 
> 
> Nits:
> 
> Nits and minor suggestions below can be considered, aiming to improve
> readability.
> 
> Working group indication:
> - indicate IDR working group at the top of the document (for now it says
> "Network working group")

It turns out that all IETF drafts say "network working group" on them, for obscure historical reasons.

> Abstract:
> - for clarity, append at the end of last sentence "and for force a full
> reset"?

Changed (in the source, not yet published as 08 pending this discussion) to "This document also defines a new BGP NOTIFICATION Cease Error subcode whose effect is to request a full session restart instead of a Graceful Restart. "

> Section 2
> - in restart flags, for completeness, recall that R flag is specified in
> RFC4724 and what it indicates.

Done.

> - recall that reserved/unspecified fields must be zeroed (and ignored)?

While this is a good suggestion as general practice, it seems beyond the scope of the present draft. If others disagree and think it's OK for the present draft to introduce clarifications and modifications to RFC 4724 beyond the definition of Notification support, I'd be happy to draft some text. But for now, I'd say it's better to hold this for a 4724bis.

> - spell out acronyms AFI and SAFI (in terminology section, as coming from
> RFC4724?) before first use in the document

Expanded terms in-line.

> - in Address family flags: remove "deprecated" specification text

To be clear, you are suggesting this text should be removed?

"The usage of second most significant bit "N" (which was defined in a
previous draft version of this specification) is deprecated. This bit MUST
be advertised as 0 and MUST be ignored upon receipt."

I am inclined to keep it since as it says, a previous version of the draft did spec such a flag and we have past experience indicating that this can cause problems when early implementations operate with more recent ones.  What's your reason for wanting to remove the text?

> Section 4:
> - "When a BGP speaker resets its session due to a HOLDTIME expiry, it
> should generate..."
>  => s/should/SHOULD

I am not inclined to use of the all-caps RFC 2119 language since this is not a new requirement imposed by the current specification, it's merely restating an existing requirement. That is, the word "should" is being used in its normal English sense. To elaborate, it's my practice whenever using SHOULD in the RFC 2119 sense, to spend a little effort considering under what circumstances an implementor might ignore the "SHOULD"  and then discuss those in a "MAY"  clause. I think doing that would be outside the scope of this document. If the word "should" causes readers pain, I'm happy to revise the sentence in a way that uses a different word.

> Section 4.1:
> - the last paragraph of section 4.2 of RFC4724 states that support for the
> stale route retain timer is a MAY.
> It seems appropriate to specify upfront that this timer is now a MUST?

It wasn't our intention to make the timer mandatory. Is there a reason you think it has to be? (As a practical matter, as far as I know all implementations do support the timer, but I think that's beside the point.)

> - "This supersedes the "consecutive restarts" requirement of [RFC4724] S.
> 4.2."
> => for convenience indicate which paragraph (3rd paragraph) of RFC4724 S

Done.