[ovs-dev] [PATCH ovn] Clear port binding flows when datapath CT zone changes.

Mark Michelson mmichels at redhat.com
Fri Nov 20 00:13:43 UTC 2020


In commit f9cab11d5fabe2ae321a3b4bad5972b61df958c0, a LOG_TO_PHY flow
was changed so that it was no longer associated with a particular port
binding. The logic there was that the particular flow contains data
pertaining to the port binding's peer's datapath, so it didn't make
sense to associate the flow with the port binding. This change was
necessary in order for flows to be recalculated properly if the
requested SNAT CT zone on a gateway router was changed. Since the
datapath was changed but no port bindings were changed, that particular
flow needed to be cleared so it could be recalculated with the new CT
zones put in place.

Unfortunately, that change broke some other behavior. Specifically, if a
router was changed from a distributed router to a gateway router, then
its port bindings and its port bindings' peers would be updated so that
they were no longer type "patch" but instead type "l3gateway". They
would attempt to remove all associated physical flows and then install
the newly relevant ones. Since the LOG_TO_PHY flow was no longer
associated with a port binding, that flow would remain. The result was
that traffic could be sent to the gateway router on chassis where the
gateway router was not pinned.

This commit seeks to fix both behaviors. Now if CT zones are changed on
a particular datapath, then all port bindings on that datapath, as well
as all of those port bindings' peers will have their physical flows
removed. When physical flows are recomputed, all of the appropriate
flows will be added.

Signed-off-by: Mark Michelson <mmichels at redhat.com>
---
 controller/ovn-controller.c | 31 ++++++++++++++++++--
 controller/physical.c       | 58 +++++++++++++++++++++++++++++--------
 controller/physical.h       |  4 +++
 3 files changed, 79 insertions(+), 14 deletions(-)

diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 25de0c72f..eceb804ac 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -64,6 +64,7 @@
 #include "timer.h"
 #include "stopwatch.h"
 #include "lib/inc-proc-eng.h"
+#include "hmapx.h"
 
 VLOG_DEFINE_THIS_MODULE(main);
 
@@ -549,7 +550,7 @@ add_pending_ct_zone_entry(struct shash *pending_ct_zones,
 static void
 update_ct_zones(const struct sset *lports, const struct hmap *local_datapaths,
                 struct simap *ct_zones, unsigned long *ct_zone_bitmap,
-                struct shash *pending_ct_zones)
+                struct shash *pending_ct_zones, struct hmapx *updated_dps)
 {
     struct simap_node *ct_zone, *ct_zone_next;
     int scan_start = 1;
@@ -563,6 +564,7 @@ update_ct_zones(const struct sset *lports, const struct hmap *local_datapaths,
     }
 
     /* Local patched datapath (gateway routers) need zones assigned. */
+    struct shash all_lds = SHASH_INITIALIZER(&all_lds);
     const struct local_datapath *ld;
     HMAP_FOR_EACH (ld, hmap_node, local_datapaths) {
         /* XXX Add method to limit zone assignment to logical router
@@ -571,6 +573,8 @@ update_ct_zones(const struct sset *lports, const struct hmap *local_datapaths,
         char *snat = alloc_nat_zone_key(&ld->datapath->header_.uuid, "snat");
         sset_add(&all_users, dnat);
         sset_add(&all_users, snat);
+        shash_add(&all_lds, dnat, ld);
+        shash_add(&all_lds, snat, ld);
 
         int req_snat_zone = datapath_snat_ct_zone(ld->datapath);
         if (req_snat_zone >= 0) {
@@ -636,6 +640,11 @@ update_ct_zones(const struct sset *lports, const struct hmap *local_datapaths,
 
         bitmap_set1(ct_zone_bitmap, snat_req_node->data);
         simap_put(ct_zones, snat_req_node->name, snat_req_node->data);
+        struct shash_node *ld_node = shash_find(&all_lds, snat_req_node->name);
+        if (ld_node) {
+            struct local_datapath *dp = ld_node->data;
+            hmapx_add(updated_dps, (void *) dp->datapath);
+        }
     }
 
     /* xxx This is wasteful to assign a zone to each port--even if no
@@ -664,10 +673,17 @@ update_ct_zones(const struct sset *lports, const struct hmap *local_datapaths,
 
         bitmap_set1(ct_zone_bitmap, zone);
         simap_put(ct_zones, user, zone);
+
+        struct shash_node *ld_node = shash_find(&all_lds, user);
+        if (ld_node) {
+            struct local_datapath *dp = ld_node->data;
+            hmapx_add(updated_dps, (void *) dp->datapath);
+        }
     }
 
     simap_destroy(&req_snat_zones);
     sset_destroy(&all_users);
+    shash_destroy(&all_lds);
 }
 
 static void
@@ -1098,6 +1114,9 @@ struct ed_type_runtime_data {
     bool tracked;
     bool local_lports_changed;
     struct hmap tracked_dp_bindings;
+
+    /* CT zone data. Contains datapaths that had updated CT zones */
+    struct hmapx ct_updated_datapaths;
 };
 
 /* struct ed_type_runtime_data has the below members for tracking the
@@ -1189,6 +1208,8 @@ en_runtime_data_init(struct engine_node *node OVS_UNUSED,
     /* Init the tracked data. */
     hmap_init(&data->tracked_dp_bindings);
 
+    hmapx_init(&data->ct_updated_datapaths);
+
     return data;
 }
 
@@ -1348,6 +1369,7 @@ en_runtime_data_run(struct engine_node *node, void *data)
         sset_init(&rt_data->egress_ifaces);
         smap_init(&rt_data->local_iface_ids);
         local_bindings_init(&rt_data->local_bindings);
+        hmapx_clear(&rt_data->ct_updated_datapaths);
     }
 
     struct binding_ctx_in b_ctx_in;
@@ -1486,9 +1508,11 @@ en_ct_zones_run(struct engine_node *node, void *data)
     struct ed_type_runtime_data *rt_data =
         engine_get_input_data("runtime_data", node);
 
+    hmapx_clear(&rt_data->ct_updated_datapaths);
     update_ct_zones(&rt_data->local_lports, &rt_data->local_datapaths,
                     &ct_zones_data->current, ct_zones_data->bitmap,
-                    &ct_zones_data->pending);
+                    &ct_zones_data->pending, &rt_data->ct_updated_datapaths);
+
 
     engine_set_node_state(node, EN_UPDATED);
 }
@@ -1698,6 +1722,7 @@ static void init_physical_ctx(struct engine_node *node,
     p_ctx->ct_zones = ct_zones;
     p_ctx->mff_ovn_geneve = ed_mff_ovn_geneve->mff_ovn_geneve;
     p_ctx->local_bindings = &rt_data->local_bindings;
+    p_ctx->ct_updated_datapaths = &rt_data->ct_updated_datapaths;
 }
 
 static void init_lflow_ctx(struct engine_node *node,
@@ -2125,6 +2150,8 @@ flow_output_physical_flow_changes_handler(struct engine_node *node, void *data)
     if (pfc_data->recompute_physical_flows) {
         /* This indicates that we need to recompute the physical flows. */
         physical_clear_unassoc_flows_with_db(&fo->flow_table);
+        physical_clear_dp_flows(&p_ctx, &rt_data->ct_updated_datapaths,
+                                &fo->flow_table);
         physical_run(&p_ctx, &fo->flow_table);
         return true;
     }
diff --git a/controller/physical.c b/controller/physical.c
index 00c4ca4fd..fa5d0d692 100644
--- a/controller/physical.c
+++ b/controller/physical.c
@@ -44,6 +44,7 @@
 #include "sset.h"
 #include "util.h"
 #include "vswitch-idl.h"
+#include "hmapx.h"
 
 VLOG_DEFINE_THIS_MODULE(physical);
 
@@ -860,6 +861,28 @@ load_logical_ingress_metadata(const struct sbrec_port_binding *binding,
     put_load(port_key, MFF_LOG_INPORT, 0, 32, ofpacts_p);
 }
 
+static const struct sbrec_port_binding *
+get_binding_peer(struct ovsdb_idl_index *sbrec_port_binding_by_name,
+                 const struct sbrec_port_binding *binding)
+{
+    const char *peer_name = smap_get(&binding->options, "peer");
+    if (!peer_name) {
+        return NULL;
+    }
+
+    const struct sbrec_port_binding *peer = lport_lookup_by_name(
+        sbrec_port_binding_by_name, peer_name);
+    if (!peer || strcmp(peer->type, binding->type)) {
+        return NULL;
+    }
+    const char *peer_peer_name = smap_get(&peer->options, "peer");
+    if (!peer_peer_name || strcmp(peer_peer_name, binding->logical_port)) {
+        return NULL;
+    }
+
+    return peer;
+}
+
 static void
 consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name,
                       enum mf_field_id mff_ovn_geneve,
@@ -882,18 +905,10 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name,
     if (!strcmp(binding->type, "patch")
         || (!strcmp(binding->type, "l3gateway")
             && binding->chassis == chassis)) {
-        const char *peer_name = smap_get(&binding->options, "peer");
-        if (!peer_name) {
-            return;
-        }
 
-        const struct sbrec_port_binding *peer = lport_lookup_by_name(
-            sbrec_port_binding_by_name, peer_name);
-        if (!peer || strcmp(peer->type, binding->type)) {
-            return;
-        }
-        const char *peer_peer_name = smap_get(&peer->options, "peer");
-        if (!peer_peer_name || strcmp(peer_peer_name, binding->logical_port)) {
+        const struct sbrec_port_binding *peer = get_binding_peer(
+                sbrec_port_binding_by_name, binding);
+        if (!peer) {
             return;
         }
 
@@ -926,7 +941,7 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name,
 
         ofctrl_add_flow(flow_table, OFTABLE_LOG_TO_PHY, 100,
                         binding->header_.uuid.parts[0],
-                        &match, ofpacts_p, hc_uuid);
+                        &match, ofpacts_p, &binding->header_.uuid);
         return;
     }
 
@@ -1874,3 +1889,22 @@ physical_clear_unassoc_flows_with_db(struct ovn_desired_flow_table *flow_table)
         ofctrl_remove_flows(flow_table, hc_uuid);
     }
 }
+
+void
+physical_clear_dp_flows(struct physical_ctx *p_ctx,
+                        struct hmapx *ct_updated_datapaths,
+                        struct ovn_desired_flow_table *flow_table)
+{
+    const struct sbrec_port_binding *binding;
+    SBREC_PORT_BINDING_TABLE_FOR_EACH (binding, p_ctx->port_binding_table) {
+        if (!hmapx_find(ct_updated_datapaths, binding->datapath)) {
+            continue;
+        }
+        const struct sbrec_port_binding *peer =
+            get_binding_peer(p_ctx->sbrec_port_binding_by_name, binding);
+        ofctrl_remove_flows(flow_table, &binding->header_.uuid);
+        if (peer) {
+            ofctrl_remove_flows(flow_table, &peer->header_.uuid);
+        }
+    }
+}
diff --git a/controller/physical.h b/controller/physical.h
index feab41df4..0a4c3bab8 100644
--- a/controller/physical.h
+++ b/controller/physical.h
@@ -56,12 +56,16 @@ struct physical_ctx {
     const struct simap *ct_zones;
     enum mf_field_id mff_ovn_geneve;
     struct shash *local_bindings;
+    struct hmapx *ct_updated_datapaths;
 };
 
 void physical_register_ovs_idl(struct ovsdb_idl *);
 void physical_run(struct physical_ctx *,
                   struct ovn_desired_flow_table *);
 void physical_clear_unassoc_flows_with_db(struct ovn_desired_flow_table *);
+void physical_clear_dp_flows(struct physical_ctx *p_ctx,
+                        struct hmapx *ct_updated_datapaths,
+                        struct ovn_desired_flow_table *flow_table);
 void physical_handle_port_binding_changes(struct physical_ctx *,
                                           struct ovn_desired_flow_table *);
 void physical_handle_mc_group_changes(struct physical_ctx *,
-- 
2.25.4



More information about the dev mailing list