On Wed, Jun 18, 2025 at 9:45 AM Ales Musil <amu...@redhat.com> wrote:
>
>
> On Wed, Jun 18, 2025 at 7:33 AM Ales Musil <amu...@redhat.com> wrote:
>
>> The function behavior would be unexpected in cases when there is
>> BFD flap between two gateway chassis. This would result in two
>> chassis claiming that the port binding should be claimed by them.
>> This leads to multiple problems like unsent gARPs. The gARP problem
>> on its own was solved [0], but unfortunately reintroduced [1] in
>> slightly different form.
>>
>> The conclusion from the investigation is that the function in
>> the current form is easy to misuse. To prevent that use
>> only SB state to determine which chassis has actually claimed
>> the port binding. This is the desired behavior in cases where
>> this function was used previously.
>>
>> The code that determines which chassis should claim port binding
>> based on BFD status remains unchanged, thus the behavior during
>> failover should be the same with minimizing the impact for BFD
>> flapping cases.
>>
>> [0] 289ec19b01ad ("pinctrl: Fix missing garp.")
>> [1] 05527bd6ccdb ("controller: Extract garp_rarp to engine node.").
>>
>> Signed-off-by: Ales Musil <amu...@redhat.com>
>> ---
>> controller/garp_rarp.c | 8 +++-----
>> controller/lport.c | 19 +++++++------------
>> controller/lport.h | 3 ---
>> controller/ovn-controller.c | 11 ++---------
>> controller/physical.c | 3 +--
>> controller/pinctrl.c | 22 ++++++++--------------
>> controller/pinctrl.h | 1 -
>> controller/route.c | 5 +----
>> controller/route.h | 2 --
>> 9 files changed, 22 insertions(+), 52 deletions(-)
>>
>> diff --git a/controller/garp_rarp.c b/controller/garp_rarp.c
>> index ef377e26b..551d8303f 100644
>> --- a/controller/garp_rarp.c
>> +++ b/controller/garp_rarp.c
>> @@ -17,6 +17,7 @@
>> #include <config.h>
>>
>> #include "controller/local_data.h"
>> +#include "lport.h"
>> #include "mac-binding-index.h"
>> #include "openvswitch/hmap.h"
>> #include "openvswitch/vlog.h"
>> @@ -147,11 +148,7 @@ consider_nat_address(struct ovsdb_idl_index
>> *sbrec_port_binding_by_name,
>> {
>> struct lport_addresses *laddrs = xmalloc(sizeof *laddrs);
>> char *lport = NULL;
>> - const struct sbrec_port_binding *cr_pb = NULL;
>> bool rc = extract_addresses_with_port(nat_address, laddrs, &lport);
>> - if (lport) {
>> - cr_pb = lport_lookup_by_name(sbrec_port_binding_by_name, lport);
>> - }
>> if (!rc
>> || (!lport && !strcmp(pb->type, "patch"))) {
>> destroy_lport_addresses(laddrs);
>> @@ -160,7 +157,8 @@ consider_nat_address(struct ovsdb_idl_index
>> *sbrec_port_binding_by_name,
>> return;
>> }
>> if (lport) {
>> - if (!cr_pb || (cr_pb->chassis != chassis)) {
>> + if (!lport_is_chassis_resident(sbrec_port_binding_by_name,
>> + chassis, lport)) {
>> sset_add(non_local_lports, lport);
>> destroy_lport_addresses(laddrs);
>> free(laddrs);
>> diff --git a/controller/lport.c b/controller/lport.c
>> index 92de375b5..1ea57a5ca 100644
>> --- a/controller/lport.c
>> +++ b/controller/lport.c
>> @@ -65,35 +65,30 @@ lport_lookup_by_key(struct ovsdb_idl_index
>> *sbrec_datapath_binding_by_key,
>>
>> bool
>> lport_pb_is_chassis_resident(const struct sbrec_chassis *chassis,
>> - const struct sset *active_tunnels,
>> const struct sbrec_port_binding *pb)
>> {
>> if (!pb || !pb->chassis) {
>> return false;
>> }
>> - if (strcmp(pb->type, "chassisredirect")) {
>> - return pb->chassis == chassis;
>> - } else {
>> - return ha_chassis_group_is_active(pb->ha_chassis_group,
>> - active_tunnels, chassis);
>> - }
>> +
>> + /* Note we relay on SB to provide the information, this is needed in
>> case
>> + * of flapping BFD when it's not detected by one side. */
>> + return pb->chassis == chassis;
>> }
>>
>> bool
>> lport_is_chassis_resident(struct ovsdb_idl_index
>> *sbrec_port_binding_by_name,
>> const struct sbrec_chassis *chassis,
>> - const struct sset *active_tunnels,
>> const char *port_name)
>> {
>> const struct sbrec_port_binding *pb
>> = lport_lookup_by_name(sbrec_port_binding_by_name, port_name);
>> - return lport_pb_is_chassis_resident(chassis, active_tunnels, pb);
>> + return lport_pb_is_chassis_resident(chassis, pb);
>> }
>>
>> bool
>> lport_is_local(struct ovsdb_idl_index *sbrec_port_binding_by_name,
>> const struct sbrec_chassis *chassis,
>> - const struct sset *active_tunnels,
>> const char *port_name)
>> {
>> const struct sbrec_port_binding *pb = lport_lookup_by_name(
>> @@ -103,14 +98,14 @@ lport_is_local(struct ovsdb_idl_index
>> *sbrec_port_binding_by_name,
>> return false;
>> }
>>
>> - if (lport_pb_is_chassis_resident(chassis, active_tunnels, pb)) {
>> + if (lport_pb_is_chassis_resident(chassis, pb)) {
>> return true;
>> }
>>
>> const struct sbrec_port_binding *cr_pb =
>> lport_get_cr_port(sbrec_port_binding_by_name, pb, NULL);
>>
>> - return lport_pb_is_chassis_resident(chassis, active_tunnels, cr_pb);
>> + return lport_pb_is_chassis_resident(chassis, cr_pb);
>> }
>>
>> const struct sbrec_port_binding *
>> diff --git a/controller/lport.h b/controller/lport.h
>> index 8b1809a27..6d48301d2 100644
>> --- a/controller/lport.h
>> +++ b/controller/lport.h
>> @@ -62,15 +62,12 @@ const struct sbrec_multicast_group
>> *mcgroup_lookup_by_dp_name(
>> bool
>> lport_is_chassis_resident(struct ovsdb_idl_index
>> *sbrec_port_binding_by_name,
>> const struct sbrec_chassis *chassis,
>> - const struct sset *active_tunnels,
>> const char *port_name);
>> bool lport_pb_is_chassis_resident(const struct sbrec_chassis *chassis,
>> - const struct sset *active_tunnels,
>> const struct sbrec_port_binding *pb);
>>
>> bool lport_is_local(struct ovsdb_idl_index *sbrec_port_binding_by_name,
>> const struct sbrec_chassis *chassis,
>> - const struct sset *active_tunnels,
>> const char *port_name);
>> const struct sbrec_port_binding *lport_get_peer(
>> const struct sbrec_port_binding *,
>> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
>> index 7940091da..e0dc754a7 100644
>> --- a/controller/ovn-controller.c
>> +++ b/controller/ovn-controller.c
>> @@ -5062,7 +5062,6 @@ en_route_run(struct engine_node *node, void *data)
>> .sbrec_port_binding_by_name = sbrec_port_binding_by_name,
>> .chassis = chassis,
>> .dynamic_routing_port_mapping = dynamic_routing_port_mapping,
>> - .active_tunnels = &rt_data->active_tunnels,
>> .local_datapaths = &rt_data->local_datapaths,
>> .local_bindings = &rt_data->lbinding_data.bindings,
>> };
>> @@ -5164,7 +5163,6 @@ route_runtime_data_handler(struct engine_node
>> *node, void *data)
>> struct tracked_lport *lport = shash_node->data;
>>
>> if (route_exchange_find_port(sbrec_port_binding_by_name,
>> chassis,
>> - &rt_data->active_tunnels,
>> lport->pb)) {
>> /* XXX: Until we get I-P support for route exchange we
>> need to
>> * request recompute. */
>> @@ -5222,8 +5220,6 @@ route_sb_port_binding_data_handler(struct
>> engine_node *node, void *data)
>> engine_ovsdb_node_get_index(
>> engine_get_input("SB_port_binding", node),
>> "name");
>> - struct ed_type_runtime_data *rt_data =
>> - engine_get_input_data("runtime_data", node);
>>
>>
>> /* There are the following cases where we need to handle updates to
>> the
>> @@ -5246,8 +5242,8 @@ route_sb_port_binding_data_handler(struct
>> engine_node *node, void *data)
>> return EN_UNHANDLED;
>> }
>>
>> - if (route_exchange_find_port(sbrec_port_binding_by_name, chassis,
>> - &rt_data->active_tunnels,
>> sbrec_pb)) {
>> + if (route_exchange_find_port(sbrec_port_binding_by_name,
>> + chassis, sbrec_pb)) {
>> /* XXX: Until we get I-P support for route exchange we need
>> to
>> * request recompute. */
>> return EN_UNHANDLED;
>> @@ -5556,7 +5552,6 @@ garp_rarp_sb_port_binding_handler(struct
>> engine_node *node,
>>
>> if (sset_contains(&data->non_local_lports, pb->logical_port) &&
>> lport_is_chassis_resident(sbrec_port_binding_by_name,
>> chassis,
>> - &rt_data->active_tunnels,
>> pb->logical_port)) {
>> /* XXX: actually handle this incrementally. */
>> return EN_UNHANDLED;
>> @@ -5564,7 +5559,6 @@ garp_rarp_sb_port_binding_handler(struct
>> engine_node *node,
>>
>> if (sset_contains(&data->local_lports, pb->logical_port) &&
>> !lport_is_chassis_resident(sbrec_port_binding_by_name,
>> chassis,
>> - &rt_data->active_tunnels,
>> pb->logical_port)) {
>> /* XXX: actually handle this incrementally. */
>> return EN_UNHANDLED;
>> @@ -6650,7 +6644,6 @@ main(int argc, char *argv[])
>> ovnsb_idl_loop.idl),
>> chassis,
>> &runtime_data->local_datapaths,
>> - &runtime_data->active_tunnels,
>>
>> &runtime_data->local_active_ports_ipv6_pd,
>>
>> &runtime_data->local_active_ports_ras,
>> ovsrec_open_vswitch_table_get(
>> diff --git a/controller/physical.c b/controller/physical.c
>> index 65b4c7335..1cc4173b2 100644
>> --- a/controller/physical.c
>> +++ b/controller/physical.c
>> @@ -872,8 +872,7 @@ put_replace_router_port_mac_flows(const struct
>> physical_ctx *ctx,
>> struct ofpact_mac *replace_mac;
>> char *cr_peer_name = xasprintf("cr-%s",
>> rport_binding->logical_port);
>> if (lport_is_chassis_resident(ctx->sbrec_port_binding_by_name,
>> - ctx->chassis, ctx->active_tunnels,
>> - cr_peer_name)) {
>> + ctx->chassis, cr_peer_name)) {
>> /* If a router port's chassisredirect port is
>> * resident on this chassis, then we need not do mac
>> replace. */
>> free(cr_peer_name);
>> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
>> index 545f1b174..d4f4da731 100644
>> --- a/controller/pinctrl.c
>> +++ b/controller/pinctrl.c
>> @@ -366,8 +366,7 @@ pinctrl_handle_bfd_msg(struct rconn *swconn, const
>> struct flow *ip_flow,
>> static void bfd_monitor_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
>> const struct sbrec_bfd_table *bfd_table,
>> struct ovsdb_idl_index
>> *sbrec_port_binding_by_name,
>> - const struct sbrec_chassis *chassis,
>> - const struct sset *active_tunnels)
>> + const struct sbrec_chassis *chassis)
>> OVS_REQUIRES(pinctrl_mutex);
>> static void init_fdb_entries(void);
>> static void destroy_fdb_entries(void);
>> @@ -1434,7 +1433,6 @@ prepare_ipv6_prefixd(struct ovsdb_idl_txn
>> *ovnsb_idl_txn,
>> struct ovsdb_idl_index *sbrec_port_binding_by_name,
>> const struct shash *local_active_ports_ipv6_pd,
>> const struct sbrec_chassis *chassis,
>> - const struct sset *active_tunnels,
>> const struct hmap *local_datapaths)
>> OVS_REQUIRES(pinctrl_mutex)
>> {
>> @@ -1463,9 +1461,8 @@ prepare_ipv6_prefixd(struct ovsdb_idl_txn
>> *ovnsb_idl_txn,
>> }
>>
>> char *redirect_name = xasprintf("cr-%s", pb->logical_port);
>> - bool resident = lport_is_chassis_resident(
>> - sbrec_port_binding_by_name, chassis, active_tunnels,
>> - redirect_name);
>> + bool resident =
>> lport_is_chassis_resident(sbrec_port_binding_by_name,
>> + chassis,
>> redirect_name);
>> free(redirect_name);
>> if ((strcmp(pb->type, "l3gateway") || pb->chassis != chassis) &&
>> !resident) {
>> @@ -4070,7 +4067,6 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
>> const struct sbrec_ecmp_nexthop_table *ecmp_nh_table,
>> const struct sbrec_chassis *chassis,
>> const struct hmap *local_datapaths,
>> - const struct sset *active_tunnels,
>> const struct shash *local_active_ports_ipv6_pd,
>> const struct shash *local_active_ports_ras,
>> const struct ovsrec_open_vswitch_table *ovs_table,
>> @@ -4086,7 +4082,7 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
>> prepare_ipv6_ras(local_active_ports_ras, sbrec_port_binding_by_name);
>> prepare_ipv6_prefixd(ovnsb_idl_txn, sbrec_port_binding_by_name,
>> local_active_ports_ipv6_pd, chassis,
>> - active_tunnels, local_datapaths);
>> + local_datapaths);
>> controller_event_run(ovnsb_idl_txn, ce_table, chassis);
>> ip_mcast_sync(ovnsb_idl_txn, chassis, local_datapaths,
>> sbrec_datapath_binding_by_key,
>> @@ -4101,7 +4097,7 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
>> sync_svc_monitors(ovnsb_idl_txn, svc_mon_table,
>> sbrec_port_binding_by_name,
>> chassis);
>> bfd_monitor_run(ovnsb_idl_txn, bfd_table, sbrec_port_binding_by_name,
>> - chassis, active_tunnels);
>> + chassis);
>> run_put_fdbs(ovnsb_idl_txn, sbrec_port_binding_by_key,
>> sbrec_datapath_binding_by_key, sbrec_fdb_by_dp_key_mac);
>> run_activated_ports(ovnsb_idl_txn, sbrec_datapath_binding_by_key,
>> @@ -7749,8 +7745,7 @@ static void
>> bfd_monitor_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
>> const struct sbrec_bfd_table *bfd_table,
>> struct ovsdb_idl_index *sbrec_port_binding_by_name,
>> - const struct sbrec_chassis *chassis,
>> - const struct sset *active_tunnels)
>> + const struct sbrec_chassis *chassis)
>> OVS_REQUIRES(pinctrl_mutex)
>> {
>> struct bfd_entry *entry;
>> @@ -7782,9 +7777,8 @@ bfd_monitor_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
>> }
>>
>> char *redirect_name = xasprintf("cr-%s", pb->logical_port);
>> - bool resident = lport_is_chassis_resident(
>> - sbrec_port_binding_by_name, chassis, active_tunnels,
>> - redirect_name);
>> + bool resident =
>> lport_is_chassis_resident(sbrec_port_binding_by_name,
>> + chassis,
>> redirect_name);
>> free(redirect_name);
>> if ((strcmp(pb->type, "l3gateway") || pb->chassis != chassis) &&
>> !resident) {
>> diff --git a/controller/pinctrl.h b/controller/pinctrl.h
>> index f977d1168..80384ac9b 100644
>> --- a/controller/pinctrl.h
>> +++ b/controller/pinctrl.h
>> @@ -57,7 +57,6 @@ void pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
>> const struct sbrec_ecmp_nexthop_table *,
>> const struct sbrec_chassis *chassis,
>> const struct hmap *local_datapaths,
>> - const struct sset *active_tunnels,
>> const struct shash *local_active_ports_ipv6_pd,
>> const struct shash *local_active_ports_ras,
>> const struct ovsrec_open_vswitch_table *ovs_table,
>> diff --git a/controller/route.c b/controller/route.c
>> index 2ea53a287..7615f3f59 100644
>> --- a/controller/route.c
>> +++ b/controller/route.c
>> @@ -52,7 +52,6 @@ advertise_route_hash(const struct in6_addr *dst,
>> unsigned int plen)
>> const struct sbrec_port_binding*
>> route_exchange_find_port(struct ovsdb_idl_index
>> *sbrec_port_binding_by_name,
>> const struct sbrec_chassis *chassis,
>> - const struct sset *active_tunnels,
>> const struct sbrec_port_binding *pb)
>> {
>> if (!pb) {
>> @@ -69,7 +68,7 @@ route_exchange_find_port(struct ovsdb_idl_index
>> *sbrec_port_binding_by_name,
>> return NULL;
>> }
>>
>> - if (!lport_pb_is_chassis_resident(chassis, active_tunnels, cr_pb)) {
>> + if (!lport_pb_is_chassis_resident(chassis, cr_pb)) {
>> return NULL;
>> }
>>
>> @@ -171,7 +170,6 @@ route_run(struct route_ctx_in *r_ctx_in,
>> const struct sbrec_port_binding *repb =
>>
>> route_exchange_find_port(r_ctx_in->sbrec_port_binding_by_name,
>> r_ctx_in->chassis,
>> - r_ctx_in->active_tunnels,
>> local_peer);
>> if (!repb) {
>> continue;
>> @@ -257,7 +255,6 @@ route_run(struct route_ctx_in *r_ctx_in,
>> if (route->tracked_port) {
>> if (lport_is_local(r_ctx_in->sbrec_port_binding_by_name,
>> r_ctx_in->chassis,
>> - r_ctx_in->active_tunnels,
>> route->tracked_port->logical_port)) {
>> priority = PRIORITY_LOCAL_BOUND;
>> sset_add(r_ctx_out->tracked_ports_local,
>> diff --git a/controller/route.h b/controller/route.h
>> index 11016d818..09aff89ff 100644
>> --- a/controller/route.h
>> +++ b/controller/route.h
>> @@ -35,7 +35,6 @@ struct route_ctx_in {
>> struct ovsdb_idl_index *sbrec_port_binding_by_name;
>> const struct sbrec_chassis *chassis;
>> const char *dynamic_routing_port_mapping;
>> - const struct sset *active_tunnels;
>> const struct hmap *local_datapaths;
>> struct shash *local_bindings;
>> };
>> @@ -81,7 +80,6 @@ struct advertise_route_entry {
>> const struct sbrec_port_binding *route_exchange_find_port(
>> struct ovsdb_idl_index *sbrec_port_binding_by_name,
>> const struct sbrec_chassis *chassis,
>> - const struct sset *active_tunnels,
>> const struct sbrec_port_binding *pb);
>> uint32_t advertise_route_hash(const struct in6_addr *dst, unsigned int
>> plen);
>> void route_run(struct route_ctx_in *, struct route_ctx_out *);
>> --
>> 2.49.0
>>
>>
> Load image error, let's try again.
>
> Recheck-request: github-robot-_Build_and_Test
>
Recheck-request: github-robot-_Build_and_Test
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
