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

["Les Ginsberg (ginsberg)" <[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
> To: [e-mail]
> Cc: [e-mail]; [e-mail];
> [e-mail]
> Subject: RtgDir Review: draft-ietf-isis-segment-routing-msd-15
> 
> Hello,
> 
> I have been selected as the Routing Directorate reviewer for this draft.
> The Routing Directorate seeks to review all routing or routing-related drafts
> as they pass through IETF last call and IESG review, and sometimes on special
> request. The purpose of the review is to provide assistance to the Routing
> ADs. For more information about the Routing Directorate, please see 
> http://trac.tools.ietf.org/area/rtg/trac/wiki/RtgDir
> 
> Although these comments are primarily for the use of the Routing ADs, it
> would be helpful if you could consider them along with any other IETF Last
> Call comments that you receive, and strive to resolve them through
> discussion or by updating the draft.
> 
> Document: draft-ietf-isis-segment-routing-msd-15
> Reviewer: Julien Meuric (with some inputs from Bruno Decraene) Review
> Date: 2018-09-07 IETF LC End Date: 2018-09-12 Intended Status: ST
> 
> *Summary:*
> I have some minor concerns about this document that I think should be
> resolved before publication.
> 
> *Comments:"
> The document is well focused on a solution to a clearly scoped problem.
> The defined encodings are simple and clear. All the same, there are a few
> loose wordings, implicit assumptions or fuzzy zones which require
> clarification before publication.
> 
> *Minor issues:*
> 
> - 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..."

> 
> - 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.

 
> - 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.

> - 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.

> - "SID" may deserve to join the terminology section.

[Les:] Done.

> 
> - s/SIDs a node or a link on a node can support./SIDs supported by a node or
> a link on a node./

[Les:] Done

> 
> - 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.

 
> - Section 3 says "any other value represents that of the link when used as an
> outgoing link." Is it not excessive to be that prescriptive about outgoing vs.
> incoming? E.g., if an implementation was to advertise ERLD as a link MSD,
> that would rather refer to a capability of the incominglink. This specification
> could be left to each MSD-Type definition.
> 
[Les:] Good point - I removed reference to outgoing.

> - In section 4: s/the Link MSD MUST take preference/the Link MSD MUST
> take precedence/
> 
[Les:] Done.

> - 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". 

> - 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.

> - Section 7 says that "an MSD that is incorrect, may result in [...] instantiation
> of a path that can't be supported". It would be more accurate to mention
> "instantiation attempt", as a proper state control mechanism is expected to
> deny unsupported instantiation (e.g. PCEP errors for "invalid objects" include
> "Unsupported number of SR-ERO subobjects").
>
[Les:] I replaced "instantiation" with "calculation".
 
> *Nits:*
> - s/(IS-IS) Router to advertise/(IS-IS) router to advertise/

[Les:] Done
> - s/link a given SR path/each node/link of a given SR path/

[Les:] Done
> - s/path doesn't exceed/path does not exceed/

[Les:] Done. You don't like contractions? :-)

> - s/and associated attributes and capabilities/as well as associated attributes
> and capabilities/

[Les:] I prefer the existing wording. 

> - s/it is expected, that/it is expected that/

[Les:] Done

> ---
> - s/an IANA managed registry/an IANA-managed registry/

[Les:] I am going to leave this to the RFC editor if you don’t mind. I am not fully convinced that a compound adjective is appropriate here.

> - s/defined by this document/defined by this document:/

[Les:] Done

> - s/path that can't be/path that cannot be/

[Les:] Done

    Les

> ---
> 
> Regards,
> 
> Julien