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

[Alia Atlas <[e-mail]>]

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

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

[Alia] Thank you.


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

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


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

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


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

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

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

Regards,
Alia