On Thu, Aug 11, 2022 at 4:07 AM Ihar Hrachyshka <[email protected]> wrote: > > Numan, thank you! > > Is it backport material? I know there may be some conflicts and am > happy to handle them if we agree this fix can be backported.
@Mark Michelson @Han Zhou Any comments or any objections on this ? Thanks Numan > > Thanks again. > > Ihar > > On Tue, Aug 9, 2022 at 8:50 PM Numan Siddique <[email protected]> wrote: > > > > On Wed, Aug 10, 2022 at 4:25 AM Ihar Hrachyshka <[email protected]> wrote: > > > > > > 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]> > > > > Thanks for the v6. I applied both the patches to the main. > > > > Numan > > > > > --- > > > 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 > > > > > > > _______________________________________________ > 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
