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'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
