On Wed, Dec 11, 2024 at 06:30:52PM +0100, [email protected] wrote: > Thanks for clarifications Felix and Dumitru. I don't mean to hold up > the review, I very much trust your judgment on the schema design. I > have few more questions below, just for my better understanding on how > to properly use this later.
Hi Martin, no worries, we definately need to have an agreement on this. > > On Wed, 2024-12-11 at 15:46 +0100, Dumitru Ceara wrote: > > On 12/11/24 1:38 PM, Dumitru Ceara wrote: > > > On 12/11/24 1:19 PM, Felix Huettner via dev wrote: > > > > On Wed, Dec 11, 2024 at 10:23:40AM +0100, > > > > [email protected] wrote: > > > > > Hi Felix, > > > > > > > > Hi Martin, > > > > thanks for the comments. > > > > > > > > > thanks for the update. I was testing this version yesterday and > > > > > I have > > > > > some thoughts regarding the referencing. > > > > > Wouldn't it be better if the Advertised_Route was a non-root > > > > > table and > > > > > instead of having strong references to Port_Binding via > > > > > "logical_port", > > > > > it would be the Port_Binding that would have strong references > > > > > to the > > > > > Advertised_Route entries? > > > > > > > > We found out here > > > > https://mail.openvswitch.org/pipermail/ovs-dev/2024-December/418890.html > > > > that we can not have strong references from a non-root table to a > > > > root > > > > table. This interferes with the correct deletion. > > > > > > > > So we could only do this if we remove any kind of outbound > > > > references > > > > from these Tables and i don't think that is reasonable. > > > > We would need to get rid of the "datapath" column (see below). > > > > > > > > Additionally tracked_port would need to be reworked. I don't know > > > > how > > > > we could reasonably model this without a outbound reference. > > > > > > > > > I haven't tried my proposed setup yet (currently working on > > > > > it), but my > > > > > intention is for the Advertised_Route entry getting > > > > > automatically > > > > > cleaned up when the "logical_port" Port_Binding is removed. > > > > > With the > > > > > current proposal, Port_Binding removal is blocked due to the > > > > > integrity > > > > > violation (Advertised_Route still has strong ref to > > > > > Port_Binding) and > > > > > requires additional logic in northd to manually remove > > > > > associated > > > > > Advertised_Routes we want to remove Port_Binding. > > > > > > > > I would agree that this might be easier for incremental > > > > processing. > > > > > > > > However for the normal runs i don't think there is too much > > > > difference. > > > > I have already logic that syncs the state of parsed_routes to the > > > > southbound database. This also needs to handle changed ips and so > > > > on. > > Oh, I must've missed it sorry. So you have a patch that already handles > I-P of Advertised_Route when Port_Binding goes away? Could you link it > please, 'cause I glanced over the "OVN Fabric Integration: > prerequisites" series and couldn't find it. I wanted to have the prerequisites series to just care about general refactoring that is needed before the rest of the OVN Fabric Integration. Therefor such thing is not part there. The current state of the code is here: https://github.com/felixhuettner/ovn/blob/test_active_active_routing_v5/northd/en-advertised-route-sync.c I-P here relies completely on the existing "route" engine node. Removing a LRP from a LR will always cause a change of routes. Thereby the "route" engine node returnes UPDATED and we will recompute the advertised routes. Thereby also old routes are removed. Reference to inc-proc-northd.c https://github.com/felixhuettner/ovn/blob/test_active_active_routing_v5/northd/inc-proc-northd.c#L273 > > > > > So deleting the routes when the logical_port no longer exists is > > > > at > > > > least there already covered. > > > > > > > > > > > > > > I have also couple questions below. > > > > > > > > > > On Tue, 2024-12-10 at 14:36 +0100, Felix Huettner via dev > > > > > wrote: > > > > > > These two tables 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 to Advertised_Route. > > > > > > Ovn-controller will insert routes that have been learned from > > > > > > the > > > > > > outside fabric to Learned_Route. > > > > > > > > > > > > Signed-off-by: Felix Huettner <[email protected]> > > > > > > --- > > > > > > v3->v4: > > > > > > * split Route to Advertised_Route and Learned_Route > > > > > > v2->v3: > > > > > > * refType is now strong > > > > > > * fixed typos > > > > > > > > > > > > ovn-sb.ovsschema | 38 ++++++++++++++++++++++++-- > > > > > > ovn-sb.xml | 70 > > > > > > ++++++++++++++++++++++++++++++++++++++++++++++++ > > > > > > 2 files changed, 106 insertions(+), 2 deletions(-) > > > > > > > > > > > > diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema > > > > > > index 73abf2c8d..61bc26457 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": "2175279464 33450", > > > > > > "tables": { > > > > > > "SB_Global": { > > > > > > "columns": { > > > > > > @@ -617,6 +617,40 @@ > > > > > > "type": {"key": "string", "value": > > > > > > "string", > > > > > > "min": 0, "max": > > > > > > "unlimited"}}}, > > > > > > "indexes": [["chassis"]], > > > > > > + "isRoot": true}, > > > > > > + "Advertised_Route": { > > > > > > + "columns": { > > > > > > + "datapath": {"type": {"key": {"type": > > > > > > "uuid", > > > > > > + "refTable": > > > > > > "Datapath_Binding", > > > > > > + "refType": > > > > > > "strong"}}}, > > > > > > > > > > Is the ref. to Datapath_Binding necessary? Even with the > > > > > current > > > > > version where this table is Root, we already have reference to > > > > > Port_Binding and if necessary, we can infer related > > > > > Datapath_Binding by > > > > > looking up Advertised_Route->logical_port->datapath. > > > > > > > > I think we could. > > > > The main idea behind the direct connect between Datapath_Binding > > > > and > > > > Advertised_Route or Learned_Route is that we generally use this > > > > connection on the northd and ovn-controller side to process the > > > > entries. > > > > > > > > Basically having the datapath in there allows us to skip an > > > > iteration > > > > over all Port_Bindings in a Datapath whenever we need all routes > > > > for it. > > > > > > > > > > Yes, that's probably the main advantage, we can use an ovsdb idl > > > index > > > that filters by datapath. > > Given that I looked at this only through the perspective of fnordhal's > Fabric Integration RFC series [0], where Advertised_Routes are only > results of NAT or LB addresses (added/modified/removed), I may be > lacking some other usages. Is there a use-case for iterating over all > Advertised_Route by DP? > My thought here was that changes that would trigger update of > Advertised_Route entry already require iterating DPs/PBs. Mainly in ovn-controller or anything that wants to export these routes. I would expect that this always happens per datapath since it needs to install all the routes of a router to somewhere specific to this datapath. > > > > > > > > > > > > > > > + "logical_port": {"type": {"key": {"type": > > > > > > "uuid", > > > > > > + > > > > > > "refTable": > > > > > > "Port_Binding", > > > > > > + "refType": > > > > > > "strong"}}}, > > > > > > + "ip_prefix": {"type": "string"}, > > > > > > + "tracked_port": {"type": {"key": {"type": > > > > > > "uuid", > > > > > > + > > > > > > "refTable": > > > > > > "Port_Binding", > > > > > > + "refType": > > > > > > "strong"}, > > > > > > > > > > Given that tracked_port is optional, wouldn't it be better for > > > > > this > > > > > reference to be weak? Otherwise we run into the same issue of > > > > > needing > > > > > manual northd logic when removing Port_Binding referenced here. > > > > > > > > The goal for the tracked_port is to specify the "nexthop" > > > > Port_Binding > > > > of a Route. If the Port_Binding would be removed then this Route > > > > is no > > > > longer valid and needs to be removed anyway. So i think because > > > > this is > > > > directly related we don't need to be weak. > > > > > > > > > > I agree. > > > > > I guess I misunderstood the docs for the "tracked_port", I thought that > it would be optional even for the host routes. Is the intention that > multiple chassis would advertise the same host route with different > metrics based on this column? Yes, at least for our usecase that would be the goal. It allows us to always have backup paths already installed in the network. Also during the ovs/ovncon i had talks with multiple people that would want to do something similar. Theoretically we could then also use tracked_port for non-host routes if the nexthop is bound to a specific chassis. E.g. if you have a static route pointing to a VM LSP (maybe a VPN gateway or so). Then the fabric might also be able to steer traffic better. Thanks a lot for the discussion Felix > > Thanks, > Martin > > [0] > https://patchwork.ozlabs.org/project/ovn/patch/20240725140009.413791-1-fnordahl%40ubuntu.com/ > > > > > I'll wait for more feedback on this patch until tomorrow. I do still > > plan to apply the schema change as soon as possible to unblock the > > rest > > of the work. > > > > Thanks, > > Dumitru > > > > > Regards, > > > Dumitru > > > > > > > Thanks a lot > > > > Felix > > > > > > > > > > > > > > Thanks, > > > > > Martin. > > > > > > > > > > > + "min": 0, > > > > > > + "max": 1}}, > > > > > > + "external_ids": { > > > > > > + "type": {"key": "string", "value": > > > > > > "string", > > > > > > + "min": 0, "max": > > > > > > "unlimited"}}}, > > > > > > + "indexes": [["datapath", "logical_port", > > > > > > "ip_prefix"]], > > > > > > + "isRoot": true}, > > > > > > + "Learned_Route": { > > > > > > + "columns": { > > > > > > + "datapath": {"type": {"key": {"type": > > > > > > "uuid", > > > > > > + "refTable": > > > > > > "Datapath_Binding", > > > > > > + "refType": > > > > > > "strong"}}}, > > > > > > + "logical_port": {"type": {"key": {"type": > > > > > > "uuid", > > > > > > + > > > > > > "refTable": > > > > > > "Port_Binding", > > > > > > + "refType": > > > > > > "strong"}}}, > > > > > > + "ip_prefix": {"type": "string"}, > > > > > > + "nexthop": {"type": "string"}, > > > > > > + "external_ids": { > > > > > > + "type": {"key": "string", "value": > > > > > > "string", > > > > > > + "min": 0, "max": > > > > > > "unlimited"}}}, > > > > > > + "indexes": [["datapath", "logical_port", > > > > > > "ip_prefix", > > > > > > "nexthop"]], > > > > > > "isRoot": true} > > > > > > } > > > > > > } > > > > > > diff --git a/ovn-sb.xml b/ovn-sb.xml > > > > > > index ea4adc1c3..6fbc8b6df 100644 > > > > > > --- a/ovn-sb.xml > > > > > > +++ b/ovn-sb.xml > > > > > > @@ -5217,4 +5217,74 @@ tcp.flags = RST; > > > > > > The set of variable values for a given chassis. > > > > > > </column> > > > > > > </table> > > > > > > + > > > > > > + <table name="Advertised_Route"> > > > > > > + <p> > > > > > > + Each record represents a route that should be exported > > > > > > from > > > > > > ovn to the > > > > > > + outside network fabric. > > > > > > + It is populated by <code>ovn-northd</code> based on > > > > > > the > > > > > > addresses, > > > > > > + routes and NAT Entries of a > > > > > > + <code>OVN_Northbound.Logical_Router_Port</code>. > > > > > > + </p> > > > > > > + > > > > > > + <column name="datapath"> > > > > > > + The datapath belonging to the > > > > > > + <code>OVN_Northbound.Logical_Router</code> that this > > > > > > route is > > > > > > valid > > > > > > + for. > > > > > > + </column> > > > > > > + > > > > > > + <column name="logical_port"> > > > > > > + This is the <ref table="Port_Binding"/> that the > > > > > > router will > > > > > > send packets > > > > > > + out that are received for the below prefix. > > > > > > + </column> > > > > > > + > > > > > > + <column name="ip_prefix"> > > > > > > + IP prefix of this route (e.g. 192.168.100.0/24). > > > > > > + </column> > > > > > > + > > > > > > + <column name="tracked_port"> > > > > > > + In combination with a host <code>ip_prefix</code> this > > > > > > tracks > > > > > > the port > > > > > > + OVN will forward the packets for this destination to. > > > > > > + If set the <ref table="Advertised_Route" > > > > > > column="ip_prefix"/> > > > > > > will always > > > > > > + contain a /32 (for ipv4) or /128 (for ipv6) prefix. > > > > > > + > > > > > > + An announcing chassis can use this information to > > > > > > check if > > > > > > this > > > > > > + destination is local and adjust the route priorities > > > > > > based on > > > > > > that. > > > > > > + </column> > > > > > > + > > > > > > + <column name="external_ids"> > > > > > > + See <em>External IDs</em> at the beginning of this > > > > > > document. > > > > > > + </column> > > > > > > + </table> > > > > > > + > > > > > > + <table name="Learned_Route"> > > > > > > + <p> > > > > > > + Each record represents a route that learned by ovn > > > > > > using some > > > > > > dynamic > > > > > > + routing logic outside of ovn. > > > > > > + It is populated by <code>ovn-controller</code> with > > > > > > routes it > > > > > > learns > > > > > > + locally. > > > > > > + </p> > > > > > > + > > > > > > + <column name="datapath"> > > > > > > + The datapath belonging to the > > > > > > + <code>OVN_Northbound.Logical_Router</code> that this > > > > > > route is > > > > > > valid > > > > > > + for. > > > > > > + </column> > > > > > > + > > > > > > + <column name="logical_port"> > > > > > > + This is the <ref table="Port_Binding"/> that the route > > > > > > was > > > > > > learned on. > > > > > > + </column> > > > > > > + > > > > > > + <column name="ip_prefix"> > > > > > > + IP prefix of this route (e.g. 192.168.100.0/24). > > > > > > + </column> > > > > > > + > > > > > > + <column name="nexthop"> > > > > > > + This is the nexthop ip we learned from outside of OVN. > > > > > > + </column> > > > > > > + > > > > > > + <column name="external_ids"> > > > > > > + See <em>External IDs</em> at the beginning of this > > > > > > document. > > > > > > + </column> > > > > > > + </table> > > > > > > </database> > > > > > > > > > _______________________________________________ > > > > 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
