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
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
