Spencer, > I have been selected as the General Area Review Team (Gen-ART) > reviewer for this draft (for background on Gen-ART, please see > http://www.alvestrand.no/ietf/gen/art/gen-art-FAQ.html). > Please resolve these comments along with any other Last Call comments > you may receive. > > Document: draft-ietf-l3vpn-2547bis-mcast-bgp-07 > Reviewer: Spencer Dawkins > Review Date: 2009-09-17 (oops!) > IETF LC End Date: 2009-09-08 > IESG Telechat date: (not known) > > Summary: This draft is almost ready for publication as a Proposed Standard. > I have some comments, mostly 2119 language plus some sentences that didn't > parse and seemed important. Many thanks for your review and comments. Response to your comments in-line... > > Abstract > > This document describes the BGP encodings and procedures for > exchanging the information elements required by Multicast in MPLS/BGP > IP VPNs, as specified in [MVPN]. > > Spencer (nit): I'd expect the RFC Editor to remove a reference in the > Abstract section. > > 2. Introduction > > This document describes the BGP encodings and procedures for > exchanging the information elements required by Multicast in MPLS/BGP > IP VPNs, as specified in [MVPN]. This document assumes a thorough > familiarity with procedures, concepts and terms described in [MVPN]. > > This document defines a new NLRI, MCAST-VPN NLRI. The MCAST-VPN NLRI > > Spencer (nit): I'd expect the RFC Editor to ask that abbreviations be > expanded on first use. > > is used for MVPN auto-discovery, advertising MVPN to I-PMSI tunnel > binding, advertising (C-S, C-G) to S-PMSI tunnel binding, VPN > customer multicast routing information exchange among PEs, choosing a > single forwarder PE, and for procedures in support of co-locating a > C-RP on a PE. > > 5. PMSI Tunnel attribute > > When a router that receives a BGP Update that contains the PMSI > Tunnel attribute with its Partial bit set determines that the > attribute is malformed, the router SHOULD treat this Update as though > all the routes contained in this Update had been withdrawn. > > Spencer (minor): why SHOULD in this case, and not MUST? (Note that similar > text occurs throughout this section) This is to be consistent with draft-ietf-idr-optional-transitive-00.txt: Instead, when such an attribute is determined to be malformed, the UPDATE message containing that attribute SHOULD be treated as though all contained routes had been withdrawn just as if they had been listed in the WITHDRAWN ROUTES field of the UPDATE message, thus causing them to be removed from the Adj-RIB-In according to the procedures of [RFC4271]. > An implementation MUST provide debugging facilities to permit issues > caused by malformed PMSI Tunnel attribute to be diagnosed. At a > minimum, such facilities SHOULD include logging an error when such an > attribute is detected. > > Spencer (minor): If debugging facilities are a MUST, shouldn't the minimum > debugging facility be a MUST? :-) Or are there well-understood reasons > ("MUST do X unless") that could be provided? (Note that similar text occurs > elsewhere in the draft) We'll change it to "MUST include logging". Btw, we will apply the same fix to the similar text in Section 8. > The PMSI Tunnel attribute is used in conjunction with Intra-AS I-PMSI > A-D routes, Inter-AS I-PMSI A-D routes, S-PMSI A-D routes, and Leaf > A-D routes. > > > 7. VRF Route Import Extended Community > > If a PE uses Route Target Constrain [RT-CONSTRAIN], the PE SHOULD > advertise all such C-multicast Import RTs using Route Target > Constrains (note that doing this requires just a single Route Target > Constraint advertisement by the PE). This allows each C-multicast > route to reach only the relevant PE. To constrain distribution of the > Route Target Constrain routes to the AS of the advertising PE these > routes SHOULD carry the NO_EXPORT Community ([RFC1997]). > > Spencer (minor): I'm not sure I understand why these are SHOULDs and not > MUSTs. (Note that similar text occurs elsewhere in the draft) I note that > you give reasons why the similar text in Section 8 is not a MUST, using this > explanation: The "SHOULD" is because use of Route Target Constrain in this case is an optimization (as indicated in the second sentence in the above paragraph). > "Note that if non-segmented inter-AS P-tunnels are being used, then > the Intra-AS I-PMSI routes need to be distributed to other ASes and > MUST NOT carry the NO_EXPORT community." > > 8. PE Distinguisher Labels Attribute > > The Next Hop field of the MP_REACH_NLRI attribute of the route SHOULD > be set to the same IP address as the one carried in the Originating > Router's IP Address field. > > Spencer (minor): I'm not sure why this is a SHOULD and not a MUST, but if it > is a SHOULD, I'd expect to see some guidance about what happens if it's not > observed - does the mechanism still work? Actually this is not in Section 8, but in Sections 9.1.1. Similar text is in 9.2.3.2,1, 9.2.3.4.1, and 12.1. In all these occurences we'll replace "SHOULD" with "MUST". > 10. Non-congruent Unicast and Multicast Connectivity > > It is possible to deploy MVPN such the multicast routing and the > > Spencer (minor): is this "such that"? This is probably a nit but I'm having > to guess at the parsing. The "guess" is correct. Will fix this. > unicast routing are "non-congruent". For instance, the CEs may be > distributing to the PEs a special set of unicast routes that are to > be used exclusively for the purpose of upstream multicast hop > selection, and not used for unicast routing at all. (For example, > when BGP is the CE-PE unicast routing protocol, the CEs may be using > SAFI 2 ("Network Layer Reachability Information used for multicast > forwarding" [IANA-SAFI]), and either IPv4 or IPv6 AFI to distribute a > special set of routes that are to be used for, and only for, upstream > multicast hop selection.) In such a situation, we will speak of the > MVPN as having two VRFs on a given PE, one containing the routes that > are used for unicast, the other containing the unicast routes that > are used for upstream multicast hop (UMH) selection. We will call the > former the "unicast routing VRF", and the latter the "UMH VRF" > (upstream multicast hop VRF). > > > 11.4. C-multicast routes aggregation > > Further a BGP receiver, that receives multiple such routes with the > same NLRI for the same C-multicast route, will potentially create > forwarding state based on a single C-multicast route. As per the > procedures described in Section "Receiving C-Multicast Routes by a > PE", this forwarding state will be the same as the state that would > have been created based an other route with same NLRI. > > Spencer (nit): I'm pretty sure this is "based on another route" - but it > doesn't parse for me without help. Correct. Will fix this. > 12.1. Originating S-PMSI A-D routes > > In all of the above cases an implementation MUST allow to modify the > default via configuration. > > Spencer (minor): sorry, this doesn't parse. I'm guessing it's missing a > noun, but I can't suggest text. We will replace By default the set of Route Targets carried by the route MUST be constructed as follow: with The route always carries a set of Route Targets. The default set of Route Targets is determined as follows: and replace In all of the above cases an implementation MUST allow to modify the default via configuration. with In each of the above cases, an implementation MUST allow the set of Route Targets carried by the route to be specified by configuration. In the absence of a configured set of Route Targets, the route MUST carry the default set of route targets as specified above. > 16.1.1. Dampening withdrawals of C-multicast routes > > + Until the withdrawls are actually sent, multicast traffic for the > > Spencer (nit): s/withdrawls/withdrawals/g, because I saw other occurances in > the document... Will fix this. > C-multicast routes in question will be continued to be > transmitted to the PE, which will just have to discard it. Note > that the PE may receive useless (multicast) traffic anyway, > irrespective of dampening of withdrawals of C-multicast routes > due to the use of I-PMSIs. > > 17. Security Considerations > > The mechanisms described in this document could re-use the existing > BGP security mechanisms. > > Specer (minor): "could" seems awfully nebulous for security considerations > :-) I'm guessing you could delete this sentence, but it needs help of some > sort. > > 18. IANA Considerations > > Spencer (minor): I see from the ID tracker that IANA has already asked for > clarification on registration procedures ... :-) And we already responded to that. Yakov. _______________________________________________ Ietf@xxxxxxxx https://www.ietf.org/mailman/listinfo/ietf