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

From: Emmanuel Baccelli <[email-masked]>
Date: Mon, 12 Sep 2016 14:05:25 +0200

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.
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")

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

Section 2
 - in restart flags, for completeness, recall that R flag is specified in
RFC4724 and what it indicates.
 - recall that reserved/unspecified fields must be zeroed (and ignored)?
 - spell out acronyms AFI and SAFI (in terminology section, as coming from
RFC4724?) before first use in the document
 - in Address family flags: remove "deprecated" specification text

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

 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?

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

---

From: "Susan Hares" <[email-masked]>
Date: Tue, 13 Sep 2016 09:32:28 -0400

Emmanuel: 

Thank you for your review.  The authors will create a revision based on your comments. 

Sue Hares
(IDR WG Co-chair) 

-----Original Message-----
From: [email-masked] [mailto:[email-masked]] On Behalf Of Emmanuel Baccelli
Sent: Monday, September 12, 2016 8:05 AM
To: <[email-masked]>
Cc: [email-masked]; [email-masked]; [email-masked]
Subject: RtgDir review: draft-ietf-idr-bgp-gr-notification-07

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.

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.
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")

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

Section 2

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

 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?

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

---

From: "John G. Scudder" <[email-masked]>
Date: Tue, 13 Sep 2016 14:43:23 -0400

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 <[email-masked]> wrote:
> 
> Hello,
> 

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

> - indicate IDR working group at the top of the document (for now it says

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

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

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. "

> 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.

> 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?

> - "When a BGP speaker resets its session due to a HOLDTIME expiry, it

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.

> 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.)

> => for convenience indicate which paragraph (3rd paragraph) of RFC4724 S

Done.

---

From: Emmanuel Baccelli <[email-masked]>
Date: Thu, 15 Sep 2016 10:44:10 +0200

Hi John,

see comments inline.

On Tue, Sep 13, 2016 at 8:43 PM, John G. Scudder <[email-masked]> wrote:

> 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. "
>
>
Fine with me.

> 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.
>
>
OK. This was just a suggestion. However, see next comment below.

> 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?
>
>
I see. But on the other hand, in the long term, you are writing an RFC, and
in my opinion, it is awkward and confusing to have this kind of text and
reference to an early draft version in an RFC.
One solution could be to remove the N bit in the figure and recall that as
per RFC4724 all reserved bits must be set to zero?
The good side is it could fix your problem without referring to "prior
versions of the draft".
The bad side would be that it is not so great to have exactly the same
piece of spec duplicated from RFC4724.
I'm not sure there is a perfect solution here.

> 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.
>
>
No strong opinion on that. I let specialists talk it over, if needed. Else,
let's do what you propose.

> 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.)
>
>
When one reads the current spec:
on one hand, I understand that you MUST do something when this timer fires,
on the other hand, in RFC4724, this timer MAY be used.
So one naturally wonders what happens if this timer is not implemented: can
it cause problems?
I was thus suggesting that making this timer mandatory would be clearer.
If all the implementations you know about have this timer, it seems like
it's a really good idea to have it anyway.
But there again, I let specialists talk it over.

Best,

Emmanuel