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.
Conflicts:
controller/binding.c
controller/binding.h
controller/ovn-controller.c
Reported: https://bugzilla.redhat.com/show_bug.cgi?id=1974898
Signed-off-by: Ihar Hrachyshka <[email protected]>
Acked-by: Mark Michelson <[email protected]>
Signed-off-by: Numan Siddique <[email protected]>
(cherry picked from commit 4dc4bc7fdb848bcc626becbd2c80ffef8a39ff9a)
(cherry picked from commit 887a8df4f4aa08a4a87b42f7aa684ed7e9aff9a1)
---
controller/binding.c | 121 +++++++++++++++++++++++++++++++++++-
controller/binding.h | 10 +++
controller/ovn-controller.c | 49 +++++++++++++++
tests/ovn.at | 41 ++++++++++++
4 files changed, 218 insertions(+), 3 deletions(-)
diff --git a/controller/binding.c b/controller/binding.c
index a43c635ac..5f2a7d117 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;
@@ -912,6 +973,21 @@ claimed_lport_set_up(const struct sbrec_port_binding *pb,
}
}
+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.
*/
@@ -922,7 +998,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);
@@ -933,7 +1010,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);
@@ -949,6 +1031,9 @@ claim_lport(const struct sbrec_port_binding *pb,
if (tracked_datapaths) {
update_lport_tracking(pb, tracked_datapaths, true);
}
+
+ register_claim_timestamp(pb->logical_port, now);
+ sset_find_and_delete(postponed_ports, pb->logical_port);
}
/* Check if the port encap binding, if any, has changed */
@@ -1080,7 +1165,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;
}
@@ -1362,7 +1448,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);
} else if (pb->chassis == b_ctx_in->chassis_rec) {
return release_lport(pb, !b_ctx_in->ovnsb_idl_txn,
b_ctx_out->tracked_dp_bindings,
@@ -1663,6 +1750,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
@@ -2555,6 +2644,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,
@@ -2947,3 +3055,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 e49e1ebb6..96b9ad715 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
@@ -193,4 +195,12 @@ enum en_lport_type {
enum en_lport_type get_lport_type(const struct sbrec_port_binding *);
+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 b5351ba3a..8806017e1 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -1128,6 +1128,41 @@ ovs_interface_shadow_ovs_interface_handler(struct
engine_node *node,
return true;
}
+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;
@@ -1158,6 +1193,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
@@ -1358,6 +1395,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;
}
@@ -1395,6 +1433,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);
@@ -3405,6 +3447,7 @@ main(int argc, char *argv[])
ENGINE_NODE(non_vif_data, "non_vif_data");
ENGINE_NODE(mff_ovn_geneve, "mff_ovn_geneve");
ENGINE_NODE(ofctrl_is_connected, "ofctrl_is_connected");
+ 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");
@@ -3534,6 +3577,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
@@ -4042,6 +4088,8 @@ main(int argc, char *argv[])
ofctrl_wait();
pinctrl_wait(ovnsb_idl_txn);
}
+
+ binding_wait();
}
if (!northd_version_match && br_int) {
@@ -4170,6 +4218,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 d3258ac73..c267fcd04 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -13895,6 +13895,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
