Thanks for the additional info and references. I think I have enough to go on and therefore no more questions/comments from my end :)
Martin. On Thu, 2024-12-12 at 13:41 +0100, Felix Huettner wrote: > 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
