[ovs-dev] [PATCH ovn 2/3] northd: support for RouteTables in LRs

Han Zhou zhouhan at gmail.com
Tue Aug 31 01:34:49 UTC 2021


On Mon, Aug 30, 2021 at 3:55 PM Numan Siddique <numans at ovn.org> wrote:
>
> On Mon, Aug 30, 2021 at 5:48 PM Numan Siddique <numans at ovn.org> wrote:
> >
> > On Mon, Aug 30, 2021 at 5:25 PM Vladislav Odintsov <odivlad at gmail.com>
wrote:
> > >
> > > Hi Numan,
> > >
> > > thanks for review.
> > > While my answers are inline, I’ve got a counterquestion:
> > >
> > > After I’ve submitted this patch series, I’ve added support for route
tables in OVN IC daemon.
> > > Is it okay if I submit a new version with requested changes and
support for interconnection as well?
> > > I know it’s upcoming soft-freeze and as I’m new to project, so I
don’t know if it is accepted for now.
> >
> > In my opinion you've submitted the patches before soft-freeze.  So it
> > should be fine and can be considered
> > for the upcoming release.
> >
> >
> > >
> > > Regards,
> > > Vladislav Odintsov
> > >
> > > > On 30 Aug 2021, at 23:44, Numan Siddique <numans at ovn.org> wrote:
> > > >
> > > > On Mon, Aug 16, 2021 at 5:15 PM Vladislav Odintsov <
odivlad at gmail.com <mailto:odivlad at gmail.com>> wrote:
> > > >>
> > > >> This patch extends Logical Router's routing functionality.
> > > >> Now user may create multiple routing tables within a Logical Router
> > > >> and assign them to Logical Router Ports.
> > > >>
> > > >> Traffic coming from Logical Router Port with assigned route_table
> > > >> is checked against global routes if any
(Logical_Router_Static_Routes
> > > >> whith empty route_table field), next against directly connected
routes
> > > >> and then Logical_Router_Static_Routes with same route_table value
as
> > > >> in Logical_Router_Port options:route_table field.
> > > >>
> > > >> A new Logical Router ingress table #10 is added -
IN_IP_ROUTING_PRE.
> > > >> In this table packets which come from LRPs with configured
> > > >> options:route_table field are checked against inport and in OVS
> > > >> register 7 unique non-zero value identifying route table is
written.
> > > >>
> > > >> Then in 11th table IN_IP_ROUTING routes which have non-empty
> > > >> `route_table` field are added with additional match on reg7 value
> > > >> associated with appropriate route_table.
> > > >>
> > > >> Signed-off-by: Vladislav Odintsov <odivlad at gmail.com>
> > > >
> > > > Hi Vladislav,
> > > >
> > > > Thanks for the patch.  Sorry for the late reviews.
> > > >
> > > > I've a few comments.  I didn't review the code completely.
> > > >
> > > > 1.  I think you can merge patch 2 and patch 3.  Without patch 3, the
> > > > test cases fail.
> > > >
> > >
> > > Ack.
> > >
> > > > 2.  ddlog implementation is missing.  Let us know if you need some
> > > > help here.  It would be great
> > > >    if you could add ddlog implementation.
> > > >
> > >
> > > I’ll try to add implementation by myself, but it can take a lot of
time, especially if I spent time changing approach. If I stuck I’ll need
somebody’s help.
> > >
> > > > 3.  I see a few problems in the present approach.  The ID allocated
> > > > for each route table entry may not
> > > >    be consistent across OVN DB runs.  This  may not be a huge
> > > > problem.  But in a scaled environment, adding
> > > >    or deleting route table entry could cause the ids to be shuffled
> > > > disrupting the datapath.
> > > >
> > > >    Instead of using the router table for each static route,  I'd
> > > > suggest instead to add a new column
> > > >    "inports" to Logical_Router_Static_Route table.  This column can
> > > > be a set of strings and CMS can
> > > >    add the inports for each static route.
> > > >
> > > >    Eg.  ovn-nbctl set logical_router_static_route <R1>
> > > > inports="lrp-lr1-ls1, lrp-lr1-ls2"
> > > >
> > > >   And ovn-northd would add a logical flow like
> > > >  table=11(lr_in_ip_routing   ), priority=65   , match=(inport ==
> > > > {"lrp-lr1-ls1, lrp-lr1-ls2"} && ip4.dst == 1.1.1.1/32)
action=(.....)
> > > >
> > > > CMS can also create a port group if desired.   With this, we don't
> > > > have to generate the route table id and store it in the reg7.
> > > > We also don't have to introduce a new stage - lr_in_ip_routing_pre.
> > > > What do you think ?
> > > >
> > >
> > > My first approach was almost same as you described. Just one
difference in that each route could have only one inport.
> > > I decided to rewrite it to support route table names, whichever user
wants, because it was more comfortable for our CMS (non-openstack).
> > >
> > > IDs allocation logic I took from ECMP groups code. It’s the same.
> > >
> > > However, your approach has its advantages in:
> > > - doesn’t have inconsistent ids for route tables
> > > - doesn’t require additional OVS register
> > > - doesn’t require additional stage (though, its not a big problem I
guess)
> > > - doesn’t disrupt datapath while topology changes.
> > >
> > > I’ve got questions here:
> > > 1. Why ids would be inconsistent in large scale across different NB
runs? Did you mean that ids can change while adding/removing routes and
assigning route tables to LRPs?
> > Lets say you have route tables - rt1, rt2, rt3 and rt4 with the same
> > ids assigned by ovn-northd.
> > Lets say you delete rt2,  then ovn-northd would reassign the ids again
> > for rt1, rt3 and rt4.  The id assignment would totally
> > depend on the order in which the route tables are loop though.  In
> > this case,  assuming rt1 was assigned '1', the ids of rt3 and rt4
> > would definitely change.
> >
> >
> > > 2. Why will datapath be disrupted? When we add/remove route table,
only its ids would be changed, so relevant OFs would be updated. How will
this change datapath flows?
> >
> > Since we are deleting and readding the OFs with the updated register 7
> > value,  this may disrupt if the datapath flow are evicted.  I don't
> > know the internals though.
> >
> >
> > > 3. Wouldn’t be datapath disrupted while changing inports list?
> >
> > Actually you're right.  The OF flows will be deleted and re-added as
> > the logical flow will be deleted and re-added with the updated inports
> > list.
> > Only way to avoid is if CMS uses port groups.
> >
> > Seems to me using inports is better than allocating the id to the route
tables.
>
> On a second thought,  I think it's better to have just "inport" as a new
column.
> CMS can either set the name of the router port or a port group name.
>
> Thanks
> Numan
>

I am curious why can't the Logical_Router_Policy table achieve the same
goal? It has a "match" column which is flexible enough to add inport in the
match.

Thanks,
Han


More information about the dev mailing list