Hi Chris, Thanks for your comments!
Please see my reply inline... > From: i2rs [mailto:[email protected]] On Behalf Of Chris Bowers > Sent: Monday, November 09, 2015 9:32 PM > To: [email protected] > Subject: [i2rs] feedback on draft-ietf-i2rs-rib-data-model-03 > > 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."; > } > 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>. > > <route> ::= <match> <nexthop> > [<route-attributes>] > [<route-vendor-attributes>] > > It would be good to address these inconsistencies in some way. 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. > > It would be good to address this inconsistency. 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. > > ------------------------ > 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. > Will fix the above typos. Thanks, Mach > ------------------------ > > > _______________________________________________ i2rs mailing list [email protected] https://www.ietf.org/mailman/listinfo/i2rs
