Subject: [netmod] review of draft-litkowski-netmod-isis-cfg-00

From: Ladislav Lhotka <[email-masked]>
Date: Thu, 19 Jun 2014 16:34:22 +0200

Hi,

I reviewed the isis module and I-D. I think it is a good start, although some adjustments will be needed to make it work with the core routing data model, see below. I don't address any specific issues of IS-IS configuration.

*** General Comments
    - As Andy already pointed out, descriptions are often missing or
      too brief, and the prose in the I-D is also extremely scarce.
    - An example instance document would be useful, e.g. a reply to
      <get> as in Appendix A of RFC 7277. (The "sample-skeleton"
      plugin of pyang can help with generating it.)  

*** YANG module design
    - Entries of the "rt:routing-protocol" list (under both "routing"
      and "routing-state") are designed to support multiple instances
      of the same protocol. In configuration it means not only that
      the nodes "isis-cfg", "instances" and "instance" are
      superfluous, but this design also prevents connecting different
      ISIS instances to different RIBs.
    - Similarly, state data for each ISIS instance should go straight
      to the corresponding entry of "routing-protocol" under
      "routing-state".
    - Nodes in state data should use descriptive names rather than
      "TLVnnn". TLV numbers are important for the protocol but IMO
      there is no need to expose operators to them. For instance,
      "dynamic-hostname" is better than "TLV137".
    - Grouping "interface-cfg" is used only once, so it is not
      needed. Other groupings are used at least twice.
    - A type definition for ISO addresses might be useful.

*** Specific YANG issues
    - The module is invalid, I am attaching a diff file with
      necessary changes. Module validity should always be verified
      before posting an I-D, e.g. with pyang.
    - The argument of "namespace" should be an URI, e.g. urn:TBD.
    - Arguments of some "augment" statements are broken, see the
      attached diff. In particular, they must not contain trailing
      slashes, see ABNF production "absolute-schema-nodeid".
    - The module should follow the guidelines of RFC 6087, they can
      be checked using

      pyang --ietf isis.yang

*** Formal issues
    - The module text should be wrapped in

      <CODE BEGINS> file "[email-masked]"
      ...
      <CODE ENDS>

      so that it can be easily extracted with rfcstrip. 
    - Assuming the module is bound for standard track, its name
      should be "ietf-isis".
    - Some descriptions exceed the line length limit of 72 characters,
      line breaks should be inserted.

Lada
--- isis.yang.orig	2014-06-19 10:34:44.000000000 +0200
+++ isis.yang	2014-06-19 10:46:31.000000000 +0200
@@ -1,5 +1,5 @@
 module isis {
-    namespace TBD;
+    namespace "urn:TBD";
 
     prefix isis;
 
@@ -73,14 +73,14 @@
         }
     }
 
-    augment "/rt:routing/rt:routing-state/rt:ribs/rt:rib/rt:routes/rt:route" {
+    augment "/rt:routing-state/rt:ribs/rt:rib/rt:routes/rt:route" {
         when "rt:source-protocol = 'isis:isis'" {
             description "ISIS-specific route attributes.";
         }
         uses isis-route-content;
       }
 
-    augment "/rt:active-route/rt:output/rt:route/"
+    augment "/rt:active-route/rt:output/rt:route"
        {
         description "ISIS-specific route attributes.";
         uses isis-route-content;
@@ -406,7 +406,7 @@
         description "ISIS is enabled on interfaces that have an entry in this list, unless 'enabled' is set to 'false' for that entry.";
         leaf name {
             type leafref {
-                path "/rt:routing/rt:router/rt:interfaces/rt:interfaces/rt:interface/rt:name";
+                path "/rt:routing/rt:routing-instance/rt:interfaces/rt:interface/rt:name";
             }
         }
         leaf level {
@@ -529,7 +529,7 @@
         }
     }
 
-    augment "/rt:routing/rt:routing-instance/rt:routing-protocols/rt:routing-protocol/" {
+    augment "/rt:routing/rt:routing-instance/rt:routing-protocols/rt:routing-protocol" {
         when "rt:type = 'isis:isis'" {
             description "This augment is only valid when routing protocol instance type is isis.";
         }

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

---

From: <[email-masked]>
Date: Fri, 20 Jun 2014 08:05:10 +0000

Hi Lada,

Thanks for your review.

Please find inline comments.

Best regards,

Stephane

-----Original Message-----
From: Ladislav Lhotka [mailto:[email-masked]] 
Sent: Thursday, June 19, 2014 16:34
To: [email-masked]; LITKOWSKI Stephane SCE/IBNF
Subject: review of draft-litkowski-netmod-isis-cfg-00

Hi,

*** General Comments

[SLI] Sure I will take this into account.

*** YANG module design

[SLI] Ok I didn't catch this point, for me there was only one protocol instance per type indexed by its name "isis" or "ospf" in the same routing instance.
If it's not the case, I would remove the instances for ISIS.
The OSPF model (draft-yeung-netmod-ospf) sounds also to implement a list of instances for OSPF protocol :
" augment "/rt:routing/rt:routing-instance/rt:routing-protocols/"
         + "rt:routing-protocol" {
     list ospf-router {

       description
         "This is a top-level container for the OSPF router.";

       when "./type=ospf";

       key "version name";
       leaf version {
         description
           "OSPF version.";
         type uint8 {
           range "2..3";
         }
       }
"

[SLI] Ack

[SLI] Ok, sometimes TLV numbers are more understable than descriptions :)

  [SLI] Ack

[SLI] Ack

*** Specific YANG issues

[SLI] Ok , I checked it but without importing modules, so only syntax checking was done.

[SLI] Ok

      pyang --ietf isis.yang

*** Formal issues

[SLI] Will be fixed.

Lada

_________________________________________________________________________________________________________________________

Ce message et ses pieces jointes peuvent contenir des informations confidentielles ou privilegiees et ne doivent donc
pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce message par erreur, veuillez le signaler
a l'expediteur et le detruire ainsi que les pieces jointes. Les messages electroniques etant susceptibles d'alteration,
Orange decline toute responsabilite si ce message a ete altere, deforme ou falsifie. Merci.

This message and its attachments may contain confidential or privileged information that may be protected by law;
they should not be distributed, used or copied without authorisation.
If you have received this email in error, please notify the sender and delete this message and its attachments.
As emails may be altered, Orange is not liable for messages that have been modified, changed or falsified.
Thank you.

---

From: t.petch <[email-masked]>
Date: Fri, 20 Jun 2014 11:33:58 +0100

I would consult the isis list about the use of TLVnnn as names.

For me, that is very much a part of is-is, that is, when I see TLVnnn I
know I am dealing with someone who is familiar with is-is.

Tom Petch

----- Original Message -----
From: <[email-masked]>
To: "Ladislav Lhotka" <[email-masked]>; <[email-masked]>
Sent: Friday, June 20, 2014 9:05 AM
Subject: Re: [netmod] review of draft-litkowski-netmod-isis-cfg-00

> I reviewed the isis module and I-D. I think it is a good start,
although some adjustments will be needed to make it work with the core
routing data model, see below. I don't address any specific issues of
IS-IS configuration.
> [SLI] Ok I didn't catch this point, for me there was only one protocol
instance per type indexed by its name "isis" or "ospf" in the same
routing instance.
>       line breaks should be inserted.
________________________________________________________________________
_________________________________________________
>
> Ce message et ses pieces jointes peuvent contenir des informations
confidentielles ou privilegiees et ne doivent donc
> pas etre diffuses, exploites ou copies sans autorisation. Si vous avez
recu ce message par erreur, veuillez le signaler
> a l'expediteur et le detruire ainsi que les pieces jointes. Les
messages electroniques etant susceptibles d'alteration,
> This message and its attachments may contain confidential or
privileged information that may be protected by law;
> _______________________________________________
> netmod mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/netmod

---

From: <[email-masked]>
Date: Fri, 20 Jun 2014 10:58:37 +0000

Tom,

Another approach could be to put TLV reference in description ...
I agree that TLV number is something meaningful for ISIS people.

I hold on this point waiting discussion with ISIS wg.

Stephane

-----Original Message-----
From: t.petch [mailto:[email-masked]] 
Sent: Friday, June 20, 2014 12:34
To: LITKOWSKI Stephane SCE/IBNF; Ladislav Lhotka; [email-masked]
Subject: Re: [netmod] review of draft-litkowski-netmod-isis-cfg-00

For me, that is very much a part of is-is, that is, when I see TLVnnn I know I am dealing with someone who is familiar with is-is.

Tom Petch

----- Original Message -----
From: <[email-masked]>
To: "Ladislav Lhotka" <[email-masked]>; <[email-masked]>
Sent: Friday, June 20, 2014 9:05 AM
Subject: Re: [netmod] review of draft-litkowski-netmod-isis-cfg-00

> I reviewed the isis module and I-D. I think it is a good start,
although some adjustments will be needed to make it work with the core routing data model, see below. I don't address any specific issues of IS-IS configuration.
> [SLI] Ok I didn't catch this point, for me there was only one protocol
instance per type indexed by its name "isis" or "ospf" in the same routing instance.

---

From: Ladislav Lhotka <[email-masked]>
Date: Sat, 21 Jun 2014 14:52:09 +0200

> If it's not the case, I would remove the instances for ISIS.

Yes, please. Maybe the routing-cfg draft can be more explicit about the possibility of having multiple instances of the same protocol in the "routing-protocol" list.

>          "This is a top-level container for the OSPF router.";

This should be changed, too.

Lada

---

From: Ladislav Lhotka <[email-masked]>
Date: Sat, 21 Jun 2014 15:11:32 +0200

> know I am dealing with someone who is familiar with is-is.

Yes, it's like those monks telling jokes just by calling out numbers. :-)

Anyway, the output of "show isis" commands in the major CLIs doesn't contain these numbers.

Lada

---

From: "Jeffrey (Zhaohui) Zhang" <[email-masked]>
Date: Mon, 23 Jun 2014 21:48:54 +0000

[+ Derek, Dean]

Please see below.

> Sent: Saturday, June 21, 2014 8:52 AM
> To: [email-masked]; [email-masked]

For OSPFv3, there is a specific instance concept in the protocol (see RFC 6969). For ISIS, I don't see it.

Stephane, with your original draft (before Lada's comment), what is your definition of instance for ISIS? Using Cisco terms, is it that rt:routing-instnace is a "logical router" (http://www.cisco.com/c/en/us/td/docs/ios_xr_sw/iosxr_r3-2/interfaces/configuration/guide/int_c32/hc32logr.pdf) and for each child VRF (or VRF-lite) there could be an ISIS instance, and those would be listed in your list of instances under isis-cfg, and under a rt:routing-instance that is a VRF there will be no rt:routing-protocol for ISIS?

> 
> Yes, please. Maybe the routing-cfg draft can be more explicit about the
> possibility of having multiple instances of the same protocol in the
> "routing-protocol" list.

Lada, Cisco supports the following configuration:

router ospfv3 1
!
address-family ipv4 unicast
exit-address-family
!
address-family ipv6 unicast
exit-address-family

IPv4/v6 address families will have their own protocol instance. If I understand you correctly, the "arbitrary name of the routing protocol instance" that is the key for the list of routing-protocols under an instance will need to include address family information, which is difficult.

> This should be changed, too.

We need to discuss that. I'll forward you some email.

Jeffrey

---

From: <[email-masked]>
Date: Tue, 24 Jun 2014 07:10:15 +0000

Hi Jeffrey,

Pls see inline comments

Best Regards,

Stephane

-----Original Message-----
From: Jeffrey (Zhaohui) Zhang [mailto:[email-masked]] 
Sent: Monday, June 23, 2014 23:49
To: Ladislav Lhotka; LITKOWSKI Stephane SCE/IBNF; [email-masked]; Derek Man-Kit Yeung (myeung); Dean Bogdanovic
Subject: RE: [netmod] review of draft-litkowski-netmod-isis-cfg-00

[+ Derek, Dean]

Please see below.

[SLI] No the idea was to be able to have multiple ISIS instance within the same routing-instance. Most of implementations are authorizing such behavior (e.g. two L2 ISIS process within global routing table). This is similar from what exists with OSPF (even for v2) since years ...

---

From: Ladislav Lhotka <[email-masked]>
Date: Tue, 24 Jun 2014 10:52:56 +0200

> Stephane, with your original draft (before Lada's comment), what is your definition of instance for ISIS? Using Cisco terms, is it that rt:routing-instnace is a "logical router" (http://www.cisco.c…

My understanding is that Stephane's draft deals with routing *protocol* instances (entries of the "routing-protocol" list), and not routing instances (entries of the "routing-instance" list). It may be a little confusing - the ietf-routing draft originally used the term "router" instead of "routing-instance" but this change in terminology was one of the "harmonisation" items with the I2RS RIB info model.  

> IPv4/v6 address families will have their own protocol instance. If I understand you

They may or may not, depending on how the protocol data model is designed.

> correctly, the "arbitrary name of the routing protocol instance" that is the key for the
> list of routing-protocols under an instance will need to include address family information, > which is difficult.

I don't tink so. Just add an "address-family" leaf under "routing-protocol" - or add a leaf-list or list that configures multiple address families within the same routing protocol instance.

> We need to discuss that. I'll forward you some email.

OK, but I think "ospf-router" should be a container, not a list, because "rt:routing-protocol" is already a list, and it can contain multiple instances of the same protocol.

Lada