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.
[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]>
---
V2:
- Addressed Han's comments:
- changed commit log: removed assumption about sub-ports.
- added ovn-nb documentation note about sub-ports monitoring.
- added ovn-controller documentation note about sub-ports monitoring.
- added TODO item.
- Added NEWS item.
---
NEWS | 3 +++
TODO.rst | 6 ++++++
controller/binding.c | 2 +-
controller/binding.h | 2 +-
controller/ovn-controller.8.xml | 6 ++++++
controller/ovn-controller.c | 19 ++++++++++++++++---
ovn-nb.xml | 6 +++++-
7 files changed, 38 insertions(+), 6 deletions(-)
diff --git a/NEWS b/NEWS
index 482970eeeb..8275877f99 100644
--- a/NEWS
+++ b/NEWS
@@ -7,6 +7,9 @@ Post v23.06.0
DHCPv4 "hostname" (12) option.
- Support to create/update MAC_Binding when GARP received from VTEP (RAMP)
switch on l3dgw port.
+ - To allow optimizing ovn-controller's monitor conditions for the regular
+ VIF case, ovn-controller now unconditionally monitors all sub-ports
+ (ports with parent_port set).
OVN v23.06.0 - 01 Jun 2023
--------------------------
diff --git a/TODO.rst b/TODO.rst
index dff163e70b..b790a9fadf 100644
--- a/TODO.rst
+++ b/TODO.rst
@@ -124,3 +124,9 @@ OVN To-do List
* Load Balancer templates
* Support combining the VIP IP and port into a single template variable.
+
+* ovn-controller conditional monitoring
+
+ * Improve sub-ports (with parent_port set) conditional monitoring; these
+ are currently unconditionally monitored, even if ovn-monitor-all is
+ set to false.
diff --git a/controller/binding.c b/controller/binding.c
index 486095ca79..359ad6d660 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -785,7 +785,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.8.xml b/controller/ovn-controller.8.xml
index f61f430084..0883d8da9b 100644
--- a/controller/ovn-controller.8.xml
+++ b/controller/ovn-controller.8.xml
@@ -128,6 +128,12 @@
to <code>true</code> for environments that all workloads need to be
reachable from each other.
</p>
+ <p>
+ NOTE: for efficiency and scalability in common scenarios
+ <code>ovn-controller</code> unconditionally monitors all sub-ports
+ (ports with <code>parent_port</code> set) regardless of the
+ <code>ovn-monitor-all</code> value.
+ </p>
<p>
Default value is <var>false</var>.
</p>
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index a474069798..96d01219e1 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);
}
diff --git a/ovn-nb.xml b/ovn-nb.xml
index cf9c92009d..1fb9fa10ee 100644
--- a/ovn-nb.xml
+++ b/ovn-nb.xml
@@ -1302,7 +1302,11 @@
<column name="parent_name">
The VM interface through which the nested container sends its network
traffic. This must match the <ref column="name"/> column for some
- other <ref table="Logical_Switch_Port"/>.
+ other <ref table="Logical_Switch_Port"/>. Note: for performance
+ reasons, unlike for regular VIFs, <code>ovn-controller</code> will
+ register to get updates about all OVN Southbound database
+ <ref db="OVN_Southbound" table="Port_Binding"/> table records
+ that correspond to nested container ports.
</column>
<column name="tag_request">
--
2.31.1
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
