[ovs-dev] [PATCH ovn v9 3/5] ovn-controller: Handle datapath changes incrementally for ct zone I-P engine node.

numans at ovn.org numans at ovn.org
Thu Jun 3 12:29:08 UTC 2021


From: Numan Siddique <numans at ovn.org>

After the commit [1], we do a full recompute of ct zone I-P engine for
any datapath binding change.  This results in physical_flow() getting
called.  In a highly scaled environment this can result in costly CPU
cycles.  This patch addressed this by handling the datapath binding
changes incrementally in the ct_zone engine node.  ct zone recomputation
is required only when a datapath binding is deleted or a logical router
datapath binding changes the snat-ct-zone option value.  This patch
handles this.

[1] - f9cab11d5fab("Allow explicit setting of the SNAT zone on a gateway router.")

Signed-off-by: Numan Siddique <numans at ovn.org>
---
 controller/ovn-controller.c | 55 ++++++++++++++++++++++++++++++++++++-
 controller/physical.c       |  5 ++++
 tests/ovn-performance.at    | 26 ++++++++++++++++++
 3 files changed, 85 insertions(+), 1 deletion(-)

diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index b49786441..5b55a45b7 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -1734,6 +1734,58 @@ en_ct_zones_run(struct engine_node *node, void *data)
     engine_set_node_state(node, EN_UPDATED);
 }
 
+/* Handles datapath binding changes for the ct_zones engine.
+ * Returns false if the datapath is deleted or if the requested snat
+ * ct zone doesn't match with the ct_zones data. */
+static bool
+ct_zones_datapath_binding_handler(struct engine_node *node, void *data)
+{
+    struct ed_type_ct_zones *ct_zones_data = data;
+    const struct sbrec_datapath_binding *dp;
+    struct ed_type_runtime_data *rt_data =
+        engine_get_input_data("runtime_data", node);
+    struct sbrec_datapath_binding_table *dp_table =
+        (struct sbrec_datapath_binding_table *)EN_OVSDB_GET(
+            engine_get_input("SB_datapath_binding", node));
+
+    SBREC_DATAPATH_BINDING_TABLE_FOR_EACH_TRACKED (dp, dp_table) {
+        if (!get_local_datapath(&rt_data->local_datapaths,
+                                dp->tunnel_key)) {
+            continue;
+        }
+
+        if (sbrec_datapath_binding_is_deleted(dp)) {
+            /* Fall back to full recompute of ct_zones engine. */
+            return false;
+        }
+
+        int req_snat_zone = datapath_snat_ct_zone(dp);
+        if (req_snat_zone == -1) {
+            /* datapath snat ct zone is not set.  This condition will also hit
+             * when CMS clears the snat-ct-zone for the logical router.
+             * In this case there is no harm in using the previosly specified
+             * snat ct zone for this datapath.  Also it is hard to know
+             * if this option was cleared or if this option is never set. */
+            continue;
+        }
+
+        /* Check if the requested snat zone has changed for the datapath
+         * or not.  If so, then fall back to full recompute of
+         * ct_zone engine. */
+        char *snat_dp_zone_key = alloc_nat_zone_key(&dp->header_.uuid, "snat");
+        struct simap_node *simap_node = simap_find(&ct_zones_data->current,
+                                                   snat_dp_zone_key);
+        free(snat_dp_zone_key);
+        if (!simap_node || simap_node->data != req_snat_zone) {
+            /* There is no entry yet or the requested snat zone has changed.
+             * Trigger full recompute of ct_zones engine. */
+            return false;
+        }
+    }
+
+    return true;
+}
+
 /* The data in the ct_zones node is always valid (i.e., no stale pointers). */
 static bool
 en_ct_zones_is_valid(struct engine_node *node OVS_UNUSED)
@@ -2758,7 +2810,8 @@ main(int argc, char *argv[])
 
     engine_add_input(&en_ct_zones, &en_ovs_open_vswitch, NULL);
     engine_add_input(&en_ct_zones, &en_ovs_bridge, NULL);
-    engine_add_input(&en_ct_zones, &en_sb_datapath_binding, NULL);
+    engine_add_input(&en_ct_zones, &en_sb_datapath_binding,
+                     ct_zones_datapath_binding_handler);
     engine_add_input(&en_ct_zones, &en_runtime_data, NULL);
 
     engine_add_input(&en_runtime_data, &en_ofctrl_is_connected, NULL);
diff --git a/controller/physical.c b/controller/physical.c
index 04259d44a..e70efc71d 100644
--- a/controller/physical.c
+++ b/controller/physical.c
@@ -15,6 +15,7 @@
 
 #include <config.h>
 #include "binding.h"
+#include "coverage.h"
 #include "byte-order.h"
 #include "encaps.h"
 #include "flow.h"
@@ -48,6 +49,8 @@
 
 VLOG_DEFINE_THIS_MODULE(physical);
 
+COVERAGE_DEFINE(physical_run);
+
 /* Datapath zone IDs for connection tracking and NAT */
 struct zone_ids {
     int ct;                     /* MFF_LOG_CT_ZONE. */
@@ -1528,6 +1531,8 @@ void
 physical_run(struct physical_ctx *p_ctx,
              struct ovn_desired_flow_table *flow_table)
 {
+    COVERAGE_INC(physical_run);
+
     if (!hc_uuid) {
         hc_uuid = xmalloc(sizeof(struct uuid));
         uuid_generate(hc_uuid);
diff --git a/tests/ovn-performance.at b/tests/ovn-performance.at
index e510c6cef..61356e16d 100644
--- a/tests/ovn-performance.at
+++ b/tests/ovn-performance.at
@@ -462,6 +462,32 @@ OVN_CONTROLLER_EXPECT_HIT_COND(
     [ovn-nbctl --wait=hv lrp-set-gateway-chassis lr1-public hv3 30 && ovn-nbctl --wait=hv sync]
 )
 
+# Test physical_run for logical router and other datapath binding changes.
+OVN_CONTROLLER_EXPECT_HIT_COND(
+    [hv1 hv2 hv3 hv4 hv5], [physical_run], [>0 >0 >0 =0 =0],
+    [ovn-nbctl --wait=hv set logical_router lr1 options:snat-ct-zone=10 && ovn-nbctl --wait=hv sync]
+)
+
+OVN_CONTROLLER_EXPECT_HIT_COND(
+    [hv1 hv2 hv3 hv4 hv5], [physical_run], [>0 >0 >0 =0 =0],
+    [ovn-nbctl --wait=hv set logical_router lr1 options:snat-ct-zone=11 && ovn-nbctl --wait=hv sync]
+)
+
+OVN_CONTROLLER_EXPECT_NO_HIT(
+    [hv1 hv2 hv3 hv4 hv5], [physical_run],
+    [ovn-nbctl --wait=hv remove logical_router lr1 options snat-ct-zone && ovn-nbctl --wait=hv sync]
+)
+
+OVN_CONTROLLER_EXPECT_NO_HIT(
+    [hv1 hv2 hv3 hv4 hv5], [physical_run],
+    [ovn-sbctl set datapath_binding lr1 external_ids:foo=bar && ovn-nbctl --wait=hv sync]
+)
+
+OVN_CONTROLLER_EXPECT_NO_HIT(
+    [hv1 hv2 hv3 hv4 hv5], [physical_run],
+    [ovn-sbctl set datapath_binding ls1 external_ids:foo=bar && ovn-nbctl --wait=hv sync]
+)
+
 # After this, BFD should be enabled from hv1 and hv2 to hv3.
 # So there should be lflow_run hits in hv1, hv2, hv3 and hv4
 OVN_CONTROLLER_EXPECT_HIT_COND(
-- 
2.31.1



More information about the dev mailing list