This is a good change, I have one concern below.
On Tue, Nov 3, 2026 at 10:30 AM Bekei Park via dev <[email protected]> wrote: > ofctrl_put() runs ecmp_nexthop_monitor_run() before it checks > ofctrl_can_put(). The monitor builds the NXT_CT_FLUSH message for a > next-hop that went away and forgets that next-hop at once. If ofctrl > cannot send at that point, because the switch connection still has > messages queued or is not in S_UPDATE_FLOWS, ofctrl_put() deletes the > messages and returns. The flows are sent on a later run, but the flush > is never built again, so the conntrack entries that carry the removed > next-hop MAC in ct_label stay until they expire. > > We hit this while removing members of an ecmp-symmetric-reply route > under load: the route was gone from the flows, but established > connections kept the removed member in ct_label and their replies kept > going to it. > > Keep a removed next-hop in a pending set instead, and build its flush > only once ofctrl_can_put() passed and while the switch connection is > up (rconn_send() drops what is queued on a down connection). A pending > flush sets need_put, so it is still sent when no flow changes. This > also stops encoding a flush with rconn_get_version() == -1 before the > first connection to the switch. > > The new test stops ovs-vswitchd, fills the OpenFlow connection with a > large flow update and removes a next-hop; without this change its flush > never reaches the switch. > > CC: Lorenzo Bianconi <[email protected]> > CC: Dumitru Ceara <[email protected]> > Fixes: b4cacd5c2142 ("ofctrl: Introduce ecmp_nexthop_monitor.") > Assisted-by: Claude Opus 5.5, Claude Code > Signed-off-by: Bekei Park <[email protected]> > --- > controller/ecmp-next-hop-monitor.c | 53 +++++++++++++++---- > controller/ecmp-next-hop-monitor.h | 9 ++-- > controller/ofctrl.c | 23 +++++---- > tests/ovn.at | 82 ++++++++++++++++++++++++++++++ > 4 files changed, 143 insertions(+), 24 deletions(-) > > diff --git a/controller/ecmp-next-hop-monitor.c > b/controller/ecmp-next-hop-monitor.c > index 39ea46a..8884c14 100644 > --- a/controller/ecmp-next-hop-monitor.c > +++ b/controller/ecmp-next-hop-monitor.c > @@ -25,8 +25,16 @@ > #include "ovn-sb-idl.h" > #include "controller/ecmp-next-hop-monitor.h" > > +VLOG_DEFINE_THIS_MODULE(ecmp_nexthop_monitor); > + > +/* Next-hops of the SB ECMP_Nexthop table that are in use on local > + * datapaths. */ > static struct hmap ecmp_nexthop; > > +/* Next-hops that went away but whose conntrack flush has not been sent to > + * the switch yet. They stay here until ofctrl is able to send it. */ > +static struct hmap ecmp_nexthop_pending_flush; > + > struct ecmp_nexthop_data { > struct hmap_node hmap_node; > uint16_t zone_id; > @@ -37,6 +45,7 @@ struct ecmp_nexthop_data { > void ecmp_nexthop_init(void) > { > hmap_init(&ecmp_nexthop); > + hmap_init(&ecmp_nexthop_pending_flush); > } > > static void > @@ -60,6 +69,7 @@ ecmp_nexthop_destroy_map(struct hmap *map) > void ecmp_nexthop_destroy(void) > { > ecmp_nexthop_destroy_map(&ecmp_nexthop); > + ecmp_nexthop_destroy_map(&ecmp_nexthop_pending_flush); > } > > static uint32_t > @@ -137,15 +147,16 @@ ecmp_nexthop_monitor_flush_ct_entry(const struct > rconn *swconn, > ovs_list_push_back(msgs, &msg->list_node); > } > > -bool > +/* Updates the monitored next-hops from 'enh_table'. A monitored next-hop > + * that is no longer there is moved to the pending flush set; its > conntrack > + * entries are flushed by ecmp_nexthop_monitor_flush(). */ > +void > ecmp_nexthop_monitor_run(const struct sbrec_ecmp_nexthop_table *enh_table, > const struct hmap *local_datapaths, > - const struct shash *current_ct_zones, > - const struct rconn *swconn, struct ovs_list > *msgs) > + const struct shash *current_ct_zones) > { > struct hmap sb_ecmp_nexthop = HMAP_INITIALIZER(&sb_ecmp_nexthop); > const struct sbrec_ecmp_nexthop *sbrec_ecmp_nexthop; > - bool ret = false; > > ovs_assert(local_datapaths); > > @@ -187,14 +198,38 @@ ecmp_nexthop_monitor_run(const struct > sbrec_ecmp_nexthop_table *enh_table, > HMAP_FOR_EACH_SAFE (e, hmap_node, &ecmp_nexthop) { > if (!ecmp_nexthop_find_entry(e->nexthop, e->mac, e->zone_id, > &sb_ecmp_nexthop)) { > - ecmp_nexthop_monitor_flush_ct_entry(swconn, e->mac, > - e->zone_id, msgs); > hmap_remove(&ecmp_nexthop, &e->hmap_node); > - ecmp_nexthop_destroy_entry(e); > - ret = true; > + if (ecmp_nexthop_find_entry(e->nexthop, e->mac, e->zone_id, > + &ecmp_nexthop_pending_flush)) { > + ecmp_nexthop_destroy_entry(e); > + } else { > + hmap_insert(&ecmp_nexthop_pending_flush, &e->hmap_node, > + ecmp_nexthop_hash(e->nexthop, e->mac, > + e->zone_id)); > + } > } > } > ecmp_nexthop_destroy_map(&sb_ecmp_nexthop); > +} > + > +bool > +ecmp_nexthop_monitor_has_pending_flush(void) > +{ > + return !hmap_is_empty(&ecmp_nexthop_pending_flush); > +} > > - return ret; > +/* Appends to 'msgs' a conntrack flush for each pending next-hop and > forgets > + * them. The caller must make sure that 'msgs' are going to be sent to > the > + * switch. */ > +void > +ecmp_nexthop_monitor_flush(const struct rconn *swconn, struct ovs_list > *msgs) > +{ > + struct ecmp_nexthop_data *e; > + HMAP_FOR_EACH_POP (e, hmap_node, &ecmp_nexthop_pending_flush) { > + VLOG_DBG("Flushing CT for ECMP next-hop %s (MAC %s) in zone > %"PRIu16, > + e->nexthop, e->mac, e->zone_id); > + ecmp_nexthop_monitor_flush_ct_entry(swconn, e->mac, e->zone_id, > + msgs); > + ecmp_nexthop_destroy_entry(e); > + } > } > I think this needs to be revalidated against the SB table. Consider a next-hop that is removed while ofctrl is backlogged, then re-added before ofctrl recovers. ecmp_nexthop_run() re-inserts it into ecmp-nexthop, but the stale copy stays in ecmp_nexthop_pending_flush. When ofctrl can send again the flush drops the conntrack entries for a next-hop that is back in service. I think we have to drop pending entries when the same (next-hop, mac, zone_id) tuple reappears in enh_table during exmp_nexthop_monitor_run(). What do you think? > diff --git a/controller/ecmp-next-hop-monitor.h > b/controller/ecmp-next-hop-monitor.h > index 59a4bc7..ac514ce 100644 > --- a/controller/ecmp-next-hop-monitor.h > +++ b/controller/ecmp-next-hop-monitor.h > @@ -18,9 +18,10 @@ > > void ecmp_nexthop_init(void); > void ecmp_nexthop_destroy(void); > -bool ecmp_nexthop_monitor_run(const struct sbrec_ecmp_nexthop_table *, > +void ecmp_nexthop_monitor_run(const struct sbrec_ecmp_nexthop_table *, > const struct hmap *local_datapaths, > - const struct shash *current_ct_zones, > - const struct rconn *swconn, > - struct ovs_list *msgs); > + const struct shash *current_ct_zones); > +bool ecmp_nexthop_monitor_has_pending_flush(void); > +void ecmp_nexthop_monitor_flush(const struct rconn *swconn, > + struct ovs_list *msgs); > #endif /* OVN_ECMP_NEXT_HOP_MONITOR_H */ > diff --git a/controller/ofctrl.c b/controller/ofctrl.c > index c90b200..12da7f7 100644 > --- a/controller/ofctrl.c > +++ b/controller/ofctrl.c > @@ -2802,30 +2802,31 @@ ofctrl_put(struct ovn_desired_flow_table > *lflow_table, > } > } > > - /* OpenFlow messages to send to the switch to bring it up-to-date. */ > - struct ovs_list msgs = OVS_LIST_INITIALIZER(&msgs); > - > if (local_datapaths) { > - need_put |= ecmp_nexthop_monitor_run(enh_table, local_datapaths, > - current_ct_zones, swconn, > &msgs); > + ecmp_nexthop_monitor_run(enh_table, local_datapaths, > + current_ct_zones); > } > + need_put |= ecmp_nexthop_monitor_has_pending_flush(); > > if (!need_put) { > - ovs_assert(ovs_list_is_empty(&msgs)); > VLOG_DBG("ofctrl_put not needed"); > return; > } > if (!ofctrl_can_put()) { > VLOG_DBG("ofctrl_put can't be performed"); > - > - struct ofpbuf *msg; > - LIST_FOR_EACH_POP (msg, list_node, &msgs) { > - ofpbuf_delete(msg); > - } > skipped_last_time = true; > return; > } > > + /* OpenFlow messages to send to the switch to bring it up-to-date. */ > + struct ovs_list msgs = OVS_LIST_INITIALIZER(&msgs); > + > + /* Removed ECMP next-hops stay pending while the connection is down, > + * because rconn drops the messages queued then. */ > + if (rconn_is_connected(swconn)) { > + ecmp_nexthop_monitor_flush(swconn, &msgs); > + } > + > /* Iterate through ct zones that need to be flushed. */ > struct shash_node *iter; > SHASH_FOR_EACH(iter, pending_ct_zones) { > diff --git a/tests/ovn.at b/tests/ovn.at > index 7801494..d14418b 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -48682,3 +48682,85 @@ AT_CHECK([ovs-ofctl dump-flows br-int > table=OFTABLE_PHY_TO_LOG \ > OVN_CLEANUP([hv1]) > AT_CLEANUP > ]) > + > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([ECMP next-hop conntrack flush when ofctrl cannot send]) > +AT_KEYWORDS([ecmp ecmp_nexthop_monitor]) > +ovn_start > + > +net_add n1 > +sim_add hv1 > +as hv1 > +check ovs-vsctl add-br br-phys > +ovn_attach n1 br-phys 192.168.0.1 > +check ovn-appctl -t ovn-controller vlog/set ofctrl:dbg > + > +check ovn-nbctl set nb_global . options:ecmp_nexthop_monitor_enable=true > +check ovn-nbctl lr-add lr0 -- set logical_router lr0 options:chassis=hv1 > +check ovn-nbctl lrp-add lr0 lr0-public 00:00:20:20:12:13 192.168.10.1/24 > +check ovn-nbctl ls-add public > +check ovn-nbctl lsp-add-router-port public public-lr0 lr0-public > +check ovn-nbctl --ecmp-symmetric-reply lr-route-add lr0 1.0.0.0/24 > 192.168.10.10 > +check ovn-nbctl --ecmp-symmetric-reply lr-route-add lr0 1.0.0.0/24 > 192.168.10.20 > +check ovn-nbctl --wait=sb sync > + > +dp=$(fetch_column Datapath_Binding _uuid external_ids:name=lr0) > +check_uuid ovn-sbctl create MAC_Binding datapath=$dp > logical_port=lr0-public \ > + ip=192.168.10.10 mac='"f0:00:00:00:10:10"' > +check_uuid ovn-sbctl create MAC_Binding datapath=$dp > logical_port=lr0-public \ > + ip=192.168.10.20 mac='"f0:00:00:00:20:20"' > +wait_row_count ECMP_Nexthop 1 nexthop=192.168.10.10 > mac='"f0:00:00:00:10:10"' > +wait_row_count ECMP_Nexthop 1 nexthop=192.168.10.20 > mac='"f0:00:00:00:20:20"' > +check ovn-nbctl --wait=hv sync > + > +zone=$(ovn-appctl -t ovn-controller ct-zone-list | grep "^lr0_dnat " | > cut -d ' ' -f 2) > +check test -n "$zone" > +flush_10="NXT_CT_FLUSH .* zone=$zone labels=0xf0000000101000000000/" > +flush_20="NXT_CT_FLUSH .* zone=$zone labels=0xf0000000202000000000/" > + > +AS_BOX([Removed next-hop is flushed]) > +n=$(wc -l < hv1/ovs-vswitchd.log) > +check ovn-nbctl --wait=hv lr-route-del lr0 1.0.0.0/24 192.168.10.20 > +tail -n +$((n + 1)) hv1/ovs-vswitchd.log > vswitchd-del.log > +AT_CHECK([grep -c "$flush_20" vswitchd-del.log], [0], [1 > +]) > +AT_CHECK([grep -c "$flush_10" vswitchd-del.log], [1], [0 > +]) > + > +AS_BOX([Removed next-hop is flushed when ofctrl could not send at > removal]) > +check ovn-nbctl --ecmp-symmetric-reply lr-route-add lr0 1.0.0.0/24 > 192.168.10.20 > +wait_row_count ECMP_Nexthop 1 nexthop=192.168.10.20 > mac='"f0:00:00:00:20:20"' > +check ovn-nbctl --wait=hv sync > + > +# Stop the switch and queue more flow updates than the connection can > +# buffer, so that ofctrl has a backlog. > +sleep_ovs hv1 > +addrs=$(for i in $(seq 0 19); do for j in $(seq 1 250); do > + printf '"10.0.%d.%d",' $i $j > + done; done) > +stats=$(ovn-appctl -t ovn-controller inc-engine/show-stats lflow_output) > +check_uuid ovn-nbctl --wait=sb create Address_Set name=big \ > + addresses="${addrs%,}" -- acl-add public to-lport 1001 'ip4.src == > $big' allow > +OVS_WAIT_UNTIL([test "$stats" != "$(ovn-appctl -t ovn-controller > inc-engine/show-stats lflow_output)"]) > + > +# Remove the next-hop while ovn-controller sleeps, so that the removal is > +# processed in one run while the backlog is still there. > +sleep_controller hv1 > +check ovn-nbctl --wait=sb lr-route-del lr0 1.0.0.0/24 192.168.10.20 > +check_row_count ECMP_Nexthop 0 nexthop=192.168.10.20 > +n_skip=$(grep -c "ofctrl_put can't be performed" hv1/ovn-controller.log) > +stats=$(ovn-appctl -t ovn-controller inc-engine/show-stats lflow_output) > +wake_up_controller hv1 > +OVS_WAIT_UNTIL([test "$stats" != "$(ovn-appctl -t ovn-controller > inc-engine/show-stats lflow_output)"]) > +OVS_WAIT_UNTIL([test $(grep -c "ofctrl_put can't be performed" > hv1/ovn-controller.log) -gt $n_skip]) > + > if you re-add the route here and change the check below from 2->1 it should exercise what I mean about needing to remove entries from the pending flush. > +wake_up_ovs hv1 > +check ovn-nbctl --wait=hv sync > +AT_CHECK([grep -c "$flush_20" hv1/ovs-vswitchd.log], [0], [2 > +]) > +AT_CHECK([grep -c "$flush_10" hv1/ovs-vswitchd.log], [1], [0 > +]) > + > +OVN_CLEANUP([hv1]) > +AT_CLEANUP > +]) > -- > 2.43.0 > > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Thanks, Jacob Tanenbaum _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
