Subject: [Gen-art] Gen-art LC review of draft-ietf-l2tpext-sbfd-discriminator-03

From: Elwyn Davies <[email-masked]>
Date: Sun, 10 Apr 2016 18:26:29 +0100

I am the assigned Gen-ART reviewer for this draft. The General Area
Review Team (Gen-ART) reviews all IETF documents being processed
by the IESG for the IETF Chair.  Please treat these comments just
like any other last call comments.

Apologies for the slightly late review - I was stricken by a stomach bug.

For more information, please see the FAQ at

<http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.

Document: draft-ietf-l2tpext-sbfd-discriminator-03.txt
Reviewer: Elwyn Davies
Review Date: 2016/04/10
IETF LC End Date: 2016/04/05
IESG Telechat date: (if known) -

Summary: Almost ready. There are a number of acronyms that need 
expansion and I am unclear how an implementation of S-BFD getting its 
discriminators via L2TPv3 would know what entities the advertised 
discriminators (whether one or many) should be associated with. This 
probably just needs a single sentence to sort it out, but I found the 
existing wording to be either inappropriate or just plain wrong.

Nearly Major Issue:
Question:
The format allows multiple discriminators to be sent in one message.  
Looking at the S-BFD base draft (s3), it is implied that if a S-BFD 
module is dealing with multiple discriminators they would be associated 
with multiple different entities.  How would the receiver know what 
entity each of the discriminators was supposed to be associated with?  
It seems to me that the receiver would want to know this but I may not 
have understood what is going on here.  I guess one way would be to use 
a part of the discriminator bit pattern but I don't know if this might 
make it more difficult to achieve uniqueness in a domain.  Alternatively 
a purpose  provided field could be sent in addition to each 
discriminator, or different AVP IDs used for well-known purposes.

The sentence at the end of para 4 of s2.1:
> When multiple S-BFD discriminators are
>    advertised, the mechanism to choose a subset of specific
>    discriminator(s) is out of scope for this document.
  may be associated with this question but I am not sure
(1) which end is supposed to be choosing - is the Initiator choosing 
which to advertise or the Responder choosing which to 'do something 
with', and
(2) it seems to me that the L2TPv3 receiver or its associated S-BFD 
module would be deciding on some well-defined basis which entities were 
associated with which designator rather than just making an arbitrary 
choice.

I suggest that a sentence indicating how the association of 
discriminators to entities is done (or even that it needs to be done but 
how it is done is out of scope) needs to be added, and the sentence 
above needs to be clarified or subsumed in this new part (unless either 
of the more draconian alternatives suggested above is thought to be 
appropriate).

Minor issues:
None

Nits/editorial comments:
Abstract:  I'm afraid you are going to have expand the acronyms AVP, 
L2TP and BFD here.  References to RFC 5880 and the new S-BFD draft RFC 
would also be helpful - in the form "(RFC 5880, 
I-D.ietf-bfd-seamless-base)".

s1.1: The  L2TP message names used in s2.1 and elsewhere (ICRQ, ICRP, 
OCRQ, OCRP)  need to be expanded  - here would be as good as anywhere.

s2.1: Need to expand AVP and ID.

---

From: "Carlos Pignataro (cpignata)" <[email-masked]>
Date: Wed, 13 Apr 2016 19:35:07 +0000

Elwyn,

This is a great review. Thank you.

Please see inline.

Deborah,

We will be queueing edits, let me know when you’d want us to post an updated revision.

> On Apr 10, 2016, at 1:26 PM, Elwyn Davies <[email-masked]> wrote:
> 

I hope you are feeling better!

> Summary: Almost ready. There are a number of acronyms that need expansion and I am unclear how an implementation of S-BFD getting its discriminators via L2TPv3 would know what entities the advertised discriminators (whether one or many) should be associated with. This probably just needs a single sentence to sort it out, but I found the existing wording to be either inappropriate or just plain wrong.
> 

These items, with the exception of the acronym expansion, have already been discussed. See below line-by-line responses to each of these potential issues, as well as a set of HTML diffs from the working copy, FYI.

> Nearly Major Issue:
> Question:
> The format allows multiple discriminators to be sent in one message.  Looking at the S-BFD base draft (s3), it is implied that if a S-BFD module is dealing with multiple discriminators they would be associated with multiple different entities.  How would the receiver know what entity each of the discriminators was supposed to be associated with?  It seems to me that the receiver would want to know this but I may not have understood what is going on here.  I guess one way would be to use a part of the discriminator bit pattern but I don't know if this might make it more difficult to achieve uniqueness in a domain.  Alternatively a purpose  provided field could be sent in addition to each discriminator, or different AVP IDs used for well-known purposes.

Thanks for raising this — although this was discussed and addressed, it might not be self-evident. See the next comment.

> may be associated with this question but I am not sure

Indeed, this is the relevant paragraph.

This paragraph was also added to other related documents (ISIS, OSPF), and goes hand-in-hand with the following paragraph from draft-ietf-bfd-seamless-base-08:
   The use of multiple S-BFD
   discriminators by a single network node is therefore discouraged
   until a means of learning the mapping is defined.

Now, that said, it is clear that if it created confusion in this review, this can also create future confusion.

> (1) which end is supposed to be choosing - is the Initiator choosing which to advertise or the Responder choosing which to 'do something with', and
> (2) it seems to me that the L2TPv3 receiver or its associated S-BFD module would be deciding on some well-defined basis which entities were associated with which designator rather than just making an arbitrary choice.
> 
> I suggest that a sentence indicating how the association of discriminators to entities is done (or even that it needs to be done but how it is done is out of scope) needs to be added, and the sentence above needs to be clarified or subsumed in this new part (unless either of the more draconian alternatives suggested above is thought to be appropriate).
> 

To really minimize the chance of that future confusion, I agree with your suggestion. I added such sentence and reworded the paragraph a bit. Further comments on the proposed text most welcome.

> Minor issues:
> None
> 
> Nits/editorial comments:
> Abstract:  I'm afraid you are going to have expand the acronyms AVP, L2TP and BFD here.  References to RFC 5880 and the new S-BFD draft RFC would also be helpful - in the form "(RFC 5880, I-D.ietf-bfd-seamless-base)”.

Acronyms expanded. Since the Abstract should not have citations, I am hesitant to add anything else. It is clearly in the Introduction.

> 
> s1.1: The  L2TP message names used in s2.1 and elsewhere (ICRQ, ICRP, OCRQ, OCRP)  need to be expanded  - here would be as good as anywhere.
> 

S1.1 already specifies expectations to readers, which include these L2TP messages:

1.1.  Terminology

   The reader is expected to be very familiar with the terminology and
   protocol constructs defined in S-BFD (see Section 2 of
   [I-D.ietf-bfd-seamless-base]) and L2TPv3 (see Section 1.3 of
   [RFC3931]).

But sure, will expand on first use as well.

> s2.1: Need to expand AVP and ID.

Seems a bit repetitive, but done.

Thanks,

— Carlos.

---

From: Elwyn Davies <[email-masked]>
Date: Fri, 15 Apr 2016 17:30:59 +0100

Hi, Carlos.

Thanks for the response.

First: a remaining acronym: L2TPv3 needs to be expanded on first use in 
the Abstract.

Second: Although citations are not allowed in the Abstract, this doesn't 
prevent explicit RFC numbers in a non-citation form.  So, yes, 
[RFC5880][I-D.draft-ietf-bfd-seamless-base] is not allowed but '(RFC 
5880, RFC <number allocated to draft-ietf-bfd-seamless-base>)' is fine

Finally:
I took a quick look around the various other S-SFD in 'xxx' drafts, of 
which the IS-IS is already in the RFC Editor queue and see that the 'out 
of scope' words for the multiple discriminator case appears to be the 
selected way forwards - so the sentence you added is good.

>    discriminator(s) is out of scope for this document.
Something being chosen is a mechanism and this is out of scope...

The form used in the IS-IS draft:
>     advertised how a given discriminator is mapped to a specific use case
>     is out of scope for this document.
seems clearer to me.

Cheers,
Elwyn

---

From: "Carlos Pignataro (cpignata)" <[email-masked]>
Date: Fri, 15 Apr 2016 17:41:10 +0000

Hi, Elwyn,

Many thanks again. I am more than happy to accommodate these editorials and further improve the doc.

Expanded acronyms (in addition to L2TPv3, there was one “BFD” missing, as well as the Title and abbrev). I updated the ‘out of scope’ text, but, finally, I did not modify the Abstract — while I understand RFC numbers in a non-citation fashion are OK (although semantically they play as a citation), I’d prioritize consistency with the other S-BFD documents (OSPF, ISIS).

Just posted a new rev:
URL:            https://www.ietf.org/internet-drafts/draft-ietf-l2tpext-sbfd-discriminator-04.txt
Status:         https://datatracker.ietf.org/doc/draft-ietf-l2tpext-sbfd-discriminator/
Htmlized:       https://tools.ietf.org/html/draft-ietf-l2tpext-sbfd-discriminator-04
Diff:           https://www.ietf.org/rfcdiff?url2=draft-ietf-l2tpext-sbfd-discriminator-04

Enjoy your weekend,

— Carlos.

From: Elwyn Davies <[email-masked]<mailto:[email-masked]>>
Date: Friday, April 15, 2016 at 12:30 PM
To: Carlos Pignataro <[email-masked]<mailto:[email-masked]>>
Cc: General area reviewing team <[email-masked]<mailto:[email-masked]>>, "[email-masked]<mailto:[email-masked]>" <[email-masked]<mailto:[email-masked]>>
Subject: Re: Gen-art LC review of draft-ietf-l2tpext-sbfd-discriminator-03

Hi, Carlos.

Thanks for the response.

First: a remaining acronym: L2TPv3 needs to be expanded on first use in the Abstract.

Second: Although citations are not allowed in the Abstract, this doesn't prevent explicit RFC numbers in a non-citation form.  So, yes, [RFC5880][I-D.draft-ietf-bfd-seamless-base] is not allowed but '(RFC 5880, RFC <number allocated to draft-ietf-bfd-seamless-base>)' is fine

Finally:
I took a quick look around the various other S-SFD in 'xxx' drafts, of which the IS-IS is already in the RFC Editor queue and see that the 'out of scope' words for the multiple discriminator case appears to be the selected way forwards - so the sentence you added is good.

seems clearer to me.

Cheers,
Elwyn

Elwyn,

Please see inline.

Deborah,

On Apr 10, 2016, at 1:26 PM, Elwyn Davies <[email-masked]><mailto:[email-masked]> wrote:

<http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq><http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.

Nearly Major Issue:
Question:

Minor issues:
None

Nits/editorial comments:

1.1.  Terminology

Thanks,

— Carlos.