diff mbox series

[ovs-dev,ovn,v8,2/2] Log missing bridge per localnet port just once

Message ID 20200519155816.24508-3-ihrachys@redhat.com
State Superseded, archived
Headers show
Series Support logical switches with multiple localnet ports | expand

Commit Message

Ihar Hrachyshka May 19, 2020, 3:58 p.m. UTC 
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: Dumitru Ceara <dceara@redhat.com>
Acked-by: Numan Siddique <numans@ovn.org>
---
 controller/ovn-controller.c |  2 ++
 controller/patch.c          | 36 ++++++++++++++++++++++++++++++++----
 controller/patch.h          |  2 ++
 3 files changed, 36 insertions(+), 4 deletions(-)

Comments

Numan Siddique May 20, 2020, 11:11 a.m. UTC | #1 
On Tue, May 19, 2020 at 9:28 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: Dumitru Ceara <dceara@redhat.com>
> Acked-by: Numan Siddique <numans@ovn.org>
> ---
>

Thanks Ihar for addressing the comments.

I applied both the patches to master.

After applying I realized that we need to add this "support for multiple
localnet ports"
in the NEWS file.

Can you please submit a follow up patch ?

Thanks
Numan

 controller/ovn-controller.c |  2 ++
>  controller/patch.c          | 36 ++++++++++++++++++++++++++++++++----
>  controller/patch.h          |  2 ++
>  3 files changed, 36 insertions(+), 4 deletions(-)
>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index c0a97a265..11fa2840d 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -1760,6 +1760,7 @@ main(int argc, char *argv[])
>
>      daemonize_complete();
>
> +    patch_init();
>      pinctrl_init();
>      lflow_init();
>
> @@ -2271,6 +2272,7 @@ main(int argc, char *argv[])
>      lflow_destroy();
>      ofctrl_destroy();
>      pinctrl_destroy();
> +    patch_destroy();
>
>      ovsdb_idl_loop_destroy(&ovs_idl_loop);
>      ovsdb_idl_loop_destroy(&ovnsb_idl_loop);
> diff --git a/controller/patch.c b/controller/patch.c
> index 7ad30d9cc..a2a7bcd79 100644
> --- a/controller/patch.c
> +++ b/controller/patch.c
> @@ -24,9 +24,25 @@
>  #include "openvswitch/hmap.h"
>  #include "openvswitch/vlog.h"
>  #include "ovn-controller.h"
> +#include "sset.h"
>
>  VLOG_DEFINE_THIS_MODULE(patch);
>
> +/* Contains list of physical bridges that were missing. */
> +static struct sset missed_bridges;
> +
> +void
> +patch_init(void)
> +{
> +    sset_init(&missed_bridges);
> +}
> +
> +void
> +patch_destroy(void)
> +{
> +    sset_destroy(&missed_bridges);
> +}
> +
>  static char *
>  patch_port_name(const char *src, const char *dst)
>  {
> @@ -223,20 +239,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);
> diff --git a/controller/patch.h b/controller/patch.h
> index 81a43bc2d..e470d502c 100644
> --- a/controller/patch.h
> +++ b/controller/patch.h
> @@ -43,5 +43,7 @@ void patch_run(struct ovsdb_idl_txn *ovs_idl_txn,
>                 const struct ovsrec_bridge *br_int,
>                 const struct sbrec_chassis *,
>                 const struct hmap *local_datapaths);
> +void patch_init(void);
> +void patch_destroy(void);
>
>  #endif /* controller/patch.h */
> --
> 2.26.2
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Ihar Hrachyshka May 20, 2020, 3:12 p.m. UTC | #2 
On 5/20/20 7:11 AM, Numan Siddique wrote:
> On Tue, May 19, 2020 at 9:28 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: Dumitru Ceara <dceara@redhat.com>
>> Acked-by: Numan Siddique <numans@ovn.org>
>> ---
>>
> Thanks Ihar for addressing the comments.
>
> I applied both the patches to master.
>
> After applying I realized that we need to add this "support for multiple
> localnet ports"
> in the NEWS file.
>
> Can you please submit a follow up patch ?


Done: 
https://patchwork.ozlabs.org/project/openvswitch/patch/20200520151058.712439-1-ihrachys@redhat.com/


>
> Thanks
> Numan
>
>   controller/ovn-controller.c |  2 ++
>>   controller/patch.c          | 36 ++++++++++++++++++++++++++++++++----
>>   controller/patch.h          |  2 ++
>>   3 files changed, 36 insertions(+), 4 deletions(-)
>>
>> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
>> index c0a97a265..11fa2840d 100644
>> --- a/controller/ovn-controller.c
>> +++ b/controller/ovn-controller.c
>> @@ -1760,6 +1760,7 @@ main(int argc, char *argv[])
>>
>>       daemonize_complete();
>>
>> +    patch_init();
>>       pinctrl_init();
>>       lflow_init();
>>
>> @@ -2271,6 +2272,7 @@ main(int argc, char *argv[])
>>       lflow_destroy();
>>       ofctrl_destroy();
>>       pinctrl_destroy();
>> +    patch_destroy();
>>
>>       ovsdb_idl_loop_destroy(&ovs_idl_loop);
>>       ovsdb_idl_loop_destroy(&ovnsb_idl_loop);
>> diff --git a/controller/patch.c b/controller/patch.c
>> index 7ad30d9cc..a2a7bcd79 100644
>> --- a/controller/patch.c
>> +++ b/controller/patch.c
>> @@ -24,9 +24,25 @@
>>   #include "openvswitch/hmap.h"
>>   #include "openvswitch/vlog.h"
>>   #include "ovn-controller.h"
>> +#include "sset.h"
>>
>>   VLOG_DEFINE_THIS_MODULE(patch);
>>
>> +/* Contains list of physical bridges that were missing. */
>> +static struct sset missed_bridges;
>> +
>> +void
>> +patch_init(void)
>> +{
>> +    sset_init(&missed_bridges);
>> +}
>> +
>> +void
>> +patch_destroy(void)
>> +{
>> +    sset_destroy(&missed_bridges);
>> +}
>> +
>>   static char *
>>   patch_port_name(const char *src, const char *dst)
>>   {
>> @@ -223,20 +239,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);
>> diff --git a/controller/patch.h b/controller/patch.h
>> index 81a43bc2d..e470d502c 100644
>> --- a/controller/patch.h
>> +++ b/controller/patch.h
>> @@ -43,5 +43,7 @@ void patch_run(struct ovsdb_idl_txn *ovs_idl_txn,
>>                  const struct ovsrec_bridge *br_int,
>>                  const struct sbrec_chassis *,
>>                  const struct hmap *local_datapaths);
>> +void patch_init(void);
>> +void patch_destroy(void);
>>
>>   #endif /* controller/patch.h */
>> --
>> 2.26.2
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>>
diff mbox series

Patch

diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index c0a97a265..11fa2840d 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -1760,6 +1760,7 @@  main(int argc, char *argv[])
 
     daemonize_complete();
 
+    patch_init();
     pinctrl_init();
     lflow_init();
 
@@ -2271,6 +2272,7 @@  main(int argc, char *argv[])
     lflow_destroy();
     ofctrl_destroy();
     pinctrl_destroy();
+    patch_destroy();
 
     ovsdb_idl_loop_destroy(&ovs_idl_loop);
     ovsdb_idl_loop_destroy(&ovnsb_idl_loop);
diff --git a/controller/patch.c b/controller/patch.c
index 7ad30d9cc..a2a7bcd79 100644
--- a/controller/patch.c
+++ b/controller/patch.c
@@ -24,9 +24,25 @@ 
 #include "openvswitch/hmap.h"
 #include "openvswitch/vlog.h"
 #include "ovn-controller.h"
+#include "sset.h"
 
 VLOG_DEFINE_THIS_MODULE(patch);
 
+/* Contains list of physical bridges that were missing. */
+static struct sset missed_bridges;
+
+void
+patch_init(void)
+{
+    sset_init(&missed_bridges);
+}
+
+void
+patch_destroy(void)
+{
+    sset_destroy(&missed_bridges);
+}
+
 static char *
 patch_port_name(const char *src, const char *dst)
 {
@@ -223,20 +239,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);
diff --git a/controller/patch.h b/controller/patch.h
index 81a43bc2d..e470d502c 100644
--- a/controller/patch.h
+++ b/controller/patch.h
@@ -43,5 +43,7 @@  void patch_run(struct ovsdb_idl_txn *ovs_idl_txn,
                const struct ovsrec_bridge *br_int,
                const struct sbrec_chassis *,
                const struct hmap *local_datapaths);
+void patch_init(void);
+void patch_destroy(void);
 
 #endif /* controller/patch.h */