Hi Peter! Thanks for the quick reaction. Inline.
Francesca
*From: *Peter Psenak <[email protected]>
*Date: *Tuesday, 19 October 2021 at 15:19
*To: *Francesca Palombini <[email protected]>, The IESG
<[email protected]>
*Cc: *[email protected]
<[email protected]>, [email protected]
<[email protected]>, [email protected] <[email protected]>, Christian Hopps
<[email protected]>, [email protected] <[email protected]>
*Subject: *Re: Francesca Palombini's Discuss on
draft-ietf-lsr-isis-srv6-extensions-17: (with DISCUSS)
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
FP: Thank you.
>
> 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:
FP: I did not realize that they are not 4 Bytes aligned, however I do
think the current figure might be misleading for implementers, hence my
comment. To keep consistency with previously published RFC, maybe a.
would be best? (Although I am more used to b, but it might be me) Either
of these are fine to me.
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.
FP: Then maybe a reference to the relevant section of ISO10589 would be
useful. Can’t hurt to repeat, even though it is specified in other
standards, and in the figure itself.
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.
FP: Again, I think adding a reference can’t hurt and would ease my mind
that this is easily understandable by implementers. I am aware ISO10589
is a normative reference, so a pointer to the right section of that
document is all I am looking for.
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
FP: Thank you!
>
> 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
FP: Thanks.
>
> 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.
FP: I understand now, thank you. My confusion was more with the way this
is phrased, rather than the BCP 14 language itself – I didn’t understand
“as indicated by “Y”” referred to the TLVs, and not the whole sentence
(I read this as “it MUST only be advertised as indicated by Y”). Changed
“as indicated by” with “marked with” or something similar would have
been clearer to me, but I’ll move this to COMMENT since this is more
clarifications for the reader and this is not an implementation issue.
Thank you for the clarification.
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"
FP: Thank you for considering, and yes, this is more a comment to
Alvaro, the wg and the experts rather than you authors. I brought it up
here because this made me notice it. My suggestion is to have the
working group work on a document fixing this. Might be annoying but the
right thing to do, IMO.
>
> 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?
FP: No preference, just making sure you had considered either (I assume
you and the working group knows best which one is better) and decided on
a precise instruction for IANA.
>
> 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.
FP: Thanks.
>
> 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
FP: Not exactly what I meant: I was asking to specify the name of the
registry’s columns, to give IANA clear instructions of what that would
look like.
thanks,
Peter
>
>
>
>
>
>
>
