On Mon, Aug 30, 2021 at 3:55 PM Numan Siddique <[email protected]> wrote: > > On Mon, Aug 30, 2021 at 5:48 PM Numan Siddique <[email protected]> wrote: > > > > On Mon, Aug 30, 2021 at 5:25 PM Vladislav Odintsov <[email protected]> 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 <[email protected]> wrote: > > > > > > > > On Mon, Aug 16, 2021 at 5:15 PM Vladislav Odintsov < [email protected] <mailto:[email protected]>> 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 <[email protected]> > > > > > > > > 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
