Re: [spring] Robert Wilton's No Objection on draft-ietf-spring-srv6-network-programming-19: (with COMMENT)

["Pablo Camarillo (pcamaril)" <[e-mail]>]

Hi Rob,

Thank you for your review. Please see inline.

Regards,
Pablo.

-----Original Message-----
From: Robert Wilton via Datatracker <[e-mail]> 
Sent: martes, 22 de septiembre de 2020 11:15
To: The IESG <[e-mail]>
Cc: [e-mail]; [e-mail]; [e-mail]; Bruno Decraene <[e-mail]>; Joel Halpern <[e-mail]>; [e-mail]
Subject: Robert Wilton's No Objection on draft-ietf-spring-srv6-network-programming-19: (with COMMENT)

Robert Wilton has entered the following ballot position for
draft-ietf-spring-srv6-network-programming-19: No Objection

When responding, please keep the subject line intact and reply to all email addresses included in the To and CC lines. (Feel free to cut this introductory paragraph, however.)


Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.


The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-spring-srv6-network-programming/



----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

Hi,

Thank for this document.  Not surprisingly given the large number of contributors working on this document I only have a few minor comments:

    2.  Terminology

       NH: Next-header field of the IPv6 header [RFC8200].  NH=SRH means
       that the next-header of the IPv6 header is Routing Header for
       IPv6(43) with the Type field set to 4.

Suggest expanding "4" to SRH(4)
[PC] Ack. Actually the terms NH and SRH[n] are not used throughout the entire document, hence I believe those two can be removed.

    10.1.  Ethernet Next Header Type

       This document requests IANA to allocate, in the "Protocol Numbers"
       registry (https://www.iana.org/assignments/protocol-numbers/protocol-
       numbers.xhtml), a new value for "Ethernet" with the following
       definition: The value 143 in the Next Header field of an IPv6 header
       or any extension header indicates that the payload is an Ethernet
       [IEEE.802.3_2012].

Should this point to the latest published version of 802.3 (2018), or does it specifically need to reference an older version?
[PC] I’ll update in next rev.

    3.1.  SID Format

       This document defines an SRv6 SID as consisting of LOC:FUNCT:ARG,
       where a locator (LOC) is encoded in the L most significant bits of
       the SID, followed by F bits of function (FUNCT) and A bits of
       arguments (ARG).  L, the locator length, is flexible, and an operator
       is free to use the locator length of their choice.  F and A may be
       any value as long as L+F+A <= 128.  When L+F+A is less than 128 then
       the remainder of the SID MUST be zero.

This was reasonably clear to me, but since the rest of the description refers to bits, then I wonder whether the last sentence would be more precise as "the remaining bits of the SID must be set to zero"?
[PC] Indeed. Fixed for next rev.

    4.1.  End: Endpoint
       S09.   If ((Last Entry > max_LE) or (Segments Left > Last Entry+1)) {

It wasn't entirely apparent to me where 'Last Entry' comes from, although that became more clear when I saw its usage later.  Possibly it would be helpful to list "Segments Left" and "Last Entry" in the terminology as taken from RFC 8754?
[PC] Good point. Fixed for next rev.

    4.1.  End: Endpoint
       S12.   Decrement Hop Limit by 1

Would it be more clear to expand this to say "Decrement IPv6 Hop Limit by 1", since it is referred to as the IPv6 Hop Limit previously.  If this is changed, then there are a couple of other places in the document that should be checked as well.
[PC] Indeed. I’ll fix in all pseudocodes (not only 4.1)

    4.2.  End.X: Layer-3 Cross-Connect

       Note that if N has an outgoing interface bundle I to a neighbor Q
       made of 10 member links, N may allocate up to 11 End.X local SIDs:
       one for the bundle(LAG) itself and then up to one for each Layer-2
       member link.

This behaviour seems slightly unusual to me.  I presume that the intent here is for the SR program to control which member link the traffic travels over rather than the normal behaviour of relying on the Bundle hashing.  Possibly adding a bit more text to clarify the expected behaviour might be helpful.
[PC] We can add the following text to clarify.
<OLD>
       Note that if N has an outgoing interface bundle I to a neighbor Q
       made of 10 member links, N may allocate up to 11 End.X local SIDs:
       one for the bundle(LAG) itself and then up to one for each Layer-2
       member link. 
</OLD>
<NEW>
       Note that if N has an outgoing interface bundle I to a neighbor Q
       made of 10 member links, N may allocate up to 11 End.X local SIDs:
       one for the bundle(LAG) itself and then up to one for each Layer-2
       member link. The flows steered using the End.X SID corresponding to the bundle(LAG) itself get load balanced across the member links via hashing while the flows steered using the End.X SID corresponding to a member link get steered over that specific member link alone.
</NEW>

    4.4.  End.DX6: Decapsulation and IPv6 Cross-Connect

       S05.  If the End.DX6 SID is bound to an array of L3 adjacencies, then
       one entry of the array is selected based on the hash of the packet's
       header Section 7.

The end of this sentence doesn't scan for me, probably it should be something end something like: ", see Section 7"
[PC] Ack. Fixed for next rev.

    6.  Counters

       A node supporting this document SHOULD implement a combined traffic
       counter (packets and bytes) per local SID entry, for traffic that
       matched that SID and was processed correctly.  The retrieval of these
       counters via MIB, NETCONF/YANG or any other means is outside the
       scope of this document.

>From my reading, a combined counter implies a single counter that is
incremented by both the number of packets processed and number of bytes processed.  Normally in the management models you would normally expect this to be modeled as a pair of counters, and I presume that is what is desired here? 
If so I would suggest clarifying this text to indicate that it is a pair of counters (one for packets, one for bytes) that are required here.
[PC] Agree. I’ll change to a pair of counters.

    Section 4, various places.

At various points the Upper-Layer Header type is compared to a number (e.g. 4, 41, 143).  It might be a bit more readable if the associated names were also included in the description.  E.g., 4(IPv4), 41(IPv6), etc.
[PC] Ack. Fixed for next rev.

    4.10.  End.DX2V: Decapsulation and VLAN L2 Table Lookup

Is this lookup just on the outermost VLAN Id, or all VLAN tags in the exposed Ethernet header?  Or is this implementation specific?
[PC] All VLAN tags. In the pseudocode we have “exposed VLANs in L2 table T”

Regards,
Rob