Subject: [Gen-art] Gen-art Yuletide LC review of draft-ietf-trill-rfc6327bis-02.txt

From: Elwyn Davies <[email-masked]>
Date: Mon, 23 Dec 2013 22:00:11 +0000

I am the assigned Gen-ART reviewer for this draft. For background on
Gen-ART, please see the FAQ at

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

Please resolve these comments along with any other Last Call comments
you may receive.

Document: draft-ietf-trill-rfc6327bis-02.txt
Reviewer: Elwyn Davies
Review Date: 23 December 2013
IETF LC End Date: 23 December 2012
IESG Telechat date: (if known) -

Summary:
Almost ready for the IESG.  There is one minor issue that seems as if it
needs a better way to explain how the defense against rogue native
packets is supposed to work and a number of nits.  Apologies if the
festive spirit got the better of a few comments.

Major issues:
None.

Minor issues:
s2.2, last para:
>    The primary TRILL defense mechanism against such loops, which is
>    mandatory, is to assure that, as far as practically possible, there
>    is only a single RBridge that is in charge of ingressing and
>    egressing native frames...
This seems to be difficult to implement:  Implementing some defense mechanism 
is stated to be mandatory but the proposed defense is at best, best efforts. 
It strikes me that given the expected catastrophic results of failure to have 
a robust defense, as delineated in the previous para, indicates that this 
either needs some better explanation or possibly a better defense.  I can't tell, 
but I guess we may be talking about some extraneous measure beyond this spec - please
explain.

Nits/editorial comments:
General: Adding titles/caption to figures and tables would be helpful.

General: s/byte/octet/

Abstract: s/IS-IS adjacency between/IS-IS adjacencies between/
s1, para 1: multipathing?? horrid non-word!!  Suggest:
OLD:
support for multipathing of both unicast and multicast traffic in
   multi-hop networks with arbitrary topology.
NEW:
support for transmission of both unicast and multicast traffic 
taking advantage of multiple paths in multi-hop networks with 
arbitrary topology.

s1, para 1: 'and a header that includes a hop count':  A header for what?  
Please make it clear what the header is added to.

s1, para 1, next to last sentence: Has two 'ands'
Suggest s/and optimization/as well as optimization/

s1, table: Probably worth expanding BFD again (even though it is 
in the abstract and terminology).

s1:  Definition and explanation of 'pseudonode':  There is no explanation 
of the pseudonode term until s7 - and even there it is buried a couple of paras
down in the text.  A definition/explanation either in s1 or s1.2 would be helpful.

s2.1, para 1, sentence 2:  This sentence is difficult to parse:  After two or three 
reads, I realised that the material starting at 'devices or services' is not part of the 
'includes' and ceased looking for an 'and'.  Suggest reversing the components of the 
main body thus:
OLD:
   Because this includes general bridged LANs
   [802.1Q], devices or services that can restrict VLAN connectivity,
   such as [802.1Q] bridges or carrier Ethernet services, can be part of
   a link between RBridges.  
NEW:
   Because this includes general bridged LANs
   [802.1Q], the links between RBridges may contain devices or services that 
   can restrict VLAN connectivity, such as [802.1Q] bridges or carrier 
   Ethernet services.

s2.4, para 1: s/resonably restricted MTU/relatively restricted MTU/

s3.3, Event A4: I was initially slightly confused as to how one timer
expiring resulted in both being expired in the braodcast case.  Maybe
reword a bit:
OLD:
      A4. Either (1) the Hello holding timer expires on a point-to-point 
          adjacency or (2) one or both Hello holding timers expire on a
          broadcast LAN adjacency resulting in them both being in the
          expired state for that adjacency.
NEW:
      A4. Either (1) the Hello holding timer expires on a point-to-point 
          adjacency or (2), on a broadcast LAN adjacency, both Hello timers
          expire together or, after one timer expired previously (see 
          Event A5), the second timer expires resulting in them both being 
          in the expired state for that adjacency.

s5, para 3: Although it isn't wrong, given that RFC 2119 language is called 
out in s1.2, it might be better to explicitly use MUST instead of
mandatory here.  MUST could also be used about the 1,470 bytes (octets)
minimum in para 2

s5, para 5: s/If it tests MTU/If it tests the MTU size/

s6, (not the) definition of BFD Enabled TLV:  The figure shows a fixed
value of 3 for the Length field and exactly one topology entry (2
octets) and a Protocol ID (1 octet).  The text indicates that there
could be several, it isn't specific as to whether there has to be
a match between topology and protocol entries but the length sort 
of indicates that there might be.  Frankly this is a mess (probably 
due to not being the definition of the TLV which isn't totally obvious).  
So if multiple topology and protocol entries are present:
- must there be a matching number of them (the length text sort of implies this)? 
- do they go in matched pairs: (topology, proto), (topology, proto), etc or
  all topologies first and then all protocols?
- Where topology ID's come from?
- Where do NLPIDs come from?
All of this can probably be done by citing the right reference as I 
eventually realised.

s7, para 3: 
> It is anticipated that many links between RBridges will actually be
>    point-to-point, in which case using a pseudonode merely adds to the
>    complexity.
Does this actually mean they actually be point-to-point even though they are 
on and using a broadcast LAN TRILL setup?   If so this should be clarified.

s8.2.1, para 1: s/should not be include/SHOULD NOT be included/

---

From: Donald Eastlake <[email-masked]>
Date: Tue, 24 Dec 2013 13:34:21 -0500

Hi Elwyn,

Seasons Greetings!

On Mon, Dec 23, 2013 at 5:00 PM, Elwyn Davies <[email-masked]>
wrote:
> Almost ready for the IESG.  There is one minor issue that seems as
> if it needs a better way to explain how the defense against rogue
> native packets is supposed to work and a number of nits.  Apologies

It’s not actually defense “against rogue native packets” but defense
against a native packet being multiply ingressed or egressed...

> if the festive spirit got the better of a few comments.

No apologies needed...

> None.

  Thanks.

>>    mandatory, is to assure that, as far as practically possible,
>>    there is only a single RBridge that is in charge of ingressing
>>    and egressing native frames...
> This seems to be difficult to implement: Implementing some defense
> mechanism is stated to be mandatory but the proposed defense is at
> best, best efforts.  It strikes me that given the expected
> catastrophic results of failure to have a robust defense, as
> delineated in the previous para, indicates that this either needs
> some better explanation or possibly a better defense.  I can't tell,
> but I guess we may be talking about some extraneous measure beyond
> this spec - please explain.

  The mandatory mechanism is explained in greatest detail in RFC
  6439. Perhaps the references to that RFC should be stronger and
  clearer. (It requires very peculiar and unlikely events for there to
  be a problem and, while such a problem will be bad, it will be
  transient - cleared up as soon as Hellos are exchanged on the link.)

> General: Adding titles/caption to figures and tables would be helpful.

  OK.

> General: s/byte/octet/

  I would agree that it is important for this to be consistent within
  the document. There seem to currently be 2 occurrences of "octet"
  and 12+ occurrences of "byte". Also, "byte" seems to be the preferred
  term for IS-IS TLV sub-field diagrams. So, I would prefer to change
  the two occurrences of "octet" to "byte".

> taking advantage of multiple paths in multi-hop networks with

  OK.

> s1, para 1: 'and a header that includes a hop count': A header for
> what?  Please make it clear what the header is added to.

  "a header" -> "a header in TRILL data packets"

> Suggest s/and optimization/as well as optimization/

  OK.

> in the abstract and terminology).

  OK.

> s1: Definition and explanation of 'pseudonode': There is no
> explanation of the pseudonode term until s7 - and even there it is
> buried a couple of paras down in the text.  A definition/explanation
> either in s1 or s1.2 would be helpful.

  "pseudonode" is a fundamental concept of IS-IS and I'm really not
  sure that it should be the role of this draft to explain it. I can
  add a reference to ISO 10589 after the first occurrence of
  "pseudonode" in Section 1.

> s2.1, para 1, sentence 2: This sentence is difficult to parse: After
> two or three reads, I realized that the material starting at
> 'devices or services' is not part of the 'includes' and ceased
> looking for an 'and'.  Suggest reversing the components of the main
> body thus:
> OLD:

  OK, I think that's an improvement.

> s2.4, para 1: s/reasonably restricted MTU/relatively restricted MTU/

  I think it really means that the restriction of MTU is reasonable,
  as opposed to unreasonable.

> expiring resulted in both being expired in the broadcast case.  Maybe
> reword a bit:
> OLD:

  There is yet another possibility: One timer could expire while the
  other is still in its initial expired state so it would be incorrect
  to speak about it having "expired previously". How about the
  following:

NEWER:

    A4. Either (1) the Hello holding timer expires on a point-to-point
        adjacency or (2), on a broadcast LAN adjacency, (2a) both
        Hello timers expire simultaneously or (2b) one Hello timer
        expires when the other Hello timer is already in the expired
        state.

> s5, para 3: Although it isn't wrong, given that RFC 2119 language is
> called out in s1.2, it might be better to explicitly use MUST
> instead of mandatory here.  MUST could also be used about the 1,470
> bytes (octets) minimum in para 2

  I don't particularly like using "MUST" here. The RFC 2119 MUSTs are
  in the TRILL base protocol specification, RFC 6325.

> s5, para 5: s/If it tests MTU/If it tests the MTU size/

  OK.

> s6, (not the) definition of BFD Enabled TLV: The figure shows a
> fixed value of 3 for the Length field and exactly one topology entry
> (2 octets) and a Protocol ID (1 octet).  The text indicates that
> there could be several, it isn't specific as to whether there has to
> be a match between topology and protocol entries but the length sort

  The BFD enabled TLV is specified in RFC 6213 that is referenced
  elsewhere in this draft but, curiously, not in Section 6. A
  reference should be added there.

> - must there be a matching number of them (the length text sort of

  Yes. The text should clarify this.

>   all topologies first and then all protocols?

    Matched pairs.

> - Where topology ID's come from?

    A few are pre-assigned but mostly they are made up by the
    network's management. See RFC 5120 that should probably be
    referenced here.

> - Where do NLPIDs come from?

    From ISO/IEC (see ISO9577), but there is a range available to
    other organizations, including the IETF. See RFC 6328 that is
    referenced here in the draft.

> eventually realized.

> on and using a broadcast LAN TRILL setup?   If so this should be clarified.

  How about replacing "... will actually be point-to-point, ..." with
  "... will actually be point-to-point even if the link technology
  supports operation as a multi-access broadcast link, ...".

> s8.2.1, para 1: s/should not be included/SHOULD NOT be included/

  OK.

Thanks,
Donald
=============================
 Donald E. Eastlake 3rd   +1-508-333-2270 (cell)
 155 Beaver Street, Milford, MA 01757 USA
 [email-masked]