Re: [Last-Call] [Lsr] Yangdoctors last call review of draft-ietf-lsr-ospfv3-extended-lsa-yang-14

[Acee Lindem <[e-mail]>]

Thanks for the review Renato - I’ve incorporated both your comments into the latest revision.

Thanks,
Acee

> On Jun 28, 2023, at 09:23, Renato Westphal <[e-mail]> wrote:
> 
> Hi all,
> 
> I think the updated module is looking pretty good. I just have one small piece of feedback to share.
> 
> According to RFC 8362 Section 3.3, the Attached-Routers TLV doesn't support Sub-TLVs. Therefore, I believe it's necessary to remove the "sub-tlvs" list from the "attached-router-tlv" container.
> 
> In the same container, the "Adjacent-neighbor-router-id" leaf-list is capitalized when it shouldn't.
> 
> Regards,
> Renato.
> 
> Em ter., 27 de jun. de 2023 às 20:40, Mahesh Jethanandani <[e-mail] <mailto:[e-mail]>> escreveu:
>> Acee/Yingzhen,
>> 
>> Thanks for addressing my comments. 
>> 
>>> On Jun 27, 2023, at 1:23 PM, Yingzhen Qu <[e-mail] <mailto:[e-mail]>> wrote:
>>> 
>>> Hi Mahesh,
>>> 
>>> We just uploaded version -17 and added a configuration example. Please let us know if you have any other comments.
>>> 
>>> Thanks,
>>> Yingzhen
>>> 
>>> On Mon, Jun 26, 2023 at 1:44 PM Acee Lindem <[e-mail] <mailto:[e-mail]>> wrote:
>>>> Hi Mahesh, 
>>>> 
>>>> Thanks for the review - a lot of good comments. See inline and -16 version.  
>>>> 
>>>> > On Jun 15, 2023, at 5:18 PM, Mahesh Jethanandani via Datatracker <[e-mail] <mailto:[e-mail]>> wrote:
>>>> > 
>>>> > Reviewer: Mahesh Jethanandani
>>>> > Review result: On the Right Track
>>>> > 
>>>> > Document reviewed: draft-ietf-lsr-ospfv3-extended-lsa-yang
>>>> > 
>>>> > Status: On the right track
>>>> > 
>>>> > I have marked it as On the Right Track, because of some of the points discussed
>>>> > below.
>>>> > 
>>>> > Summary:
>>>> > 
>>>> > This document defines a YANG data model augmenting the IETF OSPF YANG model to
>>>> > provide support for OSPFv3 Link State Advertisement (LSA) Extensibility as
>>>> > defined in RFC 8362. OSPFv3 Extended LSAs provide extensible TLV-based LSAs for
>>>> > the base LSA types defined in RFC 5340.
>>>> > 
>>>> > Nits
>>>> > 
>>>> > Please add a section on Instructions to RFC editors stating what they should do
>>>> > with references such as RFC XXXX.
>>>> > 
>>>> > It would be nice to have some consistency between having description and
>>>> > reference statements start on a new line or on the same line as the statement.
>>>> > Right now, they are all over the place.
>>>> > 
>>>> > Some of the descriptions are very cryptic. E.g.
>>>> > 
>>>> >      leaf forwarding-address {
>>>> >        type inet:ipv4-address;
>>>> >        description
>>>> >          "Forwarding address";
>>>> 
>>>> I updated the ones that were brief and cryptic. Note that you almost have to have knowledge of RFC 5340 and RFC 8362 to understand the encodings. 
>>>> 
>>>> 
>>>> > 
>>>> > s/Description/description in the YANG model. Actually, I was surprised that
>>>> > pyang did not complain, but yanglint did.
>>>> > 
>>>> > libyang err : Invalid character sequence "Description", expected a keyword.
>>>> > (Line number 318.) libyang err : Parsing module "ietf-ospfv3-extended-lsa"
>>>> > failed. YANGLINT[E]: Parsing schema module
>>>> > "[e-mail] <mailto:[e-mail]>" failed.
>>>> 
>>>> 
>>>> Fixed - I’m surprised pyang didn’t complain as well. 
>>>> 
>>>> 
>>>> > 
>>>> > s/Addrss/Address/
>>>> 
>>>> Fixed. 
>>>> 
>>>> 
>>>> > 
>>>> > s/E-/Extended / in all descriptions.
>>>> 
>>>> When referring to the actual LSAs, it is should be “E-“. For example, E-Router-LSA. In other cases, it is spelled out. See RFC 8362. 
>>>> 
>>>> 
>>>> > 
>>>> > Comments:
>>>> > 
>>>> > The grouping such as ospfv3-e-lsa-as, ospfv3-e-lsa-area, ipv6-fwd-addr-sub-tlv
>>>> > etc. are used in one place only. Is there a reason why this has not been pulled
>>>> > inline where it is used? Did not check for all groupings, but if there is only
>>>> > one use of them, ideally they should be inlined.
>>>> 
>>>> I consolidated these for the link, area, and AS scoped LSDBs. I left the fowarding-address Sub-TLV in its own grouping consistent with the other Sub-TLVs. 
>>>> 
>>>> 
>>>> 
>>>> > 
>>>> > No need to repeat parent name in the child. Just length will do in the
>>>> > following. See Section 4.3.1 of RFC 8407. E.g.
>>>> > 
>>>> >    container route-tag-sub-tlv {
>>>> >      description
>>>> >        "Route Tag Sub-TLV";
>>>> >      leaf route-tag-sub-tlv-length {
>>>> 
>>>> Fixed.
>>>> 
>>>> 
>>>> > 
>>>> > Why a double -- in  container unknown--tlv {?
>>>> 
>>>> Fixed.
>>>> 
>>>> > 
>>>> > A pyang compilation of the model with —ietf and —lint option was clean.
>>>> > 
>>>> > There are no examples of configuration instance data in the draft. It would be
>>>> > helpful not only to validate the model, but also help folks who want to use the
>>>> > model.
>>>> 
>>>> There are only two booleans that are config=true. We can look at this though.
>>>> 
>>>> > 
>>>> > A idnits run of the draft reveals a few issues. Please address them.
>>>> > 
>>>> >   Checking boilerplate required by RFC 5378 and the IETF Trust (see
>>>> >  https://trustee.ietf.org/license-info):
>>>> >  ---------------------------------------------------------------------
>>>> > 
>>>> >     No issues found here.
>>>> > 
>>>> >  Checking nits according to
>>>> >  https://www.ietf.org/id-info/1id-guidelines.txt:
>>>> >  ---------------------------------------------------------------------
>>>> > 
>>>> >     No issues found here.
>>>> > 
>>>> >  Checking nits according to https://www.ietf.org/id-info/checklist :
>>>> >  ---------------------------------------------------------------------
>>>> > 
>>>> >     No issues found here.
>>>> > 
>>>> >  Miscellaneous warnings:
>>>> >  ---------------------------------------------------------------------
>>>> > 
>>>> >  == The copyright year in the IETF Trust and authors Copyright Line
>>>> >     does not match the current year
>>>> > 
>>>> >  == Line 1266 has weird spacing: '... allows  a rou...'
>>>> > 
>>>> >  -- The document date (October 17, 2019) is 1337 days in the past.
>>>> >     Is this intentional?
>>>> > 
>>>> >  Checking references for intended status: Proposed Standard
>>>> >  ---------------------------------------------------------------------
>>>> > 
>>>> >     (See RFCs 3967 and 4897 for information about using normative
>>>> >     references to lower-maturity documents in RFCs)
>>>> > 
>>>> >  == Outdated reference: draft-ietf-bfd-yang has been published as RFC
>>>> >     9127
>>>> > 
>>>> >  ** Downref: Normative reference to an Experimental RFC: RFC 1765
>>>> > 
>>>> >  ** Downref: Normative reference to an Experimental RFC: RFC 4973
>>>> > 
>>>> >  ** Downref: Normative reference to an Informational RFC: RFC 5309
>>>> > 
>>>> >  ** Downref: Normative reference to an Informational RFC: RFC 5714
>>>> > 
>>>> >  ** Downref: Normative reference to an Informational RFC: RFC 6987
>>>> 
>>>> 
>>>> These idnits are fixed.
>>>> 
>>>> 
>>>> Thanks,
>>>> Acee
>>>> 
>>>> 
>>>> > 
>>>> >     Summary: 5 errors (**), 0 flaws (~~), 3 warnings (==), 1 comment
>>>> >     (--).
>>>> > 
>>>> > 
>>>> > 
>>>> 
>> 
>> 
>> Mahesh Jethanandani
>> [e-mail] <mailto:[e-mail]>
>> 
>> 
>> 
>> 
>> 
>> 
>> _______________________________________________
>> Lsr mailing list
>> [e-mail] <mailto:[e-mail]>
>> https://www.ietf.org/mailman/listinfo/lsr
> 
> 
> -- 
> Renato Westphal