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

[Ladislav Lhotka <[e-mail]>]

"Jeffrey (Zhaohui) Zhang" <[e-mail]> writes:

> [+ Derek, Dean]
>
> Please see below.
>
>> -----Original Message-----
>> From: Ladislav Lhotka [mailto:[e-mail]]
>> Sent: Saturday, June 21, 2014 8:52 AM
>> To: [e-mail]; [e-mail]
>> Subject: Re: [netmod] review of draft-litkowski-netmod-isis-cfg-00
>> 
>> [e-mail] writes:
>> 
>> > Hi Lada,
>> >
>> > Thanks for your review.
>> >
>> > Please find inline comments.
>> >
>> > Best regards,
>> >
>> > Stephane
>> >
>> >
>> > -----Original Message-----
>> > From: Ladislav Lhotka [mailto:[e-mail]]
>> > Sent: Thursday, June 19, 2014 16:34
>> > To: [e-mail]; LITKOWSKI Stephane SCE/IBNF
>> > Subject: review of draft-litkowski-netmod-isis-cfg-00
>> >
>> > 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.)
>> >
>> > [SLI] Sure I will take this into account.
>> >
>> > *** 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.
>> >
>> > [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.
>
> 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?

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.  

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

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.

>
>> 
>> > 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";
>> >          }
>> >        }
>> > "
>> 
>> This should be changed, too.
>
> 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

>
> Jeffrey
>
>> 
>> Lada
>> 
>> >
>> >
>> >     - Similarly, state data for each ISIS instance should go straight
>> >       to the corresponding entry of "routing-protocol" under
>> >       "routing-state".
>> >
>> > [SLI] Ack
>> >
>> >     - 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".
>> >
>> > [SLI] Ok, sometimes TLV numbers are more understable than
>> descriptions :)
>> >
>> >     - Grouping "interface-cfg" is used only once, so it is not
>> >       needed. Other groupings are used at least twice.
>> >
>> >   [SLI] Ack
>> >
>> >     - A type definition for ISO addresses might be useful.
>> >
>> > [SLI] Ack
>> >
>> > *** 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.
>> >
>> > [SLI] Ok , I checked it but without importing modules, so only syntax
>> checking was done.
>> >
>> >     - The argument of "namespace" should be an URI, e.g. urn:TBD.
>> >
>> > [SLI] Ok
>> >
>> >     - 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 "[e-mail]"
>> >       ...
>> >       <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.
>> >
>> > [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.
>> >
>> 
>> --
>> Ladislav Lhotka, CZ.NIC Labs
>> PGP Key ID: E74E8C0C
>> 
>

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