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

From: Alia Atlas <[email-masked]>
Date: Thu, 6 Apr 2017 16:57:08 -0400

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.

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.

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

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.

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.

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".

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.

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.

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.

---

From: "Les Ginsberg (ginsberg)" <[email-masked]>
Date: Fri, 7 Apr 2017 00:30:45 +0000

Alia –

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

From: Alia Atlas [mailto:[email-masked]]
Sent: Thursday, April 06, 2017 1:57 PM
To: [email-masked]; [email-masked]
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
|  Pseudonode   |P|R|R|R|R|R|R|R| if P, sub-TLV 4, 6, or 12     |  Descriptor

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

---

From: Alia Atlas <[email-masked]>
Date: Thu, 6 Apr 2017 20:53:54 -0400

On Thu, Apr 6, 2017 at 8:30 PM, Les Ginsberg (ginsberg) <[email-masked]>
wrote:

> *From:* Alia Atlas [mailto:[email-masked]]
> *Sent:* Thursday, April 06, 2017 1:57 PM
> *To:* [email-masked]; [email-masked]
> *Subject:* AD review of draft-ietf-isis-l2bundles-03
>
>
>
> 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
> *[Les:] I will negotiate with the authors and get this addressed.*
>

[Alia] Thank you.

> *[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.*
>

Yes - you are absolutely right.  I was down in the details and forgot that
one could have different members of the same L2 Bundle advertised in
separate descriptors to make this work.   Could you add a simple example to
explain/show this?  I fell into the assumption that each LAG would be
represented as a single L2 bundle in the signaling, and of course, that
isn't necessary and this mechanism provides the needed flexibility.

> *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).*
>
>
>
> *[Les:] *Ack
> *[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.*
>
>
[Alia]  Right - when I started to write it out myself, I realized exactly
how annoying it was to do as a bit layout.
What do you think about the idea of describing it via the equivalent of a
structure or such?  I'm thinking towards pseudo-code.
Granted, I don't go stare at IS-IS code that frequently and I could
basically parse it out, but it wasn't trivial and I'd prefer the RFC to
be slightly clearer.  My request is better clarity - how you figure out to
accomplish that is not forced to the layout.

> defined in rfc4202 Section-2.1.
>
>
>
> What about instead
>
> 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,
>
> *[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.*
>

[Alia] Right - I was thinking that if one ran, say, BFD across each bundle
member, or even a ping with extensions, then the far-side ifIndex could be
learned
and that would be helpful for selecting bundle members that give
resiliency.  (For instance, having a bundle member on a different remote
line card for a particular
SR path that wants to be diverse from another selected SR path for
edge-to-edge diversity.)  Given your explanation earlier, I suppose this
could be done for
each member of the L2 bundle using the appropriate sub-TLV; it would just
be highly inefficient.

> *[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
> 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
>
> *[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
>
> *[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.*
>

[Alia] Right - I saw that difference.  What would be helpful would be to
have a reference to [SR] and say that the flags are defined as in [SR]
except for the following
changes....   Where the same flag definitions are repeated twice in this
draft and in [SR], I think it would be better to be extremely clear on the
differences.  The chance
for problems for changes in the future or folks assuming the same behavior
when there are differences is high.

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
> *[Les:] *Ack
> *   Les*

Thanks very much for the rapid (as always) response.

Regards,
Alia