Re: [Teas] Yangdoctors early review of draft-ietf-teas-yang-l3-te-topo-04

[Xufeng Liu <[e-mail]>]

Hi Mahesh,

Thanks for the review. We are addressing these issues by
https://tools.ietf.org/html/draft-ietf-teas-yang-l3-te-topo-05. Some
explanations are in-line below.

Kind regards,
- Xufeng

On Tue, Jun 18, 2019 at 10:31 PM Mahesh Jethanandani via Datatracker <
[e-mail]> wrote:

> Reviewer: Mahesh Jethanandani
> Review result: On the Right Track
>
> I am not an expert in Traffic Engineering or in topology model. If my
> understanding of the network topology models, feel free to educate me and
> everyone else. This review is looking at the draft from a YANG perspective.
> With that said, 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 for layer 3 traffic engineering
> topologies.
>
> Nits
>
> This sentence construction does not sound right. Possibly add a 'that'
> between
> 'enforce' and 'the'.
>
> "YANG must statements are used to enforce the referenced objects are in the
> topologies of proper type."
>
[Xufeng]: Fixed as suggested.

>
> Some amount of consistency in writing description and presence statement
> would
> be nice. For example, the sentence should start with a capital letter.
> Description statements should start on the same line as the keyword
> 'description' or they should start on a new line. Pick one and stick with
> it
> through out the model.
>
[Xufeng]: Edited as suggested.

>
> Comments:
>
> Having 19 pages of a tree diagram without any explanation for the different
> sections of the tree diagram is hardly helpful. Suggest that --tree-depth
> and
> --tree-path options be used in pyang to reduce the size of the tree and to
> break it up into smaller pieces that can then be explained.
>
[Xufeng]: We are trying to use Section 4 to present the complete tree
diagrams as references, in case that such a lookup is needed as we have
learned before. The pieces of the tree diagrams are described and explained
in Sections 2 and 3 with explanations. Some rephrasing has been done to
clarify in these sections. Please let us know if any portions are missing
or unclear. Thanks.

> There seems to be two grouping with the same name (l3-te-topology-type),
> one
> defined in this module, and other imported. It would be better if they had
> different names. Same for l3-te-topology-attributes. Also, since the two
> grouping defined in the module are used exactly once, they should be
> inlined
> with where they are being used, instead of using the grouping/uses
> statement.
> The justification for using them in notifications as articulated in RFC
> 8346
> does not hold true for this module.
>
[Xufeng]: l3-te-topology-type is defined in ietf-l3-te-topology.yang. Sorry
that we don’t see the duplicate defined in another module. Could you please
double check and let us know where you saw the duplicate? It is true that
the grouping l3-te-topology-type is used exactly once in
ietf-l3-te-topology.yang, but the same grouping is also used in a different
module ietf-l3-te-topology-state.yang, which is the reason why a reusable
grouping is defined.

>
> But perhaps what is most confusing is how the module has been architected.
> Per
> RFC 8345, this module should augment the ietf-network module, which it
> does. It
> inserts a new network-type, which is good. But then for any data nodes
> that are
> specific to this network-type have to be defined. The only thing this
> module
> defines is a network-ref (possibly to the underlay network). No name or id
> is
> defined. If one has to reference this particular node, how would one do
> it? Are
> you saying that the l3-topology-attributes, e.g. name, flag etc. are the
> same
> between ietf-l3-unicast-topology and ietf-l3-te-topology?


[Xufeng]: Yes. An L3 TE topology object is an extension (like a derived
object in OO concept) to the L3 topology (no TE). All attributes in the L3
topology are applicable to the L3 TE topology too. The name and id are
reused. The key leaf is the same. An L3 TE topology object is also an L3
object, but with additional TE-related attributes. The way for a user to
distinguish an L3 TE topology from an L3 topology is to use the
“network-type”.

That somehow
> referencing the ietf-l3-unicast-topology attributes will give me the
> ietf-l3-te-topology attributes??

[Xufeng]: Yes for the common attributes, but an L3 TE topology has more
attributes.

And that the l3-flag-type is the same for both
> the underlay and the overlay??
>
[Xufeng]: The l3-flag-type is the same for L3 topology and TE topology, but
a TE topology is not an underlay or overlay to the L3 topology. An L3 TE
topology is an L3 topology that supports an additional capability, which is
TE.


> The same is true for l3-node-attributes, l3-link-attributes and possibly
> l3-termination-point-attributes.
>
> A pyang compilation of the model with —ietf and —lint option was clean.
> However, a validation of the example reveals errors:
>
> err : Unknown element "provider-id".
> (/ietf-network:networks/network[network-id='example-topo-te'])
>
[Xufeng]: Fixed.

>
> A idnits run of the draft reveals a few issues. Please address them.
>
[Xufeng]: Fixed.

>
>  Miscellaneous warnings:
>
> ---------------------------------------------------------------------------
>
>   == The document seems to lack the recommended RFC 2119 boilerplate, even
>      if it appears to use RFC 2119 keywords.
>
>      (The document does seem to have the reference to RFC 2119 which the
>      ID-Checklist requires).
>   -- The document date (March 11, 2019) is 99 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)
>
>   == Missing Reference: 'RFC3688' is mentioned on line 1799, but not
> defined
>
>   == Missing Reference: 'RFC6020' is mentioned on line 1826, but not
> defined
>
>   ** Obsolete normative reference: RFC 5246 (Obsoleted by RFC 8446)
>
>   ** Obsolete normative reference: RFC 6536 (Obsoleted by RFC 8341)
>
>   ** Obsolete normative reference: RFC 7810 (Obsoleted by RFC 8570)
>
>   ** Downref: Normative reference to an Informational RFC: RFC 7823
>
>   == Outdated reference: A later version (-09) exists of
>      draft-ietf-teas-yang-te-types-06
>
>   == Outdated reference: A later version (-21) exists of
>      draft-ietf-teas-yang-te-topo-19
>
>      Summary: 4 errors (**), 0 flaws (~~), 5 warnings (==), 1 comment (--).
>
>