[ovs-dev] [PATCH v3 16/16] ovn-controller: Monitor only necessary southbound rows.

Ben Pfaff blp at ovn.org
Tue Dec 20 03:32:37 UTC 2016


On Mon, Dec 19, 2016 at 11:39:43AM -0800, Mickey Spiegel wrote:
> On Sun, Dec 18, 2016 at 12:18 AM, Ben Pfaff <blp at ovn.org> wrote:
> 
> > Until now, ovn-controller has replicated all of the southbound database
> > (through the IDL).  This is inefficient, especially in a large OVN setup
> > where many logical networks are not present on an individual hypervisor.
> > This commit improves on the situation somewhat, by making ovn-controller
> > replicate (almost) only the port bindings, logical flows, and multicast
> > groups that are actually relevant to the particular hypervisor on which
> > ovn-controller is running.  This is easily possible by replicating the
> > patch ports from the Port_Binding table and using these relationships to
> > determine connections between datapaths.
> >
> 
> This approach is certainly a lot simpler than datapaths of interest.
> 
> Thinking about this direct approach using patch ports versus the
> related_datapaths concept from datapaths of interest:
> - The number of related_datapaths and the number of patch ports
>   and l3gateway ports should be the same. The complexity of
>   running the algorithm should be similar. The direct approach has
>   some advantage in not running the recursion over l3gateway ports.

I think that actually we do not need all l2gateway and l3gateway ports,
only the ones assigned to the current chassis.  I thought originally
that it would be hard to get ovsdb-server to give us just those, but it
turned out to be easy, so I folded that in.

> - One difference to ovn-controller is in getting a full port_binding for
>   each versus just a related_datapath. Probably not that significant.

The big-O asymptotic time and space cost should be the same, but I agree
that they could have different constant factors.

> - ovn-controller has a few SBREC_PORT_BINDING_FOR_EACH
>   loops, in lport.c, binding.c, patch.c, and physical.c. These do not
>   seem to involve much processing for ports that are not local.

I agree that there are likely improvements to be made in those, but I
think that the actual performance improvement is likely to be limited
because, once we have conditional monitoring, both should roughly be the
same sets of ports.

> Acked-by: Mickey Spiegel <mickeys.dev at gmail.com>

Thanks!

> > --- a/ovn/controller/binding.c
> > +++ b/ovn/controller/binding.c
> > @@ -393,6 +393,7 @@ consider_local_datapath(struct controller_ctx *ctx,
> >              add_local_datapath(ldatapaths, lports, binding_rec->datapath,
> >                                 true, local_datapaths);
> >          }
> > +        sset_add(local_lports, binding_rec->logical_port);
> >
> 
> I don't understand why this is necessary. There is already a clause for
> port_bindings of type "l3gateway" in update_sb_monitors below.
> 
> I ran ovn automated tests including kernel tests without this line.
> They all passed.

I removed it.

> > +    sbrec_port_binding_add_clause_type(&pb, OVSDB_F_EQ, "patch");
> > +    sbrec_port_binding_add_clause_type(&pb, OVSDB_F_EQ, "l2gateway");
> > +    sbrec_port_binding_add_clause_type(&pb, OVSDB_F_EQ, "l3gateway");
> > +    sbrec_port_binding_add_clause_type(&pb, OVSDB_F_EQ, "localnet");
> >
> 
> Why do you need a clause for "localnet"?
> IMO it should be good enough to receive "localnet" port_bindings based
> on the port_binding datapath clause.
> 
> I ran ovn automated tests including kernel tests without this line.
> They all passed.

Maybe we do not need it.  I was working from the assumption that, if a
localnet port has a bridge mapping on a given hypervisor, then we should
instantiate it.  But this may be unnecessary; I cannot quickly think of
a scenario where it would make a difference.

I removed it.

> > index 628d3c8..3779741 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -5403,9 +5403,9 @@ AT_CHECK([ovs-vsctl add-port br-int localvif1 -- set
> > Interface localvif1 externa
> >  # On hv1, check that there are no flows outputting bcast to tunnel
> >  OVS_WAIT_UNTIL([test `ovs-ofctl dump-flows br-int table=32 | ofctl_strip
> > | grep output | wc -l` -eq 0])
> >
> > -# On hv2, check that there is 1 flow outputting bcast to tunnel to hv1.
> > +# On hv2, check that no flow outputs bcast to tunnel to hv1.
> >  as hv2
> > -OVS_WAIT_UNTIL([test `ovs-ofctl dump-flows br-int table=32 | ofctl_strip
> > | grep output | wc -l` -eq 1])
> > +OVS_WAIT_UNTIL([test `ovs-ofctl dump-flows br-int table=32 | ofctl_strip
> > | grep output | wc -l` -eq 0])
> >
> >  # Now bind vif2 on hv2.
> >  AT_CHECK([ovs-vsctl add-port br-int localvif2 -- set Interface localvif2
> > external_ids:iface-id=localvif2])
> >
> 
> If you take my suggestion for patch 10 to filter to only local_datapaths in
> consider_mc_group in physical.c, then these changes to ovn.at would
> move to patch 10.

I moved it.

I folded in the following incremental:

--8<--------------------------cut here-------------------------->8--

diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
index f291e23..feca433 100644
--- a/ovn/controller/binding.c
+++ b/ovn/controller/binding.c
@@ -363,7 +363,6 @@ consider_local_datapath(struct controller_ctx *ctx,
             add_local_datapath(ldatapaths, lports, binding_rec->datapath,
                                true, local_datapaths);
         }
-        sset_add(local_lports, binding_rec->logical_port);
     } else if (!strcmp(binding_rec->type, "localnet")) {
         /* Add all localnet ports to local_lports so that we allocate ct zones
          * for them. */
diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index 8ec7c15..ee1e6c1 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -120,6 +120,7 @@ get_bridge(struct ovsdb_idl *ovs_idl, const char *br_name)
 
 static void
 update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
+                   const struct sbrec_chassis *chassis,
                    const struct sset *local_ifaces,
                    struct hmap *local_datapaths)
 {
@@ -137,9 +138,24 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
     struct ovsdb_idl_condition mb = OVSDB_IDL_CONDITION_INIT(&mb);
     struct ovsdb_idl_condition mg = OVSDB_IDL_CONDITION_INIT(&mg);
     sbrec_port_binding_add_clause_type(&pb, OVSDB_F_EQ, "patch");
-    sbrec_port_binding_add_clause_type(&pb, OVSDB_F_EQ, "l2gateway");
-    sbrec_port_binding_add_clause_type(&pb, OVSDB_F_EQ, "l3gateway");
-    sbrec_port_binding_add_clause_type(&pb, OVSDB_F_EQ, "localnet");
+    if (chassis) {
+        /* This should be mostly redundant with the other clauses for port
+         * bindings, but it allows us to catch any ports that are assigned to
+         * us but should not be.  That way, we can clear their chassis
+         * assignments. */
+        sbrec_port_binding_add_clause_chassis(&pb, OVSDB_F_EQ,
+                                              &chassis->header_.uuid);
+
+        /* Ensure that we find out about l2gateway and l3gateway ports that
+         * should be present on this chassis.  Otherwise, we might never find
+         * out about those ports, if their datapaths don't otherwise have a VIF
+         * in this chassis. */
+        const char *id = chassis->name;
+        const struct smap l2 = SMAP_CONST1(&l2, "l2gateway-chassis", id);
+        sbrec_port_binding_add_clause_options(&pb, OVSDB_F_INCLUDES, &l2);
+        const struct smap l3 = SMAP_CONST1(&l3, "l3gateway-chassis", id);
+        sbrec_port_binding_add_clause_options(&pb, OVSDB_F_INCLUDES, &l3);
+    }
     if (local_ifaces) {
         const char *name;
         SSET_FOR_EACH (name, local_ifaces) {
@@ -500,7 +516,7 @@ main(int argc, char *argv[])
     struct ovsdb_idl_loop ovnsb_idl_loop = OVSDB_IDL_LOOP_INITIALIZER(
         ovsdb_idl_create(ovnsb_remote, &sbrec_idl_class, true, true));
     ovsdb_idl_omit_alert(ovnsb_idl_loop.idl, &sbrec_chassis_col_nb_cfg);
-    update_sb_monitors(ovnsb_idl_loop.idl, NULL, NULL);
+    update_sb_monitors(ovnsb_idl_loop.idl, NULL, NULL, NULL);
 
     /* Track the southbound idl. */
     ovsdb_idl_get_initial_snapshot(ovnsb_idl_loop.idl);
@@ -596,7 +612,8 @@ main(int argc, char *argv[])
                 }
             }
 
-            update_sb_monitors(ctx.ovnsb_idl, &local_lports, &local_datapaths);
+            update_sb_monitors(ctx.ovnsb_idl, chassis,
+                               &local_lports, &local_datapaths);
         }
 
         mcgroup_index_destroy(&mcgroups);


More information about the dev mailing list