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. A flush only stays pending while the flows that remove the next-hop have not reached the switch either. If the same next-hop comes back in the meantime, its conntrack entries are still valid, so the pending flush is dropped. 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. It then removes the next-hop and adds it back while ofctrl is still backlogged and checks that no flush is sent. 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]> --- v2: - Drop a pending flush when the same next-hop comes back before the flush was sent, and test that case (Jacob Tanenbaum). controller/ecmp-next-hop-monitor.c | 64 +++++++++++++--- controller/ecmp-next-hop-monitor.h | 9 ++- controller/ofctrl.c | 23 +++--- tests/ovn.at | 118 +++++++++++++++++++++++++++++ 4 files changed, 190 insertions(+), 24 deletions(-) diff --git a/controller/ecmp-next-hop-monitor.c b/controller/ecmp-next-hop-monitor.c index 39ea46a..7f63e6e 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); @@ -178,6 +189,17 @@ ecmp_nexthop_monitor_run(const struct sbrec_ecmp_nexthop_table *enh_table, sbrec_ecmp_nexthop->mac, ct_zone->zone, &ecmp_nexthop); } + + /* A next-hop that is back before its flush was sent never left the + * flows on the switch, so its conntrack entries are still valid. */ + struct ecmp_nexthop_data *pending = + ecmp_nexthop_find_entry(sbrec_ecmp_nexthop->nexthop, + sbrec_ecmp_nexthop->mac, ct_zone->zone, + &ecmp_nexthop_pending_flush); + if (pending) { + hmap_remove(&ecmp_nexthop_pending_flush, &pending->hmap_node); + ecmp_nexthop_destroy_entry(pending); + } ecmp_nexthop_alloc_entry(sbrec_ecmp_nexthop->nexthop, sbrec_ecmp_nexthop->mac, ct_zone->zone, &sb_ecmp_nexthop); @@ -187,14 +209,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); + } } 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..2ba8514 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -48682,3 +48682,121 @@ 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]) + +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 +]) + +AS_BOX([Next-hop that is back before ofctrl can send is not flushed]) +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 again and queue the removal of the big ACL. +sleep_ovs hv1 +stats=$(ovn-appctl -t ovn-controller inc-engine/show-stats lflow_output) +check ovn-nbctl --wait=sb acl-del public -- destroy Address_Set big +OVS_WAIT_UNTIL([test "$stats" != "$(ovn-appctl -t ovn-controller inc-engine/show-stats lflow_output)"]) + +# Remove the next-hop and add it back, each in its own run, while ofctrl +# still cannot send. +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]) + +sleep_controller hv1 +check ovn-nbctl --wait=sb --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"' +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]) + +wake_up_ovs hv1 +check ovn-nbctl --wait=hv sync +AT_CHECK([grep -c "$flush_20" hv1/ovs-vswitchd.log], [0], [2 +]) + +OVN_CLEANUP([hv1]) +AT_CLEANUP +]) -- 2.43.0 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
