Hello Per, Thanks again for your review. We published a new version with several changes based on it.
We will respond here with the comments inline [BMPYANGA], trying to make sure we are not missing anything. Thanks, Camilo & BMP YANG authors On 20/10/2025, 13:27, "Per Andersson" <[email protected] <mailto:[email protected]>> wrote: Hi Camilo, Looking forward to your updated draft! -- Per On Mon, Oct 20, 2025 at 6:13 PM Camilo Cardona <[email protected] <mailto:[email protected]>> wrote: > > 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]> <mailto:[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. > [BMPYANGA] We already had sections explaining each part of the model. We inserted a partial tree of the model's section on each of them. > > > > 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. > [BMPYANGA] Leafrefs are now shown resolved in the tree output. We also regenerated/checked the partial trees against the current modules. > > > > All YANG Modules are not in the IANA considerations. Add entries for the > IETF XML Registry and the YANG Module Name Registration registries. [BMPYANGA] We added the modules in the IANA consideration, 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. > > [BMPYANGA] We fixed all reference on the document to single YANG modules. The document now describes the draft as containing YANG data model specified by two YANG modules: ietf-bmp and ietf-bmp-tcp-dependencies (we removed the ietf-bmp-bgp-dependencies) > > > Section 8. Open issues. Is this still valid? [BMPYANGA] The Open Issues section was removed. > > > > > 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. > > [BMPYANGA] The TCP dependency text and references were updated to referto 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?) > > [BMPYANGA] We updated the text to distinguish the overall BMP YANG data model from the specific ietf-bmp YANG module. > > > 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). > [BMPYANGA] Updated. > > 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 > > [BMPYANGA] We added the references to the two modules that remained in the draft (ietf-bmp.yang and ietf-bmp-tcp-dependencies.yang). > > > 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]> > <mailto:[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]> > <mailto:[email protected] > <mailto:[email protected]>> > [email protected] > <mailto:[email protected]> > <mailto:[email protected] > <mailto:[email protected]>> > [BMPYANGA] Resolved. > > > > The dates should reflect the document's published date. > > > The copyright stanzas need to be updated as well. > [BMPYANGA] The CODE BEGINS filenames now match the YANG module revision dates. We forgot to update the IETF copyright year, but we'll do it on the next version. > > > > ietf-bmp.yang: > > > Revision date is wrong. [BMPYANGA] Resolved. > > > import ietf-bgp-types: Should be iana-bgp-types. [BMPYANGA] Resolved. > > > import ietf-bgp-types: Note to RFC Editor is wrong, what is XXX? > > > It is not defined what RFC AAAA should reference. > [BMPYANGA] Resolved. > > 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 > [BMPYANGA] Resolved. > > - 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? > > [BMPYANGA] We considered this, but did not change it in this revision. Since we removed the BGP dependency module, we preferred to keep this modeling independent rather than adding a dependency on the BGP YANG model just for this. > > > Regarding the actions session-reset and session-counter-reset, > what is the point in having them as actions instead of rpc:s? > [BMPYANGA] We kept session-reset and session-counter-reset as actions because they are scoped to a specific BMP monitoring-station/session context. > > 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/>. > > [BMPYANGA] Addressed. The custom outcome modeling was removed, and now failures can use the normal RPC/action error reporting behavior. > > > 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. [BMPYANGA] The ietf-bmp-bgp-dependencies module was removed. We don’t want this model to depend on the BGP one (at least for an initial version) since that will facilitate implementation. > > > 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. > > [BMPYANGA] These leafrefs were replaced with absolute paths. > > > You need to fix the TODO for bmp-data/bmp-monitoring-station/id of > course. (Why is schema mount mentioned? An implementation detail?) > > [BMPYANGA] We removed the TODOs. > > > 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. > > [BMPYANGA] Resolved. > > > Use upper case abbreviations for "BGP" and "YANG" everywhere. > > [BMPYANGA] We cleanup most of them as suggested. > > > Suggest replacing AO with TCP-AO for the uninitiated reader, or spelling > out the abbreviation the first time used. > [BMPYANGA] Resolved > > > > Section 3.2. TCP Options > > > Suggest to capitalize MUST in the sentence > > > The device must have the feature "tcp-client-keepalives" enabled. > > [BMPYANGA] Resolved > > > > > In Section 3.3. > > > Don't use "we" in documents. Write something like: > [BMPYANGA] We edited most (if not all) of them in the document. > > 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/ > > [BMPYANGA] Resolved. > > > -- > Per > > > > > > > > _______________________________________________ GROW mailing list -- [email protected] To unsubscribe send an email to [email protected]
