When multiple chassis are fighting for the same port (requested-chassis is not set, e.g. for gateway ports), they may produce an unreasonable number of chassis field updates in a very short time frame (hundreds of updates in several seconds). This puts unnecessary load on OVN as well as any db notification consumers trying to keep up with the barrage.
This patch throttles port claim attempts so that they don't happen more frequently than once per 0.5 seconds. Reported: https://bugzilla.redhat.com/show_bug.cgi?id=1974898 Signed-off-by: Ihar Hrachyshka <ihrac...@redhat.com> --- v1: initial version v2: don't postpone claim when port is unclaimed (chassis == nil) v2: don't postpone claim as an additional chassis for a multichassis port v2: fixed memory corruption when modifying sset while iterating over it v3: rebased to resolve a git conflict --- controller/binding.c | 113 ++++++++++++++++++++++++++++++++++-- controller/binding.h | 8 +++ controller/ovn-controller.c | 48 +++++++++++++++ tests/ovn.at | 41 +++++++++++++ 4 files changed, 205 insertions(+), 5 deletions(-) diff --git a/controller/binding.c b/controller/binding.c index 0342c609c..150af9f82 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -48,6 +48,54 @@ VLOG_DEFINE_THIS_MODULE(binding); #define OVN_QOS_TYPE "linux-htb" +#define CLAIM_TIME_THRESHOLD_MS 500 + +struct claimed_port { + long long int last_claimed; +}; + +static struct shash _claimed_ports = SHASH_INITIALIZER(&_claimed_ports); +static struct sset _postponed_ports = SSET_INITIALIZER(&_postponed_ports); + +struct sset * +get_postponed_ports(void) +{ + return &_postponed_ports; +} + +static long long int +get_claim_timestamp(const char *port_name) +{ + struct claimed_port *cp = shash_find_data(&_claimed_ports, port_name); + if (!cp) { + return 0; + } + return cp->last_claimed; +} + +static void +register_claim_timestamp(const char *port_name, long long int t) +{ + struct claimed_port *cp = shash_find_data(&_claimed_ports, port_name); + if (!cp) { + cp = xzalloc(sizeof *cp); + shash_add(&_claimed_ports, port_name, cp); + } + cp->last_claimed = t; +} + +void +schedule_postponed_ports(void) +{ + const char *port_name; + SSET_FOR_EACH (port_name, &_postponed_ports) { + long long int t = get_claim_timestamp(port_name); + if (t) { + poll_timer_wait_until(t + CLAIM_TIME_THRESHOLD_MS); + } + } +} + struct qos_queue { struct hmap_node node; uint32_t queue_id; @@ -1011,6 +1059,23 @@ remove_additional_chassis(const struct sbrec_port_binding *pb, remove_additional_encap_for_chassis(pb, chassis_rec); } +static bool +lport_maybe_postpone(const char *port_name, long long int now, + struct sset *postponed_ports) +{ + long long int last_claimed = get_claim_timestamp(port_name); + 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); + + /* Schedule the next attempt. */ + poll_timer_wait_until(last_claimed + CLAIM_TIME_THRESHOLD_MS); + return true; +} + /* Returns false if lport is not claimed due to 'sb_readonly'. * Returns true otherwise. */ @@ -1021,7 +1086,8 @@ claim_lport(const struct sbrec_port_binding *pb, const struct ovsrec_interface *iface_rec, bool sb_readonly, bool notify_up, struct hmap *tracked_datapaths, - struct if_status_mgr *if_mgr) + struct if_status_mgr *if_mgr, + struct sset *postponed_ports) { if (!sb_readonly) { claimed_lport_set_up(pb, parent_pb, chassis_rec, notify_up, if_mgr); @@ -1036,7 +1102,12 @@ claim_lport(const struct sbrec_port_binding *pb, return false; } + long long int now = time_msec(); if (pb->chassis) { + if (lport_maybe_postpone(pb->logical_port, now, + postponed_ports)) { + return true; + } VLOG_INFO("Changing chassis for lport %s from %s to %s.", pb->logical_port, pb->chassis->name, chassis_rec->name); @@ -1053,6 +1124,9 @@ claim_lport(const struct sbrec_port_binding *pb, remove_additional_chassis(pb, chassis_rec); } update_tracked = true; + + register_claim_timestamp(pb->logical_port, now); + sset_find_and_delete(postponed_ports, pb->logical_port); } } else if (can_bind == CAN_BIND_AS_ADDITIONAL) { if (!is_additional_chassis(pb, chassis_rec)) { @@ -1075,8 +1149,10 @@ claim_lport(const struct sbrec_port_binding *pb, } } - if (update_tracked && tracked_datapaths) { - update_lport_tracking(pb, tracked_datapaths, true); + if (update_tracked) { + if (tracked_datapaths) { + update_lport_tracking(pb, tracked_datapaths, true); + } } /* Check if the port encap binding, if any, has changed */ @@ -1238,7 +1314,8 @@ consider_vif_lport_(const struct sbrec_port_binding *pb, b_lport->lbinding->iface, !b_ctx_in->ovnsb_idl_txn, !parent_pb, b_ctx_out->tracked_dp_bindings, - b_ctx_out->if_mgr)){ + b_ctx_out->if_mgr, + b_ctx_out->postponed_ports)) { return false; } @@ -1534,7 +1611,8 @@ consider_nonvif_lport_(const struct sbrec_port_binding *pb, return claim_lport(pb, NULL, b_ctx_in->chassis_rec, NULL, !b_ctx_in->ovnsb_idl_txn, false, b_ctx_out->tracked_dp_bindings, - b_ctx_out->if_mgr); + b_ctx_out->if_mgr, + b_ctx_out->postponed_ports); } if (pb->chassis == b_ctx_in->chassis_rec || @@ -2759,6 +2837,24 @@ delete_done: } } + /* Also handle any postponed (throttled) ports. */ + const char *port_name; + struct sset postponed_ports = SSET_INITIALIZER(&postponed_ports); + sset_clone(&postponed_ports, b_ctx_out->postponed_ports); + SSET_FOR_EACH (port_name, &postponed_ports) { + pb = lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name, + port_name); + if (!pb) { + sset_find_and_delete(b_ctx_out->postponed_ports, port_name); + continue; + } + handled = handle_updated_port(b_ctx_in, b_ctx_out, pb, qos_map_ptr); + if (!handled) { + break; + } + } + sset_destroy(&postponed_ports); + if (handled && qos_map_ptr && set_noop_qos(b_ctx_in->ovs_idl_txn, b_ctx_in->port_table, b_ctx_in->qos_table, @@ -3201,3 +3297,10 @@ ovs_iface_matches_lport_iface_id_ver(const struct ovsrec_interface *iface, return true; } + +void +binding_destroy(void) +{ + shash_destroy_free_data(&_claimed_ports); + sset_clear(&_postponed_ports); +} diff --git a/controller/binding.h b/controller/binding.h index 1fed06674..161057592 100644 --- a/controller/binding.h +++ b/controller/binding.h @@ -103,6 +103,8 @@ struct binding_ctx_out { struct hmap *tracked_dp_bindings; struct if_status_mgr *if_mgr; + + struct sset *postponed_ports; }; /* Local bindings. binding.c module binds the logical port (represented by @@ -219,4 +221,10 @@ struct binding_lport { size_t n_port_security; }; +struct sset *get_postponed_ports(void); +void schedule_postponed_ports(void); + +/* Clean up module state. */ +void binding_destroy(void); + #endif /* controller/binding.h */ diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 2e9138036..4deb67a9c 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -1175,6 +1175,42 @@ en_activated_ports_run(struct engine_node *node, void *data_) engine_set_node_state(node, state); } +struct ed_type_postponed_ports { + struct sset *postponed_ports; +}; + +static void * +en_postponed_ports_init(struct engine_node *node OVS_UNUSED, + struct engine_arg *arg OVS_UNUSED) +{ + struct ed_type_postponed_ports *data = xzalloc(sizeof *data); + data->postponed_ports = get_postponed_ports(); + return data; +} + +static void +en_postponed_ports_cleanup(void *data_) +{ + struct ed_type_postponed_ports *data = data_; + if (!data->postponed_ports) { + return; + } + data->postponed_ports = NULL; +} + +static void +en_postponed_ports_run(struct engine_node *node, void *data_) +{ + struct ed_type_postponed_ports *data = data_; + enum engine_node_state state = EN_UNCHANGED; + data->postponed_ports = get_postponed_ports(); + if (!sset_is_empty(data->postponed_ports)) { + schedule_postponed_ports(); + state = EN_UPDATED; + } + engine_set_node_state(node, state); +} + struct ed_type_runtime_data { /* Contains "struct local_datapath" nodes. */ struct hmap local_datapaths; @@ -1205,6 +1241,8 @@ struct ed_type_runtime_data { struct shash local_active_ports_ipv6_pd; struct shash local_active_ports_ras; + + struct sset *postponed_ports; }; /* struct ed_type_runtime_data has the below members for tracking the @@ -1405,6 +1443,7 @@ init_binding_ctx(struct engine_node *node, b_ctx_out->egress_ifaces = &rt_data->egress_ifaces; b_ctx_out->lbinding_data = &rt_data->lbinding_data; b_ctx_out->local_iface_ids = &rt_data->local_iface_ids; + b_ctx_out->postponed_ports = rt_data->postponed_ports; b_ctx_out->tracked_dp_bindings = NULL; b_ctx_out->if_mgr = ctrl_ctx->if_mgr; } @@ -1442,6 +1481,10 @@ en_runtime_data_run(struct engine_node *node, void *data) local_binding_data_init(&rt_data->lbinding_data); } + struct ed_type_postponed_ports *pp_data = + engine_get_input_data("postponed_ports", node); + rt_data->postponed_ports = pp_data->postponed_ports; + struct binding_ctx_in b_ctx_in; struct binding_ctx_out b_ctx_out; init_binding_ctx(node, rt_data, &b_ctx_in, &b_ctx_out); @@ -3542,6 +3585,7 @@ main(int argc, char *argv[]) ENGINE_NODE(mff_ovn_geneve, "mff_ovn_geneve"); ENGINE_NODE(ofctrl_is_connected, "ofctrl_is_connected"); ENGINE_NODE_WITH_CLEAR_TRACK_DATA(activated_ports, "activated_ports"); + ENGINE_NODE(postponed_ports, "postponed_ports"); ENGINE_NODE(pflow_output, "physical_flow_output"); ENGINE_NODE_WITH_CLEAR_TRACK_DATA(lflow_output, "logical_flow_output"); ENGINE_NODE(flow_output, "flow_output"); @@ -3681,6 +3725,9 @@ main(int argc, char *argv[]) runtime_data_sb_datapath_binding_handler); engine_add_input(&en_runtime_data, &en_sb_port_binding, runtime_data_sb_port_binding_handler); + /* Reuse the same handler for any previously postponed ports. */ + engine_add_input(&en_runtime_data, &en_postponed_ports, + runtime_data_sb_port_binding_handler); /* The OVS interface handler for runtime_data changes MUST be executed * after the sb_port_binding_handler as port_binding deletes must be @@ -4318,6 +4365,7 @@ loop_done: lflow_destroy(); ofctrl_destroy(); pinctrl_destroy(); + binding_destroy(); patch_destroy(); if_status_mgr_destroy(if_mgr); shash_destroy(&vif_plug_deleted_iface_ids); diff --git a/tests/ovn.at b/tests/ovn.at index d65a0ac39..0436a17d2 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -15422,6 +15422,47 @@ OVN_CLEANUP([hv1],[hv2]) AT_CLEANUP ]) +OVN_FOR_EACH_NORTHD([ +AT_SETUP([tug-of-war between two chassis for the same port]) +ovn_start + +ovn-nbctl ls-add ls0 +ovn-nbctl lsp-add ls0 lsp0 + +net_add n1 +for i in 1 2; do + sim_add hv$i + as hv$i + ovs-vsctl add-br br-phys + ovn_attach n1 br-phys 192.168.0.$i +done + +for i in 1 2; do + as hv$i + ovs-vsctl -- add-port br-int vif \ + -- set Interface vif external-ids:iface-id=lsp0 +done + +# give controllers some time to fight for the port binding +sleep 3 + +# calculate the number of port claims registered by each fighting chassis +hv1_claims=$(as hv1 grep -c 'Claiming\|Changing chassis' hv1/ovn-controller.log) +hv2_claims=$(as hv2 grep -c 'Claiming\|Changing chassis' hv2/ovn-controller.log) + +echo $hv1_claims +echo $hv2_claims + +# check that neither registered an outrageous number of port claims +max_claims=10 +AT_CHECK([test "${hv1_claims}" -le "${max_claims}"], [0], []) +AT_CHECK([test "${hv2_claims}" -le "${max_claims}"], [0], []) + +OVN_CLEANUP([hv1],[hv2]) + +AT_CLEANUP +]) + OVN_FOR_EACH_NORTHD([ AT_SETUP([options:requested-chassis with hostname]) -- 2.34.1 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
