On 8/13/25 5:17 PM, Xavier Simonart wrote:
> Hi Ales, Dumitru
> 

Hi Xavier,

> While reviewing v3, I think that spotted another issue (which was already
> present in previous revisions).
> Please see below
> 

Thanks for the review!

> Thanks
> Xavier
> 
> On Tue, Aug 12, 2025 at 4:57 PM Ales Musil <amu...@redhat.com> wrote:
> 
>> From: Dumitru Ceara <dce...@redhat.com>
>>
>> This enhances ovn-controller with (incremental-processing) support for
>> managing (Linux) neighbor table entries in the context of integrating
>> with EVPN control planes (e.g., FRR).
>>
>> The following diagram depicts the dependencies between the newly added
>> nodes and how they interact with the rest of the existing I-P engine
>> nodes in ovn-controller:
>>
>>        <SB-DATA>
>>            |
>>            |
>>            v
>> +----------+----------+   +--------------------+
>> |     en_neighbor     |   | en_host_if_monitor |
>> +----------+----------+   +----------+---------+
>>            |                         |
>>            |            +------------+
>>            |            |               +--------------------------+
>>            |            |               | en_neighbor_table_notify |
>>            |            |               +----------+---------------+
>>            |            |                          |
>>            |            |      +-------------------+
>>            |            |      |
>>            |            |      |
>>  +-----------------------------+
>>            |            |      |             | en_neighbor_exchange_status
>> |
>>            |            |      |
>>  +----------+------------------+
>>            |            |      |                        |
>>            |            v      v                        |
>>            |    +-------+------+-------+                |
>>            +----> en_neighbor_exchange <----------------+
>>                 +-----------+----------+
>>                             |
>>                             v
>>                 +-----------+----------+
>>                 | en_controller_output |
>>                 +----------------------+
>>
>> Where:
>> - <SB-DATA>:
>>   represents to-be-added input built from OVN Southbound contents
>>   (e.g., VNI information in SB.Datapath_Binding records used for
>>   EVPN integration with the fabric).
>> - en_neighbor:
>>   - builds the set of (Linux) interfaces that need to be monitored
>>     (for if-index and neighbor table changes)
>> - en_host_if_monitor:
>>   - triggers depending nodes (i.e., en_neighbor_exchange) to run if
>>     relevant if-index <-> if-name mapping changes have been detected
>> - en_neighbor_table_notify:
>>   - triggers depending nodes (i.e., en_neighbor_exchange) to run if
>>     relevant (Linux) neighbor table changes have been detected
>> - en_neighbor_exchange_status:
>>   - triggers en_neighbor_exchange to run if a previous neighbor-related
>>     Netlink operation has failed
>> - en_neighbor_exchange:
>>   - reconciles the (Linux) neighbor table state for relevant interfaces,
>>     that is, it learns routing control plane (e.g., FRR) added
>>     neighbors and it installs neighbor entries for FDB/MAC_Binding
>>     records that OVN needs to advertise to the fabric.
>>
>> NOTE: the code has two "TODO GLUE" markers where subsequent commits
>> should plug the implementation of:
>> - building inputs for the en_neighbor node from <SB-DATA>
>> - dynamically learning remote EVPN VTEPs
>> - installing OpenFlow rules for remote FDB/neighbor records learned from
>>   the fabric
>>
>> Reported-at: https://issues.redhat.com/browse/FDP-1416
>> Signed-off-by: Dumitru Ceara <dce...@redhat.com>
>> Acked-by: Xavier Simonart <xsimo...@redhat.com>
>> ---
>> Changes in V3:
>>  - Rabsed on top of current main.
>>  - Add Xavier's ack.
>>
>> Changes in V2:
>>  - Added CLEAR_NEIGHBOR_EXCHANGE_NL_STATUS() to neighbor_exchange_run().
>>  - Added missing neighbor_table_notify_wait() call.
>> ---
>>  controller/automake.mk              |   3 +
>>  controller/neighbor-exchange-stub.c |  30 ++++
>>  controller/neighbor-exchange.c      |  85 ++++++++++
>>  controller/neighbor-exchange.h      |  36 +++++
>>  controller/neighbor.c               |  10 ++
>>  controller/ovn-controller.c         | 240 +++++++++++++++++++++++++++-
>>  6 files changed, 402 insertions(+), 2 deletions(-)
>>  create mode 100644 controller/neighbor-exchange-stub.c
>>  create mode 100644 controller/neighbor-exchange.c
>>  create mode 100644 controller/neighbor-exchange.h
>>
>> diff --git a/controller/automake.mk b/controller/automake.mk
>> index 6eca5e8ed..37cfd4396 100644
>> --- a/controller/automake.mk
>> +++ b/controller/automake.mk
>> @@ -64,6 +64,7 @@ controller_ovn_controller_SOURCES = \
>>         controller/route.c \
>>         controller/garp_rarp.h \
>>         controller/garp_rarp.c \
>> +       controller/neighbor-exchange.h \
>>         controller/neighbor-table-notify.h \
>>         controller/host-if-monitor.h
>>
>> @@ -72,6 +73,7 @@ controller_ovn_controller_SOURCES += \
>>         controller/host-if-monitor.c \
>>         controller/neighbor-exchange-netlink.h \
>>         controller/neighbor-exchange-netlink.c \
>> +       controller/neighbor-exchange.c \
>>         controller/neighbor-table-notify.c \
>>         controller/route-exchange-netlink.h \
>>         controller/route-exchange-netlink.c \
>> @@ -80,6 +82,7 @@ controller_ovn_controller_SOURCES += \
>>  else
>>  controller_ovn_controller_SOURCES += \
>>         controller/host-if-monitor-stub.c \
>> +       controller/neighbor-exchange-stub.c \
>>         controller/neighbor-table-notify-stub.c \
>>         controller/route-exchange-stub.c \
>>         controller/route-table-notify-stub.c
>> diff --git a/controller/neighbor-exchange-stub.c
>> b/controller/neighbor-exchange-stub.c
>> new file mode 100644
>> index 000000000..a42df84c2
>> --- /dev/null
>> +++ b/controller/neighbor-exchange-stub.c
>> @@ -0,0 +1,30 @@
>> +/* Copyright (c) 2025, Red Hat, Inc.
>> + *
>> + * Licensed under the Apache License, Version 2.0 (the "License");
>> + * you may not use this file except in compliance with the License.
>> + * You may obtain a copy of the License at:
>> + *
>> + *     http://www.apache.org/licenses/LICENSE-2.0
>> + *
>> + * Unless required by applicable law or agreed to in writing, software
>> + * distributed under the License is distributed on an "AS IS" BASIS,
>> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
>> implied.
>> + * See the License for the specific language governing permissions and
>> + * limitations under the License.
>> + */
>> +
>> +#include <config.h>
>> +
>> +#include "neighbor-exchange.h"
>> +
>> +void
>> +neighbor_exchange_run(const struct neighbor_exchange_ctx_in *ctx_in
>> OVS_UNUSED,
>> +                      struct neighbor_exchange_ctx_out *ctx_out
>> OVS_UNUSED)
>> +{
>> +}
>> +
>> +int
>> +neighbor_exchange_status_run(void)
>> +{
>> +    return 0;
>> +}
>> diff --git a/controller/neighbor-exchange.c
>> b/controller/neighbor-exchange.c
>> new file mode 100644
>> index 000000000..8e3bf3ecb
>> --- /dev/null
>> +++ b/controller/neighbor-exchange.c
>> @@ -0,0 +1,85 @@
>> +/* Copyright (c) 2025, Red Hat, Inc.
>> + *
>> + * Licensed under the Apache License, Version 2.0 (the "License");
>> + * you may not use this file except in compliance with the License.
>> + * You may obtain a copy of the License at:
>> + *
>> + *     http://www.apache.org/licenses/LICENSE-2.0
>> + *
>> + * Unless required by applicable law or agreed to in writing, software
>> + * distributed under the License is distributed on an "AS IS" BASIS,
>> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
>> implied.
>> + * See the License for the specific language governing permissions and
>> + * limitations under the License.
>> + */
>> +
>> +#include <config.h>
>> +
>> +#include "host-if-monitor.h"
>> +#include "neighbor.h"
>> +#include "neighbor-exchange.h"
>> +#include "neighbor-exchange-netlink.h"
>> +#include "neighbor-table-notify.h"
>> +#include "vec.h"
>> +
>> +/* Last neighbor_exchange netlink operation. */
>> +static int neighbor_exchange_nl_status;
>> +
>> +#define CLEAR_NEIGHBOR_EXCHANGE_NL_STATUS() \
>> +    do {                                    \
>> +        neighbor_exchange_nl_status = 0;    \
>> +    } while (0)
>> +
>> +#define SET_NEIGHBOR_EXCHANGE_NL_STATUS(error)     \
>> +    do {                                           \
>> +        if (!neighbor_exchange_nl_status) {        \
>> +            neighbor_exchange_nl_status = (error); \
>> +        }                                          \
> 
> +    } while (0)
>> +
>> +void
>> +neighbor_exchange_run(const struct neighbor_exchange_ctx_in *n_ctx_in,
>> +                      struct neighbor_exchange_ctx_out *n_ctx_out)
>> +{
>> +    struct neighbor_interface_monitor *nim;
>> +
>> +    struct sset if_names = SSET_INITIALIZER(&if_names);
>> +    VECTOR_FOR_EACH (n_ctx_in->monitored_interfaces, nim) {
>> +        sset_add(&if_names, nim->if_name);
>> +    }
>> +    host_if_monitor_update_watches(&if_names);
>> +    sset_destroy(&if_names);
>> +
>> +    CLEAR_NEIGHBOR_EXCHANGE_NL_STATUS();
>> +    VECTOR_FOR_EACH (n_ctx_in->monitored_interfaces, nim) {
>> +        int32_t if_index = host_if_monitor_ifname_toindex(nim->if_name);
>> +
>> +        if (!if_index) {
>> +            continue;
>> +        }
>> +
>> +        struct vector received_neighbors =
>> +            VECTOR_EMPTY_INITIALIZER(struct ne_nl_received_neigh);
>> +        SET_NEIGHBOR_EXCHANGE_NL_STATUS(
>> +            ne_nl_sync_neigh(nim->family, if_index,
>> &nim->announced_neighbors,
>> +                             &received_neighbors)
>> +        );
>> +
>> +        /* XXX: TODO GLUE: sync received neighbors to:
>> +         * - SB: for remote vtep entries
>> +         *   https://issues.redhat.com/browse/FDP-1385
>> +         * - in memory table for remote neighbor entries
>> +         *   https://issues.redhat.com/browse/FDP-1387
>> +         */
>> +
>> +
>> neighbor_table_add_watch_request(&n_ctx_out->neighbor_table_watches,
>> +                                         if_index, nim->if_name);
>> +        vector_destroy(&received_neighbors);
>> +    }
>> +}
>> +
>> +int
>> +neighbor_exchange_status_run(void)
>> +{
>> +    return neighbor_exchange_nl_status;
>> +}
>> diff --git a/controller/neighbor-exchange.h
>> b/controller/neighbor-exchange.h
>> new file mode 100644
>> index 000000000..afbbe9811
>> --- /dev/null
>> +++ b/controller/neighbor-exchange.h
>> @@ -0,0 +1,36 @@
>> +/* Copyright (c) 2025, Red Hat, Inc.
>> + *
>> + * Licensed under the Apache License, Version 2.0 (the "License");
>> + * you may not use this file except in compliance with the License.
>> + * You may obtain a copy of the License at:
>> + *
>> + *     http://www.apache.org/licenses/LICENSE-2.0
>> + *
>> + * Unless required by applicable law or agreed to in writing, software
>> + * distributed under the License is distributed on an "AS IS" BASIS,
>> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
>> implied.
>> + * See the License for the specific language governing permissions and
>> + * limitations under the License.
>> + */
>> +
>> +#ifndef NEIGHBOR_EXCHANGE_H
>> +#define NEIGHBOR_EXCHANGE_H 1
>> +
>> +#include "lib/sset.h"
>> +#include "openvswitch/hmap.h"
>> +
>> +struct neighbor_exchange_ctx_in {
>> +    /* Contains struct neighbor_interface_monitor pointers. */
>> +    const struct vector *monitored_interfaces;
>> +};
>> +
>> +struct neighbor_exchange_ctx_out {
>> +    /* Contains struct neighbor_table_watch_request. */
>> +    struct hmap neighbor_table_watches;
>> +};
>> +
>> +void neighbor_exchange_run(const struct neighbor_exchange_ctx_in *,
>> +                           struct neighbor_exchange_ctx_out *);
>> +int neighbor_exchange_status_run(void);
>> +
>> +#endif  /* NEIGHBOR_EXCHANGE_H */
>> diff --git a/controller/neighbor.c b/controller/neighbor.c
>> index 01f6de17c..925bcba7c 100644
>> --- a/controller/neighbor.c
>> +++ b/controller/neighbor.c
>> @@ -52,6 +52,16 @@ neighbor_run(struct neighbor_ctx_in *n_ctx_in
>> OVS_UNUSED,
>>               struct neighbor_ctx_out *n_ctx_out OVS_UNUSED)
>>  {
>>      /* XXX: Not implemented yet. */
>> +
>> +    /* XXX: TODO GLUE: get information from (n_ctx_in) SB (runtime-data)
>> about:
>> +     * - local datapath vni (and listen on br-$vni, lo-$vni and
>> vxlan-$vni)
>> +     *   for which we want to enable neighbor monitoring
>> +     *   https://issues.redhat.com/browse/FDP-1385
>> +     * - what FDB/neighbor entries to advertise
>> +     *   https://issues.redhat.com/browse/FDP-1389
>> +     *
>> +     * And populate that in n_ctx_out.
>> +     */
>>  }
>>
>>  void
>> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
>> index 945952c5c..21d074f73 100644
>> --- a/controller/ovn-controller.c
>> +++ b/controller/ovn-controller.c
>> @@ -94,6 +94,10 @@
>>  #include "route-exchange.h"
>>  #include "route-table-notify.h"
>>  #include "garp_rarp.h"
>> +#include "host-if-monitor.h"
>> +#include "neighbor.h"
>> +#include "neighbor-exchange.h"
>> +#include "neighbor-table-notify.h"
>>
>>  VLOG_DEFINE_THIS_MODULE(main);
>>
>> @@ -4984,6 +4988,14 @@ controller_output_garp_rarp_handler(struct
>> engine_node *node OVS_UNUSED,
>>      return EN_HANDLED_UPDATED;
>>  }
>>
>> +static enum engine_input_handler_result
>> +controller_output_neighbor_exchange_handler(
>> +    struct engine_node *node OVS_UNUSED,
>> +    void *data OVS_UNUSED)
>> +{
>> +    return EN_HANDLED_UPDATED;
>> +}
>> +
>>  /* Handles sbrec_chassis changes.
>>   * If a new chassis is added or removed return false, so that
>>   * flows are recomputed.  For any updates, there is no need for
>> @@ -5657,6 +5669,200 @@ garp_rarp_runtime_data_handler(struct engine_node
>> *node, void *data OVS_UNUSED)
>>      return EN_HANDLED_UNCHANGED;
>>  }
>>
>> +struct ed_type_host_if_monitor {
>> +    /* For incremental processing this could be tracked per interface in
>> +     * the future. */
>> +    bool changed;
>> +};
>> +
>> +static void *
>> +en_host_if_monitor_init(struct engine_node *node OVS_UNUSED,
>> +                       struct engine_arg *arg OVS_UNUSED)
>> +{
>> +    struct ed_type_host_if_monitor *hifm = xmalloc(sizeof *hifm);
>> +
>> +    *hifm = (struct ed_type_host_if_monitor) {
>> +        .changed = true,
>> +    };
>> +    return hifm;
>> +}
>> +
>> +static void
>> +en_host_if_monitor_cleanup(void *data OVS_UNUSED)
>> +{
>> +}
>> +
>> +static enum engine_node_state
>> +en_host_if_monitor_run(struct engine_node *node OVS_UNUSED, void *data)
>> +{
>> +    struct ed_type_host_if_monitor *hifm = data;
>> +    enum engine_node_state state;
>> +    if (hifm->changed) {
>> +        state = EN_UPDATED;
>> +    } else {
>> +        state = EN_UNCHANGED;
>> +    }
>> +    hifm->changed = false;
>> +    return state;
>> +}
>> +
>> +struct ed_type_neighbor {
>> +    /* Contains struct neighbor_interface_monitor pointers. */
>> +    struct vector monitored_interfaces;
>> +};
>> +
>> +static void *
>> +en_neighbor_init(struct engine_node *node OVS_UNUSED,
>> +                 struct engine_arg *arg OVS_UNUSED)
>> +{
>> +    struct ed_type_neighbor *data = xzalloc(sizeof *data);
>> +
>> +    *data = (struct ed_type_neighbor) {
>> +        .monitored_interfaces =
>> +            VECTOR_EMPTY_INITIALIZER(struct neighbor_interface_monitor *),
>> +    };
>> +    return data;
>> +}
>> +
>> +static void
>> +en_neighbor_cleanup(void *data)
>> +{
>> +    struct ed_type_neighbor *ne_data = data;
>> +
>> +    neighbor_cleanup(&ne_data->monitored_interfaces);
>> +    vector_destroy(&ne_data->monitored_interfaces);
>> +}
>> +
>> +static enum engine_node_state
>> +en_neighbor_run(struct engine_node *node OVS_UNUSED, void *data)
>> +{
>> +    struct ed_type_neighbor *ne_data = data;
>> +    struct neighbor_ctx_in n_ctx_in = {
>> +    };
>> +
>> +    struct neighbor_ctx_out n_ctx_out = {
>> +        .monitored_interfaces = &ne_data->monitored_interfaces,
>> +    };
>> +
>> +    neighbor_cleanup(&ne_data->monitored_interfaces);
>> +    neighbor_run(&n_ctx_in, &n_ctx_out);
>> +
>> +    /* XXX: This should return EN_UPDATED once we actually process real SB
>> +     * changes, i.e.:
>> +     *
>> +     * return EN_UPDATED;
>> +     */
>> +
>> +    return EN_UNCHANGED;
>> +}
>> +
>> +struct ed_type_neighbor_table_notify {
>> +    /* For incremental processing this could be tracked per interface in
>> +     * the future. */
>> +    bool changed;
>> +};
>> +
>> +static void *
>> +en_neighbor_table_notify_init(struct engine_node *node OVS_UNUSED,
>> +                              struct engine_arg *arg OVS_UNUSED)
>> +{
>> +    struct ed_type_neighbor_table_notify *ntn = xmalloc(sizeof *ntn);
>> +
>> +    *ntn = (struct ed_type_neighbor_table_notify) {
>> +        .changed = true,
>> +    };
>> +    return ntn;
>> +}
>> +
>> +static void
>> +en_neighbor_table_notify_cleanup(void *data OVS_UNUSED)
>> +{
>> +}
>> +
>> +static enum engine_node_state
>> +en_neighbor_table_notify_run(struct engine_node *node OVS_UNUSED,
>> +                             void *data)
>> +{
>> +    struct ed_type_neighbor_table_notify *ntn = data;
>> +    enum engine_node_state state;
>> +    if (ntn->changed) {
>> +        state = EN_UPDATED;
>> +    } else {
>> +        state = EN_UNCHANGED;
>> +    }
>> +    ntn->changed = false;
>> +    return state;
>> +}
>> +
>> +static void *
>> +en_neighbor_exchange_init(struct engine_node *node OVS_UNUSED,
>> +                          struct engine_arg *arg OVS_UNUSED)
>> +{
>> +    return NULL;
>> +}
>> +
>> +static void
>> +en_neighbor_exchange_cleanup(void *data OVS_UNUSED)
>> +{
>> +}
>> +
>> +static enum engine_node_state
>> +en_neighbor_exchange_run(struct engine_node *node, void *data OVS_UNUSED)
>> +{
>> +    const struct ed_type_neighbor *neighbor_data =
>> +        engine_get_input_data("neighbor", node);
>> +
>> +    struct neighbor_exchange_ctx_in n_ctx_in = {
>> +        .monitored_interfaces = &neighbor_data->monitored_interfaces,
>> +    };
>> +    struct neighbor_exchange_ctx_out n_ctx_out = {
>> +        .neighbor_table_watches =
>> +            HMAP_INITIALIZER(&n_ctx_out.neighbor_table_watches),
>> +    };
>> +
>> +    neighbor_exchange_run(&n_ctx_in, &n_ctx_out);
>> +
>> neighbor_table_notify_update_watches(&n_ctx_out.neighbor_table_watches);
>> +
>> +
>> neighbor_table_watch_request_cleanup(&n_ctx_out.neighbor_table_watches);
>> +    hmap_destroy(&n_ctx_out.neighbor_table_watches);
>> +
>> +    return EN_UPDATED;
>> +}
>> +
>> +struct ed_type_neighbor_exchange_status {
>> +    bool netlink_trigger_run;
>> +};
>> +
>> +static void *
>> +en_neighbor_exchange_status_init(struct engine_node *node OVS_UNUSED,
>> +                                 struct engine_arg *arg OVS_UNUSED)
>> +{
>> +    return xzalloc(sizeof(struct ed_type_neighbor_exchange_status));
>> +}
>> +
>> +static void
>> +en_neighbor_exchange_status_cleanup(void *data OVS_UNUSED)
>> +{
>> +}
>> +
>> +static enum engine_node_state
>> +en_neighbor_exchange_status_run(struct engine_node *node OVS_UNUSED,
>> +                                void *data OVS_UNUSED)
>> +{
>> +    struct ed_type_neighbor_exchange_status *res = data;
>> +    enum engine_node_state state;
>> +
>> +    if (res->netlink_trigger_run) {
>> +        state = EN_UPDATED;
>> +        poll_immediate_wake();
>>
> This poll_immediate_wake comes too late: if an netlink error happened (e.g.
> in neighbor_exchange),
> the poll_immediate_wake must be called within that loop (e.g.
> in SET_NEIGHBOR_EXCHANGE_NL_STATUS),
> so that ovn-controller immediately wakes up, and runs another loop of I-P,
> and hence immediately runs
> this node (en_neighbor_exchange_status_run) which will
> trigger neighbor_exchange recompute.

Good catch!  You're right.  I should've payed more attention when
mimicking the route-exchange mechanism.  I'll address it for the next
revision.

> Same issue exists with route_exchange_status.
> 

I posted a separate patch for route-exchange here:
https://patchwork.ozlabs.org/project/ovn/patch/20250813155350.1169298-1-dce...@redhat.com/

Regards,
Dumitru

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to