[ovs-dev] [PATCH monitor_cond V11] ovn: implementation of conditional monitoring usage
Liran Schour
LIRANS at il.ibm.com
Tue Aug 2 08:54:58 UTC 2016
Mickey Spiegel/San Jose/IBM wrote on 31/07/2016 08:59:29 PM:
>
> Comments inline with <Mickey>
>
> -----"dev" <dev-bounces at openvswitch.org> wrote: -----
> To: Ben Pfaff <blp at ovn.org>
> From: Liran Schour
> Sent by: "dev"
> Date: 07/28/2016 04:49AM
> Cc: dev at openvswitch.org
> Subject: [ovs-dev] [PATCH monitor_cond V11] 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 on the ovn-controller
side.
> 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.
>
> 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 <lirans at il.ibm.com>
> ---
> ovn/controller/automake.mk | 4 +-
> ovn/controller/binding.c | 45 ++++++---
> ovn/controller/binding.h | 4 +-
> ovn/controller/filter.c | 207 +++++++++++++++++++++++++++++
> +++++++++++
> ovn/controller/filter.h | 36 +++++++
> ovn/controller/ovn-controller.c | 18 +++-
> ovn/controller/ovn-controller.h | 2 +
> ovn/controller/patch.c | 30 ++++--
> ovn/controller/patch.h | 4 +-
> tests/ovn-controller.at | 3 +
> tests/ovn.at | 1 +
> 11 files changed, 327 insertions(+), 27 deletions(-)
> create mode 100644 ovn/controller/filter.c
> create mode 100644 ovn/controller/filter.h
>
> <Mickey> Thanks to all_lports and possibly more, this no longer
> applies cleanly. Rebase required.
>
> <Mickey> A few nits inline.
>
>
> 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 e83c1d5..be77619 100644
> --- a/ovn/controller/binding.c
> +++ b/ovn/controller/binding.c
> @@ -27,6 +27,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);
>
> @@ -66,7 +68,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;
> @@ -92,6 +96,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;
> @@ -123,8 +130,14 @@ 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 hmap *patched_datapaths,
> + struct local_datapath *ld)
> {
> + if (!get_patched_datapath(patched_datapaths, ld->tunnel_key)) {
> + unfilter_datapath(ctx, ld->tunnel_key);
> + }
> +
> if (ld->logical_port) {
> free(ld->logical_port);
> ld->logical_port = NULL;
> @@ -136,7 +149,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)) {
> @@ -146,11 +160,13 @@ 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);
> lport_index_reset();
> mcgroup_index_reset();
> lflow_reset_processing();
> + filter_datapath(ctx, binding_rec->datapath);
> }
>
> static void
> @@ -177,7 +193,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);
> }
> @@ -216,7 +232,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")) {
> @@ -230,7 +246,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, struct lport_index *lports_index,
> + struct hmap *local_datapaths, struct hmap
*patched_datapaths)
> {
> const struct sbrec_chassis *chassis_rec;
> const struct sbrec_port_binding *binding_rec;
> @@ -242,7 +259,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 {
> @@ -258,8 +276,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, chassis_rec,
> + binding_rec, local_datapaths,
> + &lport_to_iface);
> <Mickey> There is no longer any change here, just reformatting that
> is not required.
Right.
>
> 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,
> @@ -269,7 +288,8 @@ 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,
> patched_datapaths,
> + old_ld);
> }
> }
> hmap_destroy(&keep_local_datapath_by_uuid);
> @@ -286,8 +306,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, chassis_rec,
> + binding_rec, local_datapaths,
> + &lport_to_iface);
> <Mickey> There is no longer any change here, just reformatting that
> is not required.
>
Right.
> }
> }
> }
> diff --git a/ovn/controller/binding.h b/ovn/controller/binding.h
> index 8753d44..60d99db 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, struct hmap
> *patched_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..d90fb79
> --- /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;
> + const 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);
>
> <Mickey> Wondering if the VLOGs should have a rate limit on them.
>
I do not see rate limit on VLOGS of similar cases in the code. For example
VLOG_INFO("Claiming lport %s for this chassis.",..
Will leave it untouched for now.
> + 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);
> <Mickey> Another VLOG candidate for a rate limit.
>
> +
> + 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_datapath_binding *datapath)
> +{
> + struct filtered_dp *dp;
> +
> + dp = lookup_dp_by_key(datapath->tunnel_key);
> + if (dp) {
> + return;
> + }
> +
> + VLOG_INFO("Filter DP "UUID_FMT,
UUID_ARGS(&datapath->header_.uuid));
> <Mickey> Another VLOG candidate for a rate limit.
>
> + sbrec_port_binding_add_clause_datapath(ctx->ovnsb_idl,
> + OVSDB_F_EQ,
> + datapath);
> + sbrec_mac_binding_add_clause_datapath(ctx->ovnsb_idl,
> + OVSDB_F_EQ,
> + datapath);
> + sbrec_logical_flow_add_clause_logical_datapath(ctx->ovnsb_idl,
> + OVSDB_F_EQ,
> + datapath);
> + sbrec_multicast_group_add_clause_datapath(ctx->ovnsb_idl,
> + OVSDB_F_EQ,
> + datapath);
> +
> + dp = xmalloc(sizeof *dp);
> + dp->tunnel_key = datapath->tunnel_key;
> + dp->datapath = datapath;
> + hmap_insert(&filtered_dps, &dp->hmap_node, 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,
> <Mickey> Another VLOG candidate for a rate limit.
>
> + 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..e09c1c8
> --- /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_datapath_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_datapath_binding *datapath);
> +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 5c74186..1782561 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);
>
> @@ -387,6 +388,8 @@ main(int argc, char *argv[])
> ovsdb_idl_create(ovnsb_remote, &sbrec_idl_class, true, true));
> ovsdb_idl_omit_alert(ovnsb_idl_loop.idl,
&sbrec_chassis_col_nb_cfg);
>
> + filter_init(ovnsb_idl_loop.idl);
> +
> /* Track the southbound idl. */
> ovsdb_idl_track_add_all(ovnsb_idl_loop.idl);
>
> @@ -412,6 +415,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);
> }
> @@ -431,18 +435,21 @@ main(int argc, char *argv[])
> const char *chassis_id = get_chassis_id(ctx.ovs_idl);
>
> const struct sbrec_chassis *chassis = NULL;
> +
> + lport_index_fill(&lports, ctx.ovnsb_idl);
> + mcgroup_index_fill(&mcgroups, ctx.ovnsb_idl);
> + filter_mark_unused();
> +
> if (chassis_id) {
> chassis = 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,
> + &patched_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);
> + &patched_datapaths, &lports);
>
> enum mf_field_id mff_ovn_geneve = ofctrl_run(br_int);
>
> @@ -466,6 +473,7 @@ main(int argc, char *argv[])
> }
> }
>
> + 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..8eb0b23 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;
> };
> @@ -55,6 +56,7 @@ struct patched_datapath {
> bool local; /* 'True' if the datapath is for gateway router. */
> bool stale; /* 'True' if the datapath is not referenced by any
patch
> * port. */
> + int64_t tunnel_key;
> };
>
> struct patched_datapath *get_patched_datapath(const struct hmap *,
> diff --git a/ovn/controller/patch.c b/ovn/controller/patch.c
> index 707d08b..72e39c8 100644
> --- a/ovn/controller/patch.c
> +++ b/ovn/controller/patch.c
> @@ -24,6 +24,7 @@
> #include "openvswitch/hmap.h"
> #include "openvswitch/vlog.h"
> #include "ovn-controller.h"
> +#include "filter.h"
>
> VLOG_DEFINE_THIS_MODULE(patch);
>
> @@ -258,7 +259,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);
> @@ -273,9 +275,11 @@ add_patched_datapath(struct hmap
*patched_datapaths,
> pd->local = local;
> pd->key = xasprintf(UUID_FMT,
> UUID_ARGS(&binding_rec->datapath->header_.uuid));
> + pd->tunnel_key = binding_rec->datapath->tunnel_key;
> /* stale is set to false. */
> hmap_insert(patched_datapaths, &pd->hmap_node,
> binding_rec->datapath->tunnel_key);
> + filter_datapath(ctx, binding_rec->datapath);
> }
>
> static void
> @@ -292,7 +296,9 @@ add_logical_patch_ports_preprocess(struct hmap
> *patched_datapaths)
> /* This function should cleanup stale patched datapaths and any memory
> * allocated for fields within a stale patched datapath. */
> static void
> -add_logical_patch_ports_postprocess(struct hmap *patched_datapaths)
> +add_logical_patch_ports_postprocess(struct hmap *patched_datapaths,
> + struct hmap *local_datapaths,
> + struct controller_ctx *ctx)
> {
> /* Clean up stale patched datapaths. */
> struct patched_datapath *pd_cur_node, *pd_next_node;
> @@ -300,6 +306,9 @@ add_logical_patch_ports_postprocess(struct hmap
> *patched_datapaths)
> patched_datapaths) {
> if (pd_cur_node->stale == true) {
> hmap_remove(patched_datapaths, &pd_cur_node->hmap_node);
> + if (!get_local_datapath(local_datapaths,
> pd_cur_node->tunnel_key)) {
> + unfilter_datapath(ctx, pd_cur_node->tunnel_key);
> + }
> free(pd_cur_node->key);
> free(pd_cur_node);
> }
> @@ -333,7 +342,9 @@ add_logical_patch_ports(struct controller_ctx *ctx,
> const struct ovsrec_bridge *br_int,
> const char *local_chassis_id,
> struct shash *existing_ports,
> - struct hmap *patched_datapaths)
> + struct hmap *patched_datapaths,
> + struct hmap *local_datapaths,
> + struct lport_index *lports_index)
> {
> const struct sbrec_chassis *chassis_rec;
> chassis_rec = get_chassis(ctx->ovnsb_idl, local_chassis_id);
> @@ -368,21 +379,26 @@ add_logical_patch_ports(struct controller_ctx
*ctx,
> existing_ports);
> free(dst_name);
> free(src_name);
> - add_patched_datapath(patched_datapaths, binding,
local_port);
> + add_patched_datapath(patched_datapaths, binding,
> local_port, ctx);
> if (local_port) {
> if (binding->chassis != chassis_rec &&
ctx->ovnsb_idl_txn) {
> sbrec_port_binding_set_chassis(binding,
chassis_rec);
> }
> }
> + if (peer) {
> <Mickey> There is "if (!peer) { continue; }" a few lines up, so this
> "if (peer)" is redundant.
>
Right. Will remove the if statement.
Thanks for the review.
- Liran
More information about the dev
mailing list