On Mon, Sep 28, 2026 at 4:46 AM Bekei Park <[email protected]> wrote:
> Hi Martin Kalčok > Hi Bekei, I'm looping in the mailing list, because I it was not on your reply and I think that there are some valuable findings in your message. > > I also tried the LRP option, to see whether it avoids the problem. > On our 26.03.3 build, with one backend VM and the monitor sourced > from the router port IP, I recreated the monitor ten times without > this patch and five times with it: > > - Without the patch, the first SYN left 0.24 to 0.50 ms before the > flow that sends the reply to ovn-controller was installed (10/10). > The SYN-ACK reached the tap between 0.24 ms before and 0.35 ms > after that flow's install event. All ten first probes still > succeeded, and the monitor went online on the second probe, as it > did with the patch. > So do I understand correctly that with the current codebase, the monitor comes online only on after the second probe? > - With the patch, the first SYN left after that flow (5/5). > > So the ordering is the same in LRP mode, but I could not make it > fail there. The failure we see needs the second ARP responder, that > is, the reused IP. > > Given that, I understand if you do not want this for a setup that is > outside the documented options. > Far be it for me to decide what get's merged in and what not. That's really up to maintainers, I'm just an occasional contributor :) However, IIUC and the patch improves the health monitor responsiveness, then that's imo a good reason to consider this patch. Best regards, Martin. We will keep the patch downstream, > and I will ask the ovn-octavia-provider developers to use the LRP IP > when the member subnet has a router port. If you still think > sending the first probe only after its flows are installed is worth > having, I can send a v2 with the commit message rewritten around > that. > > Regards, > Bekei > > 2026년 9월 27일 (일) 오후 8:07, Martin Kalčok <[email protected]>님이 작성: > >> Hi Bekei, >> Sorry I didn’t do a full review, I have just one question below. >> >> > On 26 Sep 2026, at 00:39, Bekei Park <[email protected]> wrote: >> > >> > 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, >> >> Wouldn’t the setup like this be a misconfiguration? The documentation [0] >> says that the source IP of the health-check probe should be either an >> unused IP, or an IP of a Logical Router in the network (the second option >> was added in 26.03). Is neither of these a viable option for you? >> >> Even with your fix, imo, the result of re-using existing IP for health >> checks is an unexpected behaviour from the POV of the monitored host. >> Because now it can’t reach the true owner of the IP and the reason for it >> is very opaque. >> >> Best regards, >> Martin. >> >> > 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 >> >> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
