Subject: [RTG-DIR] RtgDir review: draft-ietf-trill-mtu-negotiation-02

From: Julien Meuric <[email-masked]>
Date: Wed, 27 Apr 2016 22:33:38 +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-trill-mtu-negotiation-02.txt
Reviewer: Julien Meuric
Review Date: April 27, 2016
IETF LC End Date: April 5, 2016

Intended Status: Standards Track

*Summary:*
I have some minor concerns about this document that I think should be 
resolved before publication.

*Comments:*
Even though it requires to browse some other TRILL (normative) 
documents, the mechanism itself is simple and well described. 
Nevertheless, the specification deserves some improvement when it comes 
to obligations and options: this was part of my expectation after I 
realized it was upgrading a short section of the base document (RFC 
6325), which needs to be emphasized as well.

*Minor Issues:*
- The document is ST and references RFC 2119. There a some "MUST" and 
one "SHOULD", many of them inherited from specifications out of the 
referenced documents. On the other side, "must" and "should" are 
commonly used. This MUST be brought up to ST expectations.
- The document claims to only update RFC 7177. It seems however that it 
also updates RFC 6325 (section 4.3.2), RFC 7176 and maybe even RFC 7780. 
That should be either acknowledged or clarified. The 4th paragraph of 
the introduction tries to tackle that topic, but it is not clear enough 
in defining the position of the document with respect to previously 
defined mechanisms.
- The 3rd paragraph of the introduction duplicates the beginning of the 
following section 2. A possible way to limit this repetition effect may 
be to summarize that part of the introduction.
- Section 3 specifies an algorithm. Even if it does not rely on a formal 
language, consistency would be valuable. My mind compiler would suggest:
     * "If" is followed by "then" only once: "then" keyword to be dropped?
     * The algorithm relies on a break/stop or continue principle; as 
such, the instance of "Else" at the end should be replaced by "<line 
break> 4) Repeat Step1";
     * "is set to" and "<--" seem to be similar: please pick one or clarify;
     * to improve readability, I would drop the double naming introduced 
with X, X1 and X2 and rely on explicit variable names all along, as in 
the text: e.g., "linkMtuSize" instead of X, "lowerBound" for X1 and 
"upperBound" instead of X2.

*Nits:*
------
"Updates" field in header
---
- I think the "RFC" acronym should appear.
- The list may be extended with RFC RFC 6325, RFC 7176 and maybe even 
RFC 7780.
------
Abstract
---
- s/campus wide MTU feature/campus-wide MTU feature/
- s/campus wide capability/campus-wide capability/
- s/link local packets/link-local packets/
- s/link local MTUs/link-local MTUs/
- "It updates RFC..." duplicates header: either to drop or make more 
specific to point to precise sections/mechanisms.
------
Section 1.
---
- s/link scope PDUs can/link-scoped PDUs can/
------
Section 2.
---
- s/campus wide Sz MTU/campus-wide Sz MTU/
- s/area wide scope/area-wide scope/
- s/domain wide scope/domain wide scope/
- s/L1 Circuit Scoped/L1 Circuit-Scoped/
- "limited to 1470 to 65,535 bytes": I cannot parse it, is it meant to 
be a range?
------
Section 4.
- OLD
"while RB1 normally ignores link state information for any IS-IS 
unreachable RBridge RB2, originatingL1LSPBufferSize is an exception."
   NEW
"while in most cases RB1 ignores link state information for IS-IS 
unreachable RBridge RB2, originatingL1LSPBufferSize is meaningful."
[current wording suggests it is adding an exception to a mandatory 
behavior, which AFAIU it does not]
------
Section 7.
---
- "tested size can be advertised": "can" is to be addressed as part of 
the loose RFC 2119 wording comment.
------
Section 8.
---
- "value [...] had been reported": "reported" puzzles me, maybe "tested" 
was meant?
- The 3rd paragraph "For an Lz-ignorant [...] link-wide Lz." should be 
moved up to become the second paragraph, so as to clarify what an 
Lz-ignorant is.
- "The extension of TRILL MTU negociation...": this is an explicit 
positioning which should be mentioned earlier in the I-D.
------
Section 10.
---
- RFC 7180 bis is now RFC 7780.
---

Regards,

Julien

---

From: Mingui Zhang <[email-masked]>
Date: Fri, 29 Apr 2016 08:10:04 +0000

Hi Julien,

Thanks for the comments! Much appreciated! 

Please see in-lines below. An updated version will soon be uploaded to resolve the comments.

Thanks,
Mingui

> -----Original Message-----
> From: Julien Meuric [mailto:[email-masked]]
> Sent: Thursday, April 28, 2016 4:34 AM
> To: [email-masked]; [email-masked]
> Cc: [email-masked]; [email-masked]
> Subject: RtgDir review: draft-ietf-trill-mtu-negotiation-02
> 
> Hello,
> 
> 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 
> 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.
> 
> I have some minor concerns about this document that I think should be resolved
> before publication.
> 
> *Comments:*
> Even though it requires to browse some other TRILL (normative) documents, the
> mechanism itself is simple and well described.
> Nevertheless, the specification deserves some improvement when it comes to
> obligations and options: this was part of my expectation after I realized it was
> upgrading a short section of the base document (RFC 6325), which needs to be
> emphasized as well.

In the abstract, it's already mentioned as "optional updates" to RFC 6325. I think "Updates: 6325 " needs to appear in the page header as well. 

> 
> *Minor Issues:*
> - The document is ST and references RFC 2119. There a some "MUST" and one
> "SHOULD", many of them inherited from specifications out of the referenced
> documents. On the other side, "must" and "should" are commonly used. This
> MUST be brought up to ST expectations.

The usage of "must" and "should" will be updated as needed. I have checked all those appearances. The changes would be editorial.

> - The document claims to only update RFC 7177. It seems however that it also
> updates RFC 6325 (section 4.3.2), RFC 7176 and maybe even RFC 7780.

Actually, I don't think this document updates RFC7176. It simply makes use of the MTU Sub-TLV defined in RFC 7176.
The specification about the originatingL1LSPBufferSize of an unreachable RBridge is a slight update to RFC7780. This will be emphasized.

> That should be either acknowledged or clarified. The 4th paragraph of the
> introduction tries to tackle that topic, but it is not clear enough in defining the
> position of the document with respect to previously defined mechanisms.

The updated to RFC 6325 will be emphasized in this paragraph. The backward compatibility will be moved to here as well. It will help the positioning. 

> - The 3rd paragraph of the introduction duplicates the beginning of the following
> section 2. A possible way to limit this repetition effect may be to summarize that
> part of the introduction.

Yes, summarization is the proper way. 

> - Section 3 specifies an algorithm. Even if it does not rely on a formal language,
> consistency would be valuable. My mind compiler would suggest:
>      * The algorithm relies on a break/stop or continue principle; as such, the
> instance of "Else" at the end should be replaced by "<line
> break> 4) Repeat Step1";
>      * to improve readability, I would drop the double naming introduced with X,
> X1 and X2 and rely on explicit variable names all along, as in the text: e.g.,
> "linkMtuSize" instead of X, "lowerBound" for X1 and "upperBound" instead of X2.

Sure. Explicit names will be used for the sake of readability. 

> *Nits:*

The following nits will be fixed as suggested. 

> - The list may be extended with RFC RFC 6325, RFC 7176 and maybe even RFC
> 7780.
> ------
> Abstract
> ---
> - "It updates RFC..." duplicates header: either to drop or make more specific to
> point to precise sections/mechanisms.
> ------
> Section 1.
> ---
> - "limited to 1470 to 65,535 bytes": I cannot parse it, is it meant to be a range?
> ------
> Section 4.
> - OLD
> "while RB1 normally ignores link state information for any IS-IS unreachable
> RBridge RB2, originatingL1LSPBufferSize is an exception."
>    NEW
> "while in most cases RB1 ignores link state information for IS-IS unreachable
> RBridge RB2, originatingL1LSPBufferSize is meaningful."
> [current wording suggests it is adding an exception to a mandatory behavior,
> which AFAIU it does not]

OK. Will update the words.

> ------
> Section 7.
> ---
> - "tested size can be advertised": "can" is to be addressed as part of the loose
> RFC 2119 wording comment.

Will use the word "MAY" instead.

> - The 3rd paragraph "For an Lz-ignorant [...] link-wide Lz." should be moved up to
> become the second paragraph, so as to clarify what an Lz-ignorant is.

OK. It will be moved up.

> - "The extension of TRILL MTU negociation...": this is an explicit positioning which
> should be mentioned earlier in the I-D.

OK. This will be moved to the introduction.

> - RFC 7180 bis is now RFC 7780.

Yes, this will be updated.