[pim] AD Review of draft-ietf-pim-dr-improvement-09

[Alvaro Retana <[e-mail]>]

Dear authors:

Hi!  I hope everyone is safe and healthy.


I (finally!) finished my review of this document.  Thank you for the
effort so far.

I think that this document is not ready and needs significant work to
move forward.  There are many parts that are repetitive and redundant.
Also, there would be a lot of benefit from an English-language
edit/review.  I tried to cover many of the points in my comments, but
probably left some out...

Before getting into the details, I have a couple of points I want to
highlight up here:

(1) How does this document interact with rfc8775 (PIM DR Load Balancing)?
    Should be BDR always be a GDR?  Can a BDR not be a GDR?  rfc8775 is not
    even mentioned.


(2) As far as I can see draft-mankamana-pim-bdr has not been adopted yet.
    Assuming that is the plan, how would the two mechanisms interact?  Given
    that draft-mankamana-pim-bdr doesn't add options, and §5 says that if no
    options are received then the routers MUST use rfc7761, how does a router
    implementing this specification tell the difference?

    I realize that some of these questions may be better directed at
    draft-mankamana-pim-bdr, but because the WG agreed that a statement
    relating the two should be included in this document [1], then I'm
    asking now.  I would really like to understand what the WG expects.


(3) The Shepherd writeup [2] says that there is an implementation and
    deployment of this specification.  *But* the IANA Considerations section
    lists the code points as TBD.  What's the status of the implementation?
    Are there specific code points that should be suggested to IANA?  An
    rfc7942-type section would be useful to justify specific code points, if
    needed.


(4) The election algorithm (§4.2) is a word-by-word copy from rfc2328!  It is
    good to reuse technology, but in this case (1) the description is out of
    context, and (2) the election function from rfc7761 would make a much
    better (and familiar) base for the description (and, as far as I can
    tell, should yield the same result).  Please update the description.


I expect a significant change in the structure of the document to move forward.

Thanks!

Alvaro.


[1] https://mailarchive.ietf.org/arch/msg/pim/hWi6rDIbbhcEjEuQ_y3z1o6Wulw/

[2] https://datatracker.ietf.org/doc/draft-ietf-pim-dr-improvement/shepherdwriteup


[Line numbers from idnits.]


2	PIM WG                                                      Zheng. Zhang
3	Internet-Draft                                           ZTE Corporation
4	Intended status: Standards Track                             Fangwei. Hu
5	Expires: April 27, 2020                                       Individual
6	                                                            Benchong. Xu
7	                                                         ZTE Corporation
8	                                                       Mankamana. Mishra
9	                                                           Cisco Systems
10	                                                        October 25, 2019

[minor] The header should have only the authors "initial followed by
family name" [rfc7322].

12	                           PIM DR Improvement

[minor] A more descriptive name may be "PIM Designated Router Enhancements"

13	                  draft-ietf-pim-dr-improvement-09.txt

15	Abstract

17	   Protocol Independent Multicast - Sparse Mode (PIM-SM) is widely
18	   deployed multicast protocol.  As deployment for PIM protocol is
19	   growing day by day, user expects lower traffic loss and faster
20	   convergence in case of any network failure.  This document provides
21	   an extension to the existing protocol which would improve the
22	   stability of the PIM protocol with respect to traffic loss and
23	   convergence time when the PIM DR role changes.

[nit] s/is widely/is a widely

[nit] s/deployment for PIM protocol...user expects/deployment for the
PIM protocol...users expect

[nit] s/document provides an extension...which would improve...PIM
DR/document defines an extension...which improves...PIM Designated
Router (DR)


...
81	1.  Introduction

83	   Multicast technology is used widely.  Many modern technologies, such
84	   as IPTV, Net-Meeting, use PIM-SM to facilitate multicast service.
85	   There are many events that will influence the quality of multicast
86	   services.  Like the change of unicast routes, the change of the PIM-
87	   SM DR may cause the loss of multicast packets too.

[minor] IPTV and Net-Meeting are used a couple of times to generically
refer to services.  It would be very nice if a generic description is
included in the Terminology section.

[minor] Suggestion>
   Multicast technology, with PIM-SM ([RFC7761]), is used widely in modern
   services like IPTV and Net-Meeting.  Some events, such as changes in
   unicast routes, or a the change in the PIM-SM DR, may cause the loss of
   multicast packets.

[minor] This would be a good point to briefly describe what a DR does.
A few sentences should be more than enough.


89	   After a DR on a shared-media LAN goes down, other routers will elect
90	   a new DR after the expiration of Hello-Holdtime.  The default value
91	   of Hello-Holdtime is 105 seconds.  Although the minimum Hello
92	   interval can be adjusted to 1 second, the Hello-Holdtime is 3.5 times
93	   Hello interval.  Thus, the detection of DR Down event cannot be
94	   guaranteed in less than 3.5 seconds.  And it is too long for modern
95	   multicast services.  Still, many multicast packets will be lost.  The
96	   quality of IPTV and Net-Meeting will be influenced.

[major] "the detection of DR Down event cannot be guaranteed in less
than 3.5 seconds."   According to rfc7761, the Hello_Holdtime
(signaled in the Holdtime Option) could be as short as 1 sec.

The advantages of the mechanism depend on the detection speed...given
that the lowest timer can be set to 1sec, does that change the
benefits of this specification?  I'm specifically wondering about the
impact on the last 3 sentences, and the motivation overall.

Suggestion>
   As detailed in [RFC7761], the election of a DR is triggered after the
   current DR's Hello_Holdtime expires.  By default, the Hello_Holdtime is
   3.5 times the Hello Timer, or 105 seconds, and it can be set as low as 1
   second.  ...


98	                   \                                     /
99	                    \                                   /
100	                  -------                             -------
101	                  |  A  |                             |  B  |
102	                  -------                             -------
103	                     | DR                                |
104	                     |                                   |
105	                  -------                             -------
106	                  | SW  |-----------------------------| SW  |
107	                  -------                             -------
108	                     |                                   |
109	                   Figure 1: An example of multicast network

[nit] I think that including switches complicates an otherwise simple
figure.  Consider also adding a client.


111	   For example, there are two routers on one LAN.  Two SWs (Layer 2
112	   switches) provide shared-media LAN connection.  RouterA is elected as
113	   DR.  When RouterA goes down, the multicast packets are discarded
114	   until the RouterB is elected to DR and RouterB imports the multicast
115	   flows successfully.

117	   We suppose that there is only a RouterA in the LAN at first in
118	   Figure 1.  RouterA is the DR which is responsible for forwarding
119	   multicast flows.  When RouterB connects to the LAN, RouterB will be
120	   elected as DR because of its higher priority.  RouterA will stop
121	   forwarding multicast packets.  The multicast flows will not recover
122	   until RouterB pulls the multicast flows after it is elected to DR.

[minor] These two paragraphs mention two different scenarios, but the
text is not clear and so they sound confusing: in the first case
RouterB seems to already be in the network, but not in the second.

Suggestion>
   The simple network in Figure 1 presents two routers (A and B) connected to
   a shared-media LAN segment.  Two different scenarios are described to
   illustrate potential issues.

   (a) Both routers are on the network, and RouterB is elected as the DR.  If
   RouterB then fails, multicast packets are discarded until RouterA is
   elected as DR and it assumes the multicast flows on the LAN.

   (b) Only RouterA is initially on the network, making it the DR.  If
   RouterB joins the network with a higher DR Priority, it will them be
   elected as DR.  RouterA will stop forwarding multicast packets, and the
   multicast flows will not recover until RouterB assumes the multicast flows
   on the LAN.


124	   So if we want to increase the stability of DR, carrying DR/ BDR role
125	   information in PIM hello packet is a feasible way to show the DR/ BDR
126	   roles explicitly.  It avoids the confusion caused by newcomers which
127	   have a higher priority.

[minor] This paragraph describes the new functionality.

Suggestion>
   In order to increase the stability of the network, this document introduces
   the Backup Designated Router (BDR) and specifies how its identity is
   explicitly advertised.


...
137	2.  Terminology

139	   Backup Designated Router (BDR): Like DR (Designated Router), a BDR
140	   which acts on behalf of directly connected hosts in a shared-media
141	   LAN.  But BDR must not forward the flows when DR works normally.
142	   When DR goes down, the BDR will forward multicast flows immediately.
143	   A single BDR MUST be elected per interface like the DR.

[major] The definition is not clear.

Suggestion>
   Backup Designated Router (BDR):  Immediately takes over all DR functions
   ([RFC7761]) on an interface once the DR is no longer present.  A single
   BDR SHOULD be elected per interface.


145	   Designated Router Other (DROther): A router which is neither DR nor
146	   BDR.

[nit] s/neither DR nor BDR/neither a DR nor a BDR

[nit] This term is only used once in the document.  Consider not using
it and rewording the place where it is.


148	3.  PIM hello message format

150	   The PIM hello message format is defined in [RFC7761].  In this
151	   document, we define two new option values which are including Type,
152	   Length, and Value.

[minor] Suggestion>
   Two new PIM Hello Options are defined, which conform to the format defined
   in [RFC7761].


154	        0                   1                   2                   3
155	        0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
156	        +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
157	        |                   Hello message format                      |
158	        |                                                             |
159	        +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
160	        |         OptionType            +       OptionLength          |
161	        +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
162	        |                       OptionValue                           |
163	        |                                                             |
164	        +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
165	                   Figure 2: Hello message format

[minor] Please use the "standard" packet format style...and only
include the Option.

Suggestion>
    0                   1                   2                   3
    0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |          OptionType           |         OptionLength          |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |                          OptionValue                          |
   |                              ...                              |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

   Figure 2: Hello Option Format



167	3.1.  DR Address Option format
...
171	   o  OptionLength: If the IP version of PIM message is IPv4, the
172	      OptionLength is 4 octets.  If the IP version of PIM message is
173	      IPv6, the OptionLength is 16 octets.

[major] rfc7761 defines OptionLength as the "length of the OptionValue
field in bytes".

[major] "IP version of PIM message" is not clear as you are probably
referring to the IP packet itself...

Suggestion>
   OptionLength: 4 bytes if using IPv4 and 16 bytes if using IPv6.


175	   o  OptionValue: The OptionValue is IP address of DR.  If the IP
176	      version of PIM message is IPv4, the value is the IPv4 address of
177	      DR.  If the IP version of PIM message is IPv6, the value is the
178	      IPv6 address of DR.

[major] What is the expectation for this address?  For IPv6 it should
be a link-local address, right?  For IPv4...?   I think that some
wording about this is important to sanity check the contents.

Suggestion>
   OptionValue: IP address of the DR.  If...IPv4, the value MUST be...
   If...IPv6, the value MUST be...


[major] What should be done if the contents are not what is expected?
Also, the addresses MUST correspond to a known neighbor (or at least a
node that has sent a Hello...or will send one before the Hold time
expires)...right?



180	3.2.  BDR Address Option format
...
184	   o  OptionLength: If the IP version of PIM message is IPv4, the
185	      OptionLength is 4 octets.  If the IP version of PIM message is
186	      IPv6, the OptionLength is 16 octets.

188	   o  OptionValue: The OptionValue is IP address of BDR.  If the IP
189	      version of PIM message is IPv4, the value is the IPv4 address of
190	      BDR.  If the IP version of PIM message is IPv6, the value is the
191	      IPv6 address of BDR.

[] See above.

Suggestion>
   OptionLength: 4 bytes is using IPv4 and 16 bytes if using IPv6.

   OptionValue: IP address of the BDR.



193	4.  Protocol Specification

[major] This section starts by describing the election of the
DR/BDR...which is also later specified in §4.2.  Please specify things
only once!  Suggestion: point to the algorithm in §4.2 from this
section and leave the specification there.


195	   Carrying DR/ BDR role information in PIM hello packet is a feasible
196	   way to keep the stability of DR.  It avoids the confusion caused by
197	   newcomers which have a higher priority.  So there are some changes in
198	   PIM hello procedure and interface state machine.

200	   A new router starts to send hello messages with the values of DR and
201	   BDR are all set to 0 after its interface is enabled in PIM on a
202	   shared-media LAN.  When the router receives hello messages from other
203	   routers on the same shared-media LAN, the router will check if the
204	   value of DR is filled.  If the value of DR is filled with IP address
205	   of the router which is sending hello messages, the router will store
206	   the IP address as the DR address of this interface.

208	   Then the new router compares the priority and IP address itself to
209	   the stored information of DR and BDR according to the algorithm of
210	   [RFC7761].  If the new router notices that it is better to be DR than
211	   the current DR or BDR, the new router will make itself the BDR, and
212	   send new hello messages with its IP address as BDR and current DR.
213	   If the router notices that the current DR has the highest priority in
214	   the shared-media LAN, but the current BDR is set to 0.0.0.0 if IPv4
215	   addresses are in use or 0:0:0:0:0:0:0:0/128 if IPv6 addresses are in
216	   use in the received hello messages (To simplify, we use 0x0 in
217	   abbreviation in following parts of the draft), or the current BDR is
218	   not better than the new router, the new router will elect itself to
219	   BDR.  If the router notices that it is not better to be DR than
220	   current DR and BDR, the router will follow the current DR and BDR.

[minor] "new router" is used a lot...I like "or a router first starts"
from rfc7761.

[major] "If the new router notices that it is better to be DR than the
current DR or BDR, the new router will make itself the BDR..."  This
seems to say that if the new router is better than either the DR or
the BDR then it will become the BDR...however, I think that there are
some cases that may need to be clarified about preemption, or not.

For example, lets assume 3 routers: A, B and C.  If all 3 were in the
LAN, then their DR preference would be A, B and C, in that order.
Let's assume that initially only C is on the LAN: C is the DR.  Later
A joins; C would still be the DR and A would be the BDR.

Later B joins -- according to "the new router notices that it is
better to be DR than the current DR or BDR", B is better than C (the
DR), then B becomes the BDR...and A simply DROther.

Is that the expected intent?  Or is the intent for A to remain as the
BDR because it is better than B?


[major] The description above (first 3 paragraphs) needs some
Normative Language...and it might be better suited as a series of
steps.  Again, please specify things only once.

[minor] Please define 0x0 in the terminology section, and not in the
middle of a paragraph.


222	   When the new router becomes the new BDR, the router will join the
223	   current multicast groups, and import multicast flows from upstream
224	   routers.  But the BDR must not forward the multicast flows to avoid
225	   the duplicate multicast packets in the shared-media LAN.  The new
226	   router will monitor the DR.  The method that BDR monitors the DR may
227	   be Bidirectional Forwarding Detection (BFD) for Multi-point Networks
228	   and Protocol Independent Multicast [I-D.ietf-pim-bfd-p2mp-use-case]
229	   technology, BFD (Bidirectional Forwarding Detection) [RFC5880]
230	   technology, or other ways that can be used to detect link/node
231	   failure quickly.  When the DR becomes unavailable because of the down
232	   or other reasons, the BDR will forward multicast flows immediately.

[minor] Can the first two sentences be replaced with something like this?

   The BDR takes on all the functions of a DR as specified in [RFC7761],
   except that it SHOULD NOT actively forward multicast flows to avoid
   duplication.


[] The rest of the paragraph, contains some duplication with the one
below -- see comments after it.


234	   BFD for PIM function defined in [I-D.ietf-pim-bfd-p2mp-use-case], or
235	   asynchronous mode defined in BFD [RFC5880] are suggested to be used
236	   for the DR failure detection.  BDR monitors DR after the BFD session
237	   between DR and BDR is established.  For example, an aggressive BFD
238	   session that achieves a detection time of 300 milliseconds, by using
239	   a transmit interval of 100 milliseconds and a detect multiplier of 3.
240	   So BDR can replace DR to forward flows when DR goes down within sub
241	   second.  The other BFD modes can also be used to monitor the failure
242	   of DR, the network administrator should choose the most suitable
243	   function.

[major] It is relatively clear that if "the DR becomes unavailable
because of the down or other reasons", then the BDR takes over as the
DR.  I think however that the availability of the DR needs to be
better described.  It seems to me that the reason is not important,
what is important is for the DR not to be reachable on the LAN (from
the BDRs point of view) -- is that correct?  Please be clear in the
text about that.

[minor] "The new router will monitor the DR."  Here, you should be
specific that the monitoring router is now the BDR.

[major] "...will monitor the DR."  Is this a mandatory action?  Should
SHOULD/MUST be used?  Is it necessary to even say this if rfc7761
already talks about expiration in general?

[major] "...[I-D.ietf-pim-bfd-p2mp-use-case], or asynchronous mode
defined in BFD [RFC5880] are suggested to be used for the DR failure
detection"  I'm assuming these are just examples, right?  If so, then
we don't need to talk about specific settings.

Suggestion (to replace all the BFD-related text)>
   If the DR becomes unreachable on the LAN, the BDR MUST take over all the
   DR functions, including multicast flow forwarding.  Mechanisms outside the
   scope of this specification, such as [I-D.ietf-pim-bfd-p2mp-use-case] or
   BFD Asynchronous mode [RFC5880] can be used for faster failure detection.


245	4.1.  Deployment Choice

[major] This section contains redundant information (with points
already made and §5), so it may be confusing.  Please eliminate it.


247	   DR / BDR election SHOULD be handled in two ways.  Selection of which
248	   procedure to use would be totally dependent on deployment scenario.

[major] SHOULD is out of place because there is no Normative action.
s/SHOULD/can

[nit] s/would be totally dependent/depends


250	   1.  The algorithm defined in [RFC7761] should be used if it is ok to
251	   adopt with new DR as and when they are available, and the loss caused
252	   by DR changing is acceptable.

254	   2.  If the deployment requirement is to have minimum packets loss
255	   when DR changing, the mechanism defined in this draft should be used.
256	   That is, if the new router notices that it is better to be DR than
257	   the current DR or BDR, the new router will make itself the BDR, and
258	   send new hello message with its IP address as BDR and current DR.

[minor] I understand that the mechanism defined in this document is
optional, and the objective (of reducing packet loss) has already been
established...so there's no need to repeat it again.

[major] This section seems to also point to the fact that if the new
Hello Options are used then the new mechanism may not be used.  IOW,
there's some aspect of configuration (?) needed to enable the new
functionality.  Is that true?   But then §5 seems to contradict it.


260	   According to section 4.9.2 defined in [RFC7761], the device receives
261	   unknown options Hello packet will ignore it.  So the new extension
262	   defined in this draft will not influence the stability of neighbor.
263	   But if the router which has the ability defined in this draft
264	   receives non-DR/BDR capable Hello messages defined in [RFC7761], the
265	   router MAY stop sending DR/BDR capable Hello messages in the LAN and
266	   go back to use the advertisement and election algorithm defined in
267	   [RFC7761].

[major] This paragraph contains information that overlaps with §5
(Compatibility).  Please specify the behavior only in one place: take
this paragraph out.


269	4.2.  Election Algorithm

271	   The DR and BDR election is according to the rules defined below, the
272	   algorithm is similar to the DR election defined in [RFC2328].

[major] It's not similar, it's the same...even including the caveats
at the end!  Even if inspired by OSPF, please (1) update the
description to better match pim and the context of this document, and
(2) reuse what is already specified in rfc7761 (like the DR election
function) -- except for the non-pre-emptive nature it looks to me that
the DR election function in rfc7761 could be used to also elect the
BDR (with the same result!).

[] Note that just because the algorithm is already in an RFC it
doesn't mean that it is clear...  I started putting comments below
before I realized the text was the same as in rfc2328...I'm leaving
some of them here for reference...


...
320	4.3.  Sending Hello Messages

322	   According to Section 4.3.1 in [RFC7761], when a new router's
323	   interface is enabled in PIM protocol, the router sends Hello messages
324	   with the values of DR and BDR are filled with 0x0.  Then the
325	   interface is in Waiting state and starts the hold-timer which is
326	   equal to the Neighbor Liveness Timer.  When the timer is expired, the
327	   interface will elect the DR and BDR according to the DR election
328	   rules.

[major] "According to Section 4.3.1 in [RFC7761]...the router sends
Hello messages with the values of DR and BDR are filled with 0x0."
rfc7761 doesn't talk about this extension.  Suggestion: s/.../When PIM
is enabled on an interface or a router first starts, Hello messages
MUST be sent with the values of DR and BDR filled with 0x0.

[major] "interface is in Waiting state"  Where does this state come from?

[major] hold-timer is used here (and in other places), but
Hello-Holdtime is also used elsewhere.  Please pick one and be
consistent.  See §4.11/rfc7761.

[nit] s/the timer is expired/the timer expires

[minor] s/the interface will elect the DR and BDR according to the DR
election rules./the DR and BDR will be elected on the interface
according to the DR election algorithm (Section 4.2).


330	   When a new router sets itself BDR after receives hello messages from
331	   other routers, the router sends hello messages with the value of DR
332	   is set to the IP address of current DR and the value of BDR is set to
333	   the IP address of the router itself.

[minor] "When a new router sets itself BDR...the value of BDR is set
to the IP address of the router itself."  Sorry, but I don't know what
you're trying to say in this paragraph.


335	   A current BDR MUST set itself DROther after it receives Hello
336	   messages from other router which is eligible to be BDR/DR, the router
337	   will send hello messages with the value of DR is set to current DR
338	   and the value of BDR is set to the new BDR.

[minor] "...other router which is eligible to be BDR/DR...the value of
BDR is set to the new BDR."   What new BDR?  The description points at
what happens when a Hello is received -- is the election algorithm
supposed to be run first?



...
350	   For example, there is a stable LAN that includes RouterA and RouterB.
351	   RouterA is the DR which has the highest priority.  RouterC is a
352	   newcomer.  RouterC sends hello packet with the DR and BDR are all set
353	   to zero.

[nit] s/sends hello/sends a hello

[nit] s/hello packet/Hello Message/g  That's what rfc7761 uses.


355	   If RouterC cannot send hello packet with the DR/BDR capability,
356	   Router C MAY send the hello packet according to the rule defined in
357	   [RFC7761].

[nit] s/send hello/send a hello

[major] "RouterC cannot send hello packet with the DR/BDR capability"
That condition seems to be true if RouterC doesn't support this
specification.  If so, then this document can't use Normative language
to instruct a router that is not aware of it. s/MAY/may

[major] Even if using "may", why wouldn't RouterC do what rfc7761 specifies?

[nit] s/Router C/RouterC   Please use consistent names.


359	   If deployment requirement is to adopt with a new DR when it is
360	   available, a new router with the highest priority or the highest IP
361	   address sends hello packet with DR and BDR are all set to zero at
362	   first.  It sends hello packet with itself set to DR after it finish
363	   join all the existing multicast groups.  Then current DR compares
364	   with the new router, the new router will be the final DR.

[nit] s/adopt with a new DR/adopt a new DR

[nit] s/sends hello/sends a hello

[major] The election algorithm is not preemptive!  This paragraph then
seems to contradict the specified operation.  There was some
discussion on the list about the ability to optionally pre-empt -- if
that is the intent of this paragraph, then the specification has to be
much more explicit and clear.


366	4.4.  Receiving Hello Messages

368	   When the values of DR and BDR which are carried by hello messages
369	   received are all set to 0x0, the router MUST elect the DR using
370	   procedure defined in DR election algorithm after the hold-timer
371	   expires.  And elect a new BDR which is the best choice except DR.
372	   The election cases can be executed as follows:

[minor] This first paragraph is redundant with the election algorithm
itself.  A reference to the algorithm section should be enough.  BTW,
I assumed this section was going to be about what to do when a Hello
Message is received...not when all the Hello Messages are received
*and* the hole time expires...subtle difference, but the result is
that no action is taken when the Hello Messages are initially
received.


374	   In case the value of DR which is carried by received hello messages
375	   is not 0x0, and the value of BDR is set to 0x0, when the hold-timer
376	   expires there is no hello packet from other router is received, the
377	   router will elect itself to BDR.

[minor] Similar comment as above...


379	   In case either of the values of DR and BDR that are carried by
380	   received hello messages is greater than 0x0.  The router will mark
381	   the current DR, and compare itself with the BDR in the message.  When
382	   the router notices that it is better to be DR than the current BDR.
383	   The router will elect itself to the BDR.

[major] "received hello messages"  Is the action expected when one
Hello is received or only when multiple Hellos are received?

[minor] s/greater than 0x0/not 0x0

[major] The expectation is for the DR/BDR address to be an actual IP
address, right?  IOW, "greater than 0x0" (or even "not 0x0") is
enough.   See my comment on §3.1.

[major] "router will mark the current DR"  What does "mark" mean?

[minor] "the router notices that it is better to be DR than the
current BDR"   The router "notices" by running the election algorithm.
Please be clear and specific.


385	   When a router receives a new hello message with the values of DR and
386	   BDR are set to 0x0.  The router will compare the new router with
387	   current information.  If the router noticed that the new router is
388	   better to be DR than itself, or the new router is better to be BDR
389	   than the current BDR, the router will set the BDR to the new router.

[major] "receives a new hello message with the values of DR and BDR
are set to 0x0.  The router will compare the new router..."  Is a "new
hello" one from a new router?  What is the difference between
receiving 0x0 here and what is written on the first paragraph of this
section.

[major] "set the BDR"   I'm assuming that an action of sending a Hello
goes along with this...but I don't think that is specified anywhere.


391	   When current DR receives hello packet with the value of DR is set
392	   larger than zero, the algorithm defined in section 4.2 can be used to
393	   select the final DR.

[nit] s/value of DR is set/value of DR

[major] "larger than zero"  See the comment above about 0x0...

[major] "the algorithm...can be used"   Only "can"?  It is not optional.


395	   As illustrated in Figure 3, after RouterC sends hello packet, RouterC
396	   will not elect the DR until hold-timer expired.  During the period,
397	   RouterC should receive the hello packets from RouterA and RouterB.
398	   RouterC accepts the result that RouterA is the DR.  In case RouterC
399	   has the lowest priority than RouterA and RouterB, RouterC will also
400	   accept that Router B is the BDR.  In case RouterC has the
401	   intermediate priority among the three routers, RouterC will treat
402	   itself as new BDR after the hold-timer expired.  In case RouterC has
403	   the highest priority among the three routers, RouterC will treat
404	   RouterA which is the current DR as DR, and RouterC will treat itself
405	   as the new BDR.  If the network administrator thinks that RouterC
406	   should be the new DR, the DR changing should be triggered manually.
407	   That is RouterC will be elected as DR after it sends hello message
408	   with DR is set to RouterC itself.

[minor] "As illustrated in Figure 3..."  That is not explained in the
figure; it is explained in §4.3.

[nit] s/sends hello/sends a hello

[nit] s/until hold-timer expired/until the hold-timer expires

[nit] s/During the period/During that period

[nit] s/has the lowest priority/has a lower priority

[minor] s/has the intermediate priority among the three routers/has a
higher priority than RouterB

[minor] "RouterC will treat itself as new BDR"  True...but the rest of
the routers on the LAN should too.  s/treat/elect

[nit] s/In case RouterC has the highest priority among the three
routers, RouterC will treat RouterA which is the current DR as DR, and
RouterC will treat itself as the new BDR./In case RouterC has the
highest priority, RouterA will remain being the DR and RouterC will
elect itself as the new BDR.

[major] Except for the last sentence, this paragraph is a repetition
of what was already explained in §4.3.

[major] "If the network administrator thinks that RouterC should be
the new DR, the DR changing should be triggered manually. That is
RouterC will be elected as DR after it sends hello message with DR is
set to RouterC itself."

What about the other routers?  It is not enough for RouterC to elect
itself as DR, the other routers have to agree.

Note that the algorithm is not pre-emptive!


410	   Exception: In case RouterC receives only the hello packet from
411	   RouterA during the hold-timer period, when the hold-timer expired,
412	   RouterC treats RouterA as DR, and RouterC treats itself as BDR.  In
413	   case RouterC only receives the hello packet from RouterB during the
414	   hold-timer period, RouterC will compare the priority between RouterB
415	   and itself to elect the new DR.  In these situations, some interfaces
416	   or links go wrong in the LAN.

[major] This paragraph doesn't specify an "exception"; it describes a
partitioned LAN!   At best, you can describe this failure scenario in
a deployment section or in the security considerations as a potential
attack.

[major] What if RouterA is not on the LAN, but RouterB advertises it
as DR?  IOW, should there be a check for the advertised DR/BDR to
actually exist?


418	4.5.  The treatment

[major]  I'm not sure what this section is intended to be, but all the
information in it is redundant.

420	   If all the routers on a shared-media LAN have started working at the
421	   same time, then the election result of DR is same as the definition
422	   in [RFC7761].  And all the routers will elect a BDR which is next
423	   best to DR.  The routers in the network MUST store the DR and BDR.
424	   The hello messages sent by all the routers are the same with the
425	   value of DR and BDR are all set.  When a new router is activated on
426	   the shared-media LAN and receives hello messages from other routers
427	   with the value of DR is already set.  The new router will not change
428	   the current DR even if it is superior to the current DR.  If the new
429	   router is superior to current BDR, the new router will replace the
430	   current BDR.

[major] "If all the routers on a shared-media LAN have started working
at the same time, then the election result of DR is same as the
definition in [RFC7761]."  This is an important point to recognize
(and redefine!) the algorithm in §4.2.

[major] "The routers in the network MUST store the DR and BDR."   What
does "MUST store" require?  How does it affect interoperability?

[minor] s/The hello messages sent by all the routers are the same with
the value of DR and BDR are all set./The Hello Messages sent by all
the routers include the DR and BDR Address Options with the same
addresses.


...
438	   As a result, the BDR is the one which has the highest priority except
439	   for DR.  Once the DR is elected, the DR will not change until it
440	   fails or be manually adjusted.  Once the DR and BDR are elected, the
441	   routers in the network MUST store the address of DR and BDR.

[major] "MUST store the address of DR and BDR"   Also repetitive...


443	4.6.  Sender side

445	   DR/BDR function is also used in source side that multiple routers and
446	   source is in a same shared-media LAN.  The algorithm is the same as
447	   the receiver side.  Only the BDR need not build multicast tree from a
448	   downstream router.

[] Sorry, I don't know what you mean to say in this section. :-(


450	5.  Compatibility

[] s/Compatibility/Backwards Compatibility


452	   If the LAN is a hybrid network that there are some routers which
453	   support DR/BDR capability and the other routers which do not support
454	   DR/BDR capability.  All the routers MUST go backward to use the
455	   election algorithm defined in [RFC7761].  And the values of DR and
456	   BDR carried in hello message MUST be set to zero.  That is once a
457	   router sends hello messages with no DR/BDR options, the DR election
458	   MUST go backward to the definition in [RFC7761].

[major] "MUST go backward"  This phrase is not clear on what the action is.

Suggestion>
   If at least one router on a LAN doesn't send a Hello Message which
   includes the DR and BDR Address Options, then the specification in this
   document MUST NOT be used. Any router using the DR and BDR Address
   Options MUST set the corresponding OptionValues to 0x0. This action
   results in all routers using the DR election function defined in
   [RFC7761].


460	   If the routers find that all the routers in the LAN support DR/BDR
461	   capability by the hello messages with DR/BDR options set, they MUST
462	   elect DR and BDR according the algorithm defined in this document.
463	   And the routers MUST send hello messages with correct DR/BDR options
464	   set.

[major] I think that this paragraph is unnecessary because that
(everyone using the algorithm in this document) is the whole point of
this document.


466	   In case there is only one router which does not support DR/BDR
467	   capability in a shared-media LAN, the other routers in the LAN send
468	   hello messages with the values of DR and BDR are set to zero, the
469	   router which does not support DR/BDR capability ignores the options.
470	   All the routers elect DR according to the algorithm defined in
471	   [RFC7761].  When the router which does not support DR/BDR capability
472	   goes away, the routers in the LAN MUST elect DR/BDR according to the
473	   algorithm defined in this document, and send hello messages with
474	   correct DR/BDR options set.

[minor] A significant part of this paragraph is repetitive...the first
sentence is the only one with important information at this point.

Suggestion>
   A router that does not support this specification ignores [RFC7761] unknown
   options.


476	   This draft allows DR election to be sticky by not unnecessarily
477	   changing the DR when routers go down or come up.  This is done by
478	   introducing new PIM Hello options.  Both this draft, and the draft
479	   [I-D.mankamana-pim-bdr], introduce a backup DR.  The latter draft
480	   does this without introducing new options, but does not consider the
481	   sticky behavior.

[See my comment at the top.]


483	6.  Security Considerations

485	   If an attacker which has the highest priority participates in the DR
486	   election when a shared-media LAN starts to work, it will be elected
487	   as DR, but it may not forward flows to receivers.  And the attacker
488	   remains DR position even if a legal router which has a higher
489	   priority joins the LAN.

[major] The problem with the DR not forwarding traffic is not new --
it is the same as in rfc7761.  The new problem introduced by this
document is that the DR election is not pre-emptive.  Please say that.

[major] A related issue is that the router can also change its IP
address to become the DR/BDR.  Please include that.


491	   If an attacker is a newcomer which has a higher priority than the
492	   existed BDR, it will be elected as the new BDR, but it may not
493	   monitor DR, import multicast flows and forward flows to receiver when
494	   DR is down.

[major] This issue is also similar to rfc7761 in the case of a new DR
geting elected: if (going back to the first paragraph) the new DR
doesn't worward traffic...


496	   In order to avoid these situations, source authentication should be
497	   used to identify the validity of the DR/BDR candidates.
498	   Authentication methods mentioned in section 6 RFC7761 can be used.

[major] Authentication doesn't solve this problem!  It only validates
who the router is, not that the parameters are set correctly.  IOW, an
attacker can take over a router (maybe they steal the keys somehow)
and look "normal" in that authentication succeeds.

[] Change the order of the paragraphs so that the first one points at
rfc7761 -- and then follow up with the paragraphs about any new issues
introduced by this document.

First paragraph suggestion>
   [RFC7761] describes the security concerns related to PIM-SM, and points to
   other resources.


500	   And the network administrator should consider the potential BFD
501	   session attack if BFD is used between BDR and DR for DR failure
502	   detection.  The security function mentioned in section 9 RFC5880 can
503	   be used.

[major] If BFD is not a Normative dependency for this document, then
we don't need to mention is here.  Please take this paragraph out.


505	7.  IANA Considerations

507	   IANA is requested to allocate two new code points from the "PIM-Hello
508	   Options" registry within the Protocol Independent Multicast (PIM)
509	   Parameters;

[minor] s/registry within the Protocol Independent Multicast (PIM)
Parameters;/registry: