| Message ID | 20200513133853.116959-7-ihrachys@redhat.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Support logical switches with multiple localnet ports | expand |
On Wed, May 13, 2020 at 7:10 PM Ihar Hrachyshka <ihrachys@redhat.com> wrote: > Having some localnet ports missing a bridge device on a particular > chassis is a supported configuration (e.g. used to implement "routed > provider networks" for OpenStack) and should not flood logs with > duplicate messages. > > Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com> > Acked-by: Numan Siddique <numans@ovn.org> I've one comment. Please see below. > --- > controller/patch.c | 23 +++++++++++++++++++---- > 1 file changed, 19 insertions(+), 4 deletions(-) > > diff --git a/controller/patch.c b/controller/patch.c > index 7ad30d9cc..c41c34d78 100644 > --- a/controller/patch.c > +++ b/controller/patch.c > @@ -24,6 +24,7 @@ > #include "openvswitch/hmap.h" > #include "openvswitch/vlog.h" > #include "ovn-controller.h" > +#include "sset.h" > > VLOG_DEFINE_THIS_MODULE(patch); > > @@ -184,6 +185,8 @@ add_bridge_mappings(struct ovsdb_idl_txn *ovs_idl_txn, > const struct sbrec_chassis *chassis, > const struct hmap *local_datapaths) > { > + static struct sset missed_bridges = SSET_INITIALIZER(&missed_bridges); > The sset missed_bridges is not destroyed at all. Which is ok. since 'missed_bridges' is static and allocated memory will eventually be freed when ovn-controller exits. But will valgrind complain about the memory leak ? Is it worth making this file scope and adding patch_init(), patch_destroy() functions which will init the sset and destroy it. Any thoughts ? Thanks Numan > + > /* Get ovn-bridge-mappings. */ > struct shash bridge_mappings = SHASH_INITIALIZER(&bridge_mappings); > > @@ -223,20 +226,32 @@ add_bridge_mappings(struct ovsdb_idl_txn > *ovs_idl_txn, > binding->type, binding->logical_port); > continue; > } > + char *msg_key = xasprintf("%s;%s", binding->logical_port, > network); > struct ovsrec_bridge *br_ln = shash_find_data(&bridge_mappings, > network); > if (!br_ln) { > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > if (!is_localnet) { > + static struct vlog_rate_limit rl = > VLOG_RATE_LIMIT_INIT(5, 1); > VLOG_ERR_RL(&rl, "bridge not found for %s port '%s' " > "with network name '%s'", > binding->type, binding->logical_port, network); > } else { > - VLOG_INFO_RL(&rl, "bridge not found for localnet port > '%s' " > - "with network name '%s'; skipping", > - binding->logical_port, network); > + /* Since having localnet ports that are not mapped on some > + * chassis is a supported configuration used to implement > + * multisegment switches with fabric L3 routing between > + * segments, log the following message once per run but > don't > + * unnecessarily pollute the log file. */ > + if (!sset_contains(&missed_bridges, msg_key)) { > + VLOG_INFO("bridge not found for localnet port '%s' > with " > + "network name '%s'; skipping", > + binding->logical_port, network); > + sset_add(&missed_bridges, msg_key); > + } > } > + free(msg_key); > continue; > } > + sset_find_and_delete(&missed_bridges, msg_key); > + free(msg_key); > > char *name1 = patch_port_name(br_int->name, > binding->logical_port); > char *name2 = patch_port_name(binding->logical_port, > br_int->name); > -- > 2.26.2 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
diff --git a/controller/patch.c b/controller/patch.c index 7ad30d9cc..c41c34d78 100644 --- a/controller/patch.c +++ b/controller/patch.c @@ -24,6 +24,7 @@ #include "openvswitch/hmap.h" #include "openvswitch/vlog.h" #include "ovn-controller.h" +#include "sset.h" VLOG_DEFINE_THIS_MODULE(patch); @@ -184,6 +185,8 @@ add_bridge_mappings(struct ovsdb_idl_txn *ovs_idl_txn, const struct sbrec_chassis *chassis, const struct hmap *local_datapaths) { + static struct sset missed_bridges = SSET_INITIALIZER(&missed_bridges); + /* Get ovn-bridge-mappings. */ struct shash bridge_mappings = SHASH_INITIALIZER(&bridge_mappings); @@ -223,20 +226,32 @@ add_bridge_mappings(struct ovsdb_idl_txn *ovs_idl_txn, binding->type, binding->logical_port); continue; } + char *msg_key = xasprintf("%s;%s", binding->logical_port, network); struct ovsrec_bridge *br_ln = shash_find_data(&bridge_mappings, network); if (!br_ln) { - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); if (!is_localnet) { + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); VLOG_ERR_RL(&rl, "bridge not found for %s port '%s' " "with network name '%s'", binding->type, binding->logical_port, network); } else { - VLOG_INFO_RL(&rl, "bridge not found for localnet port '%s' " - "with network name '%s'; skipping", - binding->logical_port, network); + /* Since having localnet ports that are not mapped on some + * chassis is a supported configuration used to implement + * multisegment switches with fabric L3 routing between + * segments, log the following message once per run but don't + * unnecessarily pollute the log file. */ + if (!sset_contains(&missed_bridges, msg_key)) { + VLOG_INFO("bridge not found for localnet port '%s' with " + "network name '%s'; skipping", + binding->logical_port, network); + sset_add(&missed_bridges, msg_key); + } } + free(msg_key); continue; } + sset_find_and_delete(&missed_bridges, msg_key); + free(msg_key); char *name1 = patch_port_name(br_int->name, binding->logical_port); char *name2 = patch_port_name(binding->logical_port, br_int->name);
Having some localnet ports missing a bridge device on a particular chassis is a supported configuration (e.g. used to implement "routed provider networks" for OpenStack) and should not flood logs with duplicate messages. Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com> --- controller/patch.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)