[yang-doctors] YANG doctor review of draft-ietf-i2rs-yang-l3-topology-02

["Carl Moberg (camoberg)" <[e-mail]>]

All,

This is my review of draft-ietf-i2rs-yang-l3-topology-02 and the included
YANG modules. I am the assigned YANG doctor for this WG document.

 The reviewed document includes the following YANG modules:
  - A module for Layer 3 Unicast IGP topology (ietf-l3-unicast-igp-topology)
  - A module for OSPF topology (ietf-ospf-topology)
  - A module for IS-IS topology (ietf-isis-topology)

Overall, the document is in reasonable shape (though in need of some language
editing) but needs more attention around its fundamental scope and some
high-level issues.

My top-level feedback is that the relationships between the l3 unicast IGP
topology module and the protocol-specific ones are somewhat under-explained.
I don’t understand if the OSPF and IS-IS models are mere examples or if the
aim is to have them become normative. The modules themselves are not marked
as examples (no 'example-' prefix), but e.g. the OSPF module is referred to
as "[...] an example of how the general topology model can be refined […]"
in running text.

If the protocol-specific modules are to become part of the normative
specification, I believe there needs to be additional work together with the
OSPF and IS-IS WGs and YANG authors to align efforts. IMHO the main topic
being what kind of share or reuse should we do across the topology-centric
models and the respective device layer modules (ietf-ospf, ietf-isis).

If the protocol-specific modules are to be classified as examples, I would
suggest that needs to be clearly stated, and the modules marked up
accordingly.

I believe the above needs to be resolved before a more detailed review of the
OSPF and IS-IS modules and document sections can be done.

The rest of this review focuses on some feedback on the layer 3 unicast IGP
topology section of the draft and associated YANG module (ietf-network-
topology).

1. All data definitions are optional configuration (e.g. no config false,
default values or mandatory statements). This seems to open up for many odd
combinations of e.g. existing and non-existing leafs in node, link, and
termination-points. This is also true for the notifications. Is this by design
or something that needs additional work?

2. I might be missing something subtle here, but the 'network-ref', 'node-
ref', 'link-ref', and 'tp-ref' groupings seems to be copied from ietf-network
and ietf-network-topology. Unless there is some reasoning behind this, it
seems we could just drop them and use imported versions.

3. For the notifications, it is not clear to me what the semantics of the
*-attributes groupings are. Section 3.2 states that "[...] as a convenience to
applications, additional data of the affected node, or link, or termination
point (respectively) is included.". Does this mean that the (optional) content
of e.g. the igp-node-attributes grouping in the igp-node-event notification
always describes the running configuration of the referenced node at the time
of the notification? Also assuming that that point in time is after the
attribute- change event happened. I would suggest we need to be more explicit
in the running text as well as in the description field of the events.

4. Another one on the notifications. The extensibility model of the
notifications seems to be based on augmenting a presence container into the
top-level of each notification. E.g. the isis module examplifies this by
augmenting the isis-topology-type container into /l3t:igp-node-event. This
means that the resulting igp-node-event now has two topology type-containers,
one from the main module (called 'l3-unicast-igp-topology') and one from the
isis-module (called 'isis'). Are there any semantics implicit in the
combinationi (e.g. should isis-related notifications only include the isis-
container)?

5. A couple of smaller nits:
 - This has been discussed (and I haven't seen any formal conclusions), but I
   suggest we don't need value-statements for enums
 - The import prefixes are not aliged with the prefixes in the imported modules:
  + ietf-network is imported with prefix 'nw', while the module itself has 'nd'
  + ietf-network-topology is imported with prefix 'nt', while the module itself
    has 'lnk'

Regards,
--
Carl Moberg
[e-mail]