[ovs-dev] [PATCH v6 06/17] ovn-controller: port-binding incremental processing for physical flows

Han Zhou zhouhan at gmail.com
Fri May 17 19:56:31 UTC 2019


From: Han Zhou <hzhou8 at ebay.com>

This patch implements change handler for port-binding in flow_output
for physical flows computing, so that physical flow computing will
be incremental.

Signed-off-by: Han Zhou <hzhou8 at ebay.com>
---
 ovn/controller/ovn-controller.c | 107 +++++++++++++++++++++++++++++++++++++++-
 ovn/controller/physical.c       | 104 ++++++++++++++++++++++++--------------
 ovn/controller/physical.h       |   9 ++++
 3 files changed, 182 insertions(+), 38 deletions(-)

diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index 57bd5ed..66c3ed0 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -1134,6 +1134,110 @@ flow_output_sb_logical_flow_handler(struct engine_node *node)
     return handled;
 }
 
+static bool
+flow_output_sb_port_binding_handler(struct engine_node *node)
+{
+    struct ed_type_runtime_data *data =
+        (struct ed_type_runtime_data *)engine_get_input(
+                "runtime_data", node)->data;
+    struct hmap *local_datapaths = &data->local_datapaths;
+    struct sset *active_tunnels = &data->active_tunnels;
+    struct simap *ct_zones = &data->ct_zones;
+
+    struct ed_type_mff_ovn_geneve *ed_mff_ovn_geneve =
+        (struct ed_type_mff_ovn_geneve *)engine_get_input(
+            "mff_ovn_geneve", node)->data;
+    enum mf_field_id mff_ovn_geneve = ed_mff_ovn_geneve->mff_ovn_geneve;
+
+    struct ovsrec_open_vswitch_table *ovs_table =
+        (struct ovsrec_open_vswitch_table *)EN_OVSDB_GET(
+            engine_get_input("OVS_open_vswitch", node));
+    struct ovsrec_bridge_table *bridge_table =
+        (struct ovsrec_bridge_table *)EN_OVSDB_GET(
+            engine_get_input("OVS_bridge", node));
+    const struct ovsrec_bridge *br_int = get_br_int(bridge_table, ovs_table);
+    const char *chassis_id = get_chassis_id(ovs_table);
+
+    struct ovsdb_idl_index *sbrec_chassis_by_name =
+        engine_ovsdb_node_get_index(
+                engine_get_input("SB_chassis", node),
+                "name");
+    const struct sbrec_chassis *chassis = NULL;
+    if (chassis_id) {
+        chassis = chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id);
+    }
+    ovs_assert(br_int && chassis);
+
+    struct ed_type_flow_output *fo =
+        (struct ed_type_flow_output *)node->data;
+    struct ovn_desired_flow_table *flow_table = &fo->flow_table;
+
+    struct ovsdb_idl_index *sbrec_port_binding_by_name =
+        engine_ovsdb_node_get_index(
+                engine_get_input("SB_port_binding", node),
+                "name");
+
+    struct sbrec_port_binding_table *port_binding_table =
+        (struct sbrec_port_binding_table *)EN_OVSDB_GET(
+            engine_get_input("SB_port_binding", node));
+
+    /* XXX: now we handle port-binding changes for physical flow processing
+     * only, but port-binding change can have impact to logical flow
+     * processing, too, in below circumstances:
+     *
+     *  - When a port-binding for a lport is inserted/deleted but the lflow
+     *    using that lport doesn't change.
+     *
+     *    This can happen only when the lport name is used by ACL match
+     *    condition, which is specified by user. Even in that case, if the port
+     *    is actually bound on the current chassis it will trigger recompute on
+     *    that chassis since ovs interface would be updated. So the only
+     *    situation this would have real impact is when user defines an ACL
+     *    that includes lport that is not on current chassis, and there is a
+     *    port-binding creation/deletion related to that lport.e.g.: an ACL is
+     *    defined:
+     *
+     *    to-lport 1000 'outport=="A" && inport=="B"' allow-related
+     *
+     *    If "A" is on current chassis, but "B" is lport that hasn't been
+     *    created yet. When a lport "B" is created and bound on another
+     *    chassis, the ACL will not take effect on the current chassis until a
+     *    recompute is triggered later. This case doesn't seem to be a problem
+     *    for real world use cases because usually lport is created before
+     *    being referenced by name in ACLs.
+     *
+     *  - When is_chassis_resident(<lport>) is used in lflow. In this case the
+     *    port binding is not a regular VIF. It can be either "patch" or
+     *    "external", with ha-chassis-group assigned.  In current
+     *    "runtime_data" handling, port-binding changes for these types always
+     *    trigger recomputing. So it is fine even if we do not handle it here.
+     *    (due to the ovsdb tracking support for referenced table changes,
+     *    ha-chassis-group changes will appear as port-binding change).
+     *
+     *  - When a mac-binding doesn't change but the port-binding related to
+     *    that mac-binding is deleted. In this case the neighbor flow generated
+     *    for the mac-binding should be deleted. This would not cause any real
+     *    issue for now, since the port-binding related to mac-binding is
+     *    always logical router port, and any change to logical router port
+     *    would just trigger recompute.
+     *
+     * Although there is no correctness issue so far (except the unusual ACL
+     * use case, which doesn't seem to be a real problem), it might be better
+     * to handle this more gracefully, without the need to consider these
+     * tricky scenarios.  One approach is to maintain a mapping between lport
+     * names and the lflows that uses them, and reprocess the related lflows
+     * when related port-bindings change.
+     */
+    physical_handle_port_binding_changes(
+            sbrec_port_binding_by_name,
+            port_binding_table, mff_ovn_geneve,
+            chassis, ct_zones, local_datapaths,
+            active_tunnels, flow_table);
+
+    node->changed = true;
+    return true;
+}
+
 struct ovn_controller_exit_args {
     bool *exiting;
     bool *restart;
@@ -1253,7 +1357,8 @@ main(int argc, char *argv[])
     engine_add_input(&en_flow_output, &en_sb_chassis, NULL);
     engine_add_input(&en_flow_output, &en_sb_encap, NULL);
     engine_add_input(&en_flow_output, &en_sb_multicast_group, NULL);
-    engine_add_input(&en_flow_output, &en_sb_port_binding, NULL);
+    engine_add_input(&en_flow_output, &en_sb_port_binding,
+                     flow_output_sb_port_binding_handler);
     engine_add_input(&en_flow_output, &en_sb_mac_binding, NULL);
     engine_add_input(&en_flow_output, &en_sb_logical_flow,
                      flow_output_sb_logical_flow_handler);
diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
index 24aea52..d75218c 100644
--- a/ovn/controller/physical.c
+++ b/ovn/controller/physical.c
@@ -445,7 +445,7 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name,
         ofpact_finish_CLONE(ofpacts_p, &clone);
 
         ofctrl_add_flow(flow_table, OFTABLE_LOG_TO_PHY, 100, 0,
-                        &match, ofpacts_p, hc_uuid);
+                        &match, ofpacts_p, &binding->header_.uuid);
         return;
     }
 
@@ -514,7 +514,7 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name,
         }
 
         ofctrl_add_flow(flow_table, OFTABLE_LOCAL_OUTPUT, 100, 0,
-                        &match, ofpacts_p, hc_uuid);
+                        &match, ofpacts_p, &binding->header_.uuid);
 
         goto out;
     }
@@ -666,7 +666,8 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name,
         /* Resubmit to first logical ingress pipeline table. */
         put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, ofpacts_p);
         ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG,
-                        tag ? 150 : 100, 0, &match, ofpacts_p, hc_uuid);
+                        tag ? 150 : 100, 0, &match, ofpacts_p,
+                        &binding->header_.uuid);
 
         if (!tag && (!strcmp(binding->type, "localnet")
                      || !strcmp(binding->type, "l2gateway"))) {
@@ -676,7 +677,8 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name,
              * action. */
             ofpbuf_pull(ofpacts_p, ofpacts_orig_size);
             match_set_dl_tci_masked(&match, 0, htons(VLAN_CFI));
-            ofctrl_add_flow(flow_table, 0, 100, 0, &match, ofpacts_p, hc_uuid);
+            ofctrl_add_flow(flow_table, 0, 100, 0, &match, ofpacts_p,
+                            &binding->header_.uuid);
         }
 
         /* Table 65, Priority 100.
@@ -704,7 +706,7 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name,
             ofpact_put_STRIP_VLAN(ofpacts_p);
         }
         ofctrl_add_flow(flow_table, OFTABLE_LOG_TO_PHY, 100, 0,
-                        &match, ofpacts_p, hc_uuid);
+                        &match, ofpacts_p, &binding->header_.uuid);
     } else if (!tun && !is_ha_remote) {
         /* Remote port connected by localnet port */
         /* Table 33, priority 100.
@@ -727,7 +729,7 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name,
         /* Resubmit to table 33. */
         put_resubmit(OFTABLE_LOCAL_OUTPUT, ofpacts_p);
         ofctrl_add_flow(flow_table, OFTABLE_LOCAL_OUTPUT, 100, 0,
-                        &match, ofpacts_p, hc_uuid);
+                        &match, ofpacts_p, &binding->header_.uuid);
     } else {
         /* Remote port connected by tunnel */
 
@@ -828,7 +830,7 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name,
             ofpact_finish_BUNDLE(ofpacts_p, &bundle);
         }
         ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 100, 0,
-                        &match, ofpacts_p, hc_uuid);
+                        &match, ofpacts_p, &binding->header_.uuid);
     }
 out:
     if (ha_ch_ordered) {
@@ -842,8 +844,6 @@ consider_mc_group(enum mf_field_id mff_ovn_geneve,
                   const struct hmap *local_datapaths,
                   const struct sbrec_chassis *chassis,
                   const struct sbrec_multicast_group *mc,
-                  struct ofpbuf *ofpacts_p,
-                  struct ofpbuf *remote_ofpacts_p,
                   struct ovn_desired_flow_table *flow_table)
 {
     uint32_t dp_key = mc->datapath->tunnel_key;
@@ -863,15 +863,17 @@ consider_mc_group(enum mf_field_id mff_ovn_geneve,
      *    - For remote ports, add the chassis to 'remote_chassis'.
      *
      *    - For local ports (other than logical patch ports), add actions
-     *      to 'ofpacts_p' to set the output port and resubmit.
+     *      to 'ofpacts' to set the output port and resubmit.
      *
-     *    - For logical patch ports, add actions to 'remote_ofpacts_p'
+     *    - For logical patch ports, add actions to 'remote_ofpacts'
      *      instead.  (If we put them in 'ofpacts', then the output
      *      would happen on every hypervisor in the multicast group,
      *      effectively duplicating the packet.)
      */
-    ofpbuf_clear(ofpacts_p);
-    ofpbuf_clear(remote_ofpacts_p);
+    struct ofpbuf ofpacts;
+    ofpbuf_init(&ofpacts, 0);
+    struct ofpbuf remote_ofpacts;
+    ofpbuf_init(&remote_ofpacts, 0);
     for (size_t i = 0; i < mc->n_ports; i++) {
         struct sbrec_port_binding *port = mc->ports[i];
 
@@ -885,20 +887,20 @@ consider_mc_group(enum mf_field_id mff_ovn_geneve,
 
         int zone_id = simap_get(ct_zones, port->logical_port);
         if (zone_id) {
-            put_load(zone_id, MFF_LOG_CT_ZONE, 0, 32, ofpacts_p);
+            put_load(zone_id, MFF_LOG_CT_ZONE, 0, 32, &ofpacts);
         }
 
         if (!strcmp(port->type, "patch")) {
             put_load(port->tunnel_key, MFF_LOG_OUTPORT, 0, 32,
-                     remote_ofpacts_p);
-            put_resubmit(OFTABLE_CHECK_LOOPBACK, remote_ofpacts_p);
+                     &remote_ofpacts);
+            put_resubmit(OFTABLE_CHECK_LOOPBACK, &remote_ofpacts);
         } else if (simap_contains(&localvif_to_ofport,
                            (port->parent_port && *port->parent_port)
                            ? port->parent_port : port->logical_port)
                    || (!strcmp(port->type, "l3gateway")
                        && port->chassis == chassis)) {
-            put_load(port->tunnel_key, MFF_LOG_OUTPORT, 0, 32, ofpacts_p);
-            put_resubmit(OFTABLE_CHECK_LOOPBACK, ofpacts_p);
+            put_load(port->tunnel_key, MFF_LOG_OUTPORT, 0, 32, &ofpacts);
+            put_resubmit(OFTABLE_CHECK_LOOPBACK, &ofpacts);
         } else if (port->chassis && !get_localnet_port(local_datapaths,
                                          mc->datapath->tunnel_key)) {
             /* Add remote chassis only when localnet port not exist,
@@ -913,14 +915,14 @@ consider_mc_group(enum mf_field_id mff_ovn_geneve,
      *
      * Handle output to the local logical ports in the multicast group, if
      * any. */
-    bool local_ports = ofpacts_p->size > 0;
+    bool local_ports = ofpacts.size > 0;
     if (local_ports) {
         /* Following delivery to local logical ports, restore the multicast
          * group as the logical output port. */
-        put_load(mc->tunnel_key, MFF_LOG_OUTPORT, 0, 32, ofpacts_p);
+        put_load(mc->tunnel_key, MFF_LOG_OUTPORT, 0, 32, &ofpacts);
 
         ofctrl_add_flow(flow_table, OFTABLE_LOCAL_OUTPUT, 100, 0,
-                        &match, ofpacts_p, hc_uuid);
+                        &match, &ofpacts, &mc->header_.uuid);
     }
 
     /* Table 32, priority 100.
@@ -928,12 +930,12 @@ consider_mc_group(enum mf_field_id mff_ovn_geneve,
      *
      * Handle output to the remote chassis in the multicast group, if
      * any. */
-    if (!sset_is_empty(&remote_chassis) || remote_ofpacts_p->size > 0) {
-        if (remote_ofpacts_p->size > 0) {
+    if (!sset_is_empty(&remote_chassis) || remote_ofpacts.size > 0) {
+        if (remote_ofpacts.size > 0) {
             /* Following delivery to logical patch ports, restore the
              * multicast group as the logical output port. */
             put_load(mc->tunnel_key, MFF_LOG_OUTPORT, 0, 32,
-                     remote_ofpacts_p);
+                     &remote_ofpacts);
         }
 
         const char *chassis_name;
@@ -947,20 +949,22 @@ consider_mc_group(enum mf_field_id mff_ovn_geneve,
 
             if (!prev || tun->type != prev->type) {
                 put_encapsulation(mff_ovn_geneve, tun, mc->datapath,
-                                  mc->tunnel_key, remote_ofpacts_p);
+                                  mc->tunnel_key, &remote_ofpacts);
                 prev = tun;
             }
-            ofpact_put_OUTPUT(remote_ofpacts_p)->port = tun->ofport;
+            ofpact_put_OUTPUT(&remote_ofpacts)->port = tun->ofport;
         }
 
-        if (remote_ofpacts_p->size) {
+        if (remote_ofpacts.size) {
             if (local_ports) {
-                put_resubmit(OFTABLE_LOCAL_OUTPUT, remote_ofpacts_p);
+                put_resubmit(OFTABLE_LOCAL_OUTPUT, &remote_ofpacts);
             }
             ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 100, 0,
-                            &match, remote_ofpacts_p, hc_uuid);
+                            &match, &remote_ofpacts, &mc->header_.uuid);
         }
     }
+    ofpbuf_uninit(&ofpacts);
+    ofpbuf_uninit(&remote_ofpacts);
     sset_destroy(&remote_chassis);
 }
 
@@ -975,6 +979,36 @@ update_ofports(struct simap *old, struct simap *new)
     return changed;
 }
 
+void physical_handle_port_binding_changes(
+        struct ovsdb_idl_index *sbrec_port_binding_by_name,
+        const struct sbrec_port_binding_table *pb_table,
+        enum mf_field_id mff_ovn_geneve,
+        const struct sbrec_chassis *chassis,
+        const struct simap *ct_zones,
+        struct hmap *local_datapaths,
+        struct sset *active_tunnels,
+        struct ovn_desired_flow_table *flow_table)
+{
+    const struct sbrec_port_binding *binding;
+    struct ofpbuf ofpacts;
+    ofpbuf_init(&ofpacts, 0);
+    SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (binding, pb_table) {
+        if (sbrec_port_binding_is_deleted(binding)) {
+            ofctrl_remove_flows(flow_table, &binding->header_.uuid);
+        } else {
+            if (!sbrec_port_binding_is_new(binding)) {
+                ofctrl_remove_flows(flow_table, &binding->header_.uuid);
+            }
+            consider_port_binding(sbrec_port_binding_by_name,
+                                  mff_ovn_geneve, ct_zones,
+                                  active_tunnels, local_datapaths,
+                                  binding, chassis,
+                                  flow_table, &ofpacts);
+        }
+    }
+
+}
+
 void
 physical_run(struct ovsdb_idl_index *sbrec_port_binding_by_name,
              const struct sbrec_multicast_group_table *multicast_group_table,
@@ -1134,22 +1168,18 @@ physical_run(struct ovsdb_idl_index *sbrec_port_binding_by_name,
     SBREC_PORT_BINDING_TABLE_FOR_EACH (binding, port_binding_table) {
         consider_port_binding(sbrec_port_binding_by_name,
                               mff_ovn_geneve, ct_zones,
-                              active_tunnels,
-                              local_datapaths, binding, chassis,
+                              active_tunnels, local_datapaths,
+                              binding, chassis,
                               flow_table, &ofpacts);
     }
 
     /* Handle output to multicast groups, in tables 32 and 33. */
     const struct sbrec_multicast_group *mc;
-    struct ofpbuf remote_ofpacts;
-    ofpbuf_init(&remote_ofpacts, 0);
     SBREC_MULTICAST_GROUP_TABLE_FOR_EACH (mc, multicast_group_table) {
-        consider_mc_group(mff_ovn_geneve, ct_zones, local_datapaths, chassis,
-                          mc, &ofpacts, &remote_ofpacts, flow_table);
+        consider_mc_group(mff_ovn_geneve, ct_zones, local_datapaths,
+                          chassis, mc, flow_table);
     }
 
-    ofpbuf_uninit(&remote_ofpacts);
-
     /* Table 0, priority 100.
      * ======================
      *
diff --git a/ovn/controller/physical.h b/ovn/controller/physical.h
index d9b48fc..60319e4 100644
--- a/ovn/controller/physical.h
+++ b/ovn/controller/physical.h
@@ -54,5 +54,14 @@ void physical_run(struct ovsdb_idl_index *sbrec_port_binding_by_name,
                   const struct sset *local_lports,
                   const struct sset *active_tunnels,
                   struct ovn_desired_flow_table *);
+void physical_handle_port_binding_changes(
+        struct ovsdb_idl_index *sbrec_port_binding_by_name,
+        const struct sbrec_port_binding_table *,
+        enum mf_field_id mff_ovn_geneve,
+        const struct sbrec_chassis *,
+        const struct simap *ct_zones,
+        struct hmap *local_datapaths,
+        struct sset *active_tunnels,
+        struct ovn_desired_flow_table *);
 
 #endif /* ovn/physical.h */
-- 
2.1.0



More information about the dev mailing list