On Thu, Jul 21, 2016 at 4:18 AM, Liran Schour <lir...@il.ibm.com> wrote:
> Mickey Spiegel/San Jose/IBM wrote on 20/07/2016 08:53:42 AM:
>
> > From: Mickey Spiegel/San Jose/IBM
> > To: Liran Schour/Haifa/IBM@IBMIL
> > Cc: Ben Pfaff <b...@ovn.org>, dev@openvswitch.org
> > Date: 20/07/2016 08:53 AM
> > Subject: Re: [ovs-dev] [PATCH monitor_cond V10] RFC OVN:
> > Implementation of conditional monitoring usage
> >
> > Comments inline as <Mickey>
> >
> > -----"dev" <dev-boun...@openvswitch.org> wrote: -----
> > To: Ben Pfaff <b...@ovn.org>
> > From: Liran Schour
> > Sent by: "dev"
> > Date: 07/19/2016 01:45AM
> > Cc: dev@openvswitch.org
> > Subject: [ovs-dev] [PATCH monitor_cond V10] RFC OVN: Implementation
> > of conditional monitoring usage
>
> > Conditional monitor of: Port_Binding, Logical_Flow, Multicast_Group
> > MAC_Binding tables. As a result ovn-controller will be notified only
> about
> > records belongs to a datapath that is being served by this hypervisor.
> >
> > Performance evaluation:
> > OVN is the main candidate for conditional monitoring usage. It is clear
> that
> > conditional monitoring reduces computation on the ovn-controller
> (client) side
> > due to the reduced size of flow tables and update messages. Performance
> > evaluation shows up to 75% computation reduction.
> > However, performance evaluation shows also a reduction in
> > computation on the SB
> > ovsdb-server side proportional to the degree that each logical network
> is
> > spread over physical hosts in the DC. Evaluation shows that in a
> realistic
> > scenarios there is a computation reduction also in the server side.
> >
> > <Mickey> Before getting into details, please confirm whether I have
> > a proper understanding how this works:
> > When a hypervisor first comes up, it gets no port bindings.
> > When a VIF comes up on the hypervisor, this will trigger filter_port
> > on that VIF, which causes the port binding for that VIF to come down
> > to the controller.
> > Upon receipt of that port binding, the controller will add the
> > datapath for the VIF's logical switch to local datapaths, which will
> > trigger filter_datapath.
> > This will cause all of the port bindings, MAC bindings, logical
> > flows, and multicast groups for the VIF's logical switch to come
> > down to the controller.
> > Following the patch ports from the VIF's logical switch, this will
> > trigger filter_port for all peer patch ports.
> > The port bindings for those peer patch ports will trigger
> > filter_datapath for all logical router datapaths connected to the
> > logical switch.
> > This will cause all of the port bindings, MAC bindings, logical
> > flows, and multicast groups for those datapaths to come down to the
> > controller.
> > If those datapaths have patch ports to additional datapaths,
> > following the patch ports, eventually all of the SB info for all
> > connected datapaths will come down to the controller.
> > Is this correct?
> >
>
> Yes. One small addition: to minimize the number of filters, the code will
> unfilter_lport when the lport is already included in a flittered datapath.
>
> > <Mickey> Do you have any information about the latency involved in
> > this process?
> >
>
> No, I did not test it. I will write it in my TODO list.
>
Latency is my first concern with this type of approach.
Back and forth cycles between the SB DB and controller is time consuming
and unbounded in expected time to complete. The time will vary widely as
well
depending on competing work at the SB DB and otherwise.
Having datapath associations LS-patch_ports-LR-patch_ports-Transit
LS-patch_ports-GR
That is 7 extra back and forth cycles between controller and SB DB in worst
case but
even with 2 datapaths, this will be slow.
>
> > <Mickey> Further comments on the details inline.
> >
> > Evaluation on simulated environment of 50 hosts and 1000 logical ports
> shows
> > the following results (cycles #):
> >
> > LN spread over # hosts| master | patch | change
> > -------------------------------------------------------------
> > 1 | 24597200127 | 24339235374 | 1.0%
> > 6 | 23788521572 | 19145229352 | 19.5%
> > 12 | 23886405758 | 17913143176 | 25.0%
> > 18 | 25812686279 | 23675094540 | 8.2%
> > 24 | 28414671499 | 24770202308 | 12.8%
> > 30 | 31487218890 | 28397543436 | 9.8%
> > 36 | 36116993930 | 34105388739 | 5.5%
> > 42 | 37898342465 | 38647139083 | -1.9%
> > 48 | 41637996229 | 41846616306 | -0.5%
> > 50 | 41679995357 | 43455565977 | -4.2%
> >
> > Signed-off-by: Liran Schour <lir...@il.ibm.com>
> > ---
> > ovn/controller/automake.mk | 4 +-
> > ovn/controller/binding.c | 48 +++++++---
> > ovn/controller/binding.h | 4 +-
> > ovn/controller/filter.c | 207 +++++++++++++++++++++++++++++
> > +++++++++++
> > ovn/controller/filter.h | 36 +++++++
> > ovn/controller/ovn-controller.c | 14 ++-
> > ovn/controller/ovn-controller.h | 1 +
> > ovn/controller/patch.c | 7 +-
> > tests/ovn-controller.at | 3 +
> > tests/ovn.at | 1 +
> > 10 files changed, 305 insertions(+), 20 deletions(-)
> > create mode 100644 ovn/controller/filter.c
> > create mode 100644 ovn/controller/filter.h
> >
> > diff --git a/ovn/controller/automake.mk b/ovn/controller/automake.mk
> > index cf57bbd..0318654 100644
> > --- a/ovn/controller/automake.mk
> > +++ b/ovn/controller/automake.mk
> > @@ -19,7 +19,9 @@ ovn_controller_ovn_controller_SOURCES = \
> > ovn/controller/ovn-controller.c \
> > ovn/controller/ovn-controller.h \
> > ovn/controller/physical.c \
> > - ovn/controller/physical.h
> > + ovn/controller/physical.h \
> > + ovn/controller/filter.c \
> > + ovn/controller/filter.h
> > ovn_controller_ovn_controller_LDADD = ovn/lib/libovn.la
> lib/libopenvswitch.la
> > man_MANS += ovn/controller/ovn-controller.8
> > EXTRA_DIST += ovn/controller/ovn-controller.8.xml
> > diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
> > index bc6df32..dcc2a2f 100644
> > --- a/ovn/controller/binding.c
> > +++ b/ovn/controller/binding.c
> > @@ -25,6 +25,8 @@
> > #include "openvswitch/vlog.h"
> > #include "ovn/lib/ovn-sb-idl.h"
> > #include "ovn-controller.h"
> > +#include "lport.h"
> > +#include "filter.h"
> >
> > VLOG_DEFINE_THIS_MODULE(binding);
> >
> > @@ -64,7 +66,9 @@ binding_register_ovs_idl(struct ovsdb_idl *ovs_idl)
> >
> > static bool
> > get_local_iface_ids(const struct ovsrec_bridge *br_int,
> > - struct shash *lport_to_iface)
> > + struct shash *lport_to_iface,
> > + struct lport_index *lports_index,
> > + struct controller_ctx *ctx)
> > {
> > int i;
> > bool changed = false;
> > @@ -90,6 +94,9 @@ get_local_iface_ids(const struct ovsrec_bridge
> *br_int,
> > continue;
> > }
> > shash_add(lport_to_iface, iface_id, iface_rec);
> > + if (!lport_lookup_by_name(lports_index, iface_id)) {
> > + filter_lport(ctx, iface_id);
> > + }
> > if (!sset_find_and_delete(&old_local_ids, iface_id)) {
> > sset_add(&local_ids, iface_id);
> > changed = true;
> > @@ -121,8 +128,11 @@ local_datapath_lookup_by_uuid(struct hmap
> > *hmap_p, const struct uuid *uuid)
> > }
> >
> > static void
> > -remove_local_datapath(struct hmap *local_datapaths, struct
> > local_datapath *ld)
> > +remove_local_datapath(struct controller_ctx *ctx, struct hmap
> > *local_datapaths,
> > + struct local_datapath *ld)
> > {
> > + unfilter_datapath(ctx, ld->tunnel_key);
> > +
>
> > <Mickey> You have two different triggers for filter_datapath: One
> > from add_local_datapath, another from add_patched_datapath in
> > patch.c. If you call unfilter_datapath here, you might be blowing
> > away the trigger from add_patched_datapath as well. It will not
> > correct itself until another VIF, L2 gateway, L3 gateway on that
> > datapath is added, or a patch port to that datapath is added.
> >
>
> You are right. I will fix this.
>
> > if (ld->logical_port) {
> > free(ld->logical_port);
> > ld->logical_port = NULL;
> > @@ -133,7 +143,8 @@ remove_local_datapath(struct hmap
> > *local_datapaths, struct local_datapath *ld)
> >
> > static void
> > add_local_datapath(struct hmap *local_datapaths,
> > - const struct sbrec_port_binding *binding_rec)
> > + const struct sbrec_port_binding *binding_rec,
> > + struct controller_ctx *ctx)
> > {
> > if (get_local_datapath(local_datapaths,
> > binding_rec->datapath->tunnel_key)) {
> > @@ -143,8 +154,10 @@ add_local_datapath(struct hmap *local_datapaths,
> > struct local_datapath *ld = xzalloc(sizeof *ld);
> > ld->logical_port = xstrdup(binding_rec->logical_port);
> > memcpy(&ld->uuid, &binding_rec->header_.uuid, sizeof ld->uuid);
> > + ld->tunnel_key = binding_rec->datapath->tunnel_key;
> > hmap_insert(local_datapaths, &ld->hmap_node,
> > binding_rec->datapath->tunnel_key);
> > + filter_datapath(ctx, binding_rec);
> > }
>
> > <Mickey> Just noting that this should take care of L3 gateway as
> > well, once Chandra's "ovn: Add datapth of gateway port to
> local_datapaths
> > " patch lands. The addition of "ctx" to add_local_datapaths will be
> > necessary, in whichever of the two patches lands later.
> >
>
> Right.
>
> > static void
> > @@ -160,6 +173,7 @@ update_qos(const struct ovsrec_interface *iface_rec,
> >
> > static void
> > consider_local_datapath(struct controller_ctx *ctx,
> > + struct lport_index *lports_index,
> > const struct sbrec_chassis *chassis_rec,
> > const struct sbrec_port_binding *binding_rec,
> > struct hmap *local_datapaths,
> > @@ -171,7 +185,7 @@ consider_local_datapath(struct controller_ctx *ctx,
> > if (iface_rec
> > || (binding_rec->parent_port && binding_rec->parent_port[0] &&
> > sset_contains(&local_ids, binding_rec->parent_port))) {
> > - add_local_datapath(local_datapaths, binding_rec);
> > + add_local_datapath(local_datapaths, binding_rec, ctx);
> > if (iface_rec && ctx->ovs_idl_txn) {
> > update_qos(iface_rec, binding_rec);
> > }
> > @@ -210,7 +224,7 @@ consider_local_datapath(struct controller_ctx *ctx,
> > VLOG_INFO("Claiming l2gateway port %s for this chassis.",
> > binding_rec->logical_port);
> > sbrec_port_binding_set_chassis(binding_rec, chassis_rec);
> > - add_local_datapath(local_datapaths, binding_rec);
> > + add_local_datapath(local_datapaths, binding_rec, ctx);
> > }
> > } else if (chassis_rec && binding_rec->chassis == chassis_rec
> > && strcmp(binding_rec->type, "gateway")) {
> > @@ -220,11 +234,18 @@ consider_local_datapath(struct controller_ctx
> *ctx,
> > sbrec_port_binding_set_chassis(binding_rec, NULL);
> > }
> > }
> > + const char *peer = smap_get(&binding_rec->options, "peer");
> > + if (peer) {
> > + if (!lport_lookup_by_name(lports_index, peer)) {
> > + filter_lport(ctx, peer);
> > + }
> > + }
> > }
>
> > <Mickey> If you follow patch ports here, where it applies for every
> > port binding that is received, you might be following a patch port
> > that does not apply to this chassis, such as a "gateway" patch port
> > to an L3 gateway router that is not local to this chassis. It would
> > be better to add this logic in patch.c, perhaps in
> > add_logical_patch_ports around where create_patch_port is called?
> >
>
> Thanks, you are right.
>
> > 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, struct lport_index *lports_index,
> > + struct hmap *local_datapaths)
> > {
> > const struct sbrec_chassis *chassis_rec;
> > const struct sbrec_port_binding *binding_rec;
> > @@ -236,7 +257,8 @@ binding_run(struct controller_ctx *ctx, const
> > struct ovsrec_bridge *br_int,
> > }
> >
> > if (br_int) {
> > - if (ctx->ovnsb_idl_txn && get_local_iface_ids(br_int,
> > &lport_to_iface)) {
> > + if (ctx->ovnsb_idl_txn && get_local_iface_ids(br_int,
> > &lport_to_iface,
> > + lports_index,
> ctx)) {
> > process_full_binding = true;
> > }
> > } else {
> > @@ -252,8 +274,9 @@ binding_run(struct controller_ctx *ctx, const
> > struct ovsrec_bridge *br_int,
> > struct hmap keep_local_datapath_by_uuid =
> > HMAP_INITIALIZER(&keep_local_datapath_by_uuid);
> > SBREC_PORT_BINDING_FOR_EACH(binding_rec, ctx->ovnsb_idl) {
> > - consider_local_datapath(ctx, chassis_rec, binding_rec,
> > - local_datapaths, &lport_to_iface);
> > + consider_local_datapath(ctx, lports_index, chassis_rec,
> > + binding_rec, local_datapaths,
> > + &lport_to_iface);
> > struct local_datapath *ld = xzalloc(sizeof *ld);
> > memcpy(&ld->uuid, &binding_rec->header_.uuid, sizeof
> ld->uuid);
> > hmap_insert(&keep_local_datapath_by_uuid,
> &ld->uuid_hmap_node,
> > @@ -263,7 +286,7 @@ binding_run(struct controller_ctx *ctx, const
> > struct ovsrec_bridge *br_int,
> > HMAP_FOR_EACH_SAFE (old_ld, next, hmap_node, local_datapaths) {
> > if
> (!local_datapath_lookup_by_uuid(&keep_local_datapath_by_uuid,
> > &old_ld->uuid)) {
> > - remove_local_datapath(local_datapaths, old_ld);
> > + remove_local_datapath(ctx, local_datapaths, old_ld);
> > }
> > }
> > hmap_destroy(&keep_local_datapath_by_uuid);
> > @@ -280,8 +303,9 @@ binding_run(struct controller_ctx *ctx, const
> > struct ovsrec_bridge *br_int,
> > poll_immediate_wake();
> > }
> > } else {
> > - consider_local_datapath(ctx, chassis_rec, binding_rec,
> > - local_datapaths,
> &lport_to_iface);
> > + consider_local_datapath(ctx, lports_index, chassis_rec,
> > + binding_rec, local_datapaths,
> > + &lport_to_iface);
> > }
> > }
> > }
> > diff --git a/ovn/controller/binding.h b/ovn/controller/binding.h
> > index 8753d44..11e0c0f 100644
> > --- a/ovn/controller/binding.h
> > +++ b/ovn/controller/binding.h
> > @@ -25,11 +25,13 @@ struct ovsdb_idl;
> > struct ovsrec_bridge;
> > struct simap;
> > struct sset;
> > +struct lport_index;
> >
> > void binding_register_ovs_idl(struct ovsdb_idl *);
> > void binding_reset_processing(void);
> > void binding_run(struct controller_ctx *, const struct ovsrec_bridge
> *br_int,
> > - const char *chassis_id, struct hmap *local_datapaths);
> > + const char *chassis_id, struct lport_index
> *lports_index,
> > + struct hmap *local_datapaths);
> > bool binding_cleanup(struct controller_ctx *, const char *chassis_id);
> >
> > #endif /* ovn/binding.h */
> > diff --git a/ovn/controller/filter.c b/ovn/controller/filter.c
> > new file mode 100644
> > index 0000000..cc25737
> > --- /dev/null
> > +++ b/ovn/controller/filter.c
> > @@ -0,0 +1,207 @@
> > +/* 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.
> > + * You may obtain a copy of the License at:
> > + *
> > + * http://www.apache.org/licenses/LICENSE-2.0
> > + *
> > + * Unless required by applicable law or agreed to in writing, software
> > + * distributed under the License is distributed on an "AS IS" BASIS,
> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> implied.
> > + * See the License for the specific language governing permissions and
> > + * limitations under the License.
> > + */
> > +
> > +#include <config.h>
> > +#include "filter.h"
> > +
> > +#include "openvswitch/vlog.h"
> > +#include "ovn/lib/ovn-sb-idl.h"
> > +#include "ovn-controller.h"
> > +#include "lport.h"
> > +
> > +VLOG_DEFINE_THIS_MODULE(filter);
> > +
> > +static struct hmap filtered_dps = HMAP_INITIALIZER(&filtered_dps);
> > +static struct hmap filtered_lps = HMAP_INITIALIZER(&filtered_lps);
> > +
> > +struct filtered_dp {
> > + struct hmap_node hmap_node;
> > + int64_t tunnel_key;
> > + struct sbrec_datapath_binding *datapath;
> > +};
> > +
> > +struct filtered_lp {
> > + struct hmap_node hmap_node;
> > + const char *lport_name;
> > + bool used;
> > +};
> > +
> > +void
> > +filter_init(struct ovsdb_idl *idl)
> > +{
> > + sbrec_port_binding_add_clause_false(idl);
> > + sbrec_mac_binding_add_clause_false(idl);
> > + sbrec_logical_flow_add_clause_false(idl);
> > + sbrec_multicast_group_add_clause_false(idl);
> > +}
> > +
> > +void
> > +filter_mark_unused(void)
> > +{
> > + struct filtered_lp *lp;
> > +
> > + HMAP_FOR_EACH(lp, hmap_node, &filtered_lps) {
> > + lp->used = false;
> > + }
> > +}
> > +
> > +void
> > +filter_clear(struct ovsdb_idl *idl)
> > +{
> > + struct filtered_lp *lp, *next;
> > + struct filtered_lp *dp, *next_dp;
> > +
> > + HMAP_FOR_EACH_SAFE(lp, next, hmap_node, &filtered_lps) {
> > + hmap_remove(&filtered_lps, &lp->hmap_node);
> > + free(lp);
> > + }
> > + HMAP_FOR_EACH_SAFE(dp, next_dp, hmap_node, &filtered_dps) {
> > + hmap_remove(&filtered_dps, &dp->hmap_node);
> > + free(dp);
> > + }
> > +
> > + ovsdb_idl_condition_reset(idl,
> > + &sbrec_table_port_binding);
> > + ovsdb_idl_condition_reset(idl,
> > + &sbrec_table_logical_flow);
> > + ovsdb_idl_condition_reset(idl,
> > + &sbrec_table_mac_binding);
> > + ovsdb_idl_condition_reset(idl,
> > + &sbrec_table_multicast_group);
> > +}
> > +
> > +static struct filtered_dp *
> > +lookup_dp_by_key(int64_t tunnel_key)
> > +{
> > + struct filtered_dp *dp;
> > +
> > + HMAP_FOR_EACH_WITH_HASH(dp, hmap_node, tunnel_key,
> > + &filtered_dps) {
> > + if (dp->tunnel_key == tunnel_key) {
> > + return dp;
> > + }
> > + }
> > + return NULL;
> > +}
> > +
> > +void
> > +filter_remove_unused_lports(struct controller_ctx *ctx,
> > + const struct lport_index *lports_index)
> > +{
> > + struct filtered_lp *lp, *next;
> > +
> > + HMAP_FOR_EACH_SAFE(lp, next, hmap_node, &filtered_lps) {
> > + if (!lp->used) {
> > + const struct sbrec_port_binding *pb =
> > + lport_lookup_by_name(lports_index, lp->lport_name);
> > + if (!pb) {
> > + continue;
> > + }
> > + if (lookup_dp_by_key(pb->datapath->tunnel_key)) {
> > + VLOG_INFO("Unfilter Port %s", lp->lport_name);
> > + sbrec_port_binding_remove_clause_logical_port(ctx->ovnsb_idl,
> > + OVSDB_F_EQ,
> > +
> > lp->lport_name);
> > + hmap_remove(&filtered_lps, &lp->hmap_node);
> > + free(lp);
> > + }
> > + }
> > + }
> > +}
> > +
> > +void
> > +filter_lport(struct controller_ctx *ctx, const char *lport_name)
> > +{
> > + struct filtered_lp *lp;
> > + size_t hash = hash_bytes(lport_name, strlen(lport_name), 0);
> > +
> > + HMAP_FOR_EACH_WITH_HASH(lp, hmap_node, hash, &filtered_lps) {
> > + if (!strcmp(lp->lport_name, lport_name)) {
> > + lp->used = true;
> > + return;
> > + }
> > + }
> > +
> > + VLOG_INFO("Filter Port %s", lport_name);
> > +
> > + sbrec_port_binding_add_clause_logical_port(ctx->ovnsb_idl,
> > + OVSDB_F_EQ,
> > + lport_name);
> > +
> > + char *name = xmalloc(strlen(lport_name));
> > + lp = xmalloc(sizeof *lp);
> > +
> > + strcpy(name, lport_name);
> > + lp->lport_name = name;
> > + lp->used = true;
> > + hmap_insert(&filtered_lps, &lp->hmap_node,
> > + hash);
> > +}
> > +
> > +void
> > +filter_datapath(struct controller_ctx *ctx,
> > + const struct sbrec_port_binding *pb)
>
> > <Mickey> Since "pb" is only used as "pb->datapath", wondering if it
> > would be cleaner to change the second argument to
> > sbrec_datapath_binding instead?
> >
>
> Will change it to sbrec_datapath_binding *.
>
> > +{
> > + struct filtered_dp *dp;
> > +
> > + dp = lookup_dp_by_key(pb->datapath->tunnel_key);
> > + if (dp) {
> > + return;
> > + }
> > +
> > + VLOG_INFO("Filter DP "UUID_FMT,
> UUID_ARGS(&pb->datapath->header_.uuid));
> > + sbrec_port_binding_add_clause_datapath(ctx->ovnsb_idl,
> > + OVSDB_F_EQ,
> > + pb->datapath);
> > + sbrec_mac_binding_add_clause_datapath(ctx->ovnsb_idl,
> > + OVSDB_F_EQ,
> > + pb->datapath);
> > + sbrec_logical_flow_add_clause_logical_datapath(ctx->ovnsb_idl,
> > + OVSDB_F_EQ,
> > + pb->datapath);
> > + sbrec_multicast_group_add_clause_datapath(ctx->ovnsb_idl,
> > + OVSDB_F_EQ,
> > + pb->datapath);
> > +
> > + dp = xmalloc(sizeof *dp);
> > + dp->tunnel_key = pb->datapath->tunnel_key;
> > + dp->datapath = pb->datapath;
> > + hmap_insert(&filtered_dps, &dp->hmap_node,
> pb->datapath->tunnel_key);
> > +}
> > +
> > +void
> > +unfilter_datapath(struct controller_ctx *ctx, int64_t tunnel_key)
> > +{
> > + struct filtered_dp *dp = lookup_dp_by_key(tunnel_key);
> > +
> > + if (dp) {
> > + VLOG_INFO("Unfilter DP "UUID_FMT,
> > + UUID_ARGS(&dp->datapath->header_.uuid));
> > + sbrec_port_binding_remove_clause_datapath(ctx->ovnsb_idl,
> > + OVSDB_F_EQ,
> > + dp->datapath);
> > + sbrec_mac_binding_remove_clause_datapath(ctx->ovnsb_idl,
> > + OVSDB_F_EQ,
> > + dp->datapath);
> > + sbrec_logical_flow_remove_clause_logical_datapath(ctx->ovnsb_idl,
> > + OVSDB_F_EQ,
> > + dp->datapath);
> > + sbrec_multicast_group_remove_clause_datapath(ctx->ovnsb_idl,
> > + OVSDB_F_EQ,
> > + dp->datapath);
> > + hmap_remove(&filtered_dps, &dp->hmap_node);
> > + free(dp);
> > + }
> > +}
> > diff --git a/ovn/controller/filter.h b/ovn/controller/filter.h
> > new file mode 100644
> > index 0000000..203689a
> > --- /dev/null
> > +++ b/ovn/controller/filter.h
> > @@ -0,0 +1,36 @@
> > +/* Copyright (c) 2015 Nicira, Inc.
> > + *
> > + * Licensed under the Apache License, Version 2.0 (the "License");
> > + * you may not use this file except in compliance with the License.
> > + * You may obtain a copy of the License at:
> > + *
> > + * http://www.apache.org/licenses/LICENSE-2.0
> > + *
> > + * Unless required by applicable law or agreed to in writing, software
> > + * distributed under the License is distributed on an "AS IS" BASIS,
> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> implied.
> > + * See the License for the specific language governing permissions and
> > + * limitations under the License.
> > + */
> > +
> > +#ifndef OVN_FILTER_H
> > +#define OVN_FILTER_H 1
> > +
> > +#include <stdint.h>
> > +
> > +struct controller_ctx;
> > +struct ovsdb_idl;
> > +struct sbrec_port_binding;
> > +struct lport_index;
> > +
> > +void filter_init(struct ovsdb_idl *idl);
> > +void filter_clear(struct ovsdb_idl *idl);
> > +void filter_mark_unused(void);
> > +void filter_remove_unused_lports(struct controller_ctx *ctx,
> > + const struct lport_index *lports);
> > +void filter_lport(struct controller_ctx *ctx, const char *lport_name);
> > +void filter_datapath(struct controller_ctx *ctx,
> > + const struct sbrec_port_binding *pb);
> > +void unfilter_datapath(struct controller_ctx *ctx, int64_t tunnel_key);
> > +
> > +#endif /* ovn/controller/filter.h */
> > diff --git a/ovn/controller/ovn-controller.c
> b/ovn/controller/ovn-controller.c
> > index 04684b2..2ad8523 100644
> > --- a/ovn/controller/ovn-controller.c
> > +++ b/ovn/controller/ovn-controller.c
> > @@ -54,6 +54,7 @@
> > #include "stream.h"
> > #include "unixctl.h"
> > #include "util.h"
> > +#include "filter.h"
> >
> > VLOG_DEFINE_THIS_MODULE(main);
> >
> > @@ -379,6 +380,8 @@ 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));
> >
> > + filter_init(ovnsb_idl_loop.idl);
> > +
> > /* Track the southbound idl. */
> > ovsdb_idl_track_add_all(ovnsb_idl_loop.idl);
> >
> > @@ -404,6 +407,7 @@ main(int argc, char *argv[])
> > binding_reset_processing();
> > lport_index_clear(&lports);
> > mcgroup_index_clear(&mcgroups);
> > + filter_clear(ovnsb_idl_loop.idl);
> > } else {
> > free(new_ovnsb_remote);
> > }
> > @@ -422,19 +426,20 @@ main(int argc, char *argv[])
> > const struct ovsrec_bridge *br_int = get_br_int(&ctx);
> > const char *chassis_id = get_chassis_id(ctx.ovs_idl);
> >
> > + lport_index_fill(&lports, ctx.ovnsb_idl);
> > + mcgroup_index_fill(&mcgroups, ctx.ovnsb_idl);
> > + filter_mark_unused();
> > +
> > if (chassis_id) {
> > chassis_run(&ctx, chassis_id);
> > encaps_run(&ctx, br_int, chassis_id);
> > - binding_run(&ctx, br_int, chassis_id, &local_datapaths);
> > + binding_run(&ctx, br_int, chassis_id, &lports,
> &local_datapaths);
> > }
> >
> > if (br_int && chassis_id) {
> > patch_run(&ctx, br_int, chassis_id, &local_datapaths,
> > &patched_datapaths);
> >
> > - lport_index_fill(&lports, ctx.ovnsb_idl);
> > - mcgroup_index_fill(&mcgroups, ctx.ovnsb_idl);
> > -
> > enum mf_field_id mff_ovn_geneve = ofctrl_run(br_int);
> >
> > pinctrl_run(&ctx, &lports, br_int, chassis_id,
> &local_datapaths);
> > @@ -452,6 +457,7 @@ main(int argc, char *argv[])
> > ofctrl_put(&group_table);
> > }
> >
> > + filter_remove_unused_lports(&ctx, &lports);
> > sset_destroy(&all_lports);
> >
> > unixctl_server_run(unixctl);
> > diff --git a/ovn/controller/ovn-controller.h
> b/ovn/controller/ovn-controller.h
> > index 470b1f5..3469fe5 100644
> > --- a/ovn/controller/ovn-controller.h
> > +++ b/ovn/controller/ovn-controller.h
> > @@ -40,6 +40,7 @@ struct local_datapath {
> > struct hmap_node hmap_node;
> > struct hmap_node uuid_hmap_node;
> > struct uuid uuid;
> > + int64_t tunnel_key;
> > char *logical_port;
> > const struct sbrec_port_binding *localnet_port;
> > };
> > diff --git a/ovn/controller/patch.c b/ovn/controller/patch.c
> > index 52d9e8d..438dff5 100644
> > --- a/ovn/controller/patch.c
> > +++ b/ovn/controller/patch.c
> > @@ -22,6 +22,7 @@
> > #include "lib/vswitch-idl.h"
> > #include "openvswitch/vlog.h"
> > #include "ovn-controller.h"
> > +#include "filter.h"
> >
> > VLOG_DEFINE_THIS_MODULE(patch);
> >
> > @@ -250,7 +251,8 @@ add_bridge_mappings(struct controller_ctx *ctx,
> >
> > static void
> > add_patched_datapath(struct hmap *patched_datapaths,
> > - const struct sbrec_port_binding *binding_rec,
> > bool local)
> > + const struct sbrec_port_binding *binding_rec,
> > bool local,
> > + struct controller_ctx *ctx)
> > {
> > struct patched_datapath *pd =
> get_patched_datapath(patched_datapaths,
> > binding_rec->datapath->tunnel_key);
> > @@ -268,6 +270,7 @@ add_patched_datapath(struct hmap *patched_datapaths,
> > /* stale is set to false. */
> > hmap_insert(patched_datapaths, &pd->hmap_node,
> > binding_rec->datapath->tunnel_key);
> > + filter_datapath(ctx, binding_rec);
> > }
>
> > <Mickey> Should unfilter_datapath be called somewhere around line
> > 297? With a check against local_datapaths as well?
> >
>
> Right, will fix that.
>
> Thanks for the review,
> - Liran
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
