On 12/9/24 11:54 AM, Ilya Maximets wrote: > On 12/9/24 11:09, Dumitru Ceara wrote: >> On 12/9/24 9:05 AM, Felix Huettner wrote: >>> On Thu, Dec 05, 2024 at 03:23:23PM +0100, Dumitru Ceara wrote: >>>> On 12/5/24 1:59 PM, Felix Huettner wrote: >>>>> On Wed, Dec 04, 2024 at 02:18:31PM +0100, Dumitru Ceara wrote: >>>>>> On 12/3/24 3:58 PM, Felix Huettner via dev wrote: >>>>>>> The Route table will be used in the future to coordinate routing >>>>>>> information between northd and ovn-controller. >>>>>>> Northd will insert routes that should be advertised to the outside >>>>>>> fabric. >>>>>>> Ovn-controller will insert routes that have been learned from the >>>>>>> outside fabric. >>>>>>> >>>>>>> Signed-off-by: Felix Huettner <[email protected]> >>>>>>> --- >>>>>> >>>>>> Hi Felix, >>>>>> >>>>>> I have a few minor comments, otherwise, this looks ok to me. >>>>> >>>>> Hi Dimitru, >>>>> >>>>> thanks a lot for the review. >>>>> >>>>>> >>>>>>> ovn-sb.ovsschema | 29 ++++++++++++++++-- >>>>>>> ovn-sb.xml | 80 ++++++++++++++++++++++++++++++++++++++++++++++++ >>>>>>> 2 files changed, 107 insertions(+), 2 deletions(-) >>>>>>> >>>>>>> diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema >>>>>>> index 73abf2c8d..76d32197d 100644 >>>>>>> --- a/ovn-sb.ovsschema >>>>>>> +++ b/ovn-sb.ovsschema >>>>>>> @@ -1,7 +1,7 @@ >>>>>>> { >>>>>>> "name": "OVN_Southbound", >>>>>>> - "version": "20.37.0", >>>>>>> - "cksum": "1950136776 31493", >>>>>>> + "version": "20.38.0", >>>>>>> + "cksum": "550338719 32889", >>>>>>> "tables": { >>>>>>> "SB_Global": { >>>>>>> "columns": { >>>>>>> @@ -617,6 +617,31 @@ >>>>>>> "type": {"key": "string", "value": "string", >>>>>>> "min": 0, "max": "unlimited"}}}, >>>>>>> "indexes": [["chassis"]], >>>>>>> + "isRoot": true}, >>>>>>> + "Route": { >>>>>>> + "columns": { >>>>>>> + "datapath": >>>>>>> + {"type": {"key": {"type": "uuid", >>>>>>> + "refTable": >>>>>>> "Datapath_Binding"}}}, >>>>>>> + "logical_port": {"type": {"key": {"type": "uuid", >>>>>>> + "refTable": >>>>>>> "Port_Binding", >>>>>>> + "refType": "weak"}}}, >>>>>> >>>>>> It's probably fine to keep this as a strong reference, right? >>>>>> >>>>>>> + "ip_prefix": {"type": "string"}, >>>>>>> + "nexthop": {"type": "string"}, >>>>>>> + "tracked_port": {"type": {"key": {"type": "uuid", >>>>>>> + "refTable": >>>>>>> "Port_Binding", >>>>>>> + "refType": "weak"}, >>>>>> >>>>>> Same here, I guess. >>>>> >>>>> Yes, will both be changed in v3. >>>>> >>>>>> >>>>>>> + "min": 0, >>>>>>> + "max": 1}}, >>>>>>> + "type": {"type": {"key": {"type": "string", >>>>>>> + "enum": ["set", ["advertise", >>>>>>> + >>>>>>> "receive"]]}, >>>>>>> + "min": 1, "max": 1}}, >>>>>>> + "external_ids": { >>>>>>> + "type": {"key": "string", "value": "string", >>>>>>> + "min": 0, "max": "unlimited"}}}, >>>>>>> + "indexes": [["datapath", "logical_port", "ip_prefix", >>>>>>> "nexthop", >>>>>>> + "type"]], >>>>>>> "isRoot": true} >>>>>> >>>>>> Do we really need this table to be root? >>>>> >>>>> As i understood the isRoot flag it keeps the records around even if >>>>> there is no incoming reference to any of the rows. As there are no >>>>> incoming references to the Route table at all we need this to be true. >>>>> >>>> >>>> Ah, you're right about that. But shouldn't actually routes be referred >>>> by Datapath_Binding instead of routes pointing to Datapath_binding? >>>> Maybe that's a better schema definition? In that case isRoot can be >>>> false for the Routes table. >>>> >>>> It doesn't really make sense to have route records for routers that >>>> don't exist in the DB anymore. >>> >>> Hi Dumitru, >>> >> >> Hi Felix, >> >>> I tried this out and noticed something that might stand in the way of >>> this change. However it is quite likely that it might be just my limited >>> understanding of ovsdb :) >>> >>> I changed logical_port and tracked_port above to be strong references. >>> Also i removed the datapath key and added in Datapath_Binding a >>> reference to Routes. Additionally i set isRoot to false. >>> >>> If we now assume we have a working setup with LR, LRPs and some created >>> routes by northd. >>> Now if we want to delete a LRP we need to: >>> * Delete the Port_Binding in the southbound >>> * Delete all Routes referencing the Port_Binding >>> * Remove these routes from the list in the Datapath_Binding >>> >>> The problem i encountered is that this creates a "referential integrity >>> violation" because Routes are still referencing the deleted >>> Port_Bindings. >>> >>> As i understood it this is caused by: >>> 1. the ovsdb idl not actually sending deletes for nonRoot tables >>> (https://github.com/openvswitch/ovs/blob/main/lib/ovsdb-idl.c#L3290) >> >> I wasn't aware of this, thanks for pointing it out (CC Ilya). >> Introduced a long time ago by: >> https://github.com/openvswitch/ovs/commit/dcd1dbc >> >> This is actually a bigger problem than you described here. These >> records are to be created/deleted by ovn-controller. But ovn-controller >> effectively can't delete any non-root Route records if they're still >> referenced by Datapath_Binding. > > In general, there is no point sending deletions for non-root tables. > Because they will either be garbage collected anyway, or your transaction > had a referential integrity violation in the first place and wouldn't > go through anyway. >
I'm not sure I follow completely. For clarity, in this case, the behavior I was hoping for is: - allow explicit deletes of isRoot=false records, i.e., Route records when ovn-controller (the one who created them) detects they should be removed (individually, regardless if they're referred or not). - support automatic garbage collection of routes when the router (Datapath_Binding) goes away. > I'll take a closer look at what the database server is doing... > >> >>> 2. the ovsdb server first checking referential integrity and afterwards >>> deleting orphaned rows >>> >>> (https://github.com/openvswitch/ovs/blob/main/ovsdb/transaction.c#L1099-L1109) >>> >>> So from a northd perspective it sends the (from my understanding) >>> correct changes: >>> 1. Deleting the Port_Binding >>> 2. Removing any refernce to the Route entries >>> It does not send a delete for the Route entries, as that is a result of >>> the references. >>> >>> However due to the order of ovsdb server evaluating these things we get >>> an "integrity violation". >>> >>> I am not sure how to best address this issue. >>> I guess having multiple transactions in northd would work, but is quite >>> complex and i don't think we do this anywhere else. >> >> I don't think we should do that, it sounds too risky. >> >>> Alternatively we would need to remove one of the problematic parts from >>> the schema, so we could: >>> * Define Route as a root table >>> * Remove the reference from logical_port/tracked_port to Port_Binding >>> * Maybe changing the references of logical_port/tracked_port to be weak >>> might also work. >>> >>> I am quite unsure what the most appropriate solution is or if i just >>> missunderstood something somewhere. >>> So any ideas would be appreciated. >>> >> >> It seems to me that the only reasonable way forward is what you had in >> your patch originally: >> >> - Route table as "root" >> - Route records with (strong) references to port bindings >> - Route records with (strong) references to datapath_bindings >> >> Which means whenever: >> - a route is added by ovn-controller, northd only gets IDL updates for >> that new route record >> >> - a route is deleted by ovn-controller, northd only gets IDL updates for >> the removed route record >> >> - northd needs to delete a port binding it must also delete all route >> records referencing that port binding - in the same transaction. >> >> - northd needs to delete a datapath binding it must also delete all >> route records referencing that datapath binding - in the same transaction >> >> - northd updates a datapath binding (e.g., a port is added or deleted) - >> all route records referencing that datapath will be marked as "updated" >> by the IDL in ovn-controller -> assuming forwarding rules for routes are >> created by ovn-northd through logical flows, ovn-controller can probably >> ignore the Route IDL updates (ovsdb_idl_omit_alert() for all columns of >> the Route table). >> >> - northd updates a port binding (e.g., a network is added or deleted >> to/from the LRP config) - all route records referencing that port >> binding will be marked as "updated" by the IDL in ovn-controller -> same >> as above, ovn-controller can probably ignore these updates >> >> Regards, >> Dumitru >> >>> Thanks a lot >>> Felix >>> >>>> >>>> Thanks, >>>> Dumitru >>>> >>>> _______________________________________________ >>>> dev mailing list >>>> [email protected] >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> >> > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
