Skip to content

Commit

Permalink
Revert "ovn-controller: Remove monitor all of chassis private."
Browse files Browse the repository at this point in the history 
This reverts commit 354d766.

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 1098d31 ("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 <i.maximets@ovn.org>
Fixes: 354d766 ("ovn-controller: Remove monitor all of chassis private.")
Fixes: 1098d31 ("controller: Update ovn-monitor-all documentation.")
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Acked-by: Ilya Maximets <i.maximets@ovn.org>
  • Loading branch information
@dceara
dceara committed Feb 18, 2025
1 parent a5dfcee commit 08aec1a
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 50 deletions.
19 changes: 8 additions & 11 deletions controller/ovn-controller.8.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
39 changes: 17 additions & 22 deletions controller/ovn-controller.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand All @@ -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;
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -6532,7 +6527,7 @@ main(int argc, char *argv[])
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
Expand Down
37 changes: 20 additions & 17 deletions tests/ovn-controller.at
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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])
Expand Down

0 comments on commit 08aec1a

Please sign in to comment.