On Thu, 2024-12-05 at 15:28 +0100, Felix Huettner wrote: > On Thu, Dec 05, 2024 at 02:08:37PM +0100, > [email protected] wrote: > > Hi Felix and Dumitru, > > Thanks for posting this standalone change Felix. I'm going to take > > it > > for a spin and try to integrate it to Frode's original "route > > leaking" > > RFC. > > I have one question/suggestion for the schema that I'll leave > > below. > > > > On Thu, 2024-12-05 at 14:00 +0100, 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]> > > > --- > > > v2->v3: > > > * refType is now strong > > > * fixed typos > > > > > > > > > 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..88763f429 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": "1392903395 32893", > > > "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": > > > "strong"}}}, > > > + "ip_prefix": {"type": "string"}, > > > + "nexthop": {"type": "string"}, > > > + "tracked_port": {"type": {"key": {"type": > > > "uuid", > > > + "refTable": > > > "Port_Binding", > > > + "refType": > > > "strong"}, > > > + "min": 0, > > > + "max": 1}}, > > > + "type": {"type": {"key": {"type": "string", > > > + "enum": ["set", > > > ["advertise", > > > + > > > "receive"]]}, > > > + "min": 1, "max": 1}}, > > > > I wonder if this field is necessary. We could use presence or > > absence > > of "nexthop" value to determine whether the route is being learned > > or > > advertised. I'm not sure if ovsdb is doing something clever with > > enums > > or stores them as raw values in each row, but removing this could > > save > > us some space. What do you think? > > Hi Martin, > > i think generally we could use the information in the "nexthop" > column > for this. However i would find this rather surprising if i am not > deeply > in the implementation. For debugging at least it is not obvious. > > I am not sure how much space saving we would get out of this, but i > don't think that it is worth the cost of less debuggability. > Especially > since each learned/announced route will cause additionally once or > multiple logical flows to be created which are significantly larger > than > this type field. > > But i am open to other opinions as well. > > Thanks a lot > Felix
Hi Felix, I'm not hell-bent on this change and I agree that it would make the code less explicit. It just occurred to me because I recently saw similar pattern applied in NAT table [0] where NAT rules is considered "distributed" if the record contains both logical_port and external_nat. I don't have enough experience with ovsdb in large deployments, it was just a gut feel that potentially saving ~8bytes for a table that could have potentially 100s or 1000s of records could be impactful. There's also the fact that without the server-side validation (i.e. if tpye is "advertised" then nexthop must not be empty), it's up to the clients to enforce that invalid records are not created. The issue of transparency could be solved with documentation and abstraction in parsing method (like in the case of the NAT). Anyway, as I said, this is not a blocker from my side, just a suggestion for consideration. Martin [0] https://github.com/ovn-org/ovn/blob/3b32b7d08ebcd005681db5131c798da07a89133a/northd/northd.c#L16506-L16530 > > > > > Martin. > > > > > + "external_ids": { > > > + "type": {"key": "string", "value": "string", > > > + "min": 0, "max": "unlimited"}}}, > > > + "indexes": [["datapath", "logical_port", > > > "ip_prefix", > > > "nexthop", > > > + "type"]], > > > "isRoot": true} > > > } > > > } > > > diff --git a/ovn-sb.xml b/ovn-sb.xml > > > index ea4adc1c3..e435cf243 100644 > > > --- a/ovn-sb.xml > > > +++ b/ovn-sb.xml > > > @@ -5217,4 +5217,84 @@ tcp.flags = RST; > > > The set of variable values for a given chassis. > > > </column> > > > </table> > > > + > > > + <table name="Route"> > > > + <p> > > > + Each record represents a route that is exported from ovn > > > or > > > imported to > > > + ovn using some dynamic routing logic outside of ovn. > > > + It is populated on the one hand by <code>ovn-northd</code> > > > based on the > > > + addresses, routes and NAT Entries of a > > > + <code>OVN_Northbound.Logical_Router_Port</code>. > > > + On the other hand <code>ovn-controller</code> populates it > > > with routes > > > + learned from outside of OVN. > > > + </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"> > > > + <p> > > > + If the type is <code>advertise</code> then this is the > > > logical_port > > > + the router will send packets out. > > > + </p> > > > + > > > + <p> > > > + If the type is <code>receive</code> then this is the > > > logical_port > > > + the route was learned on. > > > + </p> > > > + </column> > > > + > > > + <column name="ip_prefix"> > > > + <p> > > > + IP prefix of this route (e.g. 192.168.100.0/24). > > > + </p> > > > + </column> > > > + > > > + <column name="nexthop"> > > > + <p> > > > + If the type is <code>advertise</code> then this is > > > empty. > > > + </p> > > > + > > > + <p> > > > + If the type is <code>receive</code> then this is the > > > nexthop > > > ip we > > > + from the outside. > > > + </p> > > > + </column> > > > + > > > + <column name="tracked_port"> > > > + <p> > > > + Only relevant for type <code>advertise</code>. > > > + > > > + In combination with a host <code>ip_prefix</code> this > > > tracks the port > > > + OVN will forward the packets for this destination to. > > > + > > > + An announcing chassis can use this information to check > > > if > > > this > > > + destination is local and adjust the route priorities > > > based > > > on that. > > > + </p> > > > + </column> > > > + > > > + <column name="type"> > > > + <p> > > > + If the route is to be exported from OVN to the outside > > > network or if > > > + it is imported from the outside network. > > > + </p> > > > + <ul> > > > + <li> > > > + <code>advertise</code>: This route should be > > > advertised to > > > the > > > + outside network. > > > + </li> > > > + <li> > > > + <code>receive</code>: This route has been learned from > > > the > > > outside > > > + network. > > > + </li> > > > + </ul> > > > + </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
