This reverts commit 354d766ca8924b9ba6ab8f471440bb9ae1b6e341.
As reported by Ilya this change significantly increased load on
ovsdb-servers running the Southbound database in large scale scenarios
(lots of OVN nodes). That's because ovsdb-server currently cannot use
its JSON cache if not all monitor conditions for a given client are set
to 'true'.
Until ovsdb-server behavior is changed/fixed we need to also revert the
ovn-controller change. While at it also partially revert the follow up
documentation change done by 1098d316be57 ("controller: Update
ovn-monitor-all documentation.") - we keep the part that fixes the
reference to the OVN Southbound database. Also add a comment in the code
and a test to explicitly validate the current behavior.
Reported-at:
https://mail.openvswitch.org/pipermail/ovs-dev/2025-February/421142.html
Reported-by: Ilya Maximets <[email protected]>
Fixes: 354d766ca892 ("ovn-controller: Remove monitor all of chassis private.")
Fixes: 1098d316be57 ("controller: Update ovn-monitor-all documentation.")
Signed-off-by: Dumitru Ceara <[email protected]>
---
V2:
- Address Ilya's comment:
- keep the fix for the OVN_Southbound database reference in the
documentation.
- Fix one more instance of <var>OVN Southbound</var> in the same
section.
---
controller/ovn-controller.8.xml | 19 +++++++---------
controller/ovn-controller.c | 39 ++++++++++++++-------------------
tests/ovn-controller.at | 37 +++++++++++++++++--------------
3 files changed, 45 insertions(+), 50 deletions(-)
diff --git a/controller/ovn-controller.8.xml b/controller/ovn-controller.8.xml
index 27ea77fa52..eace2c5cf2 100644
--- a/controller/ovn-controller.8.xml
+++ b/controller/ovn-controller.8.xml
@@ -117,21 +117,18 @@
<dd>
<p>
A boolean value that tells if <code>ovn-controller</code> should
- monitor all records of most tables in the
- <code>OVN Southbound</code> database. The only exception is the
- <code>Chassis_Private</code> table for which ovn-controller
- always monitors only the record that is relevant to the local
- chassis. If this option is set to <code>false</code>,
- ovn-controller will conditionally monitor only the records that
- are needed for the local chassis.
+ monitor all records of tables in the <code>OVN_Southbound</code>.
+ If this option is set to <code>false</code>, ovn-controller will
+ conditionally monitor only the records that are needed for the local
+ chassis.
</p>
<p>
It is more efficient to set it to <code>true</code> in use cases
where the chassis would anyway need to monitor most of the records in
- <var>OVN Southbound</var> database, which would save the overhead of
- conditions processing, especially for server side. Typically, set it
- to <code>true</code> for environments where all workloads need to be
- reachable from each other.
+ the <code>OVN_Southbound</code> database, which would save the
+ overhead of conditions processing, especially for server side.
+ Typically, set it to <code>true</code> for environments where all
+ workloads need to be reachable from each other.
</p>
<p>
NOTE: for efficiency and scalability in common scenarios
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index d8d1f363e5..081411cba8 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -273,17 +273,12 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
* routes until db conditions are updated. */
ovsdb_idl_condition_add_clause_true(&lr);
- if (chassis) {
- /* Monitors Chassis_Private record for current chassis only. */
- sbrec_chassis_private_add_clause_name(&chprv, OVSDB_F_EQ,
- chassis->name);
- } else {
- /* During initialization, we monitor all records in Chassis_Private so
- * that we don't try to recreate existing ones. */
- ovsdb_idl_condition_add_clause_true(&chprv);
- }
-
if (monitor_all) {
+ /* Monitor all Southbound tables unconditionally. Do that even for
+ * tables that could be easily filtered by chassis name (like
+ * Chassis_Private). That's because the current ovsdb-server
+ * implementation uses a cache whose efficiency significantly
+ * decreases when monitor conditions are present. */
ovsdb_idl_condition_add_clause_true(&pb);
ovsdb_idl_condition_add_clause_true(&lf);
ovsdb_idl_condition_add_clause_true(&mb);
@@ -293,6 +288,7 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
ovsdb_idl_condition_add_clause_true(&ce);
ovsdb_idl_condition_add_clause_true(&ip_mcast);
ovsdb_idl_condition_add_clause_true(&igmp);
+ ovsdb_idl_condition_add_clause_true(&chprv);
ovsdb_idl_condition_add_clause_true(&tv);
ovsdb_idl_condition_add_clause_true(&nh);
ovsdb_idl_condition_add_clause_true(&ar);
@@ -331,9 +327,16 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
sbrec_igmp_group_add_clause_chassis(&igmp, OVSDB_F_EQ,
&chassis->header_.uuid);
+ /* Monitors Chassis_Private record for current chassis only. */
+ sbrec_chassis_private_add_clause_name(&chprv, OVSDB_F_EQ,
+ chassis->name);
+
sbrec_chassis_template_var_add_clause_chassis(&tv, OVSDB_F_EQ,
chassis->name);
} else {
+ /* During initialization, we monitor all records in Chassis_Private so
+ * that we don't try to recreate existing ones. */
+ ovsdb_idl_condition_add_clause_true(&chprv);
/* Also, to avoid traffic disruption (e.g., conntrack flushing for
* zones that are used by OVN but not yet known due to the SB initial
* contents not being available), monitor all port bindings
@@ -712,8 +715,7 @@ static void
update_sb_db(struct ovsdb_idl *ovs_idl, struct ovsdb_idl *ovnsb_idl,
bool *monitor_all_p, bool *reset_ovnsb_idl_min_index,
struct controller_engine_ctx *ctx,
- unsigned int *sb_cond_seqno,
- struct ovsdb_idl_index *sbrec_chassis_by_name)
+ unsigned int *sb_cond_seqno)
{
const struct ovsrec_open_vswitch *cfg = ovsrec_open_vswitch_first(ovs_idl);
if (!cfg) {
@@ -739,18 +741,12 @@ update_sb_db(struct ovsdb_idl *ovs_idl, struct ovsdb_idl
*ovnsb_idl,
get_chassis_external_id_value_bool(
&cfg->external_ids, chassis_id, "ovn-monitor-all", false);
if (monitor_all) {
- const struct sbrec_chassis *chassis = NULL;
- if (chassis_id && sbrec_chassis_by_name) {
- chassis =
- chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id);
- }
-
/* Always call update_sb_monitors when monitor_all is true.
* Otherwise, don't call it here, because there would be unnecessary
* 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, chassis, NULL, NULL, NULL, true);
+ update_sb_monitors(ovnsb_idl, NULL, NULL, NULL, NULL, true);
if (sb_cond_seqno) {
*sb_cond_seqno = next_cond_seqno;
}
@@ -6044,8 +6040,7 @@ main(int argc, char *argv[])
update_sb_db(ovs_idl_loop.idl, ovnsb_idl_loop.idl, &sb_monitor_all,
&reset_ovnsb_idl_min_index,
- &ctrl_engine_ctx, &ovnsb_expected_cond_seqno,
- sbrec_chassis_by_name);
+ &ctrl_engine_ctx, &ovnsb_expected_cond_seqno);
update_ssl_config(ovsrec_ssl_table_get(ovs_idl_loop.idl));
struct ovsdb_idl_txn *ovnsb_idl_txn
@@ -6532,7 +6527,7 @@ loop_done:
bool done = !ovsdb_idl_has_ever_connected(ovnsb_idl_loop.idl);
while (!done) {
update_sb_db(ovs_idl_loop.idl, ovnsb_idl_loop.idl,
- NULL, NULL, NULL, NULL, sbrec_chassis_by_name);
+ NULL, NULL, NULL, NULL);
update_ssl_config(ovsrec_ssl_table_get(ovs_idl_loop.idl));
struct ovsdb_idl_txn *ovs_idl_txn
diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
index 47f48d8b4c..6561e177f6 100644
--- a/tests/ovn-controller.at
+++ b/tests/ovn-controller.at
@@ -484,10 +484,11 @@ OVN_CLEANUP([hv])
AT_CLEANUP
])
-# Checks that ovn-controller increments the nb_cfg value in the
Chassis_Private table
-# and each hv doesn't exchange messages of update of Chassis_Private table
-OVN_FOR_EACH_NORTHD([
-AT_SETUP([ovn-controller - Bump Chassis_Private nb_cfg value with Monitor All])
+# Checks that when ovn-controller increments the nb_cfg value in the
+# Chassis_Private table each hv receives Chassis_Private update messages.
+# NOTE: Only run this test once, it's relevant only for the case when
+# monitor_all is set to 'true'.
+AT_SETUP([ovn-controller - Chassis_Private processing with conditional
monitoring disabled])
AT_KEYWORDS([ovn])
ovn_start
@@ -502,34 +503,36 @@ as hv2
check ovs-vsctl add-br br-phys
ovn_attach n1 br-phys 192.168.0.12
-as hv1 ovs-vsctl set open . external_ids:ovn-monitor-all=true
-as hv2 ovs-vsctl set open . external_ids:ovn-monitor-all=true
+as hv1
+check ovs-vsctl set open . external_ids:ovn-monitor-all=true
+as hv2
+check ovs-vsctl set open . external_ids:ovn-monitor-all=true
# Wait until ovn-monitor-all is processed by ovn-controller.
wait_row_count Chassis 1 name=hv1 other_config:ovn-monitor-all=true
wait_row_count Chassis 1 name=hv2 other_config:ovn-monitor-all=true
# Enable debug for check the received messages.
-as hv1 ovn-appctl -t ovn-controller vlog/set dbg
-as hv2 ovn-appctl -t ovn-controller vlog/set dbg
+as hv1
+check ovn-appctl -t ovn-controller vlog/set dbg
+as hv2
+check ovn-appctl -t ovn-controller vlog/set dbg
# Bump the NB_Global nb_cfg value.
-check ovn-nbctl set NB_Global . nb_cfg=999
+check ovn-nbctl --wait=hv sync
-# ovn-controller should bump the nb_cfg in the chassis_private table.
-wait_row_count Chassis_Private 1 name=hv1 nb_cfg=999
-wait_row_count Chassis_Private 1 name=hv2 nb_cfg=999
+nb_cfg=$(fetch_column nb:NB_Global options:nb_cfg)
-AT_CHECK([grep -c 'Chassis_Private.*nb_cfg":999' hv1/ovn-controller.log], [0],
[dnl
-1
+# Both chassis should receive all Chassis_Private notifications.
+AT_CHECK([grep -c "Chassis_Private.*nb_cfg\":${nb_cfg}"
hv1/ovn-controller.log], [0], [dnl
+2
])
-AT_CHECK([grep -c 'Chassis_Private.*nb_cfg":999' hv2/ovn-controller.log], [0],
[dnl
-1
+AT_CHECK([grep -c "Chassis_Private.*nb_cfg\":${nb_cfg}"
hv2/ovn-controller.log], [0], [dnl
+2
])
OVN_CLEANUP([hv1],[hv2])
AT_CLEANUP
-])
# check that nb_cfg overflow cases handled properly
AT_SETUP([ovn-controller - overflow the nb_cfg value across the tables])
--
2.48.1
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
