Hi Éric, Thanks for your review...
On Wed, Dec 18, 2024 at 3:05 PM Éric Vyncke via Datatracker < [email protected]> wrote: > Éric Vyncke has entered the following ballot position for > draft-ietf-pce-pcep-yang-27: Discuss > > When responding, please keep the subject line intact and reply to all > email addresses included in the To and CC lines. (Feel free to cut this > introductory paragraph, however.) > > > Please refer to > https://www.ietf.org/about/groups/iesg/statements/handling-ballot-positions/ > for more information about how to handle DISCUSS and COMMENT positions. > > > The document, along with other ballot positions, can be found here: > https://datatracker.ietf.org/doc/draft-ietf-pce-pcep-yang/ > > > > ---------------------------------------------------------------------- > DISCUSS: > ---------------------------------------------------------------------- > > > # Éric Vyncke, INT AD, comments for draft-ietf-pce-pcep-yang-27 > CC @evyncke > > Thank you for the work put into this document. > > Please find below two blocking DISCUSS points (easy to address), some > non-blocking COMMENT points (but replies would be appreciated even if only > for > my own education), and some nits. > > Special thanks to Julien Meuric for the shepherd's succinct write-up > including > the WG consensus, *but it lacks* the justification of the intended status. > > I hope that this review helps to improve the document, > > Regards, > > -éric > > ## DISCUSS (blocking) > > As noted in https://www.ietf.org/blog/handling-iesg-ballot-positions/, a > DISCUSS ballot is just a request to have a discussion on the following > topics: > > ### Section 8.1 > > ``` > feature sr { > description > "Support Segment Routing (SR) for PCE."; > ``` > > and later > > ``` > container sr { > if-feature "sr"; > description > "If segment routing for MPLS is supported at the local > entity or a peer."; > ... > leaf enabled { > type boolean; > default "false"; > description > "Set to true if SR-MPLS is enabled"; > } > ``` > > The feature and container should really be renamed in sr-mpls and the > description of feature sr updated to indicate SR-MPLS. > > Dhruv: We were following the naming that was used in RFC 8664, but I can see how that could be confusing in the YANG model. I have updated as per your suggestion. > ``` > leaf pcc-id { > type inet:ip-address-no-zone; > description > "The local internet address of the PCC, that > generated the PLSP-ID."; > } > ``` > > What is a `local internet address` ? > > Dhruv: changed to local IP address > > ---------------------------------------------------------------------- > COMMENT: > ---------------------------------------------------------------------- > > > ## COMMENTS (non-blocking) > > ### Section 1 > > I support Mahesh's DISCUSS, so I won't raise the following point to a block > DISCUSS. > > Penultimate paragraph writes `PCEP statistics YANG module "ietf-pcep-stats" > which provides statistics, counters and telemetry data`. While the last > one has > `The PCEP operational state is included in the same tree as the PCEP > configuration consistent with NMDA`. For me, telemetry (and statistics) are > crucial for operations, i.e., for the operational state. The end of > section 1 > appears to be self-contradicting... > > Dhruv: I rephrased the NMDA sentence to "The YANG modules in this document conform to the Network Management Datastore Architecture (NMDA) [RFC8342]." > ### Section 4 > > `oper-status` is within the configuration part, shouldn't it be in another > branch of the tree ? As I am not a YANG expert, I can be really wrong. > > Dhruv: After NMDA we stopped doing that! > It appears that the next sub-sections are describing part of the tree, > please > have some describing this approach (which I support) and have lead text per > sub-section about which part(s) of the global tree is further detailed. > > Dhruv: Ack > ### Section 4.1.1 > > As I am not a PCEP expert, I really wonder why a PCEP speaker cannot be > associated by multiple IP addresses (even if only being dual-stack) > > Dhruv: Will change to a leaf-list. > ### Section 5 > > As this YANG model has two modules (see above), should there be any text or > consistency constraints about the future revision of one module, i.e., can > one > module be revised while the other is not ? > > Dhruv: This is per the usual YANG update and revision rules. I dont see a need to add anything specific. > ### Section 6 > > `Segment Routing in the IPv6 data plane is out of the scope of this > document.`, > this is a pity as SR-MPLS is included (see below). This makes the review > and > the ballot of this data model and the one for SRv6 very difficult... if the > goal is to have some consistency between SR-MPLS & SRv6... > > Dhruv: The SRv6 for PCEP in YANG is in progress, it is a WG adopted draft. > ### Section 8.1 > > s/identity isis-area/identity is-is-area/ ? > > Dhruv: IS-IS YANG RFC 9130 uses "isis" for the leaf names, and thus this is consistent with that. > ### Section 12 > > s/We would like to thank/The authors would like to thank/ ? > > Dhruv: Ack > ### Appendix B > > Rather than having an example with IPv4 and adding `Similarly a PCEP > session > with IPv6 address between PCE (2001:DB8::3) and a PCC (2001:DB8::4) could > also > be setup` could this happen the other way ? IPv6 example and some text > about > IPV4 ? After all, the RFC will be published in 2025 ;-) > > Also, please use RFC 5952 for IPv6 addresses. > > Dhruv: I reworked the example. > ## NITS (non-blocking / cosmetic) > > ### YANG is in uppercase > > There is at least one occurrence of lowercase "yang" in the section 4.1, > please > use uppercase everywhere. > > > Dhruv: Ack URL: https://www.ietf.org/archive/id/draft-ietf-pce-pcep-yang-28.txt Status: https://datatracker.ietf.org/doc/draft-ietf-pce-pcep-yang/ HTMLized: https://datatracker.ietf.org/doc/html/draft-ietf-pce-pcep-yang Diff: https://author-tools.ietf.org/iddiff?url2=draft-ietf-pce-pcep-yang-28 Thanks! Dhruv
_______________________________________________ Pce mailing list -- [email protected] To unsubscribe send an email to [email protected]
