[ovs-dev] [PATCH ovn v2 5/5] ovn-controller: Fix incremental processing for logical port references.

Han Zhou hzhou at ovn.org
Fri Jun 11 19:35:30 UTC 2021


If a lflow has an lport name in the match, but when the lflow is
processed the port-binding is not seen by ovn-controller, the
corresponding openflow will not be created. Later if the port-binding is
created/monitored by ovn-controller, the lflow is not reprocessed
because the lflow didn't change and ovn-controller doesn't know that the
port-binding affects the lflow. This patch fixes the problem by tracking
the references when parsing the lflow, even if the port-binding is not
found when the lflow is firstly parsed. A test case is also added to
cover the scenario.

Signed-off-by: Han Zhou <hzhou at ovn.org>
---
 controller/lflow.c          | 64 +++++++++++++++++++++++++++----------
 controller/lflow.h          |  3 ++
 controller/ovn-controller.c | 17 ++++++++--
 tests/ovn.at                | 47 +++++++++++++++++++++++++++
 4 files changed, 113 insertions(+), 18 deletions(-)

diff --git a/controller/lflow.c b/controller/lflow.c
index 34eca135a..7ae0ed15e 100644
--- a/controller/lflow.c
+++ b/controller/lflow.c
@@ -61,6 +61,7 @@ struct lookup_port_aux {
 
 struct condition_aux {
     struct ovsdb_idl_index *sbrec_port_binding_by_name;
+    const struct sbrec_datapath_binding *dp;
     const struct sbrec_chassis *chassis;
     const struct sset *active_tunnels;
     const struct sbrec_logical_flow *lflow;
@@ -98,6 +99,12 @@ lookup_port_cb(const void *aux_, const char *port_name, unsigned int *portp)
 
     const struct lookup_port_aux *aux = aux_;
 
+    /* Store the name that used to lookup the lport to lflow reference, so that
+     * in the future when the lport's port binding changes, the logical flow
+     * that references this lport can be reprocessed. */
+    lflow_resource_add(aux->lfrr, REF_TYPE_PORTBINDING, port_name,
+                       &aux->lflow->header_.uuid);
+
     const struct sbrec_port_binding *pb
         = lport_lookup_by_name(aux->sbrec_port_binding_by_name, port_name);
     if (pb && pb->datapath == aux->dp) {
@@ -149,19 +156,18 @@ is_chassis_resident_cb(const void *c_aux_, const char *port_name)
 {
     const struct condition_aux *c_aux = c_aux_;
 
+    /* Store the port name that used to lookup the lport to lflow reference, so
+     * that in the future when the lport's port-binding changes the logical
+     * flow that references this lport can be reprocessed. */
+    lflow_resource_add(c_aux->lfrr, REF_TYPE_PORTBINDING, port_name,
+                       &c_aux->lflow->header_.uuid);
+
     const struct sbrec_port_binding *pb
         = lport_lookup_by_name(c_aux->sbrec_port_binding_by_name, port_name);
     if (!pb) {
         return false;
     }
 
-    /* Store the port_name to lflow reference. */
-    int64_t dp_id = pb->datapath->tunnel_key;
-    char buf[16];
-    get_unique_lport_key(dp_id, pb->tunnel_key, buf, sizeof(buf));
-    lflow_resource_add(c_aux->lfrr, REF_TYPE_PORTBINDING, buf,
-                       &c_aux->lflow->header_.uuid);
-
     if (strcmp(pb->type, "chassisredirect")) {
         /* for non-chassisredirect ports */
         return pb->chassis && pb->chassis == c_aux->chassis;
@@ -623,8 +629,6 @@ add_matches_to_flow_table(const struct sbrec_logical_flow *lflow,
                 int64_t dp_id = dp->tunnel_key;
                 char buf[16];
                 get_unique_lport_key(dp_id, port_id, buf, sizeof(buf));
-                lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_PORTBINDING, buf,
-                                   &lflow->header_.uuid);
                 if (!sset_contains(l_ctx_in->local_lport_ids, buf)) {
                     VLOG_DBG("lflow "UUID_FMT
                              " port %s in match is not local, skip",
@@ -788,6 +792,7 @@ consider_logical_flow__(const struct sbrec_logical_flow *lflow,
     };
     struct condition_aux cond_aux = {
         .sbrec_port_binding_by_name = l_ctx_in->sbrec_port_binding_by_name,
+        .dp = dp,
         .chassis = l_ctx_in->chassis,
         .active_tunnels = l_ctx_in->active_tunnels,
         .lflow = lflow,
@@ -883,7 +888,9 @@ consider_logical_flow__(const struct sbrec_logical_flow *lflow,
 
         /* Cache new entry if caching is enabled. */
         if (lflow_cache_is_enabled(l_ctx_out->lflow_cache)) {
-            if (cached_expr && !is_cr_cond_present) {
+            if (cached_expr
+                && !lflow_ref_lookup(&l_ctx_out->lfrr->lflow_ref_table,
+                                     &lflow->header_.uuid)) {
                 lflow_cache_add_matches(l_ctx_out->lflow_cache,
                                         &lflow->header_.uuid, matches,
                                         matches_size);
@@ -1746,19 +1753,44 @@ lflow_processing_end:
     return handled;
 }
 
+/* Handles a port-binding change that is possibly related to a lport's
+ * residence status on this chassis. */
 bool
 lflow_handle_flows_for_lport(const struct sbrec_port_binding *pb,
                              struct lflow_ctx_in *l_ctx_in,
                              struct lflow_ctx_out *l_ctx_out)
 {
+    bool ret = true;
     bool changed;
-    int64_t dp_id = pb->datapath->tunnel_key;
-    char pb_ref_name[16];
-    get_unique_lport_key(dp_id, pb->tunnel_key, pb_ref_name,
-                         sizeof(pb_ref_name));
 
-    return lflow_handle_changed_ref(REF_TYPE_PORTBINDING, pb_ref_name,
-                                    l_ctx_in, l_ctx_out, &changed);
+    if (!lflow_handle_changed_ref(REF_TYPE_PORTBINDING, pb->logical_port,
+                                  l_ctx_in, l_ctx_out, &changed)) {
+        ret = false;
+    }
+    return ret;
+}
+
+/* Handles port-binding add/deletions. */
+bool
+lflow_handle_changed_port_bindings(struct lflow_ctx_in *l_ctx_in,
+                                   struct lflow_ctx_out *l_ctx_out)
+{
+    bool ret = true;
+    bool changed;
+    const struct sbrec_port_binding *pb;
+    SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb,
+                                               l_ctx_in->port_binding_table) {
+        if (!sbrec_port_binding_is_new(pb)
+            && !sbrec_port_binding_is_deleted(pb)) {
+            continue;
+        }
+        if (!lflow_handle_changed_ref(REF_TYPE_PORTBINDING, pb->logical_port,
+                                      l_ctx_in, l_ctx_out, &changed)) {
+            ret = false;
+            break;
+        }
+    }
+    return ret;
 }
 
 bool
diff --git a/controller/lflow.h b/controller/lflow.h
index e98edf81d..9d8882ae5 100644
--- a/controller/lflow.h
+++ b/controller/lflow.h
@@ -130,6 +130,7 @@ struct lflow_ctx_in {
     struct ovsdb_idl_index *sbrec_logical_flow_by_logical_datapath;
     struct ovsdb_idl_index *sbrec_logical_flow_by_logical_dp_group;
     struct ovsdb_idl_index *sbrec_port_binding_by_name;
+    const struct sbrec_port_binding_table *port_binding_table;
     const struct sbrec_dhcp_options_table *dhcp_options_table;
     const struct sbrec_dhcpv6_options_table *dhcpv6_options_table;
     const struct sbrec_datapath_binding_table *dp_binding_table;
@@ -182,4 +183,6 @@ bool lflow_handle_flows_for_lport(const struct sbrec_port_binding *,
                                   struct lflow_ctx_out *);
 bool lflow_handle_changed_mc_groups(struct lflow_ctx_in *,
                                     struct lflow_ctx_out *);
+bool lflow_handle_changed_port_bindings(struct lflow_ctx_in *,
+                                        struct lflow_ctx_out *);
 #endif /* controller/lflow.h */
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index c358675ac..681b33a69 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -2004,6 +2004,10 @@ static void init_lflow_ctx(struct engine_node *node,
                 engine_get_input("SB_multicast_group", node),
                 "name_datapath");
 
+    struct sbrec_port_binding_table *port_binding_table =
+        (struct sbrec_port_binding_table *)EN_OVSDB_GET(
+            engine_get_input("SB_port_binding", node));
+
     struct sbrec_dhcp_options_table *dhcp_table =
         (struct sbrec_dhcp_options_table *)EN_OVSDB_GET(
             engine_get_input("SB_dhcp_options", node));
@@ -2067,6 +2071,7 @@ static void init_lflow_ctx(struct engine_node *node,
     l_ctx_in->sbrec_logical_flow_by_logical_dp_group =
         sbrec_logical_flow_by_dp_group;
     l_ctx_in->sbrec_port_binding_by_name = sbrec_port_binding_by_name;
+    l_ctx_in->port_binding_table = port_binding_table;
     l_ctx_in->dhcp_options_table  = dhcp_table;
     l_ctx_in->dhcpv6_options_table = dhcpv6_table;
     l_ctx_in->mac_binding_table = mac_binding_table;
@@ -2257,6 +2262,13 @@ flow_output_sb_port_binding_handler(struct engine_node *node,
     struct ed_type_flow_output *fo = data;
     struct ovn_desired_flow_table *flow_table = &fo->flow_table;
 
+    struct lflow_ctx_in l_ctx_in;
+    struct lflow_ctx_out l_ctx_out;
+    init_lflow_ctx(node, rt_data, fo, &l_ctx_in, &l_ctx_out);
+    if (!lflow_handle_changed_port_bindings(&l_ctx_in, &l_ctx_out)) {
+        return false;
+    }
+
     struct physical_ctx p_ctx;
     init_physical_ctx(node, rt_data, &p_ctx);
 
@@ -2360,8 +2372,9 @@ _flow_output_resource_ref_handler(struct engine_node *node, void *data,
             break;
 
         case REF_TYPE_PORTBINDING:
-            /* This ref type is handled in the
-             * flow_output_runtime_data_handler. */
+            /* This ref type is handled in:
+             * - flow_output_runtime_data_handler
+             * - flow_output_sb_port_binding_handler. */
         case REF_TYPE_MC_GROUP:
             /* This ref type is handled in the
              * flow_output_sb_multicast_group_handler. */
diff --git a/tests/ovn.at b/tests/ovn.at
index 11a85c457..563c68b11 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -26775,3 +26775,50 @@ AT_CHECK([ovs-ofctl dump-flows br-int "nw_src=10.0.0.0/24" | \
 OVN_CLEANUP([hv1])
 AT_CLEANUP
 ])
+
+# Test when a logical port name is used in an ACL before it is created. When it
+# is created later, the ACL should be programmed as openflow rules by
+# ovn-controller. Although this is not likely to happen in real world use
+# cases, it is possible that a port-binding is observed by ovn-controller AFTER
+# an lflow that references the port is processed. So this test case is to make
+# sure the incremental processing in ovn-controller reprocesses the lflow when
+# the port-binding is observed.
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([ovn -- ACL referencing lport before creation])
+ovn_start
+
+net_add n1
+
+sim_add hv1
+as hv1
+check ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.1
+
+# Bind both lp1 and lp2 on the chasis.
+check ovs-vsctl add-port br-int lp1 -- set interface lp1 external_ids:iface-id=lp1
+check ovs-vsctl add-port br-int lp2 -- set interface lp2 external_ids:iface-id=lp2
+
+# Create only lport lp1, but not lp2.
+check ovn-nbctl ls-add lsw0
+check ovn-nbctl lsp-add lsw0 lp1 \
+    -- lsp-set-addresses lp1 "f0:00:00:00:00:01 10.0.0.11"
+
+# Each lport is referenced by an ACL.
+check ovn-nbctl acl-add lsw0 to-lport 1002 'outport == "lp1" && ip4.src == 10.0.0.111'  allow-related
+check ovn-nbctl acl-add lsw0 to-lport 1002 'outport == "lp2" && ip4.src == 10.0.0.222'  allow-related
+
+# The first ACL should be programmed.
+check ovn-nbctl --wait=hv sync
+AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10.0.0.111], [0], [ignore])
+
+# Now create the lport lp2.
+check ovn-nbctl lsp-add lsw0 lp2 \
+    -- lsp-set-addresses lp2 "f0:00:00:00:00:02 10.0.0.22"
+
+check ovn-nbctl --wait=hv sync
+# Now the second ACL should be programmed.
+AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10.0.0.222], [0], [ignore])
+
+OVN_CLEANUP([hv1])
+AT_CLEANUP
+])
-- 
2.30.2



More information about the dev mailing list