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 > > > > > > >
_______________________________________________ Lsr mailing list [email protected] https://www.ietf.org/mailman/listinfo/lsr
