Last Call Review of draft-ietf-pim-msdp-yang-12
review-ietf-pim-msdp-yang-12-yangdoctors-lc-rahman-2020-01-28-00

Request Review of draft-ietf-pim-msdp-yang
Requested rev. no specific revision (document currently at 18)
Type Last Call Review
Team YANG Doctors (yangdoctors)
Deadline 2020-01-30
Requested 2020-01-15
Requested by Alvaro Retana
Authors Xufeng Liu, Zheng Zhang, Anish Peter, Mahesh Sivakumar, Feng Guo, Pete McAllister
Draft last updated 2020-01-28
Completed reviews Yangdoctors Early review of -13 by Reshad Rahman (diff)
Yangdoctors Last Call review of -16 by Reshad Rahman (diff)
Yangdoctors Last Call review of -12 by Reshad Rahman (diff)
Rtgdir Last Call review of -08 by Yingzhen Qu (diff)
Secdir Last Call review of -12 by Vincent Roca (diff)
Assignment Reviewer Reshad Rahman 
State Completed
Review review-ietf-pim-msdp-yang-12-yangdoctors-lc-rahman-2020-01-28
Posted at https://mailarchive.ietf.org/arch/msg/yang-doctors/9gQjj2enJ0olkvJoaPBF3vuz7oo
Reviewed rev. 12 (document currently at 18)
Review result Ready with Issues
Review completed: 2020-01-28

Review
review-ietf-pim-msdp-yang-12-yangdoctors-lc-rahman-2020-01-28

YANG Doctor review of draft-ietf-pim-msdp-yang-12 by Reshad Rahman

1 module in this draft:
- ietf-msdp@2020-01-24.yang

No YANG validation errors or warnings

Thank you for addressing comments which were provided on rev-01 of the document.

- Page 21, the YANG module has the augmentation of “/rt:…/rt:control-plane-protocol” but there is a missing when statement. This means that on a device which supports this YANG model, the “msdp” container node will appear in all instances of “rt:control-plane-protocol”, even the non-MSDP ones. If you need an example of how to fix this with when statement, please take a look at OSPF and BFD YANG models.

- There are no examples. Just a couple of simple XML examples would help a lot.

- There are IMO still too many (14?) features for a fairly straight-forward YANG model. An explanation for this is provided in section 3.1, but this does not comply with 4.17 of RFC8407:
   The set of YANG features defined in a module should be considered
   carefully.  Very fine granular features increase interoperability
   complexity and should be avoided.  A likely misuse of the feature
   mechanism is the tagging of individual leafs (e.g., counters) with
   separate features.


- Page 12, the if-feature was taken out of password (to address a comment regarding duplicate if-feature), but I believe the remaining one should be moved up from case key-chain to container authentication.

- There is an as-number leaf in the YANG model, but no such thing in RFC3618. Do we need a reference here?

- Page 24, RPC clear-sa-cache has source-addr using type ipv4-multi-cast-source-address. But in the operational model (container sa-cache), source-addr is a union of either * or ipv4-address. Why the difference? Same question, wrt inconsistency, for leaf group (ipv4-multicast-group-address in RPC and ipv4-address in operational model). 

- Page 22, rp-address is of type ip-address, should that be ipv4-address just like rpf-peer? Or am I misunderstanding this?

- Many of the descriptions are still very terse, e.g. up-time, expire.

- I’m not an MSDP expert, but I believe adding, where appropriate in the YANG module, more references to the appropriate sections of RFC3618 or other PIM RFCs would improve the document. e.g. this could help for RPF-related nodes.

- Section 6: s/one new URIs/one new URI/