On Mon, Dec 09, 2024 at 02:12:14PM +0100, Dumitru Ceara wrote: > 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.
Thanks a lot for all that clarification. I'll go ahead for now and just make these tables root tables. > > >> - 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
