Re: [L2tpext] rtg dir review of draft-ietf-l2tpext-sbfd-discriminator

["Carlos Pignataro (cpignata)" <[e-mail]>]

Hi Loa,

Thanks much for your review!

I will be posting a new revision addressing all your comments — in the meantime, please see inline.

> On Dec 18, 2015, at 9:22 AM, Loa Andersson <[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-l2tpext-sbfd-discriminator-01.txt
> Reviewer: Loa Andersson
> Review Date: 2015-12-18
> IETF LC End Date: date-if-known
> Intended Status: Proposed Standard (ID says Standards track)
> 
> Summary:
> 
>    I have some minor concerns about this document that I think should be resolved before publication.
> 
> Comments:
> 
>     I have considerable problems reading the draft, first it does not
> really follow RFC 7322 in some important details, also the format figure
> (as I understand it) is misleading. The document need a facelift.
> 
> 

Will work on it. Thanks. See below.

> 
> Major Issues:
> 
>    "No major issues found."
> 
> Minor Issues:
> 
>    Even though the nit-picking below is pretty massive, it is purely
> editorial and should be fixed before going to IETF Last Call. I have
> no real concerns about the technical content

Ack — thanks!

> 
> Abstract:
> ---------
>    I used often "my immediate manager" as a reference and said that
> the abstract should give her/him a good idea about what the draft is
> about. I don't think the abstract meet that standard. Could you please
> flesh out.

I updated the Abstract a bit, but have in mind it is also very consistent with draft-ietf-isis-sbfd-discriminator and draft-ietf-ospf-sbfd-discriminator

> 
>    RFC 7322 says that "Similarly, the Abstract should be complete
> in itself.  It will appear in isolation in publication announcements
> and in the online index of RFCs." If I encounter this and is not up to
> speed on l2tp and bfd, this does not give a good idea what it is about.
> 

Someone not up to speed with the basicmost concepts of BFD and L2TP as used in the abstract, should do some additional reading before this document...

> Abbreviations
> 
> RFC 7322 says:
>   Abbreviations should be expanded in document titles and upon first
>   use in the document.  The full expansion of the text should be
>   followed by the abbreviation itself in parentheses.  The exception is
>   an abbreviation that is so common that the readership of RFCs can be
>   expected to recognize it immediately; examples include (but are not
>   limited to) TCP, IP, SNMP, and HTTP.  The online list of
>   abbreviations [ABBR] provides guidance.  Some cases are marginal, and
>   the RFC Editor will make the final judgment, weighing obscurity
>   against complexity.
> 
> The abbreviations are not expanded in title, abstract, and some other
> places, nor are they "well-known”.

Agreed. Good point. Added a Terminology section, and expanded some abbrevs on first use.

> 
> Examples
> AVP - the RFC Editor abbreviation list gives two expansions, since this
> is l2tp I come to the conclusion that this is "attribute-value pair" and
> not the wellknow "Audio-Visual Profile (AVP)"
> 
> S-BFD - BFD is not well-known, and S-BFD is not even in the RFC Editors
> abbreviations list.
> 
> L2TPv3 - L2TP or L2TPv3 are not well-know. There is no RFC that does
> not expand the abbreviation in the title.
> 
> ICRQ, ICRP, OCRQ, and OCRP are sued but not expanded, the pointer to where to find the expansions (RFC 3931) are nit give until the third
> time the quartet is mentioned
> 
> LCCE - used but not expanded. nor well-known.
> The RFC Editor abbreviations has two expansions
> LCCE       - Logical Cluster Computing Environment (LCCE) or
>           - L2TP Control Connection Endpoint (RFCs 3931 and 4719)
> Since this is is L2TP context, it is the latter that is correct, but
> since we have an ambiguity we need to expand.
> 

Ack. Thanks.

> Section 2
> 
> I have a problem parsing this sentence:
> 
>   This AVP is exchanged during session negotiation (ICRQ, ICRP, OCRQ,
>   OCRP).
> 
> Do you mean to say
> 
>   The "S-BFD Target Discriminator ID" AVP is exchanged using the ICRQ,
>   ICRP, OCRQ, and OCRP control messages during session negotiations.
> 

Yes, fixed.

> Section 2.1
>   There is a TBD in the first sentence of this paragraph. While I
>   agree that TDB is "well-know" I prefer using [TBA by IANA],
>   where TBA stands for To Be Assigned.
>   If you change this you also need to change in the IANA section.
> 

OK — it really does not matter as this should be assigned before publication.

> Excuse me if I don't understand this figure
> 
>                                                     No. of octets
>                 +-----------------------------+
>                 | Discriminator Value(s)      |     4/Discriminator
>                 :                             :
>                 +-----------------------------+
> 
> First I think you say that a Discriminator is 4 octets
> Second there can be a variable number of discriminators per attribute
> value field
> The box in your figure seems to 29 bits wide, this is unorthodox.
> 

It’s the same as in draft-ietf-isis-sbfd-discriminator. The figure is clear. It does not show numbers of bits.

> 
>                  0       1       2       3
>                  01234567012345670123456701234567
>                 +-----------------------------+
>                 | Discriminator Value(s)      |
>                 :                             :
>                 +-----------------------------+
> 
> Is this what you mean?
> 
>                  0       1       2       3
>                  01234567012345670123456701234567
>                 +--------------------------------+
>                 | Discriminator Value (1)        |
>                 +--------------------------------+
>                 :                                :
>                 +--------------------------------+
>                 | Discriminator Value (n-1)      |
>                 +--------------------------------+
>                 | Discriminator Value (n)        |
>                 +--------------------------------+
> 
> Discriminator - a 4 octet value

But sure, I can make it even prettier :-)


> 
> IANA consideration
> 
>     There is a practice - which I disagree with - to assume that IANA
> and readers know where to the registries. Please point it out so no
> mistakes are possible.
> 
> OLD TEXT
> This number space is managed by IANA as per [RFC3438].
> 
> NEW TEXT
> IANA maintain a sub-registry "Message Type AVP (Attribute Type 0)
> Values" in the "Control Message Attribute Value Pairs" as per
> [RFC3438]. IANA is requested to assign the first free value from this
> sub-registry as the Message typ AVP for "S-BFD Discriminators".
> 
> 

Thanks for this text

> Nits:
> 
> The nits tool does only give us the date warning.
> 
> /Loa
> 

Thanks again, Loa!

— Carlos.


> 
> 
> --
> 
> 
> Loa Andersson                        email: [e-mail]
> Senior MPLS Expert                          [e-mail]
> Huawei Technologies (consultant)     phone: [phone]