Subject: Genart last call review of draft-ietf-pim-source-discovery-bsr-07

From: Stewart Bryant <[email-masked]>
Date: Tue, 09 Jan 2018 12:16:52 -0800

Reviewer: Stewart Bryant
Review result: Ready with Nits

I am the assigned Gen-ART reviewer for this draft. The General Area
Review Team (Gen-ART) reviews all IETF documents being processed
by the IESG for the IETF Chair.  Please treat these comments just
like any other last call comments.

For more information, please see the FAQ at

<https://trac.ietf.org/trac/gen/wiki/GenArtfaq>.

Document: draft-ietf-pim-source-discovery-bsr-07
Reviewer: Stewart Bryant
Review Date: 2018-01-09
IETF LC End Date: 2018-01-10
IESG Telechat date: 2018-01-25

Summary: A well written document, but which could usefully be polished to make
it even better.

Major issues: None

Minor issues:

   E.g., if
   some information needs to be sent every 60 seconds and a triggered
   PFM is about to be sent 20 seconds before the next periodic PFM was
   scheduled, the triggered PFM might include the periodic information
   and the next periodic PFM can then be scheduled 60 seconds after
   that, rather than 20 seconds later.

SB> I am not sure is the complexity of this optimization is worth the
SB> gain. In general optimizations are a source of implementation bugs
SB>

Nits/editorial comments:

In the Abstract  RP need expanding.

topology changes, including frequest link or router failures. (frequent
incorrectly spelled)

no bandwidth left for prune message (message should be plural)

If all the TLVs in the
message are boundary TLVs, then the message is effectively ignored.
SB> Ignored or discarded?

Then a couple of lines later:
If the message was discarded or all the
TLVs were ignored, then no message is forwarded.
SB> You have both terms (ignored and discarded) which confuses me.

In 3.4.1 you pull 60s out of a hat. All becomes clearer later. It would be 
better for the reader if you explained that this was your choice of default
timer before first use.

   if the most significant bit in the type field is set (the type value
   is larger than 32767) and this system does not support the type, then
   that particular type should be omitted from the forwarded messages.
SB> What the authors describe is fine and correct, but it might be
SB> cleaner to call the top bit the "omit bit" or some memorable name.
SB>  Of course the fact that it is the top bit of a TLV type make make such
SB> a definition strange.

A Group Source Holtime ( typo Holdtime)

  The PFM messages containing the GSH
   TLV are periodically sent for as long as the multicast source is
   active, similar to how PIM registers are periodically sent.  The
   default announcement period is 60 seconds,

SB> This should be called out earlier in the text to show why you keep
SB> using the 60s value.

  The holdtime for the source is by default 210
   seconds.  Other values MAY be configured, but the holdtime MUST be
   either zero, or larger than the announcement period.  It is
   RECOMMENDED to be 3.5 times the announcement period.  A source MAY be
   announced with a holdtime of zero to indicate that the source is no
   longer active.

SB> This would be clearer if you started with the 3.5 AP recommendation
SB> and then got to 210s

  When a new PIM neighbor is detected, or an existing neighbor changes
   GenID, an implementation MAY send a triggered PFM message containing

SB> GenID seems to be used but not defined.

   This document requires the assignment of a new PIM message type for
   the PIM Flooding Mechanism (PFM).

SB> It is normal to be more explicit in describing the IANA action.

   IANA is also requested to create a
   registry for PFM TLVs,

SB> It is usual to specify the namespace for the registry.

   with type 0 assigned to the "Source Group
   Holdtime" TLV.

SB> use of zero is a common cause of uninitialized variable error.

---

From: Stig Venaas <[email-masked]>
Date: Tue, 16 Jan 2018 12:51:48 -0800

Hi, thanks for the detailed review. I've tried to address most of
these comments, please see inline.

> SB> gain. In general optimizations are a source of implementation bugs

Agree in general, but I'm trying to leave this open to the
implementation. In my implementation it was easier to include all the
information than just the triggered message. From protocol perspective
it should not be necessary to restrict this.

> In the Abstract  RP need expanding.

Done

> topology changes, including frequest link or router failures. (frequent

Done

> no bandwidth left for prune message (message should be plural)

Done

> SB> You have both terms (ignored and discarded) which confuses me.

My thinking is that one can discard a message by basically throwing it
away without processing it. While a TLV is something that can be
ignored, not discarded. In the text above what you quoted I also talk
about when a message might be discarded, and when TLVs should be
ignored.

> better for the reader if you explained that this was your choice of default

I added text that the behavior, and the value of 60s, is similar to
what is defined for the No-Forward bit in RFC 5059.

> SB> a definition strange.

I've now changed this to define a "Transitive Bit", and making the
type space 15 bits.

> A Group Source Holtime ( typo Holdtime)

Thanks

> SB> This should be called out earlier in the text to show why you keep

Earlier I only mentioned 60s related to a new neighbor coming up,
which is unrelated to this value, and one example.

> SB> This would be clearer if you started with the 3.5 AP recommendation

Done

> SB> GenID seems to be used but not defined.

Done

> SB> It is usual to specify the namespace for the registry.

I now included the exact name and initial content of the registry.

> SB> use of zero is a common cause of uninitialized variable error.

Right. I now reserved 0, and specified 1 for this TLV.

I'll submit a new revision today. Have another look if you like and
let me know if you have any further comments.

Thanks,
Stig