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..."