[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