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 <[email protected]> Acked-by: Mark Michelson <[email protected]> --- 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 v4: added opportunistic cleanup for claimed_ports shash v4: made a debug message in the new test case more intelligible v5: fixed a memleak in cleanup_claimed_port_timestamps (node->data not freed) v6: rebased, added Mark's ack. v6: removed poll_wait calls from engine handler, moved them to binding_wait. --- controller/binding.c | 127 ++++++++++++++++++++++++++++++++++-- controller/binding.h | 10 +++ controller/ovn-controller.c | 49 ++++++++++++++ tests/ovn.at | 41 ++++++++++++ 4 files changed, 222 insertions(+), 5 deletions(-) diff --git a/controller/binding.c b/controller/binding.c index 96a158225..9f5393a92 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -48,6 +48,67 @@ 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); + return cp ? cp->last_claimed : 0; +} + +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; +} + +static void +cleanup_claimed_port_timestamps(void) +{ + long long int now = time_msec(); + struct shash_node *node; + SHASH_FOR_EACH_SAFE (node, &_claimed_ports) { + struct claimed_port *cp = (struct claimed_port *) node->data; + if (now - cp->last_claimed >= 5 * CLAIM_TIME_THRESHOLD_MS) { + free(cp); + shash_delete(&_claimed_ports, node); + } + } +} + +/* Schedule any pending binding work. Runs with in the main ovn-controller + * thread context.*/ +void +binding_wait(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; @@ -996,6 +1057,21 @@ 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); + + return true; +} + /* Returns false if lport is not claimed due to 'sb_readonly'. * Returns true otherwise. */ @@ -1006,7 +1082,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); @@ -1021,7 +1098,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); @@ -1038,6 +1120,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)) { @@ -1060,8 +1145,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 */ @@ -1223,7 +1310,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; } @@ -1519,7 +1607,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 || @@ -1843,6 +1932,8 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) } destroy_qos_map(&qos_map); + + cleanup_claimed_port_timestamps(); } /* Returns true if the database is all cleaned up, false if more work is @@ -2740,6 +2831,25 @@ 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); + cleanup_claimed_port_timestamps(); + if (handled && qos_map_ptr && set_noop_qos(b_ctx_in->ovs_idl_txn, b_ctx_in->port_table, b_ctx_in->qos_table, @@ -3182,3 +3292,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..b2360bac2 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,12 @@ struct binding_lport { size_t n_port_security; }; +struct sset *get_postponed_ports(void); + +/* Schedule any pending binding work. */ +void binding_wait(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 5449743e8..8268726e6 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -1175,6 +1175,41 @@ 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)) { + 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 +1240,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 +1442,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 +1480,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 +3584,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 +3724,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 @@ -4191,6 +4237,8 @@ main(int argc, char *argv[]) ofctrl_wait(); pinctrl_wait(ovnsb_idl_txn); } + + binding_wait(); } if (!northd_version_match && br_int) { @@ -4318,6 +4366,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 23b205791..c8cc8cde4 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -15274,6 +15274,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 claimed ${hv1_claims} times" +echo "hv2 claimed ${hv2_claims} times" + +# 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 [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
