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 <julien.meu...@orange.com> > Sent: Monday, September 10, 2018 8:28 AM > To: rtg-...@ietf.org > Cc: draft-ietf-isis-segment-routing-...@ietf.org; rtg-...@ietf.org; > lsr@ietf.org > 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 _______________________________________________ Lsr mailing list Lsr@ietf.org https://www.ietf.org/mailman/listinfo/lsr
