| Message ID | 20241127074820.90216-1-vlodintsov@k2.cloud |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] docs: Fix route lookup behavior description. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
Thanks Vladislav. Please see my comments below. On Tue, Nov 26, 2024 at 11:48 PM Vladislav Odintsov <vlodintsov@k2.cloud> wrote: > > Prior to this patch documentation for routes lookup did not match the actual > behavior. > > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2024-November/418485.html > Fixes: 1655a6c146ca ("northd, utils: support for RouteTables in LRs") > Signed-off-by: Vladislav Odintsov <vlodintsov@k2.cloud> > --- > ovn-nb.xml | 30 ++++++++++++++++++++---------- > 1 file changed, 20 insertions(+), 10 deletions(-) > > diff --git a/ovn-nb.xml b/ovn-nb.xml > index 5114bbc2e..9dc68732d 100644 > --- a/ovn-nb.xml > +++ b/ovn-nb.xml > @@ -3877,22 +3877,32 @@ or > > <column name="route_table"> > <p> > - Any string to place route to separate routing table. If Logical Router > - Port has configured value in <ref table="Logical_Router_Port" > - column="options" key="route_table"/> other than empty string, OVN > - performs route lookup for all packets entering Logical Router ingress > - pipeline from this port in the following manner: > + Any string to place route to separate routing table. Maximum prefix > + length matched route takes precedence over others, while for the same > + routes with same prefix length the following lookup order applies: > </p> > > <ul> > <li> > - 1. First lookup among "global" routes: routes without > - <code>route_table</code> value set and routes to directly connected > - networks. > + 1. Lookup among directly connected to LR networks (including nit: For the grammar, it may be revised as: Lookup among routes of directly connected networks ... > + directly connected routes learned from other availability zones > + within same LR through OVN-IC). > + </li> > + </ul> > + <ul> > + <li> > + 2. Lookup among static routes with same <code>route_table</code> > + value as specified in <ref table="Logical_Router_Port" > + column="options" key="route_table"/> field. > + </li> > + <li> > + If no value specified in <ref table="Logical_Router_Port" > + column="options" key="route_table"/>, static routes with empty > + <code>route_table</code> are looked up. > </li> > <li> > - 2. Next lookup among routes with same <code>route_table</code> value > - as specified in LRP's options:route_table field. > + If route is source-based, it has lower priority than > + destination-based route with same CIDR. I think the description of the source-based route here is confusing. In the earlier summary it was mentioning: "for the same routes with same prefix length the following lookup order applies". However, source-based routes are NOT "the same routes" as the dest-based routes, because they check on totally different fields of the IP header. They don't even have to have the same CIDR. A typical example may be: Route 1: A/24, nexthop X, dst-ip Route 2: B/24, nexthop Y, src-ip In this example, the two routes have the same prefix length but totally different CIDR. When a packet with dst_ip == A/24 and src_ip == B/24 comes, it matches both routes, but the route 1 will take precedence, because it is dst based. If route 2 is B/25, it would win, although it doesn't seem to make much sense for any real world use cases. So in my opinion they shouldn't even be considered together, which is why the patch ( https://patchwork.ozlabs.org/project/ovn/patch/20241127061406.3164762-1-hzhou@ovn.org/) was proposed. But I understand that your motivation here is to document what is currently implemented accurately. I would avoid mentioning the source routing here because the problem doesn't belong to the "for the same routes with same prefix length" precondition, and there are already separate documentations about the behavior of the source-based routing, to avoid redundancy in the document. Best, Han > </li> > </ul> > </column> > -- > 2.46.2
Hi Han, On 28.11.2024 02:02, Han Zhou wrote: > > Thanks Vladislav. Please see my comments below. > > On Tue, Nov 26, 2024 at 11:48 PM Vladislav Odintsov > <vlodintsov@k2.cloud> wrote: > > > > Prior to this patch documentation for routes lookup did not match > the actual > > behavior. > > > > Reported-at: > https://mail.openvswitch.org/pipermail/ovs-dev/2024-November/418485.html > > Fixes: 1655a6c146ca ("northd, utils: support for RouteTables in LRs") > > Signed-off-by: Vladislav Odintsov <vlodintsov@k2.cloud> > > --- > > ovn-nb.xml | 30 ++++++++++++++++++++---------- > > 1 file changed, 20 insertions(+), 10 deletions(-) > > > > diff --git a/ovn-nb.xml b/ovn-nb.xml > > index 5114bbc2e..9dc68732d 100644 > > --- a/ovn-nb.xml > > +++ b/ovn-nb.xml > > @@ -3877,22 +3877,32 @@ or > > > > <column name="route_table"> > > <p> > > - Any string to place route to separate routing table. If > Logical Router > > - Port has configured value in <ref table="Logical_Router_Port" > > - column="options" key="route_table"/> other than empty > string, OVN > > - performs route lookup for all packets entering Logical > Router ingress > > - pipeline from this port in the following manner: > > + Any string to place route to separate routing table. > Maximum prefix > > + length matched route takes precedence over others, while > for the same > > + routes with same prefix length the following lookup order > applies: > > </p> > > > > <ul> > > <li> > > - 1. First lookup among "global" routes: routes without > > - <code>route_table</code> value set and routes to directly > connected > > - networks. > > + 1. Lookup among directly connected to LR networks (including > > nit: For the grammar, it may be revised as: Lookup among routes of > directly connected networks ... Ack. > > > + directly connected routes learned from other availability > zones > > + within same LR through OVN-IC). > > + </li> > > + </ul> > > + <ul> > > + <li> > > + 2. Lookup among static routes with same > <code>route_table</code> > > + value as specified in <ref table="Logical_Router_Port" > > + column="options" key="route_table"/> field. > > + </li> > > + <li> > > + If no value specified in <ref table="Logical_Router_Port" > > + column="options" key="route_table"/>, static routes with > empty > > + <code>route_table</code> are looked up. > > </li> > > <li> > > - 2. Next lookup among routes with same > <code>route_table</code> value > > - as specified in LRP's options:route_table field. > > + If route is source-based, it has lower priority than > > + destination-based route with same CIDR. > > I think the description of the source-based route here is confusing. > In the earlier summary it was mentioning: "for the same routes with > same prefix length the following lookup order applies". However, > source-based routes are NOT "the same routes" as the dest-based > routes, because they check on totally different fields of the IP > header. They don't even have to have the same CIDR. A typical example > may be: > > Route 1: A/24, nexthop X, dst-ip > Route 2: B/24, nexthop Y, src-ip > In this example, the two routes have the same prefix length but > totally different CIDR. When a packet with dst_ip == A/24 and src_ip > == B/24 comes, it matches both routes, but the route 1 will take > precedence, because it is dst based. If route 2 is B/25, it would win, > although it doesn't seem to make much sense for any real world use cases. > > So in my opinion they shouldn't even be considered together, which is > why the patch > (https://patchwork.ozlabs.org/project/ovn/patch/20241127061406.3164762-1-hzhou@ovn.org/) > was proposed. > > But I understand that your motivation here is to document what is > currently implemented accurately. I would avoid mentioning the source > routing here because the problem doesn't belong to the "for the same > routes with same prefix length" precondition, and there are already > separate documentations about the behavior of the source-based > routing, to avoid redundancy in the document. Agree. Will fix in v2 and send out shortly. > > Best, > Han > > > </li> > > </ul> > > </column> > > -- > > 2.46.2
diff --git a/ovn-nb.xml b/ovn-nb.xml index 5114bbc2e..9dc68732d 100644 --- a/ovn-nb.xml +++ b/ovn-nb.xml @@ -3877,22 +3877,32 @@ or <column name="route_table"> <p> - Any string to place route to separate routing table. If Logical Router - Port has configured value in <ref table="Logical_Router_Port" - column="options" key="route_table"/> other than empty string, OVN - performs route lookup for all packets entering Logical Router ingress - pipeline from this port in the following manner: + Any string to place route to separate routing table. Maximum prefix + length matched route takes precedence over others, while for the same + routes with same prefix length the following lookup order applies: </p> <ul> <li> - 1. First lookup among "global" routes: routes without - <code>route_table</code> value set and routes to directly connected - networks. + 1. Lookup among directly connected to LR networks (including + directly connected routes learned from other availability zones + within same LR through OVN-IC). + </li> + </ul> + <ul> + <li> + 2. Lookup among static routes with same <code>route_table</code> + value as specified in <ref table="Logical_Router_Port" + column="options" key="route_table"/> field. + </li> + <li> + If no value specified in <ref table="Logical_Router_Port" + column="options" key="route_table"/>, static routes with empty + <code>route_table</code> are looked up. </li> <li> - 2. Next lookup among routes with same <code>route_table</code> value - as specified in LRP's options:route_table field. + If route is source-based, it has lower priority than + destination-based route with same CIDR. </li> </ul> </column>
Prior to this patch documentation for routes lookup did not match the actual behavior. Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2024-November/418485.html Fixes: 1655a6c146ca ("northd, utils: support for RouteTables in LRs") Signed-off-by: Vladislav Odintsov <vlodintsov@k2.cloud> --- ovn-nb.xml | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-)