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 |
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 >
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 > > > >
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 >
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 --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; }
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(+)