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 <noreply@xxxxxxxx> 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 (--).
