[pim] AD review draft-ietf-pim-group-rp-mapping-05.txt

[Adrian Farrel <[e-mail]>]

Hi,

 I have performed an AD review of your draft.

 Don't panic!

 I review all drafts that I am responsible for before putting them forward
for IETF last call. The main objective is to catch nits and minor issues
that would show up during the last call or in IESG review. The intention is
to help polish your document and make sure it is clean and shiny so that
other reviewers will stick to the technical details.

Most of my comments are pretty trivial, but a couple have more meat on them
and I'd like to see a quick respin of the document before I issue the IETF
last call. As soon as I see a new revision posted, I'll set the ball in
motion.

Of course, all of my issues are up for discussion.

Thanks for the work,
Adrian

---

The RFC Editor is a little sensitive around the boilerplate for RFC 2119
terms.

In Section 2 you have:

   In this document, the key words "MUST", "MUST NOT", "REQUIRED",
   "SHALL", "SHALL NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY",
   and "OPTIONAL" are to be interpreted as described in RFC 2119
   [RFC2119].

idnits points out that the standard boilerplate is:

   The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT",
   "SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY", and "OPTIONAL" in
   this document are to be interpreted as described in RFC 2119
   [RFC2119].

Can you make the change?

---

Can you confirm that there is no requirement to include an IETF Trust
Legal Provisions of 28-dec-2009, Section 6.c(iii) paragraph? In other
words, can you confirm that no disclaimer for pre-RFC5378 work is
required?

You may be OK with this, but it is good to check.

---

Acronyms

It is easy to get so wrapped up in our own technology that we forget that
some of our readers may be less familiar with the acronyms.

The RFC Editor requires that all acronyms except for the ones marked with an
asterisk at their web page
http://www.rfc-editor.org/rfc-style-guide/abbrev.expansion.txt are expanded:
- on first use in the Abstract
- on first use in the rest of the document

Could you take care of this, please.

---

Section 1

   It is critical that each router select the same 'RP' for a specific
   multicast group address. This RP address may correspond to a
   different physical router but it is one logical RP address and must
   be consistent across the PIM domain.  This is usually achieved
   by using the same algorithm to select the RP in all the PIM routers
   in a domain.

In this document you are defining a new algorithm. Unless you are proposing
a simultaneous upgrade of all nodes in the network, this is going to result
in two different algorithms being in use at the same time in the same
domain.

You need to address this point at the very least by explaining that your new
algorithm will produce the same results as the old algorithm so there is no
problem with them both running in the same domain.

But wait! If the new algorithm produces the same result as the old
algorithm, why is it needed? :-)

So, I'm a bit confused :-(

- Is this document updating 4601 by replacing the algorithm?
- Is this document updating 4601 by providing a second algorithm?
- Is this document simply providing an optional second algorithm?
- How does interworking work? I.e., what are the backward
  commpatibility issues and solutions?

The way to address this probably by adding backward compatibility in your
migration situations in "5. Common use cases".

It looks like the point of the draft is that vendors have different
algorithms in choosing an RP today, so for multivendor environments it would
be good to have a consistent approach in choosing an RP. This improves the
4601 algorithm but backward compatibility needs to be discussed.

---

Router implementations and MIB modules

In section 1 you have this text:

   PIM-STD-MIB [RFC5060] has defined an algorithm that allows
   administrators to override Group-to-RP mappings with static
   configuration.  But this algorithm is not completely deterministic,
   because it includes an implementation-specific 'precedence' value.

While the RFC does describe an algorithm, I don't think it is an algorithm
that a PIM router would use to select the RP, rather, it is an algorithm
that a management station can use to work out which node will have been
selected as an RP.

But my confusion about the purpose of this document deepens! In section 6
you list in step 2 you have:

 2. If the Multicast Group Address being looked up is in the SSM
    range or is configured for Dense mode, no Group-to-RP mapping is
    selected, and this algorithm terminates.  Alternatively, a RP
    with address type 'unknown' can be selected.  Please look at
   section #8 for more details on this.

Now, section 8 is about MIB objects. So it describes how the fact that no
Group-to-RP mapping has been selected can be indicated to the management
station in the MIB module.

So I think that maybe step 2 of the algorithm should read...

  2. If the Multicast Group Address being looked up is in the SSM
     range or is configured for Dense mode, no Group-to-RP mapping is
     selected, and this algorithm terminates.  The fact that no
    Group-to-RP mapping has been selected can be represented in the
    PIM-STD-MIB module by setting the address type of the RP to
   'unknown' as described in Section 8.

---

Section 3

OLD
   Existing algorithm defined in PIM-SM (Section 4.7.1 in [RFC4601])
   does not consider following constraints:
NEW
   The existing algorithm defined in PIM-SM (Section 4.7.1 in
   [RFC4601]) does not consider the following constraints:

---

Section 4

There seems to be some contradiction between the second and third bullets.
The second says that dynamic mappings override static ones except under
specific circumstances. The third says that dynamically learned mappings are
always preferred over static configuration.

I think this needs clarification, perhaps through moving the third bullet to
the top of the list and indicating that these are the "general order of
preference".

---

Section 6 - Title

Don't be so timid!
You are defining an algorithm, not simply proposing one.

---

Section 6 - nit

  3.   From the set of all Group-to-RP mapping entries, the subset
        whose group prefix contains the multicast group that is being
        looked up, are selected.

s/are selected/is selected/

---

Section 6

  4.   If there are no entries available, then the Group-to-RP mapping
        is undefined.

Does the algorithm also terminate at this point, and does the same rule
about setting the address type in the MIB module to 'unknown' apply?

---

Section 6.

 7.   From the remaining set of Group-to-RP Mappings we select the
       subset of the entries based on the origin.  Group-to-RP mappings
       learned dynamically are preferred over static mappings.  If the
       remaining dynamic Group-to-RP mappings are from BSR and
       Auto-RP then the mappings from BSR SHOULD be preferred.

Why this sudden use of "SHOULD". You haven't used 2119 language anywhere
else in the algorithm.

I suggest you change this s/SHOULD be/is/  and add "unless..." to give an
explanation of how some other choice might be made.

---

 Section 6.

 9.   If the remaining Group-to-RP mappings were learned through BSR
      and the PIM Mode of the Group is 'PIM-SM' then the hash function
      will be used to choose the RP.  The RP with the highest
      resulting hash value will be selected.

Could you add a forward pointer to section 10 next to the mention of the
"hash function".

---

Section 7

   Group-to-RP mapping algorithm defined in PIM-STD-MIB [RFC5060] does
   not specify the usage of 'pimGroupMappingPrecedence' and
   'pimStaticRPPrecedence' objects in 'pimGroupMappingTable' table
   clearly.  With the newly proposed algorithm in this document, these
   MIB objects would not be required.  So we propose to deprecate these
   MIB objects from PIM-STD-MIB.  Also the newly proposed algorithm in
   this document MUST be preferred over Group-to-RP mapping algorithm
   defined in either PIM-SM[RFC4601] or in PIM-STD-MIB[RFC5060].

 Wow! There is a lot covered in this little section.

 a. If you want to deprecate some MIB objects, I think you need more
    than a sentence hidden in this draft. You would need a full
    revision of RFC 5060. A tidier way to handle this would be an
   erratum on RFC 5060 that points out that the objects are not used
   and updates the Description clauses, but doesn't actually deprecate
   the objects.

b. It seems to me that step 5 of the algorithm in the Description
   clause of pimGroupMappingTable *does* mention how the
   pimGroupMappingPrecedence object is used. Similarly, the
   description clause of pimStaticRPPrecedence seems relatively
   detailed. So perhaps what you want is to clarify the Description
   clauses rather than deprecate the objects?

c. What does it mean to "propose to deprecate" in an RFC?

d. s/the newly proposed algorithm/the algorithm defined/

e. See my previous comments about requiring that this new algorithm
   replaces the one in 4601. While you can require that for new
   implementations you would need to:
   - be clear that this document updates 4601
   - carefully consider how you interwork with legacy implementations

f. If you are also updating the MIB module, you will need to indicate
   that this document updates RFC 5060, but I am not sure that this is
   a very good way to go about things. We would need to consult with a
   MIB expert because I find it a bit confusing if we "hide" the
   update to a MIB Description clause in this document.

   But now I am also worried about what 5060 is trying to do with that
   description clause. If the PIM implementation is capable of working
   out the Group-to-RP mapping, why isn't the RP simply made available
   in a MIB object? Why is the management agent required to read the
   whole table and work it out for itself?

---

Section 8.

Previous comments about this section, but is this an update to a Description
clause in 5060? If so, again, are you updating 5060? Is this the best way to
do it?

---

 Section 9

   In practice, it is not usually necessary to run several dynamic
   Group-to-RP mapping mechanisms in one administrative domain.
   Specifically, interoperation of BSR and Auto-RP is OPTIONAL and not
   recommended by this document.

 a. Delete "In practice".

 b. Are *you* defining that interoperation is OPTIONAL, or is that
   definition elsewhere? If defined elsewhere, please include a
   reference.

c. According to 2119, "NOT RECOMMENDED" == "SHOULD NOT"
   "SHOULD NOT" allows the exceptional "MAY"
    "OPTIONAL" == "MAY"
   So I am not sure that "and not recommended by thus document" adds
   value. Furthermore it is slightly disconcerting since it suggests
   that maybe someone else is maybe recommending it.

 ---

 Section 9

   The algorithm in this document MUST be used.

This is definitely a statement that you are updating RFC 4601

 Then in the following paragraph...

   An implementation of PIM that supports only one mechanism for
   learning Group-to-RP mappings SHOULD also use this algorithm.  The
   algorithm has been chosen so that existing standard implementations
   are already compliant.

 The "SHOULD" appears to contradict the previous "MUST".

 If existing implementations are already compliant with this algorithm then
you surely don't need this new specification. I think this is trying to
answer all of my backward compatibility issues, but it needs some work to
explain the how and why.

---

 Section 10

   It is RECOMMENDED that
   network operators configure only one PIM-Bidir RP for each RP
   Priority.

 Is this updating RFC 5015?

---

Section 11

   An implementation of PIM SHOULD support configuration to block
   specific dynamic mechanism for a valid group prefix range.

This looks like an update to 4601. Is it?

---

Section 12

I'd suggest that this section will not (should not?) get through review by
the Security Directorate. You are proposing a change to the way in which a
PIM node operates. At the very least you should consider how the algorithm
could be gamed/disrupted and why the protocol is protected (by the standard
security mechanisms in 4601). It will turn out that the bottom line is the
same (nothing new needs to be done), but your current text is just too terse
to prove the point.