Subject: [RTG-DIR] RtgDir review: draft-ietf-isis-trill-03.txt

From: mike shand <[email-masked]>
Date: Fri, 10 Dec 2010 14:49:56 +0000

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. The purpose 
of the review is to provide assistance to the Routing ADs. For more 
information about the Routing Directorate, please see 
http://www.ietf.org/iesg/directorate/routing.html

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-isis-trill-03.txt
Reviewer: Mike Shand
Review Date: 2010-12-06
IETF LC End Date: 2010-12-14
Intended Status: proposed standard

*Summary:*

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

*Comments: *

The document is well written and structured, and generally conforms to 
the conventional definition of IS-IS TLVs.

*Major Issues:*

No major issues found.

*Minor Issues:
*

2.2.1 The Special VLANs and Flags sub-TLV

AF, AC, VM, BY, and TR:

In previous reviews of IS-IS documents it has been suggested that bits 
such as these, as well as being identified by a letter or letters, 
should also have a descriptive name. While such a name could be inferred 
by the descriptive text provided, no unique nomenclature is called out here.

Personally, I don't believe this to be a problem.

2.2.2 Enabled-VLANs sub-TLV

If this sub-TLV occurs more than once in a Hello, the set of enabled
    VLANs is the union of the VLANs indicated by the Enabled-VLAN sub-
    TLVs.

This assumes that the start VLAN IDs of the multiple sub-TLVs are 
correctly aligned so that there is no overlap between the bit maps. In 
the unlikely event that this is not the case, what is the correct action?

a) ignore the IIH because it is malformed?

b) only do so if there is a VLAN which has the bit set in one sub-TLV 
and clear in another?

c) form the union of the SET bits, ignoring the unset bits?

The intent is probably (c), but this is not unambiguously clear.

2.3.4 The Tree Identifiers Sub-TLV

This is set to 1 for
       the first sub-TLV.  Subsequent sub-TLVs will have the starting
       number of the ordered list.

I assume "first" here does NOT imply that there is any required ordering 
for the presentation of the sub-TLVs within the PDU.

2.3.6 Interested VLANs and Spanning Tree Roots sub-TLV

Appointed Forwarder Status Lost Counter:

       It is
       initialized to zero at an IS when the LSP sequence number is
       initialized.

Since the sequence numbers of individual LSPs are independent, it would 
be possible for a "new" LSP to be generated
to carry additional information. Such an LSP would have its LSP sequence 
number "initialised".
It is not clear whether this text is supposed to relate to the 
initialisation of the sequence number of the LSP carrying this sub-TLV
or whether it refers to the initialization of the sequence number of the 
zeroth LSP. The latter seems to make more sense.

2.3.7 The VLAN Group sub-TLV

Length: 4 + 2*n, where n may be 0.

n is not specified (as it is in other cases of this form of description).

Presumably it is the number of secondary VLAN ID fields"

There is no textual description of the "more secondary VLAN IDs field". 
While it is fairly obvious what this is intended to be, it would be 
clearer and more consistent if this were described.

2.5 TRILL Neighbor TLV

S: Smallest flag.  If this bit is a one, then the list of
       neighbors includes the neighbor with the smallest MAC address.

Is it necessary to define what is meant by "smallest" and "largest" when 
applied to MAC addresses?

  RESV: These seven bits are reserved for future use and MUST be set
       to zero on transmission and ignored on receipt.

Elsewhere, such bits are simply described as being reserved, without any 
mention of future use.
Are these bits specifically required to be called out as different from 
other reserved bits?

Also, elsewhere, the phrase "MUST be sent as zero" is used rather than 
"MUST be set to zero on transmission".

Although there is "obviously" no intent that any difference is implied, 
the use of gratuitously different phraseology is best avoided.

in a TRILL Neighbor TLV with the L flag zero will also appear as a

Should that "will" be a "MUST"? And similarly for S flag

If an RBridge believes
    it has no neighbors, its MUST send an empty TRILL Neighbor TLV, which
    will have both the S and L bits on.

Rather than saying "empty", which implies no content at all, would it be 
clearer to say
" a TRILL Neighbor TLV containing no neighbor records"?

It would also help to specify in

Length: 1 + 9*n, where n is the number of neighbor records.

that n can take the value zero, as has been done elsewhere... since the 
absence of that statement here could imply that it is NOT permitted to 
be zero.

3. The MTU PDUs

I have previously commented on the unnecessary use of two PDU type code 
points for the probe and ack PDUs. Indeed the two can be distinguished 
by the probe having a zero value for the ACK source ID, and the ACK 
having a non-zero value.

Do we need to update the existing registries to specify that AUTH and 
PAD TLVs are permitted in these PDU types?

4.3 Protocols Supported

    MUST appear in
    TRILL IIH PDUs and fragment zero LSP PDUs.

ISO/IEC 10589 doesn't use the term "fragment zero", but uses the term 
"LSP number zero".

6.1 Allocations From Existing Registries

    This document creates two new IS-IS PDUs, namely the MTU-PROBE-PDU,
    and MTU-ACK-PDU, as described in Section 3.  IANA will assign new PDU
    types to these PDUs and reflect them in the PDU registry. [suggested
    values below]

       MTU-PROBE-PDU     Level-1 PDU Number: 23
       MTU-ACK-PDU       Level-1 PDU Number: 28

Why are these PDUs described as Level-1?
In any case whether or not they are restricted to level 1 is a matter for the definition (or indeed title) of the PDU.
I don't see that this information needs to be included in the registry
if indeed such a registry exists... I don't think it does).

This is the first time that a new PDU has been defined for IS-IS and all current definitions are contained within
ISO/IEC 10589 itself. ALL other changes to IS-IS PDU definitions have previously been restricted to TLVs (or sub-TLVs).

It is partially for this reason that it seems gratuitous to specify TWO new PDUs, where a single one would easily suffice.

In any case note that PDU 23 was assigned by DECnet PhaseV to an XID PDU type.
Although never specified in ISO/IEC 10589 itself, and probably never now seen in the wild it would be as well to avoid any
chance of a potential clash and assign the MTU_PROBE-PDU a different number.

*Nits:*

2.2.3 Appointed Forwarders sub-TLV

   1.

          +----------------------------+
          |   Appointee Nickname       |  (2 bytes)
          +----------------------------+
          | RESV |     Start.VLAN      |  (2 bytes)
          +----------------------------+
          | RESV |     End.VLAN        |  (2 bytes)
          +----------------------------+

      It would be helpful if this diagram were formatted like the other
      fields, indicating bit positions.

2.3.6 Interested VLANs and Spanning Tree Roots sub-TLV

    An INT-VLAN sub-TLV asserts that the information provided (multicast
    router attachment, appointed forwarder status lost counter, and root
    bridges), is the same for all VLANs in the range give.

TYPO: s/give/given/

or perhaps better "specified".

2.5 TRILL Neighbor TLV

If an RBridge believes
    it has no neighbors, its MUST send an empty TRILL Neighbor TLV, which
    will have both the S and L bits on.

TYPO: s/its/it/

3. The MTU PDUs

Again, use of the conventional form of diagram would be helpful.

4.2 Area Address

Again, the standard diagram form would be helpful

---

From: Donald Eastlake <[email-masked]>
Date: Tue, 14 Dec 2010 04:09:56 -0500

Hi Mike,

Thanks for this detailed review. See responses below.

On Fri, Dec 10, 2010 at 9:49 AM, mike shand <[email-masked]> 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. The purpose of the review
> is to provide assistance to the Routing ADs. For more information about the
> Routing Directorate, please see
> 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
> Summary:
> Comments:
> The document is well written and structured, and generally conforms to the
> conventional definition of IS-IS TLVs.
>
> Major Issues:
>
> No major issues found.
>
> Minor Issues:
>
> In previous reviews of IS-IS documents it has been suggested that bits such
> as these, as well as being identified by a letter or letters, should also
> have a descriptive name. While such a name could be inferred by the
> descriptive text provided, no unique nomenclature is called out here.
>

I am uncertain as to whether this needs to be changed. It would be
relatively easy to add a descriptive word or two or three for each of
these bits if desired.

> This assumes that the start VLAN IDs of the multiple sub-TLVs are correctly
> aligned so that there is no overlap between the bit maps. In the unlikely
> event that this is not the case, what is the correct action?
>
> b) only do so if there is a VLAN which has the bit set in one sub-TLV and
> clear in another?
>

Your interpretation of the intent is correct and the above wording
seems reasonably clear to me. Each sub-TLV identifies a set of
VLANs. If there are more than one of the sub-TLVs, you take the union
of these sets. How about if I add a few words so it reads as follows:

     "If this sub-TLV occurs more than once in a Hello, the set of
     enabled VLANs is the union of the sets of VLANs indicated by each
     the Enabled-VLAN sub-TLVs in the Hello."

> I assume "first" here does NOT imply that there is any required ordering for
> the presentation of the sub-TLVs within the PDU.

Yes, you assume correctly. Perhaps the following wording would be an
improvement:

    "This is set to 1 for the sub-TLV containing the first list. Other
    Tree-Identifiers Sub-TLVs will have the number of the starting
    list they contain."

> Since the sequence numbers of individual LSPs are independent, it would be
> possible for a "new" LSP to be generated

Yes, I agree. "the LSP" should be replaced by "the zeroth LSP".

> Presumably it is the number of secondary VLAN ID fields"

Yes, I suggest replacement wording:

   "Length: 4 + 2*n, where n is the number of secondary VLAN ID
   fields, which may be zero."

> While it is fairly obvious what this is intended to be, it would be clearer
> and more consistent if this were described.

OK, how about adding

   "o  more Secondary VLAN IDs: zero or more byte pairs with the top
       four bits an RESV field and the low 12 bits a VLAN-ID."

> applied to MAC addresses?

I'd be happy to add at the end of the entries of "S" and "L" the text
"considered as an unsigned integer".

> Also, elsewhere, the phrase "MUST be sent as zero" is used rather than "MUST
> be set to zero on transmission".
>
> Although there is "obviously" no intent that any difference is implied, the
> use of gratuitously different phraseology is best avoided.

I agree. I'm fine with changing this to "These seven bits are reserved
and MUST be sent as zero and ignored on receipt."

> Should that "will" be a "MUST"? And similarly for S flag

That would probably be an improvement.

> " a TRILL Neighbor TLV containing no neighbor records"?

How about replacing "an empty TRILL Neighbor TLV" with "a TRILL
neighbor TLV with an empty list of neighbor RECORDs".

> absence of that statement here could imply that it is NOT permitted to be
> zero.

OK, easy to add "which may be zero" at the end of that entry.

> points for the probe and ack PDUs. Indeed the two can be distinguished by
> the probe having a zero value for the ACK source ID, and the ACK having a
> non-zero value.

Well, actually what you said was

"Are two seperate PDUs really required. Would not a single PDU type
with a flag to indicate probe or ACK suffice?"

although I took that question to mean that you would prefer a single
MTU PDU. I responded with

"Yes, you could use a single PDU type but I think they way they are
currently specified fits somewhat better into the pattern of PDU
definitions for IS-IS... Does anyone else out there have an opinion on
this?"

Since no one else spoke up, I left it unchanged.

This seems to be a matter of taste. Why are the L1 and L2 variants of
existing IS-IS PDUs different PDUs? That could easily be encoded
elsewhere in the PDUs. You can push the bits around in various
ways. But I don't care that much about this.

Is it clear that zero is not a valid SystemID? If you use a MAC
address for SystemID, OUI 00-00-00 appears to be a valid OUI assigned
to Xerox Corporation. If I was to make them into one PDU, I'd probably
steal the high order bit of the Probe ID to indicate probe versus ack.

> Do we need to update the existing registries to specify that AUTH and PAD
> TLVs are permitted in these PDU types?

I do not think that is necessary at this time.

> ISO/IEC 10589 doesn't use the term "fragment zero", but uses the term "LSP
> number zero".

OK, I'm fine with that change in wording.

> Why are these PDUs described as Level-1?

I have no idea. They are pretty clearly Level independent. Someone
added this at some point and it just got copied from draft to draft
under various draft titles and I failed to notice it when putting this
draft together.

> In any case whether or not they are restricted to level 1 is a matter for
> the definition (or indeed title) of the PDU.

Yes, IANA has already commented on this. Attached is suggested changed
text for the draft I created in response to IANA's and Steward Bryant's
comments.

> ISO/IEC 10589 itself. ALL other changes to IS-IS PDU definitions have
> previously been restricted to TLVs (or sub-TLVs).
>
> It is partially for this reason that it seems gratuitous to specify TWO new
> PDUs, where a single one would easily suffice.

Well, we could guarantee that the IETF would only use one new PDU ever
by having an "IETF PDU" and having a sub-PDU number or
something. Would that be a good idea?

It looks to me like there are 32 possible PDUs and the ISO standard
only specifies 9. So it is not like we are about to run out. I
continue to believe that having separate MTU-probe and MTU-ack PDUs is
consistent with the existing IS-IS PDU granularity but would agree
this is a matter of taste so I'm willing to change it to one PDU if
that's what people want.

> Although never specified in ISO/IEC 10589 itself, and probably never now
> seen in the wild it would be as well to avoid any

I have no idea where the suggested PDU numbers 23 and 28 came from and
I don't care what number or numbers are assigned, although I note that
23 and 28 are unused in the ISO standard. These numbers have just been
copied forward from draft to draft under various draft names.

> Nits:
> It would be helpful if this diagram were formatted like the other fields,
> indicating bit positions.

OK.

> or perhaps better "specified".

Sure, thanks.

>    will have both the S and L bits on.

Right,

> Again, use of the conventional form of diagram would be helpful.

OK.

> Again, the standard diagram form would be helpful

OK.

Thanks again for the review,
Donald

---

From: mike shand <[email-masked]>
Date: Tue, 14 Dec 2010 13:42:19 +0000

Donald,

     See my responses below.

     Mike

On 14/12/2010 09:09, Donald Eastlake wrote:
> On Fri, Dec 10, 2010 at 9:49 AM, mike shand<[email-masked]>  wrote:
>> Hello,
>>

Yes, how about

"If this sub-TLV occurs more than once in a Hello, the set of
      enabled VLANs is the union of the sets of enabled VLANs indicated by each of
      the Enabled-VLAN sub-TLVs in the Hello."

>      Tree-Identifiers Sub-TLVs will have the number of the starting

OK

> Yes, I agree. "the LSP" should be replaced by "the zeroth LSP".

OK

>     fields, which may be zero."

OK

>         four bits an RESV field and the low 12 bits a VLAN-ID."
OK

> "considered as an unsigned integer".
OK
> and MUST be sent as zero and ignored on receipt."

OK
> That would probably be an improvement.

OK
> neighbor TLV with an empty list of neighbor RECORDs".

OK

> OK, easy to add "which may be zero" at the end of that entry.
OK

> steal the high order bit of the Probe ID to indicate probe versus ack.
Clearly it works either way, so if nobody else cares I won't press it.

> I do not think that is necessary at this time.

I do think that needs to be done at some point.

> OK, I'm fine with that change in wording.

OK
> something. Would that be a good idea?
No, I don't think that is necessary. We can always do that later if we 
do seem to be running out.

In fact the restriction to 32 is not necessary. It was only for 
consistency with the format of an ISO 8473 data PDU,
where the top 3 bits were "segmentation permitted", More segments", and 
"Error report requested".
Since they use different NLPIs there is no actual confusion, so those 3 
bits are spare. They are of course currently
defined as "MUST be ignored on receipt".

In any case, I see no need to take any action now.

     Mike