Re: [Isis-wg] AD review of draft-ietf-isis-l2bundles-03

["Les Ginsberg (ginsberg)" <[e-mail]>]

Alia –

Thanx for the (as always) careful review.
Comments inline.

From: Alia Atlas [mailto:[e-mail]]
Sent: Thursday, April 06, 2017 1:57 PM
To: [e-mail]; [e-mail]
Subject: AD review of draft-ietf-isis-l2bundles-03

As is customary, I have done my AD review of draft-ietf-isis-l2bundles-03.   First, I would like to thank the authors - Les, Ahmed, Clarence, Stefano, Mohan and Ebben - for their work on this document.

Based upon the urgency expressed at IETF 98 for this document, of course, I would very much like to get it moving through the IETF process as rapidly as possible.  Regrettably, the document has 6 authors which is above the limit.  I would welcome either having a subset selected as the editor(s) or reducing the authorship to no more than 5.   It is most irksome to have to serve as the enforcement arm in this matter with authors and WG Chairs who have long experience with the IETF publication process and are surely aware and able to handle this issue without waiting for AD enforcement.

[Les:] I will negotiate with the authors and get this addressed.

Assuming that the author limit issue is rapidly resolved, I do also have a number of technical concerns and minor improvements.  None of these are expected to be blocking - but they do need to be resolved.    I am targeting this for the IESG telechat on April 27 which means that it needs to ideally enter into IETF Last Call tomorrow to allow adequate time for the IESG and directorate review.   Knowing the speed with which some of the authors respond, I am confident that this is possible.

As discussed at IETF 98, I will call attention to this draft and that it will be entering IETF Last Call to WGs (rtgwg, ospf, idr) that may be interested.

Major:

a) The assumption that bandwidth is only a shared attribute (and similarly weight in the new Adj-SID sub-TLV) is very concerning.  While I believe that standardized LACP only supports the same link speeds, I know that there are some routers and switches from different vendors that do allow different speeds.  This functionality is something that could meet some of the needs for RFC 7226 where the composite-links are assumed to have different speeds.   A solution to this for the bandwidth, for instance, could be to have a section describing how to take a previously defined sub-TLV and then modify it into a general sub-TLV that can apply to the L2 bundle members.  The draft even has an existing example of doing this for the Adj-SID sub-TLV.

[Les:] You have misinterpreted text.
There are two categories of sub-TLVs:

1)Those that can be shared among multiple bundle members
As stated in the draft this includes

“All existing sub-TLVs defined in the IANA Sub-TLVs for TLVs 22, 23,
   141, 222, and 223 registry are in the category of shared attribute
   sub-TLVs unless otherwise specified in this document.”

NOTE: NO exceptions are specified

2)Those that cannot be shared among multiple bundle members. These include the new sub-TLVs defined:

L2 Bundle Member Adjacency Segment Identifier sub-TLV
L2 Bundle Member LAN Adjacency Segment Identifier sub-TLV

And the draft states:

“The two new sub-TLVs defined in the following sections do not fall
   into the category of shared attribute sub-TLVs.”

The fact that an attribute CAN be shared does not mean that it MUST be shared.
If we have links with different bandwidths in a bundle then multiple sets of bundle members (i.e. multiple  L2 Bundle Attribute Descriptors) are used, one for each set of bundle members which share a given (set of) attributes.

Weight in the ADJ-SID is treated in the same way except that weight isn’t a sub-TLV on its own – rather we follow the adj-sid format for L3 links in draft-ietf-isis-segment-routing-extensions – but again if there are bundle members with different weights then they are advertised using multiple L2 Bundle Attribute Descriptors.

In the course of writing the draft we evaluated multiple options for the encoding.

The simplest encoding is to advertise attributes per L2 bundle member – but this is very inefficient as it can be expected that a given attribute (bandwidth is a good example) is very likely to be common for many bundle members.

The most efficient encoding would have involved mixing shared and unshared attributes under a single L2 Bundle Attribute Descriptor – but this proved to be overly complex.

We chose a middle ground where we group bundle members into a group that shares attributes (adj-sids being the exception case).

b)  draft-ietf-isis-segment-routing-extensions needs to be a Normative reference.

[Les:] Ack

Minor:

1) In Sec 2, the definition of the TLV would be greatly improved with a figure that shows the actual layout.   Take a look at RFC5308 for a decent example.  It takes careful reading to figure out what layouts are possible.  I think that it is something like:

 0                   1                   2                   3
 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
| Type 25?      |  Length       |   Neighbor System ID          |  Parent L3
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|       Neighbor System ID continued                            |  Neighbor
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|  Pseudonode   |P|R|R|R|R|R|R|R| if P, sub-TLV 4, 6, or 12     |  Descriptor
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
~  if P, sub-TLV 4, 6, or 12                                     ~
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

etc.   For instance, I can't tell whether the "L2 Attribute Parent Descriptor"
is more than just a length octet plus the nested contents.  I assume that is all,
but it would be much better to see it written out.  If doing a layout figure is too
painful - then a more detailed (think c-structure with bit-fields) description would
help.  It shouldn't take cross-referencing different sections to figure out the bits.

[Les:] Sigh…I tried this and found the result unsatisfactory – too many “…” embedded in too many places to make it helpful.
I’ll give it another try – but if I still don’t like it …

As regards "L2 Attribute Parent Descriptor" the text states (follow the indenting):

Parent L3 Neighbor Descriptor
        L3 Neighbor System ID + pseudonode ID (7 octets)
        Flags: 1 octet field of following flags:

There is no length here – just descriptor and flags.
Length is not needed because:

NOTE: Only one Parent L3 Neighbor Descriptor is present in a given
   TLV.  Multiple L2 Bundle Attribute Descriptors may be present in a
   single TLV.


2) In Sec 2, it states " NOTE: An L2 Bundle Member Descriptor is a Link Local Identifier as defined in [RFC5307]."
but RFC5307 merely uses the Link Local Identifier and specifies that it is defined in rfc4202 Section-2.1.

What about instead
"NOTE: An L2 Bundle Descriptor is a Link Local Identifier as defined in Section 2.1 of [RFC4202] and used as a value in
sub-TLV type=4 defined in [RFC5307]."   That makes it clearer.  I feel there is some lack of clarity because what is defined in
RFC5307 and - without a clear bit layout - it takes some assumptions (looking at the length computation) to be clear that what
is included for the L2 Bundle Member Descriptor isn't a sub-TLV type=4, but rather just the 4 octet local link identifier."  Frankly,
I'm not positive why it wasn't deemed useful to also include the Link Remote Identifier for each L2 member; clearly correlating
will be non-trivial, but understanding that the draft is implemented, I am merely requested a bit of explanatory text also in the draft.

[Les:] I think changing the reference to RFC 4202 is all that is required.
As regards the remote identifier, we do not know what it is per bundle member – the protocol only knows the remote identifier for the L3 link – not the L2 bundle members.

3) In Sec 2.2: "If multiple copies of a given  sub-TLV are present both MUST be ignored."   Substitute "all copies" instead of "both".
[Les:] Agreed.

4) In Sec 2.2: "All existing sub-TLVs defined in the IANA Sub-TLVs for TLVs 22, 23,   141, 222, and 223 registry are in the category of shared attribute
   sub-TLVs unless otherwise specified in this document."   This is not a useful statement for going forward as an RFC.   Do you mean
"At the time this document is published, all sub-TVLs are shared attribute sub-TLVs unless otherwise specified in this document.  For future sub-TLVs
to not be considered shared attribute sub-TLVs, this MUST be explicitly defined when those sub-TLVs are defined?"  Or do you intend to add a field
to the IANA registry indicating whether it is not a shared attribute - so that the question is asked every time a new sub-TLV is added?  Or do you mean
that there will be no need for sub-TLVs that aren't shared attribute except as in this document (reasoning please)?    This document needs to provide
sufficient information to be sustainable.

[Les:] Yes – this categorization would have to be incorporated into the IANA registry to make us future-proof.
Good catch.


5) In Sec 3.1 and Sec 3.2: It looks like the flags definitions are intended to be the same as in [draft-ietf-isis-segment-routing-extensions, Sec 2.1.1].  If so, then please
say so and simply refer to  [draft-ietf-isis-segment-routing-extensions, Sec 2.1.1] instead of redefining them!

6) In Sec 3.1 and Sec 3.2:  For " L2 Bundle Member Adj-SID Descriptors." - first it looks like the details are duplicating definition in [SR].  Again a direct reference with - at most - quoting is better.   Second, please add text on how to associate each of the Adj-SID Descriptors with the L2 Bundle members.  YES - I realize that it is probably assuming a consistently ordered list and 1-to-1 mapping, but there is no text that says so.

[Les:] Although we are trying to be as consistent as we can be to the L3 counterparts, encoding is NOT identical. Not all flags defined for L3 are applicable for L2 – though we did try to use the same bit values in cases where the flags provide the same functionality. Hence:

* - Is a flag used in the L3 Adj-SID sub-TLV but which is NOT
           used in this sub-TLV. These bits SHOULD be sent as 0 and MUST
           be ignored on receipt

So we cannot simply reference L3 draft.
I can add a statement that the flags – whenever possible – share bit positions with their L3 counterparts.




Nits:

1) In Sec 1: "In order to do so link attribute information about individual bundle members is required - but currently IS-IS only supports advertising link
   attributes for the Layer 3 interfaces on which it operates."
   Rephrase "currently" to "without the TLV defined in this document" or something equivalent.   Time to rephrase for it being an RFC.

[Les:] Ack

   Les