Per, Thanks a lot for the detailed review. We had a set of changes in the pipeline for publication before today's cut.. I don’t think any of the changes contradict what you mentioned here. We will get to work on your feedback.
Thanks a lot! Camilo Cardona On 14/10/25, 19:51, "Per Andersson via Datatracker" <[email protected] <mailto:[email protected]>> wrote: Document: draft-ietf-grow-bmp-yang Title: BMP YANG Module Reviewer: Per Andersson Review result: On the Right Track Hi! This is my Early Review of draft-ietf-grow-bmp-yang-05. My conclusion is that the YANG modules are on the right track. Result: On the Right Track Note, I am no BMP or BGP expert so I try to follow the proposed solution but mainly focus on reviewing the document's YANG modules as part of my YANG Doctor review. Major issues: The tree listing for the expanded schema for ietf-bmp, with augments ietf-bmp-bgp-dependencies and ietf-bmp-tcp-dependencies covers 25 pages in the text render of the document. This is extremely difficult to navigate. Please partition the tree listing in some way and explain each part. Furthermore, the tree output is different from what current pyang 2.7.1 generates. Leafrefs are not resolved, but just stated to be "leafref" in the draft. The tcp-options in the tree listing contains more things than what is defined in the YANG model. All YANG Modules are not in the IANA considerations. Add entries for the IETF XML Registry and the YANG Module Name Registration registries. Medium issues: It is stated throughout the document that only one (1) YANG module is defined, but there are three YANG modules defined. State that this document proposes three YANG modules. Section 8. Open issues. Is this still valid? ietf-bmp-tcp-dependencies.yang: In authentication/ao it is stated that Parameters for those are defined as a grouping in the TCP YANG model. Add a reference to RFC 9648. Sections 3, 3.4, 3.4.1.4, and 5.4 (The ietf-bmp-tcp-dependencies YANG module): What does the "BMP model" mean? Be specific with module name instead. (I guess it is the "ietf-bmp" YANG model?) All imports should have proper references, they are missing in some places and need to be updated for i.e. ietf-tcp-common (from I-D to RFC). In the pretext to the YANG modules, list the normative references for each imported YANG modules. ietf-bmp.yang: RFCs 1191, 6991, 7854, 8341, 8343, 8529, 8671, 9069, 9067 ietf-bmp-bgp-dependencies.yang: RFCs 7854, 8177, 8349, 8671, 9069, 9643 I-Ds draft.ietf-idr-bgp-model-17 ietf-bmp-tcp-dependencies.yang: RFCs 5925, 8177, 9643 Validating the YANG modules The YANG modules ietf-bmp, ietf-bmp-bgp-dependencies, and ietf-bmp-tcp-dependencies are named incorrently in the draft The date should match the YANG module's revision date for the following module: [email protected] <mailto:[email protected]> The ".yang" before the at sign should be removed, and the dates should match the YANG modules' revision dates for the following modules: [email protected] <mailto:[email protected]> [email protected] <mailto:[email protected]> The dates should reflect the document's published date. The copyright stanzas need to be updated as well. ietf-bmp.yang: Revision date is wrong. import ietf-bgp-types: Should be iana-bgp-types. import ietf-bgp-types: Note to RFC Editor is wrong, what is XXX? It is not defined what RFC AAAA should reference. The "ietf-bmp" YANG data model is not part of RFC 9196, remove from top level description and replace with placeholder for current document. The IETF Trust Copyright statement has some minor editorial needs in order to be correct - without modification, is permitted pursuant to, and subject to, - the license terms contained in the Revised BSD License set + without modification, is permitted pursuant to, and subject to + the license terms contained in, the Revised BSD License set Instead of copying the leaf "mtu-discovery", isn't it possible to reuse it from the "transport-config" grouping from ietf-bgp-common.yang? Regarding the actions session-reset and session-counter-reset, what is the point in having them as actions instead of rpc:s? Suggest that instrumentation of "rpc-error" is used instead of a custom "outcome" choice. Set "error-info" and related fields accordingly depending on result. Success would just report <ok/>. ietf-bmp-bgp-dependencies.yang: Add reference for ietf-bmp import. import ietf-bgp-types: Should be iana-bgp-types. import ietf-bgp-types: Note to RFC Editor is wrong, what is XXX? It is not defined what RFC AAAA should reference. The "ietf-bmp-bgp-dependencies" YANG data model is not part of RFC 9196, remove from top level description and replace with placeholder for current document. instead of having an eleven levels deep relative path in peer-group and peer-id leafrefs, make the paths absolute. You need to fix the TODO for bmp-data/bmp-monitoring-station/id of course. (Why is schema mount mentioned? An implementation detail?) ietf-bmp-tcp-dependencies.yang: Add reference for ietf-bmp import. The "ietf-bmp-tcp-dependencies" YANG data model is not part of RFC 9196, remove from top level description and replace with placeholder for current document. Nits: For all three modules, there are several changes if you run something like: pyang -f yang --yang-line-length=72 --ietf ietf-bmp.yang > new.yang diff -u ietf-bmp.yang new.yang Have a look at them the diff generated and fix the differences. Use upper case abbreviations for "BGP" and "YANG" everywhere. Suggest replacing AO with TCP-AO for the uninitiated reader, or spelling out the abbreviation the first time used. Section 3.2. TCP Options Suggest to capitalize MUST in the sentence The device must have the feature "tcp-client-keepalives" enabled. In Section 3.3. Don't use "we" in documents. Write something like: In the example in figure 5, an initial-delay of 10 is configured., (...) In Section 3.4.1 Please reword "We'll offer an introduction..." In Section 3.4.1.1 if the device supports the ietf-bgp and ietf-bmp-bgp-dependencies.yang models, Replace with something like if the device supports the "ietf-bgp" and "ietf-bmp-bgp-dependencies" YANG models, This is more consistent with the style usually used when describing YANG model relationships. s/bmp-route-mirroing/bmp-route-mirroring/ -- Per _______________________________________________ GROW mailing list -- [email protected] To unsubscribe send an email to [email protected]
