A new service monitor sends its first probe as soon as sync_svc_monitors() sees the Service_Monitor row. pinctrl_run() runs before ofctrl_put() in the same main loop iteration, so the probe leaves before the flows computed from the same SB update are in OVS. The reply depends on some of these flows. ovn-northd adds the Service_Monitor row and the ARP responder that answers for the monitor source IP with 'svc_monitor_mac' in one transaction:
priority 110, "arp.tpa == <src_ip> && arp.op == 1" The backend usually has no ARP entry for the source IP yet and broadcasts a request as soon as the SYN arrives. If that request is handled before the flow above is installed, and another responder for the same IP exists, the backend learns the wrong MAC. With ovn-octavia-provider the source IP belongs to the "ovn-lb-hm-<subnet>" localport, which has the usual priority 50 responder for broadcast requests with its own MAC. The SYN-ACKs then go to the localport and never reach ovn-controller, the monitor goes offline after failure_count probes, and the backends of the load balancer are removed until the backend refreshes its ARP entry with a unicast request, which hits the priority 110 flow. We saw 7 to 41 seconds without a working backend after the first health check of a subnet was created. On a bare metal deployment the backend sent the ARP request 0.11 ms after the first SYN and got the localport MAC every time. On a nested test setup the request came 0.36 ms after the SYN and the priority 110 flow was installed just in time, although the SYN was sent about 0.8 ms before OVS reported that flow. Keep a new monitor in a new SVC_MON_S_WAIT_FLOWS state and request an ofctrl seqno for it in sync_svc_monitors(), which runs before ofctrl_put() of the same iteration. Move the monitor to SVC_MON_S_INIT once the seqno is acked, i.e. once the flows for these SB contents are installed. If the OpenFlow connection drops, ofctrl_seqno_flush() drops the request, so request it again. Replies are ignored while a monitor waits, and new monitors are zeroed so that the probe fields are never used uninitialized. This also covers monitors that start because the backend port was bound to this chassis or came up. There is no test that makes the race deterministic: the flows have to stay uninstalled while the main loop keeps running. The existing health check tests pass and show that the probes still start. CC: Numan Siddique <[email protected]> Fixes: 8be01f4a5329 ("Send service monitor health checks") Assisted-by: Claude Opus 5.5, Claude Code Signed-off-by: Bekei Park <[email protected]> --- controller/ovn-controller.c | 2 + controller/pinctrl.c | 98 +++++++++++++++++++++++++++++++++++-- controller/pinctrl.h | 2 + 3 files changed, 99 insertions(+), 3 deletions(-) diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index c601f89dc..d98cb721c 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -1073,6 +1073,7 @@ en_ofctrl_is_connected_run(struct engine_node *node OVS_UNUSED, void *data) if (!of_data->connected) { ofctrl_seqno_flush(); if_status_mgr_clear(ctrl_ctx->if_mgr); + pinctrl_seqno_flush(); } return EN_UPDATED; } @@ -8671,6 +8672,7 @@ main(int argc, char *argv[]) stopwatch_start(OFCTRL_SEQNO_RUN_STOPWATCH_NAME, time_msec()); ofctrl_seqno_run(ofctrl_get_cur_cfg()); + pinctrl_seqno_run(); stopwatch_stop(OFCTRL_SEQNO_RUN_STOPWATCH_NAME, time_msec()); stopwatch_start(IF_STATUS_MGR_RUN_STOPWATCH_NAME, diff --git a/controller/pinctrl.c b/controller/pinctrl.c index e10091251..a8df78ee7 100644 --- a/controller/pinctrl.c +++ b/controller/pinctrl.c @@ -30,6 +30,7 @@ #include "mac-cache.h" #include "nx-match.h" #include "ofctrl.h" +#include "lib/ofctrl-seqno.h" #include "latch.h" #include "lib/packets.h" #include "lib/sset.h" @@ -7121,6 +7122,8 @@ pinctrl_handle_bind_vport( } enum svc_monitor_state { + SVC_MON_S_WAIT_FLOWS, /* New monitor: waits until the flows computed + * from the same SB contents are installed. */ SVC_MON_S_INIT, SVC_MON_S_WAITING, SVC_MON_S_ONLINE, @@ -7204,17 +7207,29 @@ struct svc_monitor { ovs_be16 icmp_id; ovs_be16 icmp_seq_no; + /* ofctrl_seqno requested for SVC_MON_S_WAIT_FLOWS, 0 if none yet. + * Accessed only by the main ovn-controller thread. */ + uint64_t flows_seqno; + bool delete; }; static struct hmap svc_monitors_map; static struct ovs_list svc_monitors; +/* ofctrl_seqno type used to learn when the flows are installed for new + * service monitors, and the last requested and acked values. Accessed + * only by the main ovn-controller thread. */ +static size_t svc_mon_seqno_type; +static uint64_t svc_mon_seqno_requested; +static uint64_t svc_mon_seqno_acked; + static void init_svc_monitors(void) { hmap_init(&svc_monitors_map); ovs_list_init(&svc_monitors); + svc_mon_seqno_type = ofctrl_seqno_add_type(); } static void @@ -7428,13 +7443,13 @@ sync_svc_monitors(struct ovsdb_idl_txn *ovnsb_idl_txn, sb_svc_mon->port, protocol, hash); if (!svc_mon) { - svc_mon = xmalloc(sizeof *svc_mon); + svc_mon = xzalloc(sizeof *svc_mon); svc_mon->dp_key = dp_key; svc_mon->port_key = port_key; svc_mon->proto_port = sb_svc_mon->port; svc_mon->ip = ip_addr; svc_mon->is_ip6 = !is_ipv4; - svc_mon->state = SVC_MON_S_INIT; + svc_mon->state = SVC_MON_S_WAIT_FLOWS; svc_mon->status = SVC_MON_ST_UNKNOWN; svc_mon->protocol = protocol; @@ -7501,12 +7516,76 @@ sync_svc_monitors(struct ovsdb_idl_txn *ovnsb_idl_txn, } } + /* The first probe of a new monitor must wait until the flows computed + * from these SB contents are installed. Otherwise the reply can race + * with them, e.g. the backend resolves the probe source IP before the + * ARP responder for 'svc_monitor_mac' exists and the replies never reach + * ovn-controller. This is called before ofctrl_put() of the same + * iteration, so the request is acked once those flows are installed. */ + bool requested = false; + LIST_FOR_EACH (svc_mon, list_node, &svc_monitors) { + if (svc_mon->state == SVC_MON_S_WAIT_FLOWS && !svc_mon->flows_seqno) { + if (!requested) { + ofctrl_seqno_update_create(svc_mon_seqno_type, + ++svc_mon_seqno_requested); + requested = true; + } + svc_mon->flows_seqno = svc_mon_seqno_requested; + } + } + if (changed) { notify_pinctrl_handler(); } } +/* Must be called by the main thread after ofctrl_seqno_run(). */ +void +pinctrl_seqno_run(void) +{ + struct ofctrl_acked_seqnos *acked = + ofctrl_acked_seqnos_get(svc_mon_seqno_type); + uint64_t last_acked = acked->last_acked; + ofctrl_acked_seqnos_destroy(acked); + + if (last_acked == svc_mon_seqno_acked) { + return; + } + svc_mon_seqno_acked = last_acked; + + bool changed = false; + struct svc_monitor *svc_mon; + ovs_mutex_lock(&pinctrl_mutex); + LIST_FOR_EACH (svc_mon, list_node, &svc_monitors) { + if (svc_mon->state == SVC_MON_S_WAIT_FLOWS && svc_mon->flows_seqno + && svc_mon->flows_seqno <= last_acked) { + svc_mon->state = SVC_MON_S_INIT; + changed = true; + } + } + ovs_mutex_unlock(&pinctrl_mutex); + + if (changed) { + notify_pinctrl_handler(); + } +} + +/* Must be called by the main thread after ofctrl_seqno_flush(). The flushed + * requests are never acked, so the waiting monitors request again. */ +void +pinctrl_seqno_flush(void) +{ + struct svc_monitor *svc_mon; + ovs_mutex_lock(&pinctrl_mutex); + LIST_FOR_EACH (svc_mon, list_node, &svc_monitors) { + if (svc_mon->state == SVC_MON_S_WAIT_FLOWS) { + svc_mon->flows_seqno = 0; + } + } + ovs_mutex_unlock(&pinctrl_mutex); +} + enum bfd_state { BFD_STATE_ADMIN_DOWN, BFD_STATE_DOWN, @@ -8452,6 +8531,10 @@ svc_monitors_run(struct rconn *swconn, long long int next_run_time = LLONG_MAX; enum svc_monitor_status old_status = svc_mon->status; switch (svc_mon->state) { + case SVC_MON_S_WAIT_FLOWS: + /* pinctrl_seqno_run() moves it to SVC_MON_S_INIT. */ + break; + case SVC_MON_S_INIT: svc_monitor_send_health_check(swconn, svc_mon); next_run_time = svc_mon->wait_time; @@ -8534,6 +8617,11 @@ static void pinctrl_handle_icmp_svc_check(struct dp_packet *pkt_in, struct svc_monitor *svc_mon) { + if (svc_mon->state == SVC_MON_S_WAIT_FLOWS) { + /* No probe sent yet. */ + return; + } + if (!svc_mon->is_ip6) { /* IPv4 ICMP echo reply */ struct icmp_header *ih = dp_packet_l4(pkt_in); @@ -8573,7 +8661,7 @@ pinctrl_handle_tcp_svc_check(struct rconn *swconn, { struct tcp_header *th = dp_packet_l4(pkt_in); - if (!th) { + if (!th || svc_mon->state == SVC_MON_S_WAIT_FLOWS) { return false; } @@ -8817,6 +8905,10 @@ pinctrl_handle_svc_check(struct rconn *swconn, const struct flow *ip_flow, return; } + if (svc_mon->state == SVC_MON_S_WAIT_FLOWS) { + return; + } + if (orig_uh->udp_src != svc_mon->tp_src) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); VLOG_WARN_RL(&rl, "handle service check: UDP src port doesn't " diff --git a/controller/pinctrl.h b/controller/pinctrl.h index 0667ac34e..a638fe29f 100644 --- a/controller/pinctrl.h +++ b/controller/pinctrl.h @@ -63,6 +63,8 @@ void pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, int64_t cur_cfg); void pinctrl_wait(struct ovsdb_idl_txn *ovnsb_idl_txn); void pinctrl_destroy(void); +void pinctrl_seqno_run(void); +void pinctrl_seqno_flush(void); void pinctrl_update_swconn(const char *target, int probe_interval); -- 2.43.0 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
