[yang-doctors] Re: Yangdoctors last call review of draft-ietf-lsr-ospf-admin-tags-16

[Yingzhen Qu <[e-mail]>]

Hi Qiufang,

Thanks for the review and sorry for the delayed response. I've uploaded
version -18 to address your comments, details below.

Thanks,
Yingzhen

On Tue, Mar 12, 2024 at 5:29 AM Qiufang Ma via Datatracker <[e-mail]>
wrote:

> Reviewer: Qiufang Ma
> Review result: Almost Ready
>
> Hi, this is my YANG Doctor review of draft-ietf-lsr-ospf-admin-tags-16, I
> marked the review as “Almost Ready” with a couple of comments and questions
> below.
>
> (1) For the "ietf-ospf-admin-tags" YANG data model, what is the
> consideration
> for the “advertise-prefixes” data node to be defined as a list with only
> one
> child key inside it? Will this list node be populated with new parameters
> in
> the future? Otherwise I think a simpler definition might be the following:
> OLD:
>     augment /rt:routing/rt:control-plane-protocols
>             /rt:control-plane-protocol/ospf:ospf/ospf:areas/ospf:area
>             /ospf:interfaces/ospf:interface:
>       +--rw admin-tags
>          +--rw tags* [tag]
>             +--rw tag                   uint32
>             +--rw advertise-prefixes* [prefix]
>                +--rw prefix    inet:ip-prefix
>
> NEW:
>     augment /rt:routing/rt:control-plane-protocols
>             /rt:control-plane-protocol/ospf:ospf/ospf:areas/ospf:area
>             /ospf:interfaces/ospf:interface:
>       +--rw admin-tags
>          +--rw tags* [tag]
>             +--rw tag                   uint32
>             +--rw advertise-prefix* inet:ip-prefix
>
> There is nothing wrong to be defined in the current draft (e.g., PYANG
> won’t
> complain), but I think it can be hierarchically reduced this way, thoughts?
>
> [Yingzhen]: we've modified the interface configuration. Please see whether
it addressed your comments.


> (2) For the grouping "prefix-admin-tag-sub-tlvs", it is defined and used to
> augment the OSPF YANG data model and the OSPFv3 Extended LSA YANG data
> model.
> There is a list defined inside the grouping definition, with only one child
> declared as leaf-list. I am wondering why the type and length are not
> defined
> inside admin-tag-sub-tlv list? Aren’t they part of the admin tag TLV?
>
> [Yingzhen]: removed the list. As for the type and length, we're following
the style as in
the base OSPF model (RFC 9129). When a tlv is known to an implementation,
the type
and length is used to parse the tlv, so only the content is shown under the
tlv container.
However for unknown TLVs, type and length are shown.


> (3) The reference info inside the “revision” statement for this draft is
> inconsistent with its real title. It is using “RFC XXXX: YANG Data Model
> for
> OSPF Prefix Administrative Tags.”, while it should be “RFC XXXX:
> Extensions to
> OSPF for Advertising Prefix Administrative Tags”.
>
> [Yingzhen]: fixed.


> (4) There are some list/leaf-list data nodes defined in the YANG data model
> with their names defined as plural (e.g., tags, advertise-prefixes,
> admin-tags), but the naming convention is to have the list/leaf-list name
> singular form. See RFC8407bis
> (
> https://www.ietf.org/archive/id/draft-ietf-netmod-rfc8407bis-09.html#section-4.3.1
> )
> for the following: “List identifiers SHOULD be singular with the
> surrounding
> container name plural. Similarly, "leaf-list" identifiers SHOULD be
> singular.”
>
> [Yingzhen]: fixed.

(5) There is a YANG tree diagram included in the draft, an informative
> reference to RFC 8340 should be added in the draft. See RFC8407bis
> (
> https://www.ietf.org/archive/id/draft-ietf-netmod-rfc8407bis-09.html#section-3.4
> )
> for the following: “If YANG tree diagrams are used, then an informative
> reference to the YANG tree diagrams specification MUST be included in the
> document. Refer to Section 2.2 of [RFC8349] for an example of such a
> reference.”
>
> [Yingzhen]: fixed.


> (6) It would be good if the tree structure of the YANG module could be
> defined
> in a separate section/subsection, so that readers are able to navigate to
> the
> overview (i.e., tree structure) of the model quickly if they want. Just a
> suggestion for the authors to consider.
>
> [Yingzhen]: split into sub sections now.


> (7) Please include a note to the RFC editor requesting RFC XXXX and xxxx(or
> even better, use RFC YYYY for draft-ietf-lsr-ospfv3-extended-lsa-yang) is
> replaced with the RFC number that is assigned to the document.
>
> [Yingzhen]:  draft-ietf-lsr-ospfv3-extended-lsa-yang is in Auth48 state,
and will be published as RFC 9587. Once it's published, we'll update the
draft accordingly.

(8) I see a couple of “TBD” throughout the draft, did the authors leave them
> intentional?
>
[Yingzhen]: yes, those are TLV types to be assigned by IANA.

>
> (9) I think it would be useful if some example instance snippets of this
> YANG
> data model can be added, either throughout the document or in an appendix.
> Is
> this something the authors would consider to help understand the use of
> YANG
> module?
>

[Yingzhen]: The configuration is very straightforward. However it's very
difficult to do an example for the database as we don't have an
implementation, so we're not planning to add an example for now.