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