We don't need to explicitly add port bindings that were already bound
locally. We implicitly get those because we monitor the datapaths
they're attached to.
When performing an ovn-heater 500-node density-heavy scale test [0], with
conditional monitoring enabled, the unreasonably long poll intervals on
the Southbound database (the ones that took more than one second) are
measured as:
---------------------------------------------------------------------
Count Min Max Median Mean 95 percentile
---------------------------------------------------------------------
56.0 1010.0 2664.0 1455.5 1544.9 2163.0
77.0 1015.0 3460.0 1896.0 1858.2 2907.8
69.0 1010.0 3118.0 1618.0 1688.1 2582.4
---------------------------------------------------------------------
202.0 1010.0 3460.0 1610.0 1713.3 2711.5
Compared to the baseline results (also with conditional monitoring
enabled):
---------------------------------------------------------------------
Count Min Max Median Mean 95 percentile
---------------------------------------------------------------------
141.0 1003.0 18338.0 1721.0 2625.4 7626.0
151.0 1019.0 80297.0 1808.0 3410.7 9089.0
165.0 1007.0 50736.0 2354.0 3067.7 7309.8
---------------------------------------------------------------------
457.0 1003.0 80297.0 2131.0 3044.6 7799.6
We see significant improvement on the server side. This is explained
by the fact that now the Southbound database server doesn't need to
apply as many conditions as before when filtering individual monitor
contents.
Note: Sub-ports - OVN switch ports with parent_port set - have to be
monitored unconditionally as we cannot efficiently determine their local
datapaths without including all local OVS interfaces in the monitor.
This, however, should not be a huge issue because the majority of ports
are regular VIFs, not sub-ports.
[0]
https://github.com/ovn-org/ovn-heater/blob/main/test-scenarios/ocp-500-density-heavy.yml
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2139194
Reported-by: Ilya Maximets <[email protected]>
Signed-off-by: Dumitru Ceara <[email protected]>
---
controller/binding.c | 2 +-
controller/binding.h | 2 +-
controller/ovn-controller.c | 19 ++++++++++++++++---
3 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/controller/binding.c b/controller/binding.c
index 9b0647b70e..ad03332915 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -761,7 +761,7 @@ local_binding_data_destroy(struct local_binding_data
*lbinding_data)
}
const struct sbrec_port_binding *
-local_binding_get_primary_pb(struct shash *local_bindings,
+local_binding_get_primary_pb(const struct shash *local_bindings,
const char *pb_name)
{
struct local_binding *lbinding =
diff --git a/controller/binding.h b/controller/binding.h
index 0e57f02ee5..319cbd263c 100644
--- a/controller/binding.h
+++ b/controller/binding.h
@@ -150,7 +150,7 @@ void local_binding_data_init(struct local_binding_data *);
void local_binding_data_destroy(struct local_binding_data *);
const struct sbrec_port_binding *local_binding_get_primary_pb(
- struct shash *local_bindings, const char *pb_name);
+ const struct shash *local_bindings, const char *pb_name);
ofp_port_t local_binding_get_lport_ofport(const struct shash *local_bindings,
const char *pb_name);
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 3a81a13fb0..c82f0697e8 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -199,6 +199,7 @@ static unsigned int
update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
const struct sbrec_chassis *chassis,
const struct sset *local_ifaces,
+ const struct shash *local_bindings,
struct hmap *local_datapaths,
bool monitor_all)
{
@@ -297,10 +298,21 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
if (local_ifaces) {
const char *name;
+
+ ovs_assert(local_bindings);
SSET_FOR_EACH (name, local_ifaces) {
+ /* Skip the VIFs we bound already, we should have a local datapath
+ * for those. */
+ const struct sbrec_port_binding *local_pb
+ = local_binding_get_primary_pb(local_bindings, name);
+ if (local_pb && get_lport_type(local_pb) == LP_VIF) {
+ continue;
+ }
sbrec_port_binding_add_clause_logical_port(&pb, OVSDB_F_EQ, name);
- sbrec_port_binding_add_clause_parent_port(&pb, OVSDB_F_EQ, name);
}
+ /* Monitor all sub-ports unconditionally; we don't expect a lot of
+ * them in the SB database. */
+ sbrec_port_binding_add_clause_parent_port(&pb, OVSDB_F_NE, NULL);
}
if (local_datapaths) {
const struct local_datapath *ld;
@@ -609,7 +621,7 @@ update_sb_db(struct ovsdb_idl *ovs_idl, struct ovsdb_idl
*ovnsb_idl,
* extra cost. Instead, it is called after the engine execution only
* when it is necessary. */
unsigned int next_cond_seqno =
- update_sb_monitors(ovnsb_idl, NULL, NULL, NULL, true);
+ update_sb_monitors(ovnsb_idl, NULL, NULL, NULL, NULL, true);
if (sb_cond_seqno) {
*sb_cond_seqno = next_cond_seqno;
}
@@ -4712,7 +4724,7 @@ main(int argc, char *argv[])
ovsdb_idl_omit(ovnsb_idl_loop.idl,
&sbrec_chassis_private_col_external_ids);
- update_sb_monitors(ovnsb_idl_loop.idl, NULL, NULL, NULL, false);
+ update_sb_monitors(ovnsb_idl_loop.idl, NULL, NULL, NULL, NULL, false);
stopwatch_create(CONTROLLER_LOOP_STOPWATCH_NAME, SW_MS);
stopwatch_create(OFCTRL_PUT_STOPWATCH_NAME, SW_MS);
@@ -5355,6 +5367,7 @@ main(int argc, char *argv[])
update_sb_monitors(
ovnsb_idl_loop.idl, chassis,
&runtime_data->local_lports,
+ &runtime_data->lbinding_data.bindings,
&runtime_data->local_datapaths,
sb_monitor_all);
}
--
2.31.1
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
