[ovs-dev] [PATCH ovn v5 6/6] Log missing bridge per localnet port just once
Ihar Hrachyshka
ihrachys at redhat.com
Tue May 12 16:52:47 UTC 2020
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 <ihrachys at redhat.com>
>> ---
>> 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).
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.
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.
Thanks again, appreciate all the comments!
Ihar
More information about the dev
mailing list