On 12/9/24 1:27 PM, Ilya Maximets wrote: > 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. >
Ack I see, we should just remove the reference to the non-root table record, but in this case that record has a reference to a different table too (e.g., Route points to Port_Binding in the OVN case). If that reference target is also removed in the same transaction then we hit the referential integrity violation problem that Felix pointed out on the server side - integrity check happening before garbage collection. >> - 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. > Thanks, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
