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

From: "Carl Moberg (camoberg)" <[email-masked]>
Date: Mon, 4 Jul 2016 11:54:47 +0000

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
[email-masked]

---

From: "Susan Hares" <[email-masked]>
Date: Wed, 6 Jul 2016 03:54:06 -0400

Carl: 

Thank you for the yang doctor's review of draft-ietf-i2rs-yang-l3-topology-02.txt. 

Sue 

-----Original Message-----
From: Carl Moberg (camoberg) [mailto:[email-masked]] 
Sent: Monday, July 4, 2016 7:55 AM
To: [email-masked]; YANG Doctors
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.

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

Regards,
--
Carl Moberg
[email-masked]

---

From: "Alexander Clemm (alex)" <[email-masked]>
Date: Thu, 7 Jul 2016 00:58:40 +0000

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: [email-masked]; YANG Doctors <[email-masked]>
Subject: YANG doctor review of draft-ietf-i2rs-yang-l3-topology-02

All,

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

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

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

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

<ALEX>Thank, will remove
</ALEX>

<ALEX>Thanks, will fix
</ALEX>

Regards,
--
Carl Moberg
[email-masked]