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

[Yingzhen Qu <[e-mail]>]

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]> 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]> 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]" 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
> >     (--).
> >
> >
> >
>
>