[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