Subject: Re: [RTG-DIR] RtgDir review: draft-ietf-pim-yang-12

From: Xufeng Liu <[email-masked]>
Date: Wed, 28 Feb 2018 20:03:02 +0000

Hi Adrian,

Thank you much for the review and comments. We have posted an updated revision https://tools.ietf.org/html/draft-ietf-pim-yang-15 to address the comments.

Best regards,
- Xufeng

> -----Original Message-----
> From: Adrian Farrel [mailto:[email-masked]]
> Sent: Saturday, December 30, 2017 10:13 AM
> To: [email-masked]
> Cc: [email-masked]; [email-masked]; [email-masked]
> Subject: RtgDir review: draft-ietf-pim-yang-12
> 
> 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.

[Xufeng] All defaults have been added to the model.
> 
> 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.

[Xufeng] The default priority 1 has been added.
> 
> ==Minor Issues:==
> 
> The introduction could use a citation of RFC 7761.
[Xufeng] Sec 2.1. has the following:
   The model covers PIM Sparse Mode [RFC7761], including the Source-
   Specific subset [RFC3569] [RFC4607], Dense Mode [RFC3973], and Bi-
   directional PIM [RFC5015].
Do you think that this is good enough?
> 
> ---
> 
> 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?).
[Xufeng] The "other" mode is for future extensions. YANG can do better for this purpose, so we have change the enumeration type to identity, which is augmentable. The definition of the "other" mode is removed from the model. A statement about the extensibility has also been added to Sec 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.
[Xufeng] We are trying to use the leaf "ro dr-priority?" to capture this. In YANG, we don't always have to use a boolean flag to indicate the existence of a field. An optional YANG leaf may or may not hold a value. The state of being "not present" is used to indicate that "the DR priority was not present", which is equivalent to the flag.
> 
> ==Nits:==
> 
> The Document Shepherd write-up will need to explain why >5 front page authors
> 
> ---
> 
> The document title needs correct capitalisation.
[Xufeng] Fixed.
> 
> ---
> 
> Please choose and consistently hyphenate "Protocol Independent Multicast"
[Xufeng] Fixed.
> 
> ---
> 
> 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.

[Xufeng] Fixed.
> 
> ---
> 
> Please fix capitalisation in section headings
[Xufeng] Fixed.
> 
> ---
> 
> 3.4 and 3.5 you have "This module will cover..."
> You should probably use "This module covers..."
[Xufeng] Fixed.