Hi Jon, many thanks for your review. Some comments inline.
where you don’t see any answer to your comments is because I applied them to the draft. > On Mar 7, 2017, at 7:35 PM, Jonathan Hardwick > <jonathan.hardw...@metaswitch.com> wrote: > > Hello > > I have been selected to do a routing directorate “early” review of this draft. > https://datatracker.ietf.org/doc/draft-ietf-spring-segment-routing-central-epe/ > > The routing directorate will, on request from the working group chair, > perform an “early” review of a draft before it is submitted for publication > to the IESG. The early review can be performed at any time during the > draft’s lifetime as a working group document. The purpose of the early > review depends on the stage that the document has reached. As this document > is in working group last call, my focus for the review was to determine > whether the document is ready to be published. Please consider my comments > along with the other working group last call comments. > > For more information about the Routing Directorate, please see > http://trac.tools.ietf.org/area/rtg/trac/wiki/RtgDir > > Best regards > Jon > > > Document: draft-ietf-spring-segment-routing-central-epe-04.txt > Reviewer: Jonathan Hardwick > Review Date: 7 March 2017 > Intended Status: Informational > > Summary > Congratulations on a very clear and well-written document. I have a few > minor comments below but otherwise the document looks ready to advance. > > Abstract > s/It requires minor/It requires a minor/ > Expand acronym SDN on 1st use > > Section 1 > s/SID's/SIDs/ > 3rd bullet - why is the reference here? I believe you refer to this paragraph: [I-D.ietf-spring-segment-routing] defines three types of BGP peering segments/SID's: PeerNode SID, PeerAdj SID and PeerSet SID. the peerNode/Adj/Set segment are indeed defined in draft-ietf-spring-segment-routing. In this draft we illustrate the use case and the SR solution to it. > "The solution is described for IPv4..." - I am obliged to discourage the use > of exclusively IPv4 examples in this document. See > https://www.iab.org/2016/11/07/iab-statement-on-ipv6/. I’ll work out that... there’s a substantial amount of addresses so I’ll be sure not to mess up everything ;-) > Section 1.1 can be removed - section 13 lists the references. > Section 1.2 bullet 6: s/ingress EPE/ingress PE/ > Section 1.2 bullet 6: s/at an source/at a source/ > > Section 3 > I found it a bit strange that you did not list the PeerNode segments > contiguously in this section (they are 1012, 1022 and 1052). But it's not a > big deal - I can live with it. > Section 3.6 s/An BGP-EPE enabled/A BGP-EPE enabled/ > > It's not clear if the FRR behaviour you are specifying in 3.6 is mandatory or > just an example. in fact it’s just an illustrative example. I changed the “SHOULD” into a “MAY” and added more text. > However, the PeerNode SID and PeerAdj SID have the following backup rule. > "2. Else backup via another PeerNode SID to the same AS." > > That's reasonable under some circumstances but it might not agree with the > policy of the adjacent AS. For whatever reason that AS might want to steer > traffic to certain IP destinations away from certain links, by not > advertising BGP routes over those links, or advertising them with different > MEDs. Is there scope for the EPE controller taking these preferences into > account? yes, that’s a good point and I added text on that. > Section 4 > s/an BGP-EPE controller/a BGP-EPE controller/ > Section 4.1: When you say "engineered peers" do you mean "BGP-EPE enabled > border routers"? > Section 4.1: "add-path all" sounds like a vendor specific CLI command. Could > you rephrase as "with the router configured to advertise all paths using BGP > add-path [RFC7911]"? > > Section 4.3: s/described in the section 2 (BGP-LS advertisements)/described > in section 2/ > Section 4.4 s/an BGP-EPE/a BGP-EPE/ > > Section 4.6 This section leaves me with a few questions. What are "business > policies"? How should they be collected, and why? Do you mean "collected" > or "configured”?s it could be both but of course these mechanisms are out of scope of this draft. > Section 4.7: What is SID 64? I infer it's the SID for PE C. It should > probably be given in section 3. it is defined in 1.2 “C’s loopback is 203.0.113.3/32 with SID 64” I added a reference. > Section 5 > Section 5.2 "The tunnel and the steering policy could be configured via..." - > Do we need a list? It could also be configured by CLI - does it matter? > Section 5.3 s/them BGP upstream peers/their BGP upstream peers/ > Section 5.4 This example confused me as it appears to contradict section 1.2 > bullet 1 when applied to Internet traffic. Or is this example just talking > about an inter-AS L3VPN service? It’s doesn’t need to be “inter-AS”=. It’s a way to build a vpn route in the controller with a vpn label representing an EPE resource (peer, adj, set). > Section 5.5 Unlike the other examples in section 5, the details of the > FlowSpec route do not contain the actual IP addresses and SID/Labels in use. I’d propose to remove the FlowSpec section since it has more to do with a SR policy definition and we have other drafts for that. > > Section 7 > I don't think this section is required - I recommend taking it out. I’m ok removing it, however I found it useful for the reader to grab the benefits (or characteristics/properties if you prefer) of the solution. > Bullet 2 says that this works with "next hop self" but the example in section > 4.1 does not use next hop self and I don't immediately see how it could work > if next hop self was enabled on the BGP-EPE border router. why not ? in fact, the re-writing of the next-hop is orthogonal to the solution. It is there just to facilitate the resolution (i.e.: avoid redistributing external interface routes into the igp. Thanks. s. > s/avail the/assuming the/ _______________________________________________ spring mailing list spring@ietf.org https://www.ietf.org/mailman/listinfo/spring
