Re: [i2rs] FW: Routing directorate QA review of draft-ietf-i2rs-rib-info-model

[Nitin Bahadur <[e-mail]>]

Thanks Ravi for the detailed review.

Please see comments inline below. I will submit the updated draft once all
comments have been satisfactorily resolved.
 
From: Ravi Singh [mailto:[e-mail]]
Sent: 2016年5月18日 0:55
To: [e-mail]
Cc: 'Jonathan Hardwick'; 'Jon Hudson'; Susan Hares; Zhangxian (Xian)
Subject: RE: Routing directorate QA review of draft-ietf-i2rs-rib-info-model
 
Hi

I had been designated the RTG-DIR QA-reviewer for
https://tools.ietf.org/html/draft-ietf-i2rs-rib-info-model-08

 

I reviewed this doc.

Overall, the doc is clear and does a decent job of creating a RIB model.

However, I have a minor concern with the tone of the doc at certain places.

The document, at places, reads like a requirements doc specifying what an
implementation of the RIB "SHOULD"/MUST do.

I am not sure if that is correct form for an informational draft documenting
a specific RIB model.

Examples of such instances would be:

A.     Section 8
B.     Section 9

NB> Yes, we added SHOULD & MUST to indicate what the data-model needs to
implement. There is no separate “requirements” draft for the RIB.

C.     Wherever in the doc a "SHOULD" or "MUST" shows up stating
desirability of certain behavior of an external entity accessing the RIB.
 

NB> The desired behavior was specified using SHOULD/MUST so that inter-op is
not an issue. Otherwise people implementing this on the agent and client
side might end up having different semantics.

An aspect that has not been touched-upon in the document, that however might
be worthy of consideration is about how this RIB model accommodates an
external input about traffic-statistics-monitoring desired for the various
constructs.

 

NB> The external input would go into a “controller”. And the controller
would have the “smarts” do take that input into account before programming
any rules. So I’m unclear the relationship of that with the RIB model. Maybe
you meant something else.


Specific comments on the various sections in the text:

1.      Introduction:
a.      First 2 paras: some typos and sentences with redundant words.

NB> I fixed a grammar issue….could not find the typos :(. Please help with
typo identification.
 
2.      2.1:
a.      "type" is somewhat ambiguous. Suggest reword "type" as
"address-family"

NB> Done
 
3.      2.2: 
a.      Some sentences could be made shorter/broken-up to improve
readability of this section.

NB> Updated.

b.     Interface_list and router-id: For a functioning routing-instance,
can't think of a routing-instance without either of those defined. So,
either the optionality aspect needs to be changed to "required" or specify
how a routing-instance would work with either missing.

NB> A routing instance can contain a RIB that has next-hops to other RIBs.
That is the reason interface-list and router-id are optional. I can add that
clarification in.

c.      Interface-list: per-interface parameters could also be listed (since
the interface-list is called out in a RIB model): address, families, MTU,
extensibility-consideration-for-other-interface-attributes

NB> Could be. However it was not deemed necessary.
 
4.      2.3:
a.      ROUTE_PREFERENCE: The text is mixing-up route-preference with
"route-metric". Administrative-distance (the route metric) is the IGP cost
of a route.

NB> No it’s not. At least one vendor (Juniper) uses route preference and
admin distance inter-changeably.  See
 <http://www.juniper.net/documentation/en_US/junos15.1/topics/reference/gene
ral/routing-protocols-default-route-preference-values.html>
<http://www.juniper.net/documentation/en_US/junos15.1/topics/reference/gener
al/routing-protocols-default-route-preference-values.html>
<http://www.juniper.net/documentation/en_US/junos15.1/topics/reference/gener
al/routing-protocols-default-route-preference-values.html>
<http://www.juniper.net/documentation/en_US/junos15.1/topics/reference/gener
al/routing-protocols-default-route-preference-values.html>
http://www.juniper.net/documentation/en_US/junos15.1/topics/reference/genera
l/routing-protocols-default-route-preference-values.html
Both route_preference and route-metric would be attributes of the route.


NB> Note that there is only 1 value that makes sense when the network device
is being programmed by a controller.
b.     An additional attribute that should be included is "installing
protocol". That would require defining a list of protocols that may install
a route.

NB> "Installing protocol" is something that the controller needs to worry
about. A network device should not care about the N different protocols
running on the controller.
 
5.      2.4:
a.      Second paragraph could use rewording to enhance clarity.
Specifically:
                                      i.           Need to mention about
"(appearing to be) directly connected IP" to distinguish between:
1.      Nexthops that don't need to be resolved (by other RIB events) to be
installable

NB> As per the draft…"A resolved nexthop has adequate information to send
the outgoing packet to the destination”.  So if the controller provides all
the information, no further resolution is necessary.

2.      Nexthops that need to be resolved (by other RIB events/properties)
to be installable:
a.      Those that are currently resolved
b.     Those that are currently not-resolved

NB> Please provide clarifying/re-wording text…
At a macro level, this document is not supposed to be a treatise on the
design of nexthops.

b.     Next-hop property should also include IP of (appearing to be)
locally-connected device for which to ARP

NB> That is already specified in the grammar
<EGRESS_INTERFACE> (<ipv4-address> | <ipv6-address>)
 

6.      2.4.1:
a.      Last paragraph: "preceded by" would be more accurate than "followed
by"

NB> Hmm….I still think followed by is correct. If you want I can change it
to “encapsulated by”.

 
7.      2.4.3:
a.      Under "tunnel encap": The following text
"

An optional

      egress interface can be chained to the tunnel encap to indicate

      which interface to send the packet out on.  The egress interface

      is useful when the network device contains Ethernet interfaces and

      one needs to perform address resolution for the IP packet."

appears a bit incorrect.

If one wishes to do resolution for the tunnel-remote-dst then specifying an
interface serves no purpose. Either that address does not need resolution
and this specified interface is a p2p interface or there is a need for
resolution (without needing to specify an interface-name). Can't be both.

 

NB> Probably some confusion here. If you attach an IP encap to a packet,
then before you send it out as an Ethernet frame, you need to fill the
Ethernet header. For that you need the address resolution.

8.      Sections 4 & 5 can be merged. What is the point of having a separate
section 5 when it is not really saying anything new beyond what text exists
in section 4.


NB> Section 4 talks about “return codes” on commands sent to a network
device. Section 5 talks about asynchronous messages sent by the network
device (without a corresponding incoming command). That is the difference.

 

9.      Section 6:
a.      Not repeating remarks made about specific attributes (listed above)
for each item in the BNF. Eg. Route-metric/preference related remark made
above about 2.3.

NB> Didn’t understand this comment. Can you rephrase.
 

b.     In-label is not logically a nexthop attribute. It is infact a route.
This should be fixed.
  <mpls-label-operation> ::= (<MPLS_PUSH> <MPLS_LABEL> [<S_BIT>]

                                          [<TOS_VALUE>] [<TTL_VALUE>]) |

                             (<MPLS_SWAP> <IN_LABEL> <OUT_LABEL>

                                         [<TTL_ACTION>])


NB>  The grammar already has content for MPLS style route lookups.


<match> ::= <IPV4> <ipv4-route> | <IPV6> <ipv6-route> |

            <MPLS> <MPLS_LABEL> | <IEEE_MAC> <MAC_ADDRESS> |

            <INTERFACE> <INTERFACE_IDENTIFIER>


<mpls-label-operation> is a type of <tunnel-encap>, which in turn is a type
of <nexthop>. The <mpls-label-operation> grammar allows one to perform MPLS
SWAP operations on a given packet. So you would first <match> the MPLS label
and the nexthop operation for the route would be a SWAP and the grammar
above specifies that.
c.      VXLAN headers needs to have a way to specify src/dst MAC in inner
header, since it is possible to use VXLAN as a general-purpose encapsulation
without L2-learning semantics.

NB> We discussed this offline a while back. The end discussion was that we
did not want to provide ways to completely specify L2 frames in the context
of RIB info model. Note that the current grammar does not allow you to build
a complete ethernet header.

10.  Section 6 describes the RIB grammar. The nexthop grammar is a part of
that. However, some of that sub-grammar appears under section 7.

NB> Section 7 are examples of using the RIB grammar. Sure it could be part
of Section 6. It won’t make a meaningful difference either ways.

11.  Section 7 "Using the RIB grammar" starts out by explaining how the
complex nexthops maybe used. However, it ends up being a listing of the
nexthop sub-grammar which should really have been listed in section 6 along
with the RIB grammar.
I'd suggest either take the entirety of the next-hop grammar listing to the
section 6, or break section 7 so that the next-hop grammar is listed in
section 7 & the "using the rib" grammar is a purely text only description of
Rib/NH grammar maybe used.


NB> Section 7 is targeted towards how developers would use the grammar to
write network applications. So they should all stay together. For instance,
Section 7.3 has all text, but it refers to a “programming example” in
Section 7.2.2. So moving Section 7.3 up would do more harm than good.
 

12.  Syntax for <nexthop-replicate>  needs to be reconciled beween section
7.2.3 and section 6 where
there is an syntax mismatch,

Doesn’t section 6 need to say:

<nexthop-replicate> ::= <NEXTHOP_REPLICATE> <nexthop> <nexthop> ...

 

NB> Version –06 of the draft had what you are asking for. See
 <https://tools.ietf.org/html/draft-ietf-i2rs-rib-info-model-06#section-6>
 <https://tools.ietf.org/html/draft-ietf-i2rs-rib-info-model-06#section-6>
https://tools.ietf.org/html/draft-ietf-i2rs-rib-info-model-06#section-6

We were specifically asked to remove the keyword <NEXTHOP_REPLICATE> because
it was not correct use of rBNF. The data-model can have such things, but not
the info model. Folks were ok with keeping the examples (Section 7) as is.

Thanks
Nitin