On Thu, Jun 26, 2025 at 12:11 PM Xavier Simonart <xsimo...@redhat.com>
wrote:
> Hi Ales
>
> Thanks for the patch.
>
Hi Xavier,
thank you for the review.
>
> On Tue, Jun 24, 2025 at 4:54 PM Ales Musil via dev <
> ovs-dev@openvswitch.org> wrote:
>
>> The garp_rarp node was doing recompute on datapaths that are not
>> local, this is problematic with ovn-monitor-all enabled as the
>> whole cluster gets notifications about unrelated port bindings.
>> This could lead to the node being cancelled on large scale
>> deployments as there is chance that SB might be readonly when
>> the port binding recompute is triggered.
>>
>> To avoid that skip datapaths that are not local and add condition
>> to recompute when the datapath becomes local. This should prevent
>> an issue when the port is remote and becomes local as there are
>> only 2 conditions when that could happen, the datapath is already
>> local in that case it will be processed by the port binding handler.
>> In the second case the datapath is remote and becomes local when
>> the port is bound to local chassis.
>>
> Just wondering: why do we need to recompute in this second case, when
> the dp becomes local, and there is no localnet_port ?
>
There still might be other ports that need to be announced.
>
>> It follows the same semantics in the opposite case local->remote,
>> with one exception, currently we are not removing datapath that
>> is local during incremental processing.
>>
>> Fixes: 05527bd6ccdb ("controller: Extract garp_rarp to engine node.")
>> Reported-by: Ilya Maximets <i.maxim...@ovn.org>
>> Tested-by: Ilya Maximets <i.maxim...@ovn.org>
>> Signed-off-by: Ales Musil <amu...@redhat.com>
>> ---
>> controller/ovn-controller.c | 20 +++++++++++++++-----
>> 1 file changed, 15 insertions(+), 5 deletions(-)
>>
>> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
>> index 1365f3e65..ad2c77485 100644
>> --- a/controller/ovn-controller.c
>> +++ b/controller/ovn-controller.c
>> @@ -5545,7 +5545,11 @@ garp_rarp_sb_port_binding_handler(struct
>> engine_node *node,
>> struct local_datapath *ld = get_local_datapath(
>> &rt_data->local_datapaths, pb->datapath->tunnel_key);
>>
>> - if (!ld || ld->localnet_port) {
>> + if (!ld) {
>> + continue;
>> + }
>> +
>> + if (ld->localnet_port) {
>> /* XXX: actually handle this incrementally. */
>> return EN_UNHANDLED;
>> }
>> @@ -5589,16 +5593,22 @@ garp_rarp_runtime_data_handler(struct engine_node
>> *node, void *data OVS_UNUSED)
>>
>> struct tracked_datapath *tdp;
>> HMAP_FOR_EACH (tdp, node, &rt_data->tracked_dp_bindings) {
>> - if (tdp->tracked_type == TRACKED_RESOURCE_REMOVED) {
>> - /* This is currently not handled incrementally in
>> runtime_data
>> - * so it should never happen. Recompute just in case. */
>> + if (tdp->tracked_type == TRACKED_RESOURCE_REMOVED ||
>> + tdp->tracked_type == TRACKED_RESOURCE_NEW) {
>> + /* The removal is currently not handled incrementally in
>> + * runtime_data so it should never happen. Addition can
>> happen,
>> + * recompute in both cases. */
>> return EN_UNHANDLED;
>> }
>>
>> struct local_datapath *ld = get_local_datapath(
>> &rt_data->local_datapaths, tdp->dp->tunnel_key);
>>
>> - if (!ld || ld->localnet_port) {
>> + if (!ld) {
>> + continue;
>> + }
>> +
>> + if (ld->localnet_port) {
>> /* XXX: actually handle this incrementally. */
>> return EN_UNHANDLED;
>> }
>> --
>> 2.49.0
>>
>> Thanks
> Xavier
>
>> _______________________________________________
>> dev mailing list
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>>
Thanks,
Ales
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
