Hi, Most of my comments are resolved, a few nits remaining.
See inline below. On Wed, Jun 3, 2026 at 5:09 PM Camilo Cardona <[email protected]> wrote: > > 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. Ok. > > > > > > > > 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. Ok. > > > > > > 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. Ok. > > > > > > > > > > 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) Ok. > > > > > > Section 8. Open issues. Is this still valid? > > > [BMPYANGA] The Open Issues section was removed. Ok. > > > > > > > > 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 . Ok. > > > > > > > > > > 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. Ok. > > > > > > 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. Ok. > > > > 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 Missing mentioning normative references to RFCs 7854, 8671, and 9069. > > 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). Missing mentioning normative references to RFCs 2385, 5925, and 9293. A good trick is to add a reference in the preamble to the YANG module, this way the reference is used and not missed when processing the draft. > > > > > > 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. Ok. > > > > > > 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. Ok. > > ietf-bmp.yang: > > > > > > Revision date is wrong. > > [BMPYANGA] Resolved. Ok. > > import ietf-bgp-types: Should be iana-bgp-types. > > [BMPYANGA] Resolved. This seems it is not used anymore? > > import ietf-bgp-types: Note to RFC Editor is wrong, what is XXX? > > > > > > It is not defined what RFC AAAA should reference. > > > [BMPYANGA] Resolved. Not used anymore? > > 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. Ok. > > - 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. Ok. > > > > 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. Ok. > > 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. 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. > > [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. Ok. > > > > 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. Ok. > > > > 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. Ok. Suggest renaming "network-instance-name" to just "name", not duplicating the list identifier in the leaf identifier. > > > > 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. Ok. > > > > Use upper case abbreviations for "BGP" and "YANG" everywhere. > > > > > [BMPYANGA] We cleanup most of them as suggested. Ok. > > Suggest replacing AO with TCP-AO for the uninitiated reader, or spelling > > out the abbreviation the first time used. > > > [BMPYANGA] Resolved Ok. > > Section 3.2. TCP Options > > > > > > Suggest to capitalize MUST in the sentence > > > > > > The device must have the feature "tcp-client-keepalives" enabled. > > > > > [BMPYANGA] Resolved Ok. > > 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. Ok. > > 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. Ok. -- Per > > > > -- > > Per > > > > > > > > > > > > > > > > > > > > _______________________________________________ GROW mailing list -- [email protected] To unsubscribe send an email to [email protected]
