Subject: [netmod] comments to draft-clemm-netmod-yang-network-topo-00

From: Ladislav Lhotka <[email-masked]>
Date: Wed, 21 Aug 2013 18:27:57 +0200

Hi,

here are my comments to the draft, oriented mainly on YANG aspects.

It is a remarkable and promising work that might be generally
useful, i.e. not only for I2RS. An interesting exercise would be to
use an instance topology description and generate configurations of
individual network nodes from it.

The data model doesn't fit to the standard context of NETCONF
client-server interaction. This shows that YANG can indeed be
useful outside its original domain, and, at the same time, that
certain YANG idiosyncrasies and CLRs mean complications for
non-NETCONF data modelling (details follow).

Specific comments:

*** topology-type
    The use of XML element hierarchies is a clever alternative to YANG
    identities. Unlike identities, this works perfectly in standard
    XPath expressions, and also supports multiple inheritance. The
    only potential drawback is that the complete hierarchy of types
    has to be specified with each instance – in this particular case
    it is not an issue though.

*** config true/false
    It is difficult to decide a priori which nodes should be defined as
    configuration and which should be operational state. For example,
    one could imagine that the basic topology is given by the
    physical network (hence operational state) while the IGP topology
    and above can be configured. However, other possibilities are
    plausible, too. I'd suggest to ignore this disctinction for the
    time being, and define everything as configuration, which means
    there are almost no restrictions with respect to XPath context,
    leafrefs etc.

*** augments
    The "augment" statement was originally intended for ad hoc and
    optional stuff, so augmenting mandatory nodes is not
    allowed. However, "augment" became an essential tool for building
    data models in a top-down way (not only in this case), and the
    "no-mandatory-nodes" rule is an annoying limitation.

*** semantic constraints
    Many of the XPath expressions in "must" or "when" are
    incorrect. Attached is a diff file that shows improved versions
    (at least they work for my simple examples).

    In general, if a semantic constraint is to be applied to all list
    entries, it should be specified under the "list" node. Specifying
    it under the parent container usually means that it is sufficient
    to satisfy the constraint for a single entry.

    The "when" condition inside the grouping "ospf-prefix-attributes"
    (module "ospf-topology") seems completely broken.

    The explanations of the constraints should be moved from comments
    to "description" statements, and "error-message" should be
    specified, too.

*** regex patterns

    The pattern for "iso-net-id" is invalid (the asterisk at the end):
    OLD
        pattern '[0-9a-fA-F]{2}((.[0-9a-fA-F]{4}){6}*)';
    NEW
        pattern '[0-9a-fA-F]{2}((\.[0-9a-fA-F]{4}){6})';

    Maybe it is legal to use "." (dot) unescaped in this case, but I'd
    suggest to escape it, just to be sure (also in "iso-system-id").

*** prefix of an imported module
    The module l3-unicast-igp-topology declares its prefix as "l3t"
    but the {ospf|isis}-topology modules import it with the prefix
    "igp". This should be avoided, also because it breaks some tools
    (I know, it shouldn't, but ...).

*** additional containers
    I understand the lists "node" and "link" were not put into
    containers in order to keep the paths short. This could get quite
    messy though, because it means the lists can be interleaved with
    each other (and with other sibling elements, too). For example,
    this order is possible: link, node, link, topology-id, node, ...

*** IGP metric
    The "metric" parameter could be defined separately in
    "ospf-topology" and "isis-topology", and specify
    the size (3 bytes) there, perhaps by means of refining a grouping
    defined in l3-unicast-igp-topology - see the comment inside the
    "igp-link-attributes" grouping.

*** Minor nits
    - The module namespace URIs are inconsistent, some use the "TBD"
      component, some don't.
    - For link source and destination, shouldn't the source-tp/dest-tp
      leafs be mandatory?
    - Do the "supporting-*" nodes have to be containers with a
      "*-ref" leaf inside?
    - Using the "prefix" key for the "prefix" list is legal but maybe
      a little confusing. I'd suggest to rename the list to
      "prefix-spec" or similar.
    - Another "must" may be needed to ensure that router-ids are unique.
    - "underlay-topology" could be renamed to "supporting-topology"
      because it represents the same relationship as "supporting-{node|link}".

Lada

diff --git a/isis-topology.yang b/isis-topology.yang
index a633e39..57a44ee 100644
--- a/isis-topology.yang
+++ b/isis-topology.yang
@@ -7,7 +7,7 @@ module isis-topology {
         prefix nt;
     }
     import l3-unicast-igp-topology {
-        prefix igp;
+        prefix l3t;
     }
     import ted {
         prefix ted;
@@ -47,26 +47,26 @@ module isis-topology {
         }
     }
 
-    augment "/nt:network-topology/nt:topology/nt:topology-types/igp:l3-unicast-igp-topology" {
+    augment "/nt:network-topology/nt:topology/nt:topology-types/l3t:l3-unicast-igp-topology" {
         uses isis-topology-type;
     }
 
-    augment "/nt:network-topology/nt:topology/igp:igp-topology-attributes" {
-        when "../../topology-types/isis";
-        container isis-topogloy-attributes {
+    augment "/nt:network-topology/nt:topology/l3t:igp-topology-attributes" {
+        when "../nt:topology-types/l3t:l3-unicast-igp-topology/isis";
+        container isis-topology-attributes {
             leaf net {
                 type iso-net-id;
             }
         }
     }
 
-    augment "/nt:network-topology/nt:topology/nt:node/igp:igp-node-attributes" {
-        when "../../../topology-types/isis";
+    augment "/nt:network-topology/nt:topology/nt:node/l3t:igp-node-attributes" {
+        when "../../nt:topology-types/l3t:l3-unicast-igp-topology/isis";
         uses isis-node-attributes;
     }
 
-    augment "/nt:network-topology/nt:topology/nt:link/igp:igp-link-attributes" {
-        when "../../../../topology-types/isis";
+    augment "/nt:network-topology/nt:topology/nt:link/l3t:igp-link-attributes" {
+        when "../../nt:topology-types/l3t:l3-unicast-igp-topology/isis";
         uses isis-link-attributes;
     }
 
@@ -128,12 +128,12 @@ module isis-topology {
         }
     }
 
-    augment "/igp:igp-node-event" {
+    augment "/l3t:igp-node-event" {
         uses isis-topology-type;
         uses isis-node-attributes;
     }
 
-    augment "/igp:igp-link-event" {
+    augment "/l3t:igp-link-event" {
         uses isis-topology-type;
         uses isis-link-attributes;
     }
diff --git a/l3-unicast-igp-topology.yang b/l3-unicast-igp-topology.yang
index 7f56a00..39b1699 100644
--- a/l3-unicast-igp-topology.yang
+++ b/l3-unicast-igp-topology.yang
@@ -153,20 +153,20 @@ module l3-unicast-igp-topology {
     }
 
     augment "/nt:network-topology/nt:topology" {
-        when "topology-types/l3-unicast-igp-topology";
+        when "nt:topology-types/l3-unicast-igp-topology";
         uses igp-topology-attributes;
     }
 
     augment "/nt:network-topology/nt:topology/nt:node" {
-        when "../../topology-types/l3-unicast-igp-topology";
+        when "../nt:topology-types/l3-unicast-igp-topology";
         uses igp-node-attributes;
     }
     augment "/nt:network-topology/nt:topology/nt:link" {
-        when "../../topology-types/l3-unicast-igp-topology";
+        when "../nt:topology-types/l3-unicast-igp-topology";
         uses igp-link-attributes;
     }
     augment "/nt:network-topology/nt:topology/nt:node/nt:termination-point" {
-        when "../../topology-types/l3-unicast-igp-topology";
+        when "../../nt:topology-types/l3-unicast-igp-topology";
         uses igp-termination-point-attributes;
     }
 
diff --git a/network-topology.yang b/network-topology.yang
index bc39aea..e42cd57 100644
--- a/network-topology.yang
+++ b/network-topology.yang
@@ -149,6 +149,9 @@ module network-topology  {
                 A node is specific to a topology to which it belongs.";
         }
         list supporting-node {
+	    must "/network-topology/topology[topology-id =
+	          current()/../../underlay-topology/topology-ref]/
+		  node[node-id = current()]";
             description
                 "This list defines vertical layering information for nodes.
                 It allows to capture for any given node, which node (or nodes)
@@ -206,6 +209,9 @@ module network-topology  {
             }
         }
         list supporting-link {
+	    must "/network-topology/topology[topology-id =
+	          current()/../../underlay-topology/topology-ref]/
+		  link[link-id = current()]";
             key "link-ref";
             leaf link-ref {
                 type link-ref;
@@ -269,9 +275,6 @@ module network-topology  {
                 description "The list of network nodes defined for the topology.";
                 key "node-id";
                 uses node-attributes;
-                must "boolean(../underlay-topology[*]/node[./supporting-nodes/node-ref])";
-                    // This constraint is meant to ensure that a referenced node is in fact
-                    // a node in an underlay topology.
                 list termination-point {
                     description
                         "A termination point can terminate a link.
@@ -300,17 +303,20 @@ module network-topology  {
                 ";
                 key "link-id";
                 uses link-attributes;
-                must "boolean(../underlay-topology/link[./supporting-link]";
-                    // Constraint: any supporting link must be part of an underlay topology
-                must "boolean(../node[./source/source-node])";
-                    // Constraint: A link must have as source a node of the same topology
-                must "boolean(../node[./destination/dest-node])";
-                    // Constraint: A link must have as source a destination of the same topology
-                must "boolean(../node/termination-point[./source/source-tp])";
-                    // Constraint: The source termination point must be contained in the source node
-                must "boolean(../node/termination-point[./destination/dest-tp])";
-                    // Constraint: The destination termination point must be contained
-                    // in the destination node
+                must "../node[node-id = current()/source/source-node and
+		      termination-point/tp-id = current()/source/source-tp]" {
+		    description
+                        "The source of a link must be a node in the same topology,
+			 and the source termination point must be one of
+                         termination points of that node.";
+		}
+                must "../node[node-id = current()/destination/dest-node and
+		      termination-point/tp-id = current()/destination/dest-tp]" {
+		    description
+                        "The source of a link must be a node in the same topology,
+			 and the source termination point must be one of
+                         termination points of that node.";
+		}
             }
         }
     }
diff --git a/ospf-topology.yang b/ospf-topology.yang
index 7f3471f..0ab494b 100644
--- a/ospf-topology.yang
+++ b/ospf-topology.yang
@@ -9,7 +9,7 @@ module ospf-topology {
     }
 
     import l3-unicast-igp-topology {
-        prefix "igp";
+        prefix "l3t";
     }
     import ietf-inet-types {
         prefix "inet";
@@ -38,12 +38,12 @@ module ospf-topology {
         }
     }
 
-    augment "/nt:network-topology/nt:topology/nt:topology-types/igp:l3-unicast-igp-topology" {
+    augment "/nt:network-topology/nt:topology/nt:topology-types/l3t:l3-unicast-igp-topology" {
         uses ospf-topology-type;
     }
 
-    augment "/nt:network-topology/nt:topology/igp:igp-topology-attributes" {
-        when "../topology-types/ospf";
+    augment "/nt:network-topology/nt:topology/l3t:igp-topology-attributes" {
+        when "../nt:topology-types/l3t:l3-unicast-igp-topology/ospf";
         container ospf-topology-attributes {
             leaf area-id {
                 type area-id;
@@ -51,18 +51,18 @@ module ospf-topology {
         }
     }
 
-    augment "/nt:network-topology/nt:topology/nt:node/igp:igp-node-attributes" {
-        when "../../../topology-types/ospf";
+    augment "/nt:network-topology/nt:topology/nt:node/l3t:igp-node-attributes" {
+        when "../../nt:topology-types/l3t:l3-unicast-igp-topology/ospf";
         uses ospf-node-attributes;
     }
 
-    augment "/nt:network-topology/nt:topology/nt:link/igp:igp-link-attributes" {
-        when "../../../topology-types/ospf";
+    augment "/nt:network-topology/nt:topology/nt:link/l3t:igp-link-attributes" {
+        when "../../nt:topology-types/l3t:l3-unicast-igp-topology/ospf";
         uses ospf-link-attributes;
     }
 
-    augment "/nt:network-topology/nt:topology/nt:node/igp:igp-node-attributes/igp:prefix" {
-        when "../../../../topology-types/ospf";
+    augment "/nt:network-topology/nt:topology/nt:node/l3t:igp-node-attributes/l3t:prefix" {
+        when "../../../nt:topology-types/l3t:l3-unicast-igp-topology/ospf";
         uses ospf-prefix-attributes;
     }
 
@@ -148,23 +148,23 @@ module ospf-topology {
     grouping ospf-prefix-attributes {
         container ospf-prefix-attributes {
             leaf forwarding-address {
-                when "../../igp:l3-unicast-igp-topology/igp:ospf/igp:router-type/igp:asbr";
+                when "../../l3t:l3-unicast-igp-topology/l3t:ospf/l3t:router-type/l3t:asbr";
                 type inet:ipv4-address;
             }
         }
     }
 
-    augment "/igp:igp-node-event" {
+    augment "/l3t:igp-node-event" {
         uses ospf-topology-type;
         uses ospf:ospf-node-attributes;
     }
 
-    augment "/igp:igp-link-event" {
+    augment "/l3t:igp-link-event" {
         uses ospf-topology-type;
         uses ospf:ospf-link-attributes;
     }
 
-    augment "/igp:igp-prefix-event" {
+    augment "/l3t:igp-prefix-event" {
         uses ospf-topology-type;
         uses ospf:ospf-prefix-attributes;
     }
diff --git a/ted.yang b/ted.yang
index d39379b..7c4eba0 100644
--- a/ted.yang
+++ b/ted.yang
@@ -248,7 +248,7 @@ module ted {
         }
       }
       container packet-switch-capable {
-        when "../switching-capability = PSC-1 or ../switching-capability = PSC-2 or ../switching-capability = PSC-3 or ../switching-capability = PSC-4";
+        when "../switching-capability = 'PSC-1' or ../switching-capability = 'PSC-2' or ../switching-capability = 'PSC-3' or ../switching-capability = 'PSC-4'";
         description
           "Interface has packet-switching capabilities";
         leaf minimum-lsp-bandwidth {
@@ -265,7 +265,7 @@ module ted {
         }
       }
       container time-division-multiplex-capable {
-        when "../switching-capability = TDM";
+        when "../switching-capability = 'TDM'";
         description
           "Interface has time-division multiplex capabilities";
         leaf minimum-lsp-bandwidth {

-- 
Ladislav Lhotka, CZ.NIC Labs
PGP Key ID: E74E8C0C