Hi Camilo, Looking forward to your updated draft!
-- Per On Mon, Oct 20, 2025 at 6:13 PM Camilo Cardona <[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]>> 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]
