ovn-controller: Handle only relevant ports and flows.

On a particular hypervisor, ovn-controller only needs to handle ports
and datapaths that have some relationship with it, that is, the
ports that actually reside on the hypervisor, plus all the other ports on
those ports' datapaths, plus all of the ports and datapaths that are
reachable from those via logical patch ports.  Until now, ovn-controller
has done a poor job of limiting what it deals with to this set.  This
commit improves the situation.

This commit gets rid of the concept of a "patched_datapath" which until now
was used to represent any datapath that contained a logical patch port.
Previously, the concept of a "local_datapath" meant a datapath with a VIF
that resides on the local hypervisor.  This commit extends that concept to
include any other datapath that can be reached from a VIF on the local
hypervisor, which is a simplification that makes the code easier to
understand in a few places.

CC: Gurucharan Shetty <[email protected]>
Signed-off-by: Ben Pfaff <[email protected]>
Acked-by: Mickey Spiegel <[email protected]>

ovn/controller/binding.c

@@ -105,17 +105,61 @@ get_local_iface_ids(const struct ovsrec_bridge *br_int,
 }
 
 static void
-add_local_datapath(struct hmap *local_datapaths,
-        const struct sbrec_port_binding *binding_rec)
+add_local_datapath__(const struct ldatapath_index *ldatapaths,
+                     const struct lport_index *lports,
+                     const struct sbrec_datapath_binding *datapath,
+                     bool has_local_l3gateway, int depth,
+                     struct hmap *local_datapaths)
 {
-    if (get_local_datapath(local_datapaths,
-                           binding_rec->datapath->tunnel_key)) {
+    uint32_t dp_key = datapath->tunnel_key;
+
+    struct local_datapath *ld = get_local_datapath(local_datapaths, dp_key);
+    if (ld) {
+        if (has_local_l3gateway) {
+            ld->has_local_l3gateway = true;
+        }
         return;
     }
 
-    struct local_datapath *ld = xzalloc(sizeof *ld);
-    hmap_insert(local_datapaths, &ld->hmap_node,
-                binding_rec->datapath->tunnel_key);
+    ld = xzalloc(sizeof *ld);
+    hmap_insert(local_datapaths, &ld->hmap_node, dp_key);
+    ld->datapath = datapath;
+    ld->ldatapath = ldatapath_lookup_by_key(ldatapaths, dp_key);
+    ovs_assert(ld->ldatapath);
+    ld->localnet_port = NULL;
+    ld->has_local_l3gateway = has_local_l3gateway;
+
+    if (depth >= 100) {
+        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
+        VLOG_WARN_RL(&rl, "datapaths nested too deep");
+        return;
+    }
+
+    /* Recursively add logical datapaths to which this one patches. */
+    for (size_t i = 0; i < ld->ldatapath->n_lports; i++) {
+        const struct sbrec_port_binding *pb = ld->ldatapath->lports[i];
+        if (!strcmp(pb->type, "patch")) {
+            const char *peer_name = smap_get(&pb->options, "peer");
+            if (peer_name) {
+                const struct sbrec_port_binding *peer = lport_lookup_by_name(
+                    lports, peer_name);
+                if (peer && peer->datapath) {
+                    add_local_datapath__(ldatapaths, lports, peer->datapath,
+                                         false, depth + 1, local_datapaths);
+                }
+            }
+        }
+    }
+}
+
+static void
+add_local_datapath(const struct ldatapath_index *ldatapaths,
+                   const struct lport_index *lports,
+                   const struct sbrec_datapath_binding *datapath,
+                   bool has_local_l3gateway, struct hmap *local_datapaths)
+{
+    add_local_datapath__(ldatapaths, lports, datapath, has_local_l3gateway, 0,
+                         local_datapaths);
 }
 
 static void
@@ -276,6 +320,8 @@ setup_qos(const char *egress_iface, struct hmap *queue_map)
 
 static void
 consider_local_datapath(struct controller_ctx *ctx,
+                        const struct ldatapath_index *ldatapaths,
+                        const struct lport_index *lports,
                         const struct sbrec_chassis *chassis_rec,
                         const struct sbrec_port_binding *binding_rec,
                         struct hmap *qos_map,
@@ -293,7 +339,8 @@ consider_local_datapath(struct controller_ctx *ctx,
             /* Add child logical port to the set of all local ports. */
             sset_add(all_lports, binding_rec->logical_port);
         }
-        add_local_datapath(local_datapaths, binding_rec);
+        add_local_datapath(ldatapaths, lports, binding_rec->datapath,
+                           false, local_datapaths);
         if (iface_rec && qos_map && ctx->ovs_idl_txn) {
             get_qos_params(binding_rec, qos_map);
         }
@@ -328,7 +375,8 @@ consider_local_datapath(struct controller_ctx *ctx,
         }
 
         sset_add(all_lports, binding_rec->logical_port);
-        add_local_datapath(local_datapaths, binding_rec);
+        add_local_datapath(ldatapaths, lports, binding_rec->datapath,
+                           false, local_datapaths);
         if (binding_rec->chassis == chassis_rec) {
             return;
         }
@@ -341,8 +389,9 @@ consider_local_datapath(struct controller_ctx *ctx,
     } else if (!strcmp(binding_rec->type, "l3gateway")) {
         const char *chassis = smap_get(&binding_rec->options,
                                        "l3gateway-chassis");
-        if (!strcmp(chassis, chassis_rec->name) && ctx->ovnsb_idl_txn) {
-            add_local_datapath(local_datapaths, binding_rec);
+        if (!strcmp(chassis, chassis_rec->name)) {
+            add_local_datapath(ldatapaths, lports, binding_rec->datapath,
+                               true, local_datapaths);
         }
     } else if (chassis_rec && binding_rec->chassis == chassis_rec) {
         if (ctx->ovnsb_idl_txn) {
@@ -364,7 +413,8 @@ consider_local_datapath(struct controller_ctx *ctx,
 
 void
 binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int,
-            const char *chassis_id, struct hmap *local_datapaths,
+            const char *chassis_id, const struct ldatapath_index *ldatapaths,
+            const struct lport_index *lports, struct hmap *local_datapaths,
             struct sset *all_lports)
 {
     const struct sbrec_chassis *chassis_rec;
@@ -388,7 +438,8 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int,
      * chassis and update the binding accordingly.  This includes both
      * directly connected logical ports and children of those ports. */
     SBREC_PORT_BINDING_FOR_EACH(binding_rec, ctx->ovnsb_idl) {
-        consider_local_datapath(ctx, chassis_rec, binding_rec,
+        consider_local_datapath(ctx, ldatapaths, lports,
+                                chassis_rec, binding_rec,
                                 sset_is_empty(&egress_ifaces) ? NULL :
                                 &qos_map, local_datapaths, &lport_to_iface,
                                 all_lports);

ovn/controller/binding.h

@@ -1,4 +1,4 @@
-/* Copyright (c) 2015 Nicira, Inc.
+/* Copyright (c) 2015, 2016 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -21,14 +21,17 @@
 
 struct controller_ctx;
 struct hmap;
+struct ldatapath_index;
+struct lport_index;
 struct ovsdb_idl;
 struct ovsrec_bridge;
 struct simap;
 struct sset;
 
 void binding_register_ovs_idl(struct ovsdb_idl *);
 void binding_run(struct controller_ctx *, const struct ovsrec_bridge *br_int,
-                 const char *chassis_id, struct hmap *local_datapaths,
+                 const char *chassis_id, const struct ldatapath_index *,
+                 const struct lport_index *, struct hmap *local_datapaths,
                  struct sset *all_lports);
 bool binding_cleanup(struct controller_ctx *, const char *chassis_id);
 

ovn/controller/lflow.c

@@ -68,7 +68,6 @@ static void consider_logical_flow(const struct lport_index *lports,
                                   const struct mcgroup_index *mcgroups,
                                   const struct sbrec_logical_flow *lflow,
                                   const struct hmap *local_datapaths,
-                                  const struct hmap *patched_datapaths,
                                   struct group_table *group_table,
                                   const struct simap *ct_zones,
                                   struct hmap *dhcp_opts_p,
@@ -111,7 +110,6 @@ static void
 add_logical_flows(struct controller_ctx *ctx, const struct lport_index *lports,
                   const struct mcgroup_index *mcgroups,
                   const struct hmap *local_datapaths,
-                  const struct hmap *patched_datapaths,
                   struct group_table *group_table,
                   const struct simap *ct_zones,
                   struct hmap *flow_table,
@@ -137,7 +135,7 @@ add_logical_flows(struct controller_ctx *ctx, const struct lport_index *lports,
 
     SBREC_LOGICAL_FLOW_FOR_EACH (lflow, ctx->ovnsb_idl) {
         consider_logical_flow(lports, mcgroups, lflow, local_datapaths,
-                              patched_datapaths, group_table, ct_zones,
+                              group_table, ct_zones,
                               &dhcp_opts, &dhcpv6_opts, &conj_id_ofs,
                               flow_table, expr_address_sets_p);
     }
@@ -151,7 +149,6 @@ consider_logical_flow(const struct lport_index *lports,
                       const struct mcgroup_index *mcgroups,
                       const struct sbrec_logical_flow *lflow,
                       const struct hmap *local_datapaths,
-                      const struct hmap *patched_datapaths,
                       struct group_table *group_table,
                       const struct simap *ct_zones,
                       struct hmap *dhcp_opts_p,
@@ -167,41 +164,8 @@ consider_logical_flow(const struct lport_index *lports,
     if (!ldp) {
         return;
     }
-    if (is_switch(ldp)) {
-        /* For a logical switch datapath, local_datapaths tells us if there
-         * are any local ports for this datapath.  If not, we can skip
-         * processing logical flows if that logical switch datapath is not
-         * patched to any logical router.
-         *
-         * Otherwise, we still need both ingress and egress pipeline
-         * because even if there are no local ports, we still may need to
-         * execute the ingress pipeline after a packet leaves a logical
-         * router and we need to do egress pipeline for a switch that
-         * is connected to only routers.  Further optimization is possible,
-         * but not based on what we know with local_datapaths right now.
-         *
-         * A better approach would be a kind of "flood fill" algorithm:
-         *
-         *   1. Initialize set S to the logical datapaths that have a port
-         *      located on the hypervisor.
-         *
-         *   2. For each patch port P in a logical datapath in S, add the
-         *      logical datapath of the remote end of P to S.  Iterate
-         *      until S reaches a fixed point.
-         *
-         * This can be implemented in northd, which can generate the sets and
-         * save it on each port-binding record in SB, and ovn-controller can
-         * use the information directly. However, there can be update storms
-         * when a pair of patch ports are added/removed to connect/disconnect
-         * large lrouters and lswitches. This need to be studied further.
-         */
-
-        if (!get_local_datapath(local_datapaths, ldp->tunnel_key)) {
-            if (!get_patched_datapath(patched_datapaths,
-                                      ldp->tunnel_key)) {
-                return;
-            }
-        }
+    if (!get_local_datapath(local_datapaths, ldp->tunnel_key)) {
+        return;
     }
 
     /* Determine translation of logical table IDs to physical table IDs. */
@@ -410,7 +374,6 @@ void
 lflow_run(struct controller_ctx *ctx, const struct lport_index *lports,
           const struct mcgroup_index *mcgroups,
           const struct hmap *local_datapaths,
-          const struct hmap *patched_datapaths,
           struct group_table *group_table,
           const struct simap *ct_zones,
           struct hmap *flow_table)
@@ -419,8 +382,7 @@ lflow_run(struct controller_ctx *ctx, const struct lport_index *lports,
 
     update_address_sets(ctx, &expr_address_sets);
     add_logical_flows(ctx, lports, mcgroups, local_datapaths,
-                      patched_datapaths, group_table, ct_zones, flow_table,
-                      &expr_address_sets);
+                      group_table, ct_zones, flow_table, &expr_address_sets);
     add_neighbor_flows(ctx, lports, flow_table);
 
     expr_macros_destroy(&expr_address_sets);

ovn/controller/lflow.h

@@ -64,7 +64,6 @@ void lflow_init(void);
 void lflow_run(struct controller_ctx *, const struct lport_index *,
                const struct mcgroup_index *,
                const struct hmap *local_datapaths,
-               const struct hmap *patched_datapaths,
                struct group_table *group_table,
                const struct simap *ct_zones,
                struct hmap *flow_table);

ovn/controller/ovn-controller.c

@@ -78,16 +78,6 @@ get_local_datapath(const struct hmap *local_datapaths, uint32_t tunnel_key)
             : NULL);
 }
 
-struct patched_datapath *
-get_patched_datapath(const struct hmap *patched_datapaths, uint32_t tunnel_key)
-{
-    struct hmap_node *node = hmap_first_with_hash(patched_datapaths,
-                                                  tunnel_key);
-    return (node
-            ? CONTAINER_OF(node, struct patched_datapath, hmap_node)
-            : NULL);
-}
-
 const struct sbrec_chassis *
 get_chassis(struct ovsdb_idl *ovnsb_idl, const char *chassis_id)
 {
@@ -227,13 +217,12 @@ get_ovnsb_remote(struct ovsdb_idl *ovs_idl)
 }
 
 static void
-update_ct_zones(struct sset *lports, struct hmap *patched_datapaths,
+update_ct_zones(struct sset *lports, const struct hmap *local_datapaths,
                 struct simap *ct_zones, unsigned long *ct_zone_bitmap,
                 struct shash *pending_ct_zones)
 {
     struct simap_node *ct_zone, *ct_zone_next;
     int scan_start = 1;
-    struct patched_datapath *pd;
     const char *user;
     struct sset all_users = SSET_INITIALIZER(&all_users);
 
@@ -242,13 +231,14 @@ update_ct_zones(struct sset *lports, struct hmap *patched_datapaths,
     }
 
     /* Local patched datapath (gateway routers) need zones assigned. */
-    HMAP_FOR_EACH(pd, hmap_node, patched_datapaths) {
-        if (!pd->local) {
+    const struct local_datapath *ld;
+    HMAP_FOR_EACH (ld, hmap_node, local_datapaths) {
+        if (!ld->has_local_l3gateway) {
             continue;
         }
 
-        char *dnat = alloc_nat_zone_key(&pd->key, "dnat");
-        char *snat = alloc_nat_zone_key(&pd->key, "snat");
+        char *dnat = alloc_nat_zone_key(&ld->datapath->header_.uuid, "dnat");
+        char *snat = alloc_nat_zone_key(&ld->datapath->header_.uuid, "snat");
         sset_add(&all_users, dnat);
         sset_add(&all_users, snat);
         free(dnat);
@@ -495,11 +485,8 @@ main(int argc, char *argv[])
 
         update_probe_interval(&ctx);
 
-        /* Contains "struct local_datapath" nodes whose hash values are the
-         * tunnel_key of datapaths with at least one local port binding. */
+        /* Contains "struct local_datapath" nodes. */
         struct hmap local_datapaths = HMAP_INITIALIZER(&local_datapaths);
-
-        struct hmap patched_datapaths = HMAP_INITIALIZER(&patched_datapaths);
         struct sset all_lports = SSET_INITIALIZER(&all_lports);
 
         const struct ovsrec_bridge *br_int = get_br_int(&ctx);
@@ -516,31 +503,29 @@ main(int argc, char *argv[])
         if (chassis_id) {
             chassis = chassis_run(&ctx, chassis_id, br_int);
             encaps_run(&ctx, br_int, chassis_id);
-            binding_run(&ctx, br_int, chassis_id, &local_datapaths,
-                        &all_lports);
+            binding_run(&ctx, br_int, chassis_id, &ldatapaths, &lports,
+                        &local_datapaths, &all_lports);
         }
 
         if (br_int && chassis) {
-            patch_run(&ctx, br_int, chassis_id, &local_datapaths,
-                      &patched_datapaths);
+            patch_run(&ctx, br_int, chassis_id, &local_datapaths);
 
             enum mf_field_id mff_ovn_geneve = ofctrl_run(br_int,
                                                          &pending_ct_zones);
 
             pinctrl_run(&ctx, &lports, br_int, chassis_id, &local_datapaths);
-            update_ct_zones(&all_lports, &patched_datapaths, &ct_zones,
+            update_ct_zones(&all_lports, &local_datapaths, &ct_zones,
                             ct_zone_bitmap, &pending_ct_zones);
             if (ctx.ovs_idl_txn) {
                 commit_ct_zones(br_int, &pending_ct_zones);
 
                 struct hmap flow_table = HMAP_INITIALIZER(&flow_table);
                 lflow_run(&ctx, &lports, &mcgroups, &local_datapaths,
-                          &patched_datapaths, &group_table, &ct_zones,
-                          &flow_table);
+                          &group_table, &ct_zones, &flow_table);
 
                 physical_run(&ctx, mff_ovn_geneve,
                              br_int, chassis_id, &ct_zones, &flow_table,
-                             &local_datapaths, &patched_datapaths);
+                             &local_datapaths);
 
                 ofctrl_put(&flow_table, &pending_ct_zones,
                            get_nb_cfg(ctx.ovnsb_idl));
@@ -567,14 +552,6 @@ main(int argc, char *argv[])
         }
         hmap_destroy(&local_datapaths);
 
-        struct patched_datapath *pd_cur_node, *pd_next_node;
-        HMAP_FOR_EACH_SAFE (pd_cur_node, pd_next_node, hmap_node,
-                &patched_datapaths) {
-            hmap_remove(&patched_datapaths, &pd_cur_node->hmap_node);
-            free(pd_cur_node);
-        }
-        hmap_destroy(&patched_datapaths);
-
         unixctl_server_run(unixctl);
 
         unixctl_server_wait(unixctl);