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.
>> 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.
>
>>>
>>>> + "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'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
