Hi Francesca,
thanks for your review, please see my responses inline (##PP):
On 18/10/2021 23:57, Francesca Palombini via Datatracker wrote:
Francesca Palombini has entered the following ballot position for
draft-ietf-lsr-isis-srv6-extensions-17: Discuss
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/blog/handling-iesg-ballot-positions/
for more information about how to handle DISCUSS and COMMENT positions.
The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-lsr-isis-srv6-extensions/
----------------------------------------------------------------------
DISCUSS:
----------------------------------------------------------------------
Thanks for the work on this document.
I was not around for RFC 8986, and I am not sure I understand the use case
fully (I agree with Ben there), but I'll trust the responsible AD and the wg.
I'll also note that I was hoping to see "Implementation status report" in the
draft, as mentioned in the shepherd writeup, and was disappointed not to find
any.
##PP
there was one in the earlier version of the document, but it was removed
as part of the process to prepare for publication. Please look at
version 11:
https://datatracker.ietf.org/doc/html/draft-ietf-lsr-isis-srv6-extensions-11
However, I'd like to discuss a number of points, mostly on the IANA
considerations and on detailed fields descriptions. I also want to bring 7.
below regarding the IANA registries names to your attention, although it's not
a hill I am willing to die on (that one is a "let's talk" DISCUSS, the rest I
hope can be acted upon).
As noted in https://www.ietf.org/blog/handling-iesg-ballot-positions/, a
DISCUSS ballot is a request to have a discussion; I really think that the
document would be improved with a change here, but can be convinced otherwise.
Francesca
1. -----
Section 7.1
FP: The locator entries ASCII figure is not consistent with the descriptive
text following it: specifically, Loc Size should follow Algorithm directly;
instead, the picture seems to show there are 2 octets unused between Algorithm
and Loc Size.
##PP
there isn't anything between the "Algorithm" and "Loc Size" fields. If
there was, there would have been a "|" at the end of the two bytes extra
section. ISIS TLVs are not 4 bytes aligned, so it's sometimes tricky to
draw them in a 4 bytes boundary. We have been using this style in other
documents:
Example:
https://datatracker.ietf.org/doc/html/rfc8667#section-3.1
I can offer two alternatives, if you like them better:
a) divide to multiple blocks:
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
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
| Metric |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
| Flags | Algorithm |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
| Loc Size | Locator (variable)... |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
| Sub-TLV-len | Sub-TLVs (variable) . . . |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
b) fully packed to 4 bytes boundary:
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
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
| Metric |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
| Flags | Algorithm | Loc Size | Locator ...
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
// Locator (continued, variable) //
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
| Sub-TLV-len | Sub-TLVs (variable) . . . |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
Please let me know your preference and I will update all drawings
consistently.
2. -----
Type: 5.
FP: For consistency (and to make sure implementers don't rely on the ASCII
figure), it would be good to indicate Type's length (1 octet I assume).
##PP
It's specified in ISO10589 as 1 octet. I looked at several RFCs and we
never specify the length of the Type field.
Example:
https://datatracker.ietf.org/doc/html/rfc8667#section-3.1
3. -----
Length: variable.
FP: This does not help much understanding what this field is supposed to
contain.
##PP
It's specified in ISO10589 as "total length of the value field". If the
value is of constant length, we indicate the constant, if the length is
variable we say "Variable". That is what we have been consistently doing
in many ISIS RFCs.
Example:
https://datatracker.ietf.org/doc/html/rfc8667#section-3.1
4. -----
Section 7.2
FP: Same issue as in 1. for the ASCII figure.
##PP
will address the same way for all cases based on your preference
5. -----
Sections 8.1 and 8.2
FP: Same comments as 1. 2. and 3.
##PP
will address the same way for all cases based on your preference
6. -----
If a behavior is advertised it MUST
only be advertised in the TLV[s] as indicated by "Y" in the table
below, and MUST NOT be advertised in the TLV[s] as indicated by "N"
in the table below.
FP: I find the sentence after the comma confusing, and don't understand the
presence of the MUST NOT here.
##PP
We have two categories of SRv6 SID in ISIS
a) those that are bound to an adjacency
b) those that are not bound to an adjacency
Based on the above the SID must be advertised in a different set of TLVs.
The table indicates which ISIS TLV(s) to use for advertising SRv6 SIDs
supported by ISIS. The table has "Y" and "N" variables. We need to say
what "Y" and "N" means.
Please note that the Y/N is something that we use in many ISIS RFCs,
even ISIS IANA uses it for several registries.
Example:
https://www.iana.org/assignments/isis-tlv-codepoints/isis-tlv-codepoints.xhtml#isis-tlv-codepoints-22-23-25-141-222-223
which says:
"n - sub-TLV MUST NOT appear in TLV 25"
If the above still does not address your comment, please let me know
what would be your preferred way to address it.
7. -----
Section 11.1.1
FP: It sounds like a bad idea in general to have to rename the registry every
time a TLV needs to be added to the registry... Maybe the wg and the AD should
consider renaming the registries so not to have this sort of dependency. (I
understand that this is a low priority comment, but still, it feels wrong to
put in titles what would fit really well in a registry itself). This very much
applies to Section 11.6 as well: the registry's name with the hierarchy of TLVs
as part of the name feels like a really bad idea. That is typically data that
goes into registries.
##PP
I followed the process we used in the past and consulted with the
Designated Experts we have. I'm open to rename the registry to whatever
we agree on. Changing the name of the old registry is going to be more
difficult ( Section 11.1.1), but for the Section 11.6 maybe we should
start with a shorter name and skip the TLVs:
"sub-sub-TLVs for SRv6 End SID and SRv6 End.X SID"
8. -----
Section 11.3
FP: The registry needs to be defined in the document. In particular, I see that
IANA is interpreting the columns as "Value" "Description" "Reference"; is that
right or should this be "Type" "Description" "Reference" (I see a mix of the
two for different IANA registries)?
##PP
I see both ["Type", "Description", "Reference"] as well as
["Value", "Name", "Reference"] being used for ISIS registries.
Any preference from your side?
9. -----
Section 11.8
FP: Are bits 0, 2-15 reserved or unassigned? The terminology in section 2 is
ambiguous, as it talks about "reserved for future use" (but the IANA section
leaves them unassigned). Please clarify for IANA.
##PP
sure, will add.
10. -----
Section 11.10
FP: Please define the registry (I assume it is going to be "Bit #", "Name",
"Reference").
##PP
ok, I will specify:
Bits 0-7 - Unassigned
thanks,
Peter
_______________________________________________
Lsr mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/lsr
