Re: [Lsr] [RTG-DIR] RtgDir Review: draft-ietf-isis-segment-routing-msd-15

[Julien Meuric <[e-mail]>]

Hi Les,

Please see inline.

Thank you,

Julien


Sep. 26, 2018 - [e-mail]:
> Julien -
> 
> Thanx for the additional comments.
> V17 has been published to address these - subject to my responses below.
> See Les2
> 
>> -----Original Message-----
>> From: rtg-dir <[e-mail]> On Behalf Of Julien Meuric
>> Sent: Monday, September 24, 2018 8:12 AM
>>
>> Hi Les,
>>
>> Thanks for you feedback. Please find some responses, marked [JM] below.
>> You may consider the trimmed ones as resolved. For others, please keep 
>> in mind that the directorate's purpose is to improve documents'
>> readability before publication, as opposed to store clarification in 
>> mailing list archives.
>>
> [Les2:]  I fully appreciate both the purpose of the review and the effort you have put in. Yours is one of the better reviews I have seen in my years...
> If I push back it is not because I do not value your input - but simply because I disagree. :-)
> 
>> Julien
>>
>>
>> Sep. 24, 2018 - [e-mail]:
>>> Julien -
>>>
>>> Thanx for your detailed review - and your patience in waiting for a
>> response (I returned from vacation only a few days ago).
>>>
>>> I have published V16 of the draft which addresses your comments 
>>> except
>> as noted inline below.
>>>
>>>
>>>> -----Original Message-----
>>>> From: Julien Meuric <[e-mail]>
>>>> Sent: Monday, September 10, 2018 8:28 AM
>>>>
>> <snip>
>>>>
>>>> - The abstract uses the acronym "SID", however:
>>>>     * It should be expanded at first use,
>>>>     * It should be defined, e.g. by adding a (normative) reference 
>>>> to RFC 8402 (at least in the introduction),
>>>>     * The SR context is missing and should be explicitly mentioned 
>>>> (adding a phrase such as "in a Segment Routing-enabled network" 
>>>> would
>> be enough).
>>>
>>> [Les:] SID has been added to the terminology section and a reference 
>>> to
>> RFC 8402 added.
>>> However, I don’t think "SR context" is missing. The first sentence 
>>> of the Introduction starts with
>>>
>>> " When Segment Routing (SR) paths are computed..."
>>>
>> [JM] I fully agree about the introduction, but my comment was about 
>> the abstract where the SR context needs to be guessed from the use of 
>> the "SID" terminology: an explicit phrase would be welcome to a rookie 
>> reading the abstract.
>>
> [Les2:] I have modified the abstract.
> 
[JM2] ...and addressed most of my comment, thanks. (The RFC Editor may
still request you to expand "SID".)

>>>>
>>>> - OLD
>>>>    Path Computation Element Protocol (PCEP) SR extensions draft
>>>>    [I-D.ietf-pce-segment-routing] signals MSD in SR Path 
>>>> Computation
>>>>    Element (PCE) Capability TLV and METRIC Object.
>>>>   NEW
>>>>    The Path Computation Element communication Protocol (PCEP) SR
>>>>    extension document [I-D.ietf-pce-segment-routing] defines how to
>>>>    signal MSD using the SR-PCE-CAPABILITY sub-TLV (per node) and
>>>>    the METRIC object (per request).
>>>>
>>> [Les:] I have updated this and included a reference to RFC 5440 
>>> where
>> METRIC object was first defined.
>>>
>> [JM] Even better about METRIC. Consistently, "SR Path Computation 
>> Element
>> (PCE) Capability TLV" still remains a loose phrase, the technical name 
>> from [I- D.ietf-pce-segment-routing] is "SR-PCE-CAPABILITY", which 
>> avoids ambiguity.
>>
> 
> [Les2:] I removed naming the specific TLVs and replaced it with " Path  Computation Element Protocol (PCEP)".
> I think this is better than mentioning specific TLV names - particularly since one of them is part of a draft and the TLV name might change before the document becomes an RFC. So any possible naming inconsistency is now eliminated.
> 
[JM2] As there's an early IANA allocation on this TLV, I don't expect
any change in its name, but I can hear that argument. If you want a full
phrase, then I prefer the text from v16, because "SR Path Computation
Element (PCE) Capability" matches the section title of the PCEP draft
(and a slight change later wouldn't have much impact). In case you're
into removing the TLV/objects names and just point to PCEP, then you
MUST use the full expansion which is "Path Computation Element
*communication* Protocol" (and SHOULD avoid asking about the missing C
;-) ).

>>>
>>>> - The introduction says that the solution to complement BGP-LS lies 
>>>> in IS-IS (the OSPF draft claiming the counterpart on its side). 
>>>> This is a bit rough, the sentence 2 paragraph below already does 
>>>> the necessary scoping job: "This document defines an extension to 
>>>> <your favorite IGP
>>>> here>". I suggest to temper the early sentence by rephrasing the
>>>> beginning of page 3 into: "MSD capabilities should be advertised by 
>>>> every router in the network involved in the IGP."
>>>>
>>> [Les:] The draft says
>>>
>>> " MSD capabilities should be
>>>    advertised by every Intermediate System to Intermediate System(IS-IS)
>>>    router in the network."
>>>
>>> which I believe meets your suggestion.
>>>
>> [JM] My comment was exactly starting from there. Let me rephrase:
>> - This I-D says "should be advertised by every IS-IS router";
>> - draft-ietf-ospf-segment-routing-msd says "should be advertised by 
>> every OSPF router".
>> Now that we have a single LSR WG, I suggest to drop these competing 
>> wordings and consider a more consensual "should be advertised in the IGP".
>> Both documents already include a sentence "This document defines an 
>> extension to IS-IS/OSPF" at the end of their introduction, which is 
>> enough to define the scope.
>>
> [Les2:] Sorry - here I will pushback. 
> If the initial version of the MSD drafts had been written after (or close to) when the two IGP WGs were combined into LSR, we undoubtedly would have written one draft and included the IGP specific encodings for each protocol in that one draft - and we then would have used "IGP" in many places. But by the time LSR was formed the MSD drafts were already fairly mature, so we have kept them separate. Given that, I see no reason to replace "OSPF/IS-IS" with "IGP" given that we have protocol specific drafts.
> 
[JM2] The protocol-specific scope is already well defined by the last
paragraph of the introduction. Whatever the WG legacy, there is no
technical reason to say "I, IS-IS, am the solution" on one side and "I,
OSPF, am the solution" on the other side. I still believe this claim is
both wrong and unnecessary in these drafts, but it's rhetorical and
doesn't impact the technical specification, so I can live with that.

>>>> - The wording of the following sentence is not clear: "Note that in 
>>>> a non-SR MPLS network, label depth is what is defined by the MSD 
>>>> advertisements." Is it intended to say: "Note that MSD 
>>>> advertisements are applicable beyond SR- enabled networks and may 
>>>> refer to label depth
>> in MPLS networks."?
>>>>
>>> [Les:] The draft says:
>>>
>>> " Although MSD advertisements are associated with Segment Routing, the
>>>    advertisements MAY be present even if Segment Routing itself is not
>>>    enabled."
>>>
>>> I have made this sentence and the following sentence (which you 
>>> quoted)
>> a separate paragraph - which hopefully makes this a bit clearer.
>>>
>> [JM] Indeed, a separate paragraph is an improvement but doesn't fully 
>> address my comment. The 2nd sentence I previously quoted combines the 
>> passive voice on top of a subordinate clause starting with "what":
>> though grammatically correct, don't you disagree that the current 
>> wording barely succeeds in achieving its purpose of building a loud 
>> and clear message to be conveyed by an easy-to-parse sentence? Or do you?
>> ;-) (Should you decide to leave it as is, be prepared to RFC Editor's
>> review.)
>>
> [Les2:] I rewrote the paragraph - hopefully you will find it more acceptable.
> 
[JM2] Yes, it's much clearer now.

>> <snip>
>>>>
>>>> - The same ASCII art is used for the 2 figures (sections 2 and 3). 
>>>> It would be nice to specialize each one a little bit by explicitly 
>>>> adding their respective codepoint in the type field (23 and 15).
>>>>
>>> [Les:] The figure is intended to define the class of information in 
>>> each field -
>> not the actual values. The actual values are specified in the text 
>> which follows the ASCII art.
>>>
>> [JM] This is acceptable to me, but twice the exact same figure makes 
>> the reader wonder why not just two contexts with pointers to a single definition.
>>
> [Les2:] With TLV/sub-TLV encodings it is commonplace to provide the ASCII art in the section(s) specific to each advertisement. This avoids "flipping pages" in order to see the actual encoding.
> 
[JM2] I agree with that, all the more as a small figure instantiated
twice in a 9-page document isn't a big deal.

>> <snip>
>>>
>>>> - In section 5, BMI-MSD is defined as "the total number of MPLS 
>>>> labels which can be imposed" (which is OK when the incoming packet 
>>>> is unlabled). When the incoming packet is labeled (e.g. use of 
>>>> segment routing binding SID), if the incoming label is to be 
>>>> "replaced" by N outgoing labels, what processing model is assumed 
>>>> when advertising the
>> MSD:
>>>>  * one swap and one imposition of N-1 labels?
>>>>  * one pop and one imposition of N labels?
>>>>
>>> [Les:] What is being defined is the maximum number of SIDs/labels 
>>> which
>> can be "imposed". If popping or swapping affects the MSD which can be 
>> supported this needs to be accounted for in the advertisement. BMI-MSD 
>> is not being used to advertise a value specific to labeled or 
>> unlabeled packets nor a value which is "swap specific" or "pop specific".
>>>
>> [JM] We seem to agree on an architecture-agnostic use of BMI-MSD. "If 
>> popping or swapping affects the MSD [...] this needs to be accounted 
>> for" is a great introduction to the issue and deserves to be included 
>> in the I-D. My comment now binds to: in order to have interoperable 
>> implementations, I believe the document should be more prescriptive on 
>> how we account that for.
>>
> [Les2:] Added text
>  
[JM2] Thanks, this is a significant improvement. After talking to Bruno,
we however feel that the proposed wording remains ambiguous, and
especially the phrases "imposed under all conditions" and "be accounted
for". Indeed, section 3.10 of RFC 3031 defines "the operation to perform
on the packet's label stack" and doesn't mention "impose". The closest
concept may be found in:
"        replace the label at the top of the label stack with a
         specified new label, and then push one or more specified new
         labels onto the label stack."
Combining this split to your current text may lead to different
interpretations. Could you clarify the paragraph to be more specific on
the corresponding definitions and fully clear the fuzzy zone?

>>>> - For the BMI-MSD use case, the draft seems to clearly target a 
>>>> value attached to the outgoing link. However, this capability on a 
>>>> router is highly
>>>> hardware-dependent: some implementations may push a stack on the 
>>>> egress linecard while some may perform it on the ingress linecard.
>>>> The I-D seems to make some implicit hardware assumption and the 
>>>> current link MSD advertisement is not suited to describe these 
>>>> possible combinations of hardware implementations. This needs to be
>> addressed somehow.
>>>>
>>> [Les:] If a platform does imposition on ingress, then it should 
>>> advertise only
>> a Node MSD - as there would be no way to advertise a value which is 
>> specific to the ingress-egress interface combination. So, link MSDs 
>> for BMI are associated with the use of that interface as the outgoing link.
>>> [JM] OK. Could you please include that clarification in the I-D itself?
>>
> [Les2:] Done
> 
[JM2] Thanks.

>     Les
> 
>> <snip>
>>>> - s/path doesn't exceed/path does not exceed/
>>>
>>> [Les:] Done. You don't like contractions? :-)
>>>
>> [JM] I don't dislike 'em, but I prefer a slightly more formal language 
>> when writing standard text. :-)
>>
>>>> - s/and associated attributes and capabilities/as well as 
>>>> associated attributes and capabilities/
>>>
>>> [Les:] I prefer the existing wording.
>>> [JM] Acceptable to me, but may trigger a two-cycle reading on some
>> parser implementations.
>>
>> Regards,
>>
>> Julien
>