Subject: [i2rs] feedback on draft-ietf-i2rs-rib-data-model-03

From: Chris Bowers <[email-masked]>
Date: Mon, 9 Nov 2015 13:32:12 +0000

I2RS WG and draft authors,

Below is some feedback on the text of draft-ietf-i2rs-rib-data-model-03.

Thanks,
Chris

------------------------
It would be good to provide a way to get the yang model text file without ietf draft headers interspersed throughout, so that people can run it through pyang more easily.
One way to do this is to publish the xml of the draft via the draft submission tool.
Another common way is github, but  I couldn't find it there.

------------------------
The description of the nexthop-preference-def (see below) is confusing.  First, I assume there is an error in the text since the example of downloading primary/standby/tertiary nexthops to the FIB should presumably select three nexthops, but the text refers to selecting the two resolved nexthops with the highest preference .  More fundamentally, this example seems to  imply that only two next-hops will get downloaded to the FIB, whereas  one could imagine an implementation that uses three, four, five or more nexthops of different preferences.  If hardware supports more than two next-hop preferences being installed in the FIB, then what is the mechanism for the client to learn how many preference values are supported?  This should all be made clearer in the text.

     typedef nexthop-preference-def {
       type uint8 {
         range "1..99";
       }
       description
         "Nexthop-preference is used for protection schemes.
          It is an integer value between 1 and 99.  A lower
          value indicates higher preference.  To download a
          primary/standby/tertiary group to the FIB, the
          nexthops that are resolved and have two highest
          preferences are selected.";
     }

------------------------
If I understand the yang syntax and semantics correctly, the "nh-add" RPC creates a nexthop in a given RIB that is not associated with any match condition on a route.  I assume the intention is to create a nexthop with a nexthop-id but not associated with any prefix that can then be referenced by multiple other route match conditions.   This seems like a reasonable thing to do.  However, I can see two possible issues with this.

The first issue is that the structure of the data model doesn't seem to not allow this.   "grouping route" uses "grouping route-prefix"  next to "container next-hop".  It appears to me that as currently written "container match" in  "grouping route-prefix" is a mandatory node based on section 3.1 of RFC6020 , since the "mandatory" statements below choice/cases cascade up to the container.    So the current form of the "nh-add" RPC may not be consistent with the data model as currently defined.

The second issue is that creating a next-hop without an associated match appears to differ from the RIB grammar defined in section 6 of draft-ietf-i2rs-rib-info-model-08. In the RIB info model, it seems that the only way for a <nexthop> to appear is together with a <match>.

  <route> ::= <match> <nexthop>
              [<route-attributes>]
              [<route-vendor-attributes>]

It would be good to address these inconsistencies in some way.

------------------------
In the definition of nh-add, "rpc nh-add { input { "  uses "grouping next-hop"  which has the leaf nexthop-id as a mandatory element.  Based on section 7.13.2 of RFC 6020, it seems that the nexthop-id would need to be present in the RPC invocation defined by "rpc nh-add".  This seems inconsistent with the description of nh-add in section 2.5 of this document where no nexthop-id is supplied in the input, and instead the network element allocates a nexthop-id and returns it in the output of the rpc.

It would be good to address this inconsistency.

------------------------
Treating tunnel-decap as a type of next-hop doesn't make sense to me. I assume the desire to include tunnel-decap as well as tunnel-encap is to have a usable stand-alone data model module which to deal with some use cases without having to rely on another module that defines tunnel-decap.  However, the result doesn't make sense.  I think the most common scenario involving routers would be to install a route on router A for prefix P whose nexthop is a tunnel-encap whose destination is router B.   One router B one would need to install a corresponding tunnel-decap whose source is router A.  The most common scenario is then that router B does a normal route lookup on the decapsulated packet independent of the interface it entered on.  I don't see how this most common use case would be handled with the tunnel-decap next-hop in this document.

Section 7.2.5. of draft-ietf-i2rs-rib-info-model-08 gives an example usage for tunnel-decap where the tunnel-decap nexthop is used to pop an MPLS label and send the traffic out an egress interface next-hop.  This example is not the most common scenario. And if we want to accomplish this scenario of  decapsulating and sending to a particular nexthop, it makes more sense to treat tunnel-decap as a route match condition similar to an interface-route in the existing data model.  However, I think the model should be able to handle the more common scenario described above when traffic needs to be decapsulated and routed based on a normal route lookup.

Treating tunnel-decap as a next-hop type really makes no sense to me.  I think this aspect of the model should be changed.

------------------------
The description of "identity ttl-action" on p.22 is missing the action "decrease-and-copy-to-next".  I suggest either adding  it to the description or getting rid of the other 3 from the description.  Listing only 3 of 4 options is confusing.

------------------------
These are some typos that I noticed as well.

------------------------
s/Struture/Structure/
Figure 2: Routing Instance Stuture

------------------------
s/nexhtop/nexthop/

There are several occurrences of this misspelling.
------------------------
s/tunnel-decap-nexthp/tunnel-decap-nexthop/

------------------------
s/attribtes/attributes/

------------------------
s/nexhop/nexthop/

------------------------
s/usesd/used/
on p34.

------------------------

---

From: Mach Chen <[email-masked]>
Date: Tue, 10 Nov 2015 09:43:27 +0000

Hi Chris, 

Thanks for your comments!

Please see my reply inline...

> From: i2rs [mailto:[email-masked]] On Behalf Of Chris Bowers
> Sent: Monday, November 09, 2015 9:32 PM
> To: [email-masked]
> Subject: [i2rs] feedback on draft-ietf-i2rs-rib-data-model-03
> 
> It would be good to provide a way to get the yang model text file without ietf
> draft headers interspersed throughout, so that people can run it through pyang
> more easily.
> The description of the nexthop-preference-def (see below) is confusing.  First,
> I assume there is an error in the text since the example of downloading
> primary/standby/tertiary nexthops to the FIB should presumably select three
> nexthops, but the text refers to selecting the two resolved nexthops with the
> highest preference .  More fundamentally, this example seems to  imply that
> only two next-hops will get downloaded to the FIB, whereas  one could
> imagine an implementation that uses three, four, five or more nexthops of
> different preferences.  If hardware supports more than two next-hop
> preferences being installed in the FIB, then what is the mechanism for the
> client to learn how many preference values are supported?  This should all be
> made clearer in the text.
> 

As Nitin said, it a typo. The data model changed with time, but the description is not be updated accordingly. Will update it in the next version.

> ------------------------
> If I understand the yang syntax and semantics correctly, the "nh-add" RPC
> creates a nexthop in a given RIB that is not associated with any match
> condition on a route.  I assume the intention is to create a nexthop with a
> nexthop-id but not associated with any prefix that can then be referenced by
> multiple other route match conditions.   This seems like a reasonable thing to
> do.  However, I can see two possible issues with this.
> 
> The first issue is that the structure of the data model doesn't seem to not
> allow this.   "grouping route" uses "grouping route-prefix"  next to
> "container next-hop".  It appears to me that as currently written "container
> match" in  "grouping route-prefix" is a mandatory node based on section 3.1
> of RFC6020 , since the "mandatory" statements below choice/cases cascade
> up to the container.    So the current form of the "nh-add" RPC may not be
> consistent with the data model as currently defined.

The nh-add is only about how to add a nexthop to a rib, why this is relevant to the "grouping route"?

"grouping route" is mainly related to the route-add rpc. To add a route to a rib, if the nexthop-id is needed, the i2rs client needs to call the nh-add RPC to request a nexthop-id. After that, it calls the route-add to install the route. And when calling the route-add rpc, the route and nexthop (including the nexthop-id) are already there. 

> 
> The second issue is that creating a next-hop without an associated match
> appears to differ from the RIB grammar defined in section 6 of
> draft-ietf-i2rs-rib-info-model-08. In the RIB info model, it seems that the only
> way for a <nexthop> to appear is together with a <match>.
> 

I am not sure that this is an inconsistency. The info model is just to define what components a <match> should include, but it does not forbid a specific component can be processed or created alone. 

> 
> ------------------------
> In the definition of nh-add, "rpc nh-add { input { "  uses "grouping
> next-hop"  which has the leaf nexthop-id as a mandatory element.  Based
> on section 7.13.2 of RFC 6020, it seems that the nexthop-id would need to be
> present in the RPC invocation defined by "rpc nh-add".  This seems
> inconsistent with the description of nh-add in section 2.5 of this document
> where no nexthop-id is supplied in the input, and instead the network element
> allocates a nexthop-id and returns it in the output of the rpc.
> 

Not every implementation will support nexthop-id, so it is an optional attribute. Seem it's safe to remove the "mandatory" statement. 

> 
> ------------------------
> Treating tunnel-decap as a type of next-hop doesn't make sense to me. I
> assume the desire to include tunnel-decap as well as tunnel-encap is to have a
> usable stand-alone data model module which to deal with some use cases
> without having to rely on another module that defines
> tunnel-decap.  However, the result doesn't make sense.  I think the most
> common scenario involving routers would be to install a route on router A for
> prefix P whose nexthop is a tunnel-encap whose destination is router B.   One
> router B one would need to install a corresponding tunnel-decap whose source
> is router A.  The most common scenario is then that router B does a normal
> route lookup on the decapsulated packet independent of the interface it
> entered on.  I don't see how this most common use case would be handled
> with the tunnel-decap next-hop in this document.
> 
> Section 7.2.5. of draft-ietf-i2rs-rib-info-model-08 gives an example usage for
> tunnel-decap where the tunnel-decap nexthop is used to pop an MPLS label and
> send the traffic out an egress interface next-hop.  This example is not the
> most common scenario. And if we want to accomplish this scenario
> of  decapsulating and sending to a particular nexthop, it makes more sense to
> treat tunnel-decap as a route match condition similar to an interface-route in
> the existing data model.  However, I think the model should be able to handle
> the more common scenario described above when traffic needs to be
> decapsulated and routed based on a normal route lookup.
> 
> Treating tunnel-decap as a next-hop type really makes no sense to me.  I think
> this aspect of the model should be changed.
> 
> ------------------------
> The description of "identity ttl-action" on p.22 is missing the action
> "decrease-and-copy-to-next".  I suggest either adding  it to the description
> or getting rid of the other 3 from the description.  Listing only 3 of 4 options is
> confusing.
> 
> ------------------------

Will fix the above typos.

Thanks,
Mach