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