On 5/12/20 6:52 PM, Ihar Hrachyshka wrote: > Hi Dumitru, > > thanks a lot for attentive review. I'm pushing another version of the > series with your comments addressed. Some answers inline. > > > On 5/12/20 10:30 AM, Dumitru Ceara wrote: >> On 5/11/20 7:00 PM, Ihar Hrachyshka 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 <[email protected]> >>> --- >>> controller/patch.c | 16 ++++++++++++---- >>> 1 file changed, 12 insertions(+), 4 deletions(-) >>> >>> diff --git a/controller/patch.c b/controller/patch.c >>> index 52255cc3a..2a757bb00 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); >>> @@ -220,20 +223,25 @@ 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); >>> + 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); >> Shouldn't we rate limit this log message too? > > > The idea behind the set holding all keys for all unwired localnet ports > is to have a single info message reported per controller run. This > function is called from under the main process loop trying to reconcile > database with network stack configuration. It makes sense to warn a user > over and over (and hence also rate limit) when we detect an invalid > configuration. But having multiple localnet ports per switch, some that > belong to unmapped / unwired networks is a legal use case and not > misconfiguration (since this series), so the set is used to guarantee > that a user will see the info message once in controller lifetime. Hence > no need for explicit rate limit. > > > Side note that there *is* a scenario where several messages for the same > localnet port will be reported. This can happen when a port was unwired > (hence the first message logged), then it got wired (so the main loop > reconciled the database and wired it, also clearing the msg_key from the > set), then it got unwired again (the message is logged again since the > set doesn't contain msg_key). While the code as written handles the > scenario, I am not sure if it's a valid scenario to care about. Still, I > make some effort here to handle it. > > >> Also, I'm a bit confused about how this should work: the >> "<logical-port>;<network>" msg_key will generate unique values for each >> port binding. So, won't the condition above always be true? > > > As explained in more words above, it will not be true on all executions > except the very first one (note the set is static hence survives > returning from the function). >
My bad, I had missed the "static" completely. > > I've added an inline comment to explain this. > > >>> + sset_add(&missed_bridges, msg_key); >>> + } >>> }> continue; >>> } >>> + sset_find_and_delete(&missed_bridges, msg_key); >>> >> We need to free msg_key in some cases, e.g., if is_localnet == false. > > > Actually, it should be always freed since adding a key to a set clones it. > Right. > > I am sorry for all the embarrassing memory leaks I left, I have to be a > lot more cautious about it. In my pitty excuse, this is what using gc > languages for years makes to you. Oh well. > No problem :) > > Thanks again, appreciate all the comments! > > Ihar > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
