Hi Ahmed,
Cosmetic minor nit:
2.5. Incoming Label Collision
[...]
both links in the '(Mirrored SID)' section need a cleanup / update
thanks,
pk
On Fri, Mar 29, 2019 at 2:14 AM Ahmed Bashandy
<[email protected] <mailto:[email protected]>> wrote:
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: +972-39266302
Cell: +972-549266302
Email: [email protected]
<mailto:[email protected]>
___________________________________________________________________________
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.
___________________________________________________________________________
_______________________________________________
spring mailing list
[email protected] <mailto:[email protected]>
https://www.ietf.org/mailman/listinfo/spring
--
Przemyslaw "PK" Krol | Strategic Network Engineer ing
|[email protected] <mailto:[email protected]>
