Subject: [trill] AD review of draft-ietf-trill-rfc7180bis

From: Alia Atlas <[email-masked]>
Date: Fri, 25 Sep 2015 12:12:54 -0400

As is customary, I have done my AD review of
draft-ietf-trill-rfc7180bis-05.  First, I'd like to thank the authors
Donald, Mingui, Radia, Ayan, Anoop, and Sujay for their work on this.

Due to some oddity in the datatracker, I didn't learn that the WG had
requested publication until I got a private email.  I apologize for the
delay in review.

Below are my comments.  The only one that is really blocking for starting
IETF Last Call is about Appendix C.   If we can get the comments resolved
and a new draft issued before next Thursday, then this draft should be able
to make the Oct 22 telechat.

1)In Sec 8.2: " Control packets can be ordered into priority classes as
shown below.
   Although few implementations will actually treat all of these classes
   differently, lower numbered classes SHOULD NOT be treated as lower
   priority than higher numbered class."

  Can you clarify what the point of listing these different priority
classes, if the lower number classes aren't lower priority & treated that
way??

2) Sec 8.4:  I would really like to see an explanation for why the Nickname
Flags APPSub-TLV is needed.  I see the description and efforts to clarify
edge cases, which is great, but nothing that says "This is the problem a
Nickname Falgs APP sub-TLV is solving and the types of problems additional
flags might solve in the future."

3) In Sec 10.2.1.3: "   3. If the Hop Count is zero, it is set to the
maximum value of 63 and
      the extended Hop Count is decremented."  What happens to the Critical
Bit Summary when the extended Hop Count goes to 0?  I don't see any mention
of it after the ingress RBridge setting it.

4) In Sec 10.2.2: "The meaning of the possible non-zero values of this
2-bit field (1, 2 or 3) is implementation dependent."  Is this assuming
that the zero value has a specific meaning of being unset or something
else?  Please clarify.

5) In Sec 10.3: "Finally,   there is the Non-Critical Ingress-to-Egress
field, the top two bits
   of which are specified herein as the Extended Color field."  The figure
shows bits 27 and 28 of the word, which are the low bits of the field - not
the top 2 bits.

5) Appendix C makes me uncomfortable in a Proposed Standard RFC.  Either
you are loosening the restrictions in a normative way or not...  This reads
like an "implementation hints to not quite conform".  Can you and the WG
figure out a better plan?

Thanks again,
Alia

---

From: Donald Eastlake <[email-masked]>
Date: Sun, 27 Sep 2015 22:55:18 -0400

Hi Alia,

Thanks for the review. See below.

On Fri, Sep 25, 2015 at 12:12 PM, Alia Atlas <[email-masked]>
wrote:
> draft-ietf-trill-rfc7180bis-05.  First, I'd like to thank the
> authors Donald, Mingui, Radia, Ayan, Anoop, and Sujay for their work
> on this.
>
> Due to some oddity in the datatracker, I didn't learn that the WG
> had requested publication until I got a private email.  I apologize
> for the delay in review.
>
> Below are my comments.  The only one that is really blocking for
> starting IETF Last Call is about Appendix C.  If we can get the
> comments resolved and a new draft issued before next Thursday, then
> this draft should be able to make the Oct 22 telechat.
>
> 1) In Sec 8.2: " Control packets can be ordered into priority
>    classes as shown below.  Although few implementations will
>    actually treat all of these classes differently, lower numbered
>    classes SHOULD NOT be treated as lower priority than higher
>    numbered class."
>   classes, if the lower number classes aren't lower priority &
>   treated that way??

With this draft, a new class of IS-IS PDUs, the Flooding Scoped PDUs
[RFC7356], is added to the repertoire used by TRILL. The TRILL base
protocol [RFC6325] just says that all IS-IS PDUs SHOULD be sent with
the maximum priority 7. But as additional data being sent is somewhat
more "application" and less core routing between TRILL switches
oriented, particularly APPsub-TLVs being sent in FS-LSPs, some
implementations might want to send some IS-IS PDUs at lower
priority. This section provides some guidance.

If you don't like the direction of the priority numbering, which I
admit is inconsistent with priority field values in TRILL, I don't see
any problem in renumbering the classes so the highest priority has the
highest number. Or we could use Roman numeral or something :-)

> 2) Sec 8.4: I would really like to see an explanation for why the
> Nickname Flags APPSub-TLV is needed.  I see the description and
> efforts to clarify edge cases, which is great, but nothing that says
> "This is the problem a Nickname Flags APP sub-TLV is solving and the
> types of problems additional flags might solve in the future."

There are a number of anticipated additional uses including specific
Nickname Flags bits defined in draft-ietf-trill-irb (currently in WG
Last Call) and draft-ietf-trill-centralized-replication.  These drafts
are normatively dependent on rfc7180bis. In my opinion, the IS-IS
Nickname sub-TLV (originally specified in RFC 6326, which has been
obsoleted by RFC 7176) would have been improved by having some flags
associated with each nickname. The use specified in this draft could
have been included in the base TRILL specification, RFC 6325, if we
had thought of it at the time.

We could make a change such as:

OLD
   Additional flags may be assigned for other purposes out of the RESV
   field in the future.

NEW
   (Additional flags are assigned from those labeled RESV above and
   specified in draft-ietf-trill-irb and
   draft-ietf-trill-centralized-replication, works in progress.)

> maximum value of 63 and the extended Hop Count is decremented."
> What happens to the Critical Bit Summary when the extended Hop Count
> goes to 0?  I don't see any mention of it after the ingress RBridge
> setting it.

Right. If the extended Hop Count goes to zero, the Critical Reserved
bit MUST also be set to zero. Arguably this is implied by RFC 7179 but
it would be good to state here. Item 3 should be extended something
like the following:

OLD
   3. If the Hop Count is zero, it is set to the maximum value of 63 and
      the extended Hop Count is decremented.

NEW
   3. If the Hop Count is zero, it is set to the maximum value of 63 and
      the extended Hop Count is decremented. If this results in the
      extended Hop Count being zero, the Critical Reserved bit in the
      Criticial Summary bits is set to zero.

> 4) In Sec 10.2.2: "The meaning of the possible non-zero values of
> this 2-bit field (1, 2 or 3) is implementation dependent."  Is this
> assuming that the zero value has a specific meaning of being unset
> or something else?  Please clarify.

Sure.

Bits 27 and 28 in the "Capabilities and Header Flags Supported" field
of the TRILL Version Sub-TLV are already defined to specify capability
support in connection with bits 27 and 28 of the TRILL Header Flag
Word in TRILL Data packets. This draft defines that pair of bits in
the TRILL Header as an Extended Color field but, other than requiring
that transit TRILL switches be transparent to them, doesn't say much
else. So actual use is implementation dependent.

If not supported by a TRILL switch, that TRILL switch will set those
capapbility bits and those Flag Word bits in any TRILL Data packet
whose TRILL Header has the optional Flag Word to zero and will ignore
any of these four bits that it sees as non-zero in the Link State for
capabilities or in a received TRILL Data packet for the header flags.

So, it Extended Color is supported, it seems reasonable that the bit
27-28 field in the capabilities should be non-zero. The current text
makes this a SHOULD but I don't think it would be a big deal to make
it a MUST. However, it seems better to leave just which non-zero
capability value (1, 2, or 3) to the implementation which might
want to signal something through this value.

How about the following change:

OLD
   As provided in Section 2.3.1 of [RFC7176], support for these bits is
   indicated by the same bits (27 and 28) in the Capabilities and Header
   Flags Supported field of the TRILL Version Sub-TLV. In the spirit of
   indicating support, a TRILL switch that sets or senses the Extended
   Color field SHOULD set the corresponding 2-bit field in the TRILL
   Version Sub-TLV non-zero. The meaning of the possible non-zero values
   of this 2-bit field (1, 2 or 3) is implementation dependent.

NEW
   As provided in Section 2.3.1 of [RFC7176], support for these bits
   is indicated by the same bits (27 and 28) in the Capabilities and
   Header Flags Supported field of the TRILL Version Sub-TLV. If these
   bits are zero in those capabilies, Extended Color is not
   supported. A TRILL switch that does not support Extended Color will
   ignore the corresponding bits in any TRILL Header Flag Word it
   receives as part of a TRILL Data Packet and will set those bits to
   zero in any TRILL Header Flag word it creates. A TRILL switch that
   sets or senses the Extended Color field on transmitting or
   receiving TRILL Data packets MUST set the corresponding 2-bit field
   in the TRILL Version Sub-TLV non-zero. Any difference in meaning of
   the three possible non-zero values of this 2-bit capability field
   (0b01, 0b10 or 0b11) is implementation dependent.

Also, in the 2nd paragraph of 10.2.2, "this bit" -> "these bits" and
"Color bit" -> "Extended Color bits". (Looks like this paragraph was
copied from 10.1 but only partially adjusted for the new context.)

> 5) In Sec 10.3: "Finally, there is the Non-Critical
> Ingress-to-Egress field, the top two bits of which are specified
> herein as the Extended Color field."  The figure shows bits 27 and
> 28 of the word, which are the low bits of the field - not the top 2
> bits.

The diagram assumes big-endian (network order) bit ordering. Bit 0 is
the top / highest / sign bit. I think higher order is left and lower
order is right...

> 5) Appendix C makes me uncomfortable in a Proposed Standard RFC.
> Either you are loosening the restrictions in a normative way or
> not...  This reads like an "implementation hints to not quite
> conform".  Can you and the WG figure out a better plan?

OK, that's a reasonable criticism.

There are two kinds of comments in Appendix C (which, as noted, came
from a question on the list by an implementer). First, it lists some
cases where the Appointed Forwarder Status Lost Counter is not useful
and there is no harm and some possible gain from just leaving it zero
or setting it to a value matching that for an adjacent VLAN
ID. Second, it describes the effect of "incorrectly" setting/changing
that counter and strongly implies that the tradeoffs in a real-world
implementation might favor such behavior.

For those cases where the Appointed Forwarder Status Lost Counter has
no effect, the new normative guidance could be that it MAY be set to
any value. For other cases, the mandate in Section 4.8.3 of RFC 6325
could be change to SHOULD. To make this into normative changes to RFC
6325, it should probably be made into a new Section 11 with existing
sections 11 and 12 re-numbered to 12 and 13. Attached is such a
possible new Section 10. (Appendix D would also become Appendix C and
would need to list this change.)

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