On 12/9/24 13:05, Dumitru Ceara wrote: > 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).
You can't delete a record if there is a strong reference to it, as that will violate referential integrity. And if there is no reference, then you don't need to remove it, because it will be just garbage-collected. So, there should be no scenario where you actually need to explicitly delete a record from a non-root table. > - support automatic garbage collection of routes when the router > (Datapath_Binding) goes away. This should already work. The problem is that those garbage-cllected records hold strong references to other table (port bindings) and that, as Felix says, breaks referential integrity check. That is likely a bug in ovsdb-server, unless we document somewhere that non-root tables can't hold strong references to root ones (I don't remember if that's the case). That part needs checking and potentially fixing. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
