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

["Alexander Clemm (alex)" <[e-mail]>]

Hello Carl,

thank you for your thorough review!  

Perhaps the most important comment concerns your top-level comment as to whether the protocol-specific modules, OSPF and IS-IS, are intended as examples or should be normative.  While it was certainly one goal to "exercise" the higher-level l3 Unicast IGP topology model, the intent had been for them to be normative as well.  I agree that we should work with folks in OSPF and ISIS WGs, as considerable time elapsed since when we first embarked on this model to today.  I hope we can synch up with them in person at IETF 96 in Berlin.  Any alignment as a result of this will be presumably addressed after Berlin, so I am not sure we will have an updated draft in a few days, realistically probably more likely in a few weeks instead after we have had a chance to talk.  

Please find further replies inline, <ALEX>

Regards
--- Alex

-----Original Message-----
From: Carl Moberg (camoberg) 
Sent: Monday, July 04, 2016 4:55 AM
To: [e-mail]; YANG Doctors <[e-mail]>
Subject: YANG doctor review of draft-ietf-i2rs-yang-l3-topology-02

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?

<ALEX> This is actually by design, as there is always the issue for leaving flexibility for implementations (I know, this flexibility can be at odds with ease-of-use) but we shall review this. 
</ALEX>

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.

<ALEX> Yes, this needs to be changed.  Those should be reused from ietf-network and ietf-network-topology, not redefined.  
</ALEX>

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.

<ALEX> We need to look at and revise the descriptions to provide clarity.  The purpose of the *-attributes groupings is to be able to include the corresponding data in the notifications.  In each case, the data to be included will be that of the node / link that the notification is sent against, and at the point in time when the notification is generated. 
 </ALEX>

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)?

<ALEX>Semantics implicit in the combination - not really; not sure I understand the question fully.  Basically, it is possible for a topology to be of multiple types, some of it "polymorphically" - i.e. isis and l3unicastigp.  The attributes specific to a type, and introduced at that level, are included within that container.  
</ALEX>

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
<ALEX>Thank, will remove
</ALEX>
 - 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'

<ALEX>Thanks, will fix
</ALEX>

Regards,
--
Carl Moberg
[e-mail]