[ovs-dev] [PATCH ovn v5 6/6] Log missing bridge per localnet port just once

[Dumitru Ceara]

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?

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?

> +                    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.

Regards,
Dumitru

>          const char *patch_port_id;
>          if (is_localnet) {
>