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."
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.
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.
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.
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? That somehow
referencing the ietf-l3-unicast-topology attributes will give me the
ietf-l3-te-topology attributes?? And that the l3-flag-type is the same for both
the underlay and the overlay??
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'])
A idnits run of the draft reveals a few issues. Please address them.
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 (--).
