Thanks for comment. Please find inline comment. Waiting on some more review comments before publish the next version of document.
From:
Brian Trammell via Datatracker <noreply@xxxxxxxx>
Date: Wednesday, September 8, 2021 at 12:12 AM
To: tsv-art@xxxxxxxx <tsv-art@xxxxxxxx>
Cc: bess@xxxxxxxx <bess@xxxxxxxx>, draft-ietf-bess-evpn-igmp-mld-proxy.all@xxxxxxxx <draft-ietf-bess-evpn-igmp-mld-proxy.all@xxxxxxxx>, last-call@xxxxxxxx <last-call@xxxxxxxx>
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-art@xxxxxxxx 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 .
-
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.
-
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
