Re: [mpls] RTG-DIR Last Call review of draft-ietf-spring-segment-routing-mpls-18

[Ahmed Bashandy <[e-mail]>]

Thanks a lot for the review

I uploaded version 19 of the draft, which, IMO, addresses all your comments

See the reply "#Ahmed"

On 3/10/19 9:55 AM, Alexander Vainshtein wrote:
>
> 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: review of draft-ietf-spring-segment-routing-mpls-18
> Reviewer: Alexander (“Sasha”) Vainshtein
> Review Date: 10-Mar-19
> IETF LC End Date: 07-Mar-2019
> Intended Status: Proposed Standard
>
> *Summary:*I have some minor concerns about this document that I think 
> should be resolved before publication.
>
> *Comments*:
>
> I have done an early RTG-DIR review of the -14 version of the draft 
> half a year ago, and the issues I’ve raised then have been resolved in 
> the subsequent versions one way or another). Therefore this review has 
> been intentionally focused on the changes done to the draft in the few 
> recent versions.
>
> In my previous review I have noticed that the draft was not easy 
> reading for me. Since then readability of the draft has been improved. 
> However, there are still several places in the new text that are still 
> difficult to parse.
>
> I did not run the nits checker on the draft, so my list of nits is 
> probably incomplete.
>
> Just as with my earlier review, I send this one also to the MPLS WG 
> list – and for the same reasons.
>
> I tried to discuss my review privately with the authors, but they did 
> not respond.
>
> *Major Issues*: No major issues found.
>
> *Minor Issues*:
>
> 1.The text in Section 1 states that  “*a network operator SHOULD configure at least one node segment per 
> routing instance, topology, algorithm*”and continues that  “*An implementation MAY check that an IGP node-SID is not associated 
> with a prefix that is owned by more than one router within the same 
> routing domain, If so, it SHOULD NOT use this Node-SID, MAY use 
> another one if available, and SHOULD log an error*”. This looks somewhat controversial to me because:
> a.The check of the Node SID not being owned by more than one router in 
> the routing domain is defined as purely optional. According to RFC 
> 2119, implementations that choose to implement such a check must be 
> able to interoperate with implementations that do not implement it
> b.The recommended handling of the results of this check (fully aligned 
> with the text in Section 3.2 pf RFC 8402 that prohibits using prefixes 
> owned by more than one router in the domain as Node-SODs) strongly 
> suggests that the prefix that is owned by more than one router in the 
> domain is unusable as the Node SID
> I see two possibilities to resolve this controversy: either make the 
> check in question a “real requirement” (i.e., replace *MAY*with *SHOULD*or even *MUST*), or explain why it is safe enough not to implement such a check 
> (i.e., how implementations that support this check and implementations 
> that do not support it can interoperate within a given routing domain).The first of these options seems to me aligned with Section 3.2 in RFC 
> 8402 that says that “*An IGP Node-SID MUST NOT be associated with a 
> prefix that is owned by more than one router within the same routing 
> domain*”.     
#Ahmed: I replaced the MAY with SHOULD
>
> 2.I have  a problem with the highlighted part of the following text in 
> Section 2.5:
>
> *An implementation MUST NOT allow the MCCs belonging to the same*
>
> *   router to assign the same incoming label to more than one SR FEC. An*
>
> *implementation that allows such behavior is considered as faulty.***
>
> *Procedures defined in this document equally applies to this case,*
>
> *   both for incoming label collision (Section 2.5 
> <https://tools.ietf.org/html/draft-ietf-spring-segment-routing-mpls-18#section-2.5>) 
> and the effect on*
>
> *outgoing label programming (Section 2.6 
> <https://tools.ietf.org/html/draft-ietf-spring-segment-routing-mpls-18#section-2.6>).***
>
> a.The Section in question deals with incoming label collision (in 
> fact, the text that immediately follows the problematic fragment 
> states that “*The objective of the following steps is to 
> deterministically install in the MPLS Incoming Label Map, also known 
> as label FIB, a single FEC with the incoming label "L1"*”
> b.As a consequence, any mention of *outgoing label programming*, looks out of context (even accompanied by a forward reference to 
> Section 2.6)
> c.Section 2.6 covers the impact of incoming label collision on 
> programming of outgoing labels in quite a generic way. Therefore I 
> think that the highlighted part of the quoted fragment can be safely 
> removed (complete with the grammar mistake).
> d.I also do not see any value in stating that an implementations that 
> violates a mandatory requirement of the spec is faulty – isn’t that 
> self-evident?

#Ahmed: I removed the highlighted text because I agree with what you 
said in item (d) that it has no value


> 3.The highlighted text in Section 2.8 is not accurate:
> *   For Local SIDs, the MCC is responsible for downloading the correct*
> *   label value to FIB. For example, an IGP with SR extensions [I-D.ietf-*
> *   isis-segment-routing-extensions, I-D.ietf-ospf-segment-routing-*
> *   extensions] allocates and downloads the MPLS label corresponding to*
> *   an Adj-SID [RFC8402 <https://tools.ietf.org/html/rfc8402>].*
> *a.*IGP with SR extensions**may indeed dynamically allocate and download MPLS labels acting as 
> local Adj-SIDs **
> *b.*However, these labels can be allocated by configuration (e.g. as 
> mentioned in the tie-breaking rules in Section 2.5.1 and in the 
> example in Section A.2.3 in the draft), in which case IGP with SR 
> extensions would only responsible for its advertisement and 
> installation. **
#Ahmed: I removed the highlighted word "allocated"
> *NITS*:
> :**
> 1.In section 2.5:
> a.In the sentence “*Procedures defined in this document equally 
> applies to this case*” the noun is in plural but the verb is in singular. (If this sentence 
> is removed as suggested above, this nit disappears)
> b.The same problem exists in the sentence “*An incoming label 
> collision occurs if the SIDs of the set of FECs {FEC1, FEC2,..., FECk} 
> maps to the same incoming SR MPLS label "L1"*”
#Ahmed: The sentence is removed as you suggested
>
> 2.In section 2.10.1 the preposition “*to*” between the words 
> “*according*” and “*MPLS*” is missing in the fragment “*Push the 
> calculated label according the MPLS label pushing rules specified in 
> [RFC3032]*”.
>
#Ahmed added the missing "to"
>
> 3.Problems with references:
>
> a.As reported by Sergey 
> <https://mailarchive.ietf.org/arch/msg/spring/C_W3KBcL2AWxmlB7Sp53_PvqbQA>, 
> there are two occurrences of references to RFC 8042 “OSPF Two-Part 
> Metric” instead of RFC 8402. Lots of thanks to Sergey for catching this
>
#Ahmed: Corrected, thanks again
>
> b.Reference to RFC 8174 mistakenly contains a link to  RFC 7274.
>
#Ahmed: Corrected
>
> Hopefully these notes will be useful.
>
#Ahmed: VERY useful
>
> Regards,
>
> Sasha
>
> Office: [phone]
>
> Cell:      [phone]
>
> Email:   [e-mail]
>
>
> ___________________________________________________________________________
>
> This e-mail message is intended for the recipient only and contains 
> information which is
> CONFIDENTIAL and which may be proprietary to ECI Telecom. If you have 
> received this
> transmission in error, please inform us by e-mail, phone or fax, and 
> then delete the original
> and all copies thereof.
> ___________________________________________________________________________