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

From: Alia Atlas <[email-masked]>
Date: Mon, 5 Oct 2015 12:26:45 -0400

Hi Donald,

On Sun, Sep 27, 2015 at 10:55 PM, Donald Eastlake <[email-masked]> wrote:

> Hi Alia,
>
> Thanks for the review. See below.
>
> On Fri, Sep 25, 2015 at 12:12 PM, Alia Atlas <[email-masked]>
> wrote:
> > 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??
>
> 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 :-)
>

Roman numerals or alphabetic would help.  My key point, though,
is that it's confusing to say here's the prioritization but implementations
SHOULD NOT treat them differently.  That's not guidance on how they
could be treated differently.

Perhaps a "this is the relative criticality of these types of messages,
where
the most critical relate to the core routing between TRILL switches and the
less critical closer to "application" information.  An implementation MAY
choose to prioritize more critical messages over less critical.  However,
for .... an implementation SHOULD NOT ...."

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

Sounds good to me.  Make sure to add them to the Informative References.

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

Are there cases where the Critical Summary bit might remain set for a
reason other than the extended Hop Count?

Otherwise, LGTM.

>
> > 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.)
>
> LGTM

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

Sure - but having bits 27/28 of 27-31 described as the top two bits is
potentially confusing.  How about just naming the bits?
Not a big deal either way.

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

LGTM

Thanks,
Alia

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

---

From: Donald Eastlake <[email-masked]>
Date: Mon, 5 Oct 2015 16:04:27 -0400

Hi Alia,

Just responding where you have any additional comment or suggestion:

On Mon, Oct 5, 2015 at 12:26 PM, Alia Atlas <[email-masked]> wrote:
> Roman numerals or alphabetic would help. My key point, though, is
> that it's confusing to say here's the prioritization but
> implementations SHOULD NOT treat them differently. That's not
> guidance on how they could be treated differently.

What it was trying to say is that you "SHOULD NOT" send lower priority
cateogry packets in preference to higher priority category packets.

> Perhaps a "this is the relative criticality of these types of
> messages, where the most critical relate to the core routing between
> TRILL switches and the less critical closer to "application"
> information. An implementation MAY choose to prioritize more
> critical messages over less critical. However, for .... an
> implementation SHOULD NOT ...."

Sure, we can go for wording like that.

> Sounds good to me.  Make sure to add them to the Informative References.

Will do.

> reason other than the extended Hop Count?

Not for that particular Critial Summary bit.

> Otherwise, LGTM.

...

Thanks,
Donald

---

From: Alia Atlas <[email-masked]>
Date: Mon, 5 Oct 2015 16:09:30 -0400

Excellent - if you can get a new draft in, I'll put it in for IETF Last
Call.
It'll barely make it for the Oct 22 telechat.

Thanks,
Alia