| Message ID | 20241129074736.55603-1-vlodintsov@k2.cloud |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v2] 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 | success | github build: passed |
On Thu, Nov 28, 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> > --- > v1 -> v2: > - replaced unordered with ordered list > - removed src-ip based routes description as suggested by Han. Thanks Vladislav for v2. Please see some more comment below. > --- > ovn-nb.xml | 26 ++++++++++++++------------ > 1 file changed, 14 insertions(+), 12 deletions(-) > > diff --git a/ovn-nb.xml b/ovn-nb.xml > index 5114bbc2e..a41dc6c1c 100644 > --- a/ovn-nb.xml > +++ b/ovn-nb.xml > @@ -3877,24 +3877,26 @@ 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 Sorry that I didn't notice this in v1. It seems confusing here because maximum prefix length only applies within the same route table (and with directly connected routes) but not across route tables. I think this needs to be emphasized to avoid confusion. For example: route_table A: prefix X/24, nexthop-1 route_table B: prefix X/25, nexthop-2 For packets enters LRP with options:route_table = A, they should go to nexthop-1 according to the implementation, but according to the above documentation it should go to nexthop-2 because X/25 has longer prefix length than X/24, which is misleading. > + routes with same prefix length the following lookup order applies: > </p> > > - <ul> > + <ol> > <li> > - 1. First lookup among "global" routes: routes without > - <code>route_table</code> value set and routes to directly connected > - networks. > + Lookup among routes of directly connected networks (including > + directly connected networks learned from other availability zones > + within same LR through OVN-IC). > </li> > <li> > - 2. Next lookup among routes with same <code>route_table</code> value > - as specified in LRP's options:route_table field. > + Lookup among static routes with same <ref > + table="Logical_Router_Static_Route" column="route_table"/> value > + as specified in <ref table="Logical_Router_Port" column="options" > + key="route_table"/> field. If no value specified in <ref > + table="Logical_Router_Port" column="options" key="route_table"/>, Please note that the part (ref table="xxx") will not appear in the text after the XML is converted to manpage. It needs to be something like: Lookup among static routes with same <ref table="Logical_Router_Static_Route" column="route_table"/> value of the <ref table="Logical_Router_Static_Route"/> table as specified in the <ref table="Logical_Router_Port" column="options" key="route_table"/> field of the <ref table="Logical_Router_Port"/>. We should also mention that the LRP is where the packets enter the LR. Best, Han > + static routes with empty <code>route_table</code> are looked up. > </li> > - </ul> > + </ol> > </column> > > <column name="external_ids" key="ic-learned-route"> > -- > 2.46.2
Hi Han, Thanks for your comments. I've decided to rewrite docs for this option taking your comments into account. Please, see v3 here: https://patchwork.ozlabs.org/project/ovn/patch/20241206112614.40509-1-vlodintsov@k2.cloud/ On 04.12.2024 05:20, Han Zhou wrote: > > > On Thu, Nov 28, 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> > > --- > > v1 -> v2: > > - replaced unordered with ordered list > > - removed src-ip based routes description as suggested by Han. > > Thanks Vladislav for v2. Please see some more comment below. > > > --- > > ovn-nb.xml | 26 ++++++++++++++------------ > > 1 file changed, 14 insertions(+), 12 deletions(-) > > > > diff --git a/ovn-nb.xml b/ovn-nb.xml > > index 5114bbc2e..a41dc6c1c 100644 > > --- a/ovn-nb.xml > > +++ b/ovn-nb.xml > > @@ -3877,24 +3877,26 @@ 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 > > Sorry that I didn't notice this in v1. It seems confusing here because > maximum prefix length only applies within the same route table (and > with directly connected routes) but not across route tables. I think > this needs to be emphasized to avoid confusion. For example: > > route_table A: prefix X/24, nexthop-1 > route_table B: prefix X/25, nexthop-2 > > For packets enters LRP with options:route_table = A, they should go to > nexthop-1 according to the implementation, but according to the above > documentation it should go to nexthop-2 because X/25 has longer prefix > length than X/24, which is misleading. > > > + routes with same prefix length the following lookup order > applies: > > </p> > > > > - <ul> > > + <ol> > > <li> > > - 1. First lookup among "global" routes: routes without > > - <code>route_table</code> value set and routes to directly > connected > > - networks. > > + Lookup among routes of directly connected networks (including > > + directly connected networks learned from other > availability zones > > + within same LR through OVN-IC). > > </li> > > <li> > > - 2. Next lookup among routes with same > <code>route_table</code> value > > - as specified in LRP's options:route_table field. > > + Lookup among static routes with same <ref > > + table="Logical_Router_Static_Route" > column="route_table"/> value > > + as specified in <ref table="Logical_Router_Port" > column="options" > > + key="route_table"/> field. If no value specified in <ref > > + table="Logical_Router_Port" column="options" > key="route_table"/>, > > Please note that the part (ref table="xxx") will not appear in the > text after the XML is converted to manpage. It needs to be something > like: Lookup among static routes with same <ref > table="Logical_Router_Static_Route" column="route_table"/> value of > the <ref table="Logical_Router_Static_Route"/> table as specified in > the <ref table="Logical_Router_Port" column="options" > key="route_table"/> field of the <ref table="Logical_Router_Port"/>. > > We should also mention that the LRP is where the packets enter the LR. > > Best, > Han > > > + static routes with empty <code>route_table</code> are > looked up. > > </li> > > - </ul> > > + </ol> > > </column> > > > > <column name="external_ids" key="ic-learned-route"> > > -- > > 2.46.2
diff --git a/ovn-nb.xml b/ovn-nb.xml index 5114bbc2e..a41dc6c1c 100644 --- a/ovn-nb.xml +++ b/ovn-nb.xml @@ -3877,24 +3877,26 @@ 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> + <ol> <li> - 1. First lookup among "global" routes: routes without - <code>route_table</code> value set and routes to directly connected - networks. + Lookup among routes of directly connected networks (including + directly connected networks learned from other availability zones + within same LR through OVN-IC). </li> <li> - 2. Next lookup among routes with same <code>route_table</code> value - as specified in LRP's options:route_table field. + Lookup among static routes with same <ref + table="Logical_Router_Static_Route" column="route_table"/> value + as specified in <ref table="Logical_Router_Port" column="options" + key="route_table"/> field. 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> - </ul> + </ol> </column> <column name="external_ids" key="ic-learned-route">
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> --- v1 -> v2: - replaced unordered with ordered list - removed src-ip based routes description as suggested by Han. --- ovn-nb.xml | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-)