On Thu, Jun 26, 2025 at 2:31 PM Xavier Simonart <xsimo...@redhat.com> wrote:
> Hi Ales
>
> Thanks - I still a question below.
>
> On Thu, Jun 26, 2025 at 12:50 PM Ales Musil <amu...@redhat.com> wrote:
>
>>
>>
>> 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.
>>
> Sorry, I am still confused and might be missing something.
> If a datapath becomes local (and has no localnet), we would now recompute
> i.e. run garp_rarp_run().
> But garp_rarp_run, through get_localnet_vifs_l3gwports, will "ignore" the
> new datapath if it does not contain a localnet port, and
> we will not have any local_l3gw_ports or localnet_vifs on that datapath.
> So "other ports" on that datapath would anyway not be announced, and
> "other ports" on different dp are handled by port_binding_handler.
> So, which additional ports would be announced by a recompute ?
> I might be missing something really obvious
>
You are right it was me missing the connection, everything is based on the
'get_localnet_vifs_l3gwports()' which as you pointed out checks for
localnet.
I will send v2 that takes that into account.
>
>>
>>>
>>>> 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
>>
>
Thanks,
Ales
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
