Hi Brian, Thanks for comment. Please find inline comment. Waiting on some more review comments before publish the next version of document.
AI from this comment : * Add reference to RFC6513, RFC6514 in terminology section * Define (S,G), (*,G) in terminology section Mankamana From: Brian Trammell via Datatracker <nore...@ietf.org> Date: Wednesday, September 8, 2021 at 12:12 AM To: tsv-...@ietf.org <tsv-...@ietf.org> Cc: bess@ietf.org <bess@ietf.org>, draft-ietf-bess-evpn-igmp-mld-proxy....@ietf.org <draft-ietf-bess-evpn-igmp-mld-proxy....@ietf.org>, last-c...@ietf.org <last-c...@ietf.org> Subject: Tsvart last call review of draft-ietf-bess-evpn-igmp-mld-proxy-12 Reviewer: Brian Trammell Review result: Ready with Issues This document has been reviewed as part of the transport area review team's ongoing effort to review key IETF documents. These comments were written primarily for the transport area directors, but are copied to the document's authors and WG to allow them to address any issues raised and also to the IETF discussion list for information. When done at the time of IETF Last Call, the authors should consider this review as part of the last-call comments they receive. Please always CC tsv-...@ietf.org if you reply to or forward this review. Caveat: I've reviewed this document primarily for transport-related issues. I have not made an effort to comprehensively review the architectural context in which it is defined; this review assumes that the operation of IGMP and Ethernet and IP multicast over EVPN is fundamentally sound. tl;dr: This document is acceptable from a transport standpoint: the algorithm used to reduce IGMP flooding seems safe within the limits of the specification's scope, recovery aspects are addressed, and though there is some fuzziness with respect to some implicit temporal limits I don't see anything that would impact the transport layer inordinately. However, from an editorial standpoint, it was not a joy to review, because it appears to assume a fairly deep familiarity with terminology and notation used in some context (which I assume is local to the BESS WG). Most of my issues are therefore editorial, and assume that the document is intended for general consumption. I'll start with the design and specification level questions first. General observation: the correctness of design of a system such as this tends to hang on the interactions among various timers. The only timer I see defined in this document is the Maximum Response Timer, which handles leave group timeouts. There are temporal interactions in other parts of the specification which remain implicit or undefined, though: for instance, Step 1 in section 4.1.1 specifies that IGMP messages should be grouped, but specifies no temporal limit to this process. I assume that the temporal scope is "the corresponding BGP session", but it would be nice to make this explicit in the document (especially if I'm wrong). This document does not define any new timer. And does not change any process of IGMP host / router processing. It does mention concepts from IGMP V2 (RFC 2236) and IGMP V3 (RFC3376). The algorithm (and that in section 4.1.2) assume statefulness on the proxy, but since both BGP and IGMP are fundamentally stateful that's perfectly fine. Section 6.3 adequately addresses the failure recovery aspects of this, but it would also be nice to see some guidance as to the temporal limits of the recovery process (e.g. if the proxy immediately group queries on link down, it seems like a link flap could lead to a group query flood). There are two parts . 1. IGMP host side processing : This is not stateful. Periodic query would be sent and state would be refreshed. All the processing here are again from IGMP RFC no change. 2. BGP route processing : This is stateful state. Once routes have been advertised, no more refresh or update until something changes. So all the processing defined in this document is to make sure how do we react to processing of step1 here. Editorial nit: the Terminology section does not define most of its terms, instead providing only expansions. This does not serve the reader well, and reinforces the impression of RFCs as acronym soup. If I don't know what Ingress Replication means in a BESS context, then "IR: Ingress Replication" serves only to tell the reader that we're not discussing infrared radiation. Please either provide definitions (with references; e.g. IGMP has a set of supporting RFCs), or reference another EVPN document providing definitions (as it is perfectly acceptable for a multi-document protocol suite like EVPN to have an introductory document). Even with the list of acronym expansions, some acronyms are missing: I had to google NLRI, for instance, and I'm assuming ES means Ethernet Segment. Indeed, I was pleasantly surprised to see Maximum Response Timer not abbreviated to MRT. This document needs a thorough review of acronym usage, expansion, and definition before it can be published for a general audience. This document does refer to main EVPN document RFC7432 for terminology. Most of abbreviation are defined in different document in BESS WG. Would add familiarity with other RFC in terminology section. Editorial nit: A VM is a kind of host; the repeated use of "hosts/VMs" doesn't add much and makes the doc a bit more awkward to follow -- where "hosts" appears alone am I to assume that the passage does not apply to VMs? VM has been removed from document. Since VM and host were denoting the hosts. Editorial nit: The notation used in the algorithms in Sections 4.1.1 and 4.1.2 "(*, G), (S, G)" is neither introduced by reference nor defined, and I can't find a reference to it in IGMP documentation. Would add in terminology section
_______________________________________________ BESS mailing list BESS@ietf.org https://www.ietf.org/mailman/listinfo/bess
