diff mbox series

[ovs-dev,ovn,v1,2/2] northd: Log all dynamic address assignments

Message ID 20191208041143.374689-2-russell@ovn.org
State Accepted
Headers show
Series [ovs-dev,ovn,v1,1/2] tests: Updated expected log message | expand

Commit Message

Russell Bryant Dec. 8, 2019, 4:11 a.m. UTC 
This patch adds INFO level log messages for all dynamic address
assignments (MAC, IPv4, IPv6).  While debugging some issues in
ovn-kubernetes, I found it would be helpful to see ovn-northd's view
of what addresses were assigned where and when from its perspective.

Signed-off-by: Russell Bryant <russell@ovn.org>
---
 northd/ovn-northd.c | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Dumitru Ceara Dec. 9, 2019, 8 a.m. UTC | #1 
On Sun, Dec 8, 2019 at 5:12 AM Russell Bryant <russell@ovn.org> wrote:
>
> This patch adds INFO level log messages for all dynamic address
> assignments (MAC, IPv4, IPv6).  While debugging some issues in
> ovn-kubernetes, I found it would be helpful to see ovn-northd's view
> of what addresses were assigned where and when from its perspective.
>

Hi Russsell,

While I agree that having this information is really useful for
debugging, the INFO logs are enabled by default.
Should we consider rate limiting the logs you added?

For example, looking at the WARN logs in northd, all of them are rate limited.

Thanks,
Dumitru

> Signed-off-by: Russell Bryant <russell@ovn.org>
> ---
>  northd/ovn-northd.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index f0847d81e..33d3ff2ad 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -1714,6 +1714,8 @@ update_dynamic_addresses(struct dynamic_address_update *update)
>          break;
>      case DYNAMIC:
>          ip4 = htonl(ipam_get_unused_ip(update->od));
> +        VLOG_INFO("Assigned dynamic IPv4 address '"IP_FMT"' to port '%s'",
> +                  IP_ARGS(ip4), update->op->nbsp->name);
>      }
>
>      struct eth_addr mac;
> @@ -1728,6 +1730,8 @@ update_dynamic_addresses(struct dynamic_address_update *update)
>          break;
>      case DYNAMIC:
>          eth_addr_from_uint64(ipam_get_unused_mac(ip4), &mac);
> +        VLOG_INFO("Assigned dynamic MAC address '"ETH_ADDR_FMT"' to port '%s'",
> +                  ETH_ADDR_ARGS(mac), update->op->nbsp->name);
>          break;
>      }
>
> @@ -1745,6 +1749,11 @@ update_dynamic_addresses(struct dynamic_address_update *update)
>          break;
>      case DYNAMIC:
>          in6_generate_eui64(mac, &update->od->ipam_info.ipv6_prefix, &ip6);
> +        struct ds ip6_ds = DS_EMPTY_INITIALIZER;
> +        ipv6_format_addr(&ip6, &ip6_ds);
> +        VLOG_INFO("Assigned dynamic IPv6 address '%s' to port '%s'",
> +                  ip6_ds.string, update->op->nbsp->name);
> +        ds_destroy(&ip6_ds);
>          break;
>      }
>
> --
> 2.23.0
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Russell Bryant Dec. 9, 2019, 1:16 p.m. UTC | #2 
On Mon, Dec 9, 2019 at 3:01 AM Dumitru Ceara <dceara@redhat.com> wrote:

> On Sun, Dec 8, 2019 at 5:12 AM Russell Bryant <russell@ovn.org> wrote:
> >
> > This patch adds INFO level log messages for all dynamic address
> > assignments (MAC, IPv4, IPv6).  While debugging some issues in
> > ovn-kubernetes, I found it would be helpful to see ovn-northd's view
> > of what addresses were assigned where and when from its perspective.
> >
>
> Hi Russsell,
>
> While I agree that having this information is really useful for
> debugging, the INFO logs are enabled by default.
> Should we consider rate limiting the logs you added?
>
> For example, looking at the WARN logs in northd, all of them are rate
> limited.
>

We could ... it'd be a little bit extra tracking that would hopefully never
be needed.  It'd be a bug if the same message was emitted more than once at
all.

>
> Thanks,
> Dumitru
>
> > Signed-off-by: Russell Bryant <russell@ovn.org>
> > ---
> >  northd/ovn-northd.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > index f0847d81e..33d3ff2ad 100644
> > --- a/northd/ovn-northd.c
> > +++ b/northd/ovn-northd.c
> > @@ -1714,6 +1714,8 @@ update_dynamic_addresses(struct
> dynamic_address_update *update)
> >          break;
> >      case DYNAMIC:
> >          ip4 = htonl(ipam_get_unused_ip(update->od));
> > +        VLOG_INFO("Assigned dynamic IPv4 address '"IP_FMT"' to port
> '%s'",
> > +                  IP_ARGS(ip4), update->op->nbsp->name);
> >      }
> >
> >      struct eth_addr mac;
> > @@ -1728,6 +1730,8 @@ update_dynamic_addresses(struct
> dynamic_address_update *update)
> >          break;
> >      case DYNAMIC:
> >          eth_addr_from_uint64(ipam_get_unused_mac(ip4), &mac);
> > +        VLOG_INFO("Assigned dynamic MAC address '"ETH_ADDR_FMT"' to
> port '%s'",
> > +                  ETH_ADDR_ARGS(mac), update->op->nbsp->name);
> >          break;
> >      }
> >
> > @@ -1745,6 +1749,11 @@ update_dynamic_addresses(struct
> dynamic_address_update *update)
> >          break;
> >      case DYNAMIC:
> >          in6_generate_eui64(mac, &update->od->ipam_info.ipv6_prefix,
> &ip6);
> > +        struct ds ip6_ds = DS_EMPTY_INITIALIZER;
> > +        ipv6_format_addr(&ip6, &ip6_ds);
> > +        VLOG_INFO("Assigned dynamic IPv6 address '%s' to port '%s'",
> > +                  ip6_ds.string, update->op->nbsp->name);
> > +        ds_destroy(&ip6_ds);
> >          break;
> >      }
> >
> > --
> > 2.23.0
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>
>
Numan Siddique Dec. 9, 2019, 4:18 p.m. UTC | #3 
On Mon, Dec 9, 2019 at 8:17 AM Russell Bryant <rbryant@redhat.com> wrote:
>
> On Mon, Dec 9, 2019 at 3:01 AM Dumitru Ceara <dceara@redhat.com> wrote:
>
> > On Sun, Dec 8, 2019 at 5:12 AM Russell Bryant <russell@ovn.org> wrote:
> > >
> > > This patch adds INFO level log messages for all dynamic address
> > > assignments (MAC, IPv4, IPv6).  While debugging some issues in
> > > ovn-kubernetes, I found it would be helpful to see ovn-northd's view
> > > of what addresses were assigned where and when from its perspective.
> > >
> >
> > Hi Russsell,
> >
> > While I agree that having this information is really useful for
> > debugging, the INFO logs are enabled by default.
> > Should we consider rate limiting the logs you added?
> >
> > For example, looking at the WARN logs in northd, all of them are rate
> > limited.
> >
>
> We could ... it'd be a little bit extra tracking that would hopefully never
> be needed.  It'd be a bug if the same message was emitted more than once at
> all.
>

Since this code would not hit all the time when ovn_db_run is called, I think
VLOG_INFO should not cause any log flooding.


Acked-by: Numan Siddique <numans@ovn.org>.

Thanks
Numan

> >
> > Thanks,
> > Dumitru
> >
> > > Signed-off-by: Russell Bryant <russell@ovn.org>
> > > ---
> > >  northd/ovn-northd.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > >
> > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > > index f0847d81e..33d3ff2ad 100644
> > > --- a/northd/ovn-northd.c
> > > +++ b/northd/ovn-northd.c
> > > @@ -1714,6 +1714,8 @@ update_dynamic_addresses(struct
> > dynamic_address_update *update)
> > >          break;
> > >      case DYNAMIC:
> > >          ip4 = htonl(ipam_get_unused_ip(update->od));
> > > +        VLOG_INFO("Assigned dynamic IPv4 address '"IP_FMT"' to port
> > '%s'",
> > > +                  IP_ARGS(ip4), update->op->nbsp->name);
> > >      }
> > >
> > >      struct eth_addr mac;
> > > @@ -1728,6 +1730,8 @@ update_dynamic_addresses(struct
> > dynamic_address_update *update)
> > >          break;
> > >      case DYNAMIC:
> > >          eth_addr_from_uint64(ipam_get_unused_mac(ip4), &mac);
> > > +        VLOG_INFO("Assigned dynamic MAC address '"ETH_ADDR_FMT"' to
> > port '%s'",
> > > +                  ETH_ADDR_ARGS(mac), update->op->nbsp->name);
> > >          break;
> > >      }
> > >
> > > @@ -1745,6 +1749,11 @@ update_dynamic_addresses(struct
> > dynamic_address_update *update)
> > >          break;
> > >      case DYNAMIC:
> > >          in6_generate_eui64(mac, &update->od->ipam_info.ipv6_prefix,
> > &ip6);
> > > +        struct ds ip6_ds = DS_EMPTY_INITIALIZER;
> > > +        ipv6_format_addr(&ip6, &ip6_ds);
> > > +        VLOG_INFO("Assigned dynamic IPv6 address '%s' to port '%s'",
> > > +                  ip6_ds.string, update->op->nbsp->name);
> > > +        ds_destroy(&ip6_ds);
> > >          break;
> > >      }
> > >
> > > --
> > > 2.23.0
> > >
> > > _______________________________________________
> > > dev mailing list
> > > dev@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >
> >
> >
>
> --
> Russell Bryant
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Russell Bryant Dec. 9, 2019, 4:22 p.m. UTC | #4 
On Mon, Dec 9, 2019 at 11:18 AM Numan Siddique <numans@ovn.org> wrote:

> On Mon, Dec 9, 2019 at 8:17 AM Russell Bryant <rbryant@redhat.com> wrote:
> >
> > On Mon, Dec 9, 2019 at 3:01 AM Dumitru Ceara <dceara@redhat.com> wrote:
> >
> > > On Sun, Dec 8, 2019 at 5:12 AM Russell Bryant <russell@ovn.org> wrote:
> > > >
> > > > This patch adds INFO level log messages for all dynamic address
> > > > assignments (MAC, IPv4, IPv6).  While debugging some issues in
> > > > ovn-kubernetes, I found it would be helpful to see ovn-northd's view
> > > > of what addresses were assigned where and when from its perspective.
> > > >
> > >
> > > Hi Russsell,
> > >
> > > While I agree that having this information is really useful for
> > > debugging, the INFO logs are enabled by default.
> > > Should we consider rate limiting the logs you added?
> > >
> > > For example, looking at the WARN logs in northd, all of them are rate
> > > limited.
> > >
> >
> > We could ... it'd be a little bit extra tracking that would hopefully
> never
> > be needed.  It'd be a bug if the same message was emitted more than once
> at
> > all.
> >
>
> Since this code would not hit all the time when ovn_db_run is called, I
> think
> VLOG_INFO should not cause any log flooding.
>
>
> Acked-by: Numan Siddique <numans@ovn.org>.
>

Thanks, I've pushed this to master.


>
> Thanks
> Numan
>
> > >
> > > Thanks,
> > > Dumitru
> > >
> > > > Signed-off-by: Russell Bryant <russell@ovn.org>
> > > > ---
> > > >  northd/ovn-northd.c | 9 +++++++++
> > > >  1 file changed, 9 insertions(+)
> > > >
> > > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > > > index f0847d81e..33d3ff2ad 100644
> > > > --- a/northd/ovn-northd.c
> > > > +++ b/northd/ovn-northd.c
> > > > @@ -1714,6 +1714,8 @@ update_dynamic_addresses(struct
> > > dynamic_address_update *update)
> > > >          break;
> > > >      case DYNAMIC:
> > > >          ip4 = htonl(ipam_get_unused_ip(update->od));
> > > > +        VLOG_INFO("Assigned dynamic IPv4 address '"IP_FMT"' to port
> > > '%s'",
> > > > +                  IP_ARGS(ip4), update->op->nbsp->name);
> > > >      }
> > > >
> > > >      struct eth_addr mac;
> > > > @@ -1728,6 +1730,8 @@ update_dynamic_addresses(struct
> > > dynamic_address_update *update)
> > > >          break;
> > > >      case DYNAMIC:
> > > >          eth_addr_from_uint64(ipam_get_unused_mac(ip4), &mac);
> > > > +        VLOG_INFO("Assigned dynamic MAC address '"ETH_ADDR_FMT"' to
> > > port '%s'",
> > > > +                  ETH_ADDR_ARGS(mac), update->op->nbsp->name);
> > > >          break;
> > > >      }
> > > >
> > > > @@ -1745,6 +1749,11 @@ update_dynamic_addresses(struct
> > > dynamic_address_update *update)
> > > >          break;
> > > >      case DYNAMIC:
> > > >          in6_generate_eui64(mac, &update->od->ipam_info.ipv6_prefix,
> > > &ip6);
> > > > +        struct ds ip6_ds = DS_EMPTY_INITIALIZER;
> > > > +        ipv6_format_addr(&ip6, &ip6_ds);
> > > > +        VLOG_INFO("Assigned dynamic IPv6 address '%s' to port '%s'",
> > > > +                  ip6_ds.string, update->op->nbsp->name);
> > > > +        ds_destroy(&ip6_ds);
> > > >          break;
> > > >      }
> > > >
> > > > --
> > > > 2.23.0
> > > >
> > > > _______________________________________________
> > > > dev mailing list
> > > > dev@openvswitch.org
> > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > > >
> > >
> > >
> >
> > --
> > Russell Bryant
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>
>
diff mbox series

Patch

diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index f0847d81e..33d3ff2ad 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -1714,6 +1714,8 @@  update_dynamic_addresses(struct dynamic_address_update *update)
         break;
     case DYNAMIC:
         ip4 = htonl(ipam_get_unused_ip(update->od));
+        VLOG_INFO("Assigned dynamic IPv4 address '"IP_FMT"' to port '%s'",
+                  IP_ARGS(ip4), update->op->nbsp->name);
     }
 
     struct eth_addr mac;
@@ -1728,6 +1730,8 @@  update_dynamic_addresses(struct dynamic_address_update *update)
         break;
     case DYNAMIC:
         eth_addr_from_uint64(ipam_get_unused_mac(ip4), &mac);
+        VLOG_INFO("Assigned dynamic MAC address '"ETH_ADDR_FMT"' to port '%s'",
+                  ETH_ADDR_ARGS(mac), update->op->nbsp->name);
         break;
     }
 
@@ -1745,6 +1749,11 @@  update_dynamic_addresses(struct dynamic_address_update *update)
         break;
     case DYNAMIC:
         in6_generate_eui64(mac, &update->od->ipam_info.ipv6_prefix, &ip6);
+        struct ds ip6_ds = DS_EMPTY_INITIALIZER;
+        ipv6_format_addr(&ip6, &ip6_ds);
+        VLOG_INFO("Assigned dynamic IPv6 address '%s' to port '%s'",
+                  ip6_ds.string, update->op->nbsp->name);
+        ds_destroy(&ip6_ds);
         break;
     }