Hi Ales Thanks for the feedback. I'll send v2.
Thanks Xavier On Tue, Jul 8, 2025 at 1:39 PM Ales Musil <[email protected]> wrote: > > > On Wed, Jul 2, 2025 at 11:50 AM Xavier Simonart via dev < > [email protected]> wrote: > >> If BFD is down between two gateways in a chassis group, while both >> gateways >> are alive, both ovn-controller were fighting to claim the router port. >> There was a 500 msec, hardcoded, grace time preventing both controller >> to run at 100% CPU. >> Depending of sb dynamics, either gw1 or gw2 ended up claiming the port >> most of the time. >> For instance: >> - gw1 claims the port at time T >> - gw2 claims the port at time T (+ few micros) >> - gw1 postpone the claims. >> - gw1 claims the port at time T+500. >> - gw2 claims the port at time T+500 (+ few micros) >> - gw1 postpone the claims >> However, computes still send their traffic towards the highest priority >> gateway. >> >> This patch guarantees that, if the highest priority gateway is alive, it >> always claims >> the port. >> This patch does not change the behavior if the highest priority chassis >> "dies". >> >> Note: computes send their traffic towards ha gateways based on BFD status >> and priority, i.e. >> without ovn-controller and sb handling. >> We do not want to base the traffic on which gateway claims the router >> port, as this would require >> ovn-controller & sb handling, potentially delaying migration of the >> traffic. >> >> Signed-off-by: Xavier Simonart <[email protected]> >> --- >> >> > Thank you Xavier, > I have two small nits down below. > > >> controller/binding.c | 24 +++++++++++++++++++----- >> controller/binding.h | 4 +++- >> controller/ha-chassis.c | 2 +- >> controller/ha-chassis.h | 4 ++++ >> controller/if-status.c | 10 ++++++++-- >> 5 files changed, 35 insertions(+), 9 deletions(-) >> >> diff --git a/controller/binding.c b/controller/binding.c >> index 8347e39d9..947340203 100644 >> --- a/controller/binding.c >> +++ b/controller/binding.c >> @@ -1359,16 +1359,30 @@ remove_additional_chassis(const struct >> sbrec_port_binding *pb, >> } >> >> bool >> -lport_maybe_postpone(const char *port_name, long long int now, >> +lport_maybe_postpone(const struct sbrec_port_binding *pb, >> + const struct sbrec_chassis *chassis_rec, >> + long long int now, >> struct sset *postponed_ports) >> { >> - long long int last_claimed = get_claim_timestamp(port_name); >> + if (pb->ha_chassis_group) { >> + struct ha_chassis_ordered *ordered_ha_ch = >> + get_ordered_ha_chassis_list(pb->ha_chassis_group, NULL, >> + chassis_rec); >> + if (ordered_ha_ch && >> + ordered_ha_ch->ha_ch[0].chassis == chassis_rec) { >> + ha_chassis_destroy_ordered(ordered_ha_ch); >> + return false; >> + } else if (ordered_ha_ch) { >> > > nit: The check is done internally in that function. > > + ha_chassis_destroy_ordered(ordered_ha_ch); >> + } >> + } >> + long long int last_claimed = get_claim_timestamp(pb->logical_port); >> if (now - last_claimed >= CLAIM_TIME_THRESHOLD_MS) { >> return false; >> } >> >> - sset_add(postponed_ports, port_name); >> - VLOG_DBG("Postponed claim on logical port %s.", port_name); >> + sset_add(postponed_ports, pb->logical_port); >> + VLOG_DBG("Postponed claim on logical port %s.", pb->logical_port); >> >> return true; >> } >> @@ -1399,7 +1413,7 @@ claim_lport(const struct sbrec_port_binding *pb, >> if (pb->chassis != chassis_rec) { >> long long int now = time_msec(); >> if (pb->chassis) { >> - if (lport_maybe_postpone(pb->logical_port, now, >> + if (lport_maybe_postpone(pb, chassis_rec, now, >> postponed_ports)) { >> return true; >> } >> diff --git a/controller/binding.h b/controller/binding.h >> index 8a5c6996c..8d978544f 100644 >> --- a/controller/binding.h >> +++ b/controller/binding.h >> @@ -282,7 +282,9 @@ void update_qos(struct ovsdb_idl_index * >> sbrec_port_binding_by_name, >> const struct ovsrec_open_vswitch_table *ovs_table, >> const struct ovsrec_bridge_table *bridge_table); >> >> -bool lport_maybe_postpone(const char *port_name, long long int now, >> +bool lport_maybe_postpone(const struct sbrec_port_binding *pb, >> + const struct sbrec_chassis *chassis_rec, >> + long long int now, >> struct sset *postponed_ports); >> >> void claimed_lport_set_up(const struct sbrec_port_binding *pb, >> diff --git a/controller/ha-chassis.c b/controller/ha-chassis.c >> index 945c306b6..ad0b3ef0b 100644 >> --- a/controller/ha-chassis.c >> +++ b/controller/ha-chassis.c >> @@ -59,7 +59,7 @@ compare_chassis_prio_(const void *a_, const void *b_) >> * If active_tunnels is set is empty and local_chassis is HA3, >> * then it returns NULL. >> */ >> -static struct ha_chassis_ordered * >> +struct ha_chassis_ordered * >> get_ordered_ha_chassis_list(const struct sbrec_ha_chassis_group >> *ha_ch_grp, >> const struct sset *active_tunnels, >> const struct sbrec_chassis *local_chassis) >> diff --git a/controller/ha-chassis.h b/controller/ha-chassis.h >> index 3768c2a5c..c7c91e001 100644 >> --- a/controller/ha-chassis.h >> +++ b/controller/ha-chassis.h >> @@ -46,5 +46,9 @@ struct ha_chassis_ordered *ha_chassis_get_ordered( >> >> void ha_chassis_destroy_ordered( >> struct ha_chassis_ordered *ordered_ha_ch); >> +struct ha_chassis_ordered *get_ordered_ha_chassis_list( >> + const struct sbrec_ha_chassis_group *ha_ch_grp, >> + const struct sset *active_tunnels, >> + const struct sbrec_chassis *local_chassis); >> >> #endif /* OVN_HA_CHASSIS_H */ >> diff --git a/controller/if-status.c b/controller/if-status.c >> index 32b6064ba..2358b4b3d 100644 >> --- a/controller/if-status.c >> +++ b/controller/if-status.c >> @@ -515,7 +515,10 @@ if_status_mgr_update(struct if_status_mgr *mgr, >> chassis_rec)) { >> if (!sb_readonly) { >> long long int now = time_msec(); >> - if (lport_maybe_postpone(iface->id, now, >> + const struct sbrec_port_binding *pb = >> + sbrec_port_binding_table_get_for_uuid(pb_table, >> + >> &iface->pb_uuid); >> + if (pb && lport_maybe_postpone(pb, chassis_rec, now, >> get_postponed_ports())) { >> > > nit: Wrong indentation. > > >> continue; >> } >> @@ -572,7 +575,10 @@ if_status_mgr_update(struct if_status_mgr *mgr, >> if (!local_bindings_pb_chassis_is_set(bindings, iface->id, >> chassis_rec)) { >> long long int now = time_msec(); >> - if (lport_maybe_postpone(iface->id, now, >> + const struct sbrec_port_binding *pb = >> + sbrec_port_binding_table_get_for_uuid(pb_table, >> + >> &iface->pb_uuid); >> + if (pb && lport_maybe_postpone(pb, chassis_rec, now, >> get_postponed_ports())) { >> > > nit: Wrong indentation. > > >> continue; >> } >> -- >> 2.47.1 >> >> _______________________________________________ >> dev mailing list >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> > With that addressed: > Acked-by: Ales Musil <[email protected]> > > Regards, > Ales > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
