Telechat Review of draft-ietf-pim-yang-12
review-ietf-pim-yang-12-rtgdir-telechat-farrel-2017-12-30-00

Hello, 

I have been selected as the Routing Directorate reviewer for this draft. The
Routing Directorate seeks to review all routing or routing-related drafts as
they pass through IETF last call and IESG review, and sometimes on special
request. The purpose of the review is to provide assistance to the Routing ADs.
For more information about the Routing Directorate, please see
http://trac.tools.ietf.org/area/rtg/trac/wiki/RtgDir 
Although these comments are primarily for the use of the Routing ADs, it would
be helpful if you could consider them along with any other IETF Last Call
comments that you receive, and strive to resolve them through discussion or by
updating the draft. 

Adrian

Document: draft-ietf-pim-yang-12.txt 
Reviewer: Adrian Farrel
Review Date: 2017-12-30
IETF LC End Date: date-if-known 2017-12-22
Intended Status: Standards Track

==Summary:==

Sorry that this review comes after last call, but the request only reached me on
2017-12-27

I have some minor concerns about this document that I think should be resolved
before publication. 

==Comments:==

This document supplies a collection of YANG modules to configure and monitor a
PIM implementation. The separate modules are intended to make it possible to
operate an implementation that uses only a subset of the features of the
protocol.

This is good work. A relatively easy read despite combining YANG and PIM in one
document.

I would note that it has been a long time since I last looked at PIM, and I was
never an expert. I am also not a YANG expert.

Ultimately the test of this sort of work is to know that it has been
implemented and successfully used to control/operate PIM devices.
The information at https://trac.ietf.org/trac/pim/wiki/yang is a 
good start although does not appear to show wide support for all the
objects in this model.

==Major Issues:==

I expected to see defaults defined in the modules. 2.4 says...
>  Where fields are not genuinely essential to protocol operation, they
>  are marked as optional.  Some fields will be essential but have a
>  default specified, so they need not be explicitly configured.
...which is a strong hint that there should be some defaults, but a
search for the "default2 statement turns up none.

Now, if I look at 7761 (which, surprisingly, I did) I see statements
like...
>  The DR Priority option allows a network administrator to give
>  preference to a particular router in the DR election process by
>  giving it a numerically larger DR Priority.  The DR Priority option
>  SHOULD be included in every Hello message, even if no DR Priority is
>  explicitly configured on that interface.  This is necessary because
>  priority-based DR election is only enabled when all neighbors on an
>  interface advertise that they are capable of using the DR Priority
>  option.  The default priority is 1.
...which is a stronger hint that there should be a least one default
available for the configuration of the local DR priority.

==Minor Issues:==

The introduction could use a citation of RFC 7761.

---

typedef pim-mode has an enum called "other". AFAICS there is no mention
of other modes anywhere else in the document. It might be good to flesh
the description out a little and/or add text to the overview (maybe
2.4?).

---

DR priority is optional on Hello messages. 7761 ss 4.3.2 notes that 
"the following information about the sending neighbor is recorded" and
lists (as well as dr_priority) dr_priority_present which it describes as
"A flag indicating if the DR Priority field was present in the Hello
message."

I can't see how that piece of information is accessed in this model.

==Nits:==

The Document Shepherd write-up will need to explain why >5 front page
authors

---

The document title needs correct capitalisation.

---

Please choose and consistently hyphenate "Protocol Independent
Multicast"

---

Section 1
Your wording...

> YANG [RFC6020] [RFC7950] is a data definition language

...is unusual. Normally say it is a "data modeling language". That is
how 7950 describes YANG.

---

Please fix capitalisation in section headings

---

3.4 and 3.5 you have "This module will cover..."
You should probably use "This module covers..."