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

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 <
mjethanandani@gmail.com> escreveu:

> Acee/Yingzhen,
>
> Thanks for addressing my comments.
>
> On Jun 27, 2023, at 1:23 PM, Yingzhen Qu <yingzhen.ietf@gmail.com> 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 <acee.ietf@gmail.com> 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 <
>> noreply@ietf.org> 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
>> > "ietf-ospfv3-extended-lsa@2023-06-08.yang" 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
> mjethanandani@gmail.com
>
>
>
>
>
>
> _______________________________________________
> Lsr mailing list
> Lsr@ietf.org
> https://www.ietf.org/mailman/listinfo/lsr
>

-- 
Renato Westphal