| Message ID | 20210923114810.25300-1-david.marchand@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] ofproto/bond: Improve admissibility debug readability. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
On 23/09/2021 12:48, David Marchand wrote: > The admissibility check currently log a message like (line wrapped in > this commitlog): > bond(revalidator11)|DBG|member (dpdk0): Admissibility > verdict is to drop pkt as different port is learned.active member: false, > may_enable: true enable: true LACP status:2 > > Fix spaces around the period character and separate debug infos with > commas. > Prefix all log messages in this check with bond and member names. > Display a human readable string for LACP status. > > New logs look like: > bond(revalidator11)|DBG|bond dpdkbond0: member dpdk0: admissibility > verdict is to drop pkt as different port is learned, active member: false, > may_enable: true, enable: true, LACP status: off > LGTM. One small nit below as you are touching the log anyway but not sure it's worth a respin. Either way, Acked-by: Kevin Traynor <ktraynor@redhat.com> > Signed-off-by: David Marchand <david.marchand@redhat.com> > --- > lib/lacp.c | 14 ++++++++++++++ > lib/lacp.h | 1 + > ofproto/bond.c | 39 +++++++++++++-------------------------- > 3 files changed, 28 insertions(+), 26 deletions(-) > > diff --git a/lib/lacp.c b/lib/lacp.c > index 540b2aa8ca..89d711225f 100644 > --- a/lib/lacp.c > +++ b/lib/lacp.c > @@ -429,6 +429,20 @@ lacp_status(const struct lacp *lacp) OVS_EXCLUDED(mutex) > } > } > > +const char *lacp_status_description(enum lacp_status lacp_status) > +{ > + switch (lacp_status) { > + case LACP_NEGOTIATED: > + return "negotiated"; > + case LACP_CONFIGURED: > + return "configured"; > + case LACP_DISABLED: > + return "off"; > + default: > + return "<unknown>"; > + } > +} > + > /* Registers 'member_' as subordinate to 'lacp'. This should be called at > * least once per member in a LACP managed bond. Should also be called > * whenever a member's settings change. */ > diff --git a/lib/lacp.h b/lib/lacp.h > index 908ec201c6..1ca06f762b 100644 > --- a/lib/lacp.h > +++ b/lib/lacp.h > @@ -49,6 +49,7 @@ bool lacp_is_active(const struct lacp *); > bool lacp_process_packet(struct lacp *, const void *member, > const struct dp_packet *packet); > enum lacp_status lacp_status(const struct lacp *); > +const char *lacp_status_description(enum lacp_status); > > struct lacp_member_settings { > char *name; /* Name (for debugging). */ > diff --git a/ofproto/bond.c b/ofproto/bond.c > index 2dcfeda717..f0fa1f95b0 100644 > --- a/ofproto/bond.c > +++ b/ofproto/bond.c > @@ -876,7 +876,7 @@ bond_check_admissibility(struct bond *bond, const void *member_, > if (!member->enabled && member->may_enable) { > VLOG_DBG_RL(&rl, "bond %s: member %s: " > "main thread has not yet enabled member", > - bond->name, bond->active_member->name); > + bond->name, bond->active_member->name); > } > goto out; > case LACP_CONFIGURED: > @@ -913,9 +913,9 @@ bond_check_admissibility(struct bond *bond, const void *member_, > /* Drop all packets which arrive on backup members. This is similar to > * how Linux bonding handles active-backup bonds. */ > if (bond->active_member != member) { > - VLOG_DBG_RL(&rl, "active-backup bond received packet on backup" > - " member (%s) destined for " ETH_ADDR_FMT, > - member->name, ETH_ADDR_ARGS(eth_dst)); > + VLOG_DBG_RL(&rl, "bond %s: member %s: active-backup bond received " > + "packet on backup member destined for " ETH_ADDR_FMT, > + bond->name, member->name, ETH_ADDR_ARGS(eth_dst)); > goto out; > } > verdict = BV_ACCEPT; > @@ -935,17 +935,17 @@ bond_check_admissibility(struct bond *bond, const void *member_, > OVS_NOT_REACHED(); > out: > if (member && (verdict != BV_ACCEPT)) { > - VLOG_DBG_RL(&rl, "member (%s): " > - "Admissibility verdict is to drop pkt %s." > - "active member: %s, may_enable: %s enable: %s " > - "LACP status:%d", > - member->name, > + VLOG_DBG_RL(&rl, "bond %s: member %s: " > + "admissibility verdict is to drop pkt%s, " > + "active member: %s, may_enable: %s, enable: %s, " "enabled:" would read better than "enable:" > + "LACP status: %s", > + bond->name, member->name, > (verdict == BV_DROP_IF_MOVED) ? > - "as different port is learned" : "", > + " as different port is learned" : "", > (bond->active_member == member) ? "true" : "false", > member->may_enable ? "true" : "false", > member->enabled ? "true" : "false", > - bond->lacp_status); > + lacp_status_description(bond->lacp_status)); > } > > ovs_rwlock_unlock(&rwlock); > @@ -1503,21 +1503,8 @@ bond_print_details(struct ds *ds, const struct bond *bond) > bond->next_rebalance - time_msec()); > } > > - ds_put_cstr(ds, "lacp_status: "); > - switch (bond->lacp_status) { > - case LACP_NEGOTIATED: > - ds_put_cstr(ds, "negotiated\n"); > - break; > - case LACP_CONFIGURED: > - ds_put_cstr(ds, "configured\n"); > - break; > - case LACP_DISABLED: > - ds_put_cstr(ds, "off\n"); > - break; > - default: > - ds_put_cstr(ds, "<unknown>\n"); > - break; > - } > + ds_put_format(ds, "lacp_status: %s\n", > + lacp_status_description(bond->lacp_status)); > > ds_put_format(ds, "lacp_fallback_ab: %s\n", > bond->lacp_fallback_ab ? "true" : "false"); >
diff --git a/lib/lacp.c b/lib/lacp.c index 540b2aa8ca..89d711225f 100644 --- a/lib/lacp.c +++ b/lib/lacp.c @@ -429,6 +429,20 @@ lacp_status(const struct lacp *lacp) OVS_EXCLUDED(mutex) } } +const char *lacp_status_description(enum lacp_status lacp_status) +{ + switch (lacp_status) { + case LACP_NEGOTIATED: + return "negotiated"; + case LACP_CONFIGURED: + return "configured"; + case LACP_DISABLED: + return "off"; + default: + return "<unknown>"; + } +} + /* Registers 'member_' as subordinate to 'lacp'. This should be called at * least once per member in a LACP managed bond. Should also be called * whenever a member's settings change. */ diff --git a/lib/lacp.h b/lib/lacp.h index 908ec201c6..1ca06f762b 100644 --- a/lib/lacp.h +++ b/lib/lacp.h @@ -49,6 +49,7 @@ bool lacp_is_active(const struct lacp *); bool lacp_process_packet(struct lacp *, const void *member, const struct dp_packet *packet); enum lacp_status lacp_status(const struct lacp *); +const char *lacp_status_description(enum lacp_status); struct lacp_member_settings { char *name; /* Name (for debugging). */ diff --git a/ofproto/bond.c b/ofproto/bond.c index 2dcfeda717..f0fa1f95b0 100644 --- a/ofproto/bond.c +++ b/ofproto/bond.c @@ -876,7 +876,7 @@ bond_check_admissibility(struct bond *bond, const void *member_, if (!member->enabled && member->may_enable) { VLOG_DBG_RL(&rl, "bond %s: member %s: " "main thread has not yet enabled member", - bond->name, bond->active_member->name); + bond->name, bond->active_member->name); } goto out; case LACP_CONFIGURED: @@ -913,9 +913,9 @@ bond_check_admissibility(struct bond *bond, const void *member_, /* Drop all packets which arrive on backup members. This is similar to * how Linux bonding handles active-backup bonds. */ if (bond->active_member != member) { - VLOG_DBG_RL(&rl, "active-backup bond received packet on backup" - " member (%s) destined for " ETH_ADDR_FMT, - member->name, ETH_ADDR_ARGS(eth_dst)); + VLOG_DBG_RL(&rl, "bond %s: member %s: active-backup bond received " + "packet on backup member destined for " ETH_ADDR_FMT, + bond->name, member->name, ETH_ADDR_ARGS(eth_dst)); goto out; } verdict = BV_ACCEPT; @@ -935,17 +935,17 @@ bond_check_admissibility(struct bond *bond, const void *member_, OVS_NOT_REACHED(); out: if (member && (verdict != BV_ACCEPT)) { - VLOG_DBG_RL(&rl, "member (%s): " - "Admissibility verdict is to drop pkt %s." - "active member: %s, may_enable: %s enable: %s " - "LACP status:%d", - member->name, + VLOG_DBG_RL(&rl, "bond %s: member %s: " + "admissibility verdict is to drop pkt%s, " + "active member: %s, may_enable: %s, enable: %s, " + "LACP status: %s", + bond->name, member->name, (verdict == BV_DROP_IF_MOVED) ? - "as different port is learned" : "", + " as different port is learned" : "", (bond->active_member == member) ? "true" : "false", member->may_enable ? "true" : "false", member->enabled ? "true" : "false", - bond->lacp_status); + lacp_status_description(bond->lacp_status)); } ovs_rwlock_unlock(&rwlock); @@ -1503,21 +1503,8 @@ bond_print_details(struct ds *ds, const struct bond *bond) bond->next_rebalance - time_msec()); } - ds_put_cstr(ds, "lacp_status: "); - switch (bond->lacp_status) { - case LACP_NEGOTIATED: - ds_put_cstr(ds, "negotiated\n"); - break; - case LACP_CONFIGURED: - ds_put_cstr(ds, "configured\n"); - break; - case LACP_DISABLED: - ds_put_cstr(ds, "off\n"); - break; - default: - ds_put_cstr(ds, "<unknown>\n"); - break; - } + ds_put_format(ds, "lacp_status: %s\n", + lacp_status_description(bond->lacp_status)); ds_put_format(ds, "lacp_fallback_ab: %s\n", bond->lacp_fallback_ab ? "true" : "false");
The admissibility check currently log a message like (line wrapped in this commitlog): bond(revalidator11)|DBG|member (dpdk0): Admissibility verdict is to drop pkt as different port is learned.active member: false, may_enable: true enable: true LACP status:2 Fix spaces around the period character and separate debug infos with commas. Prefix all log messages in this check with bond and member names. Display a human readable string for LACP status. New logs look like: bond(revalidator11)|DBG|bond dpdkbond0: member dpdk0: admissibility verdict is to drop pkt as different port is learned, active member: false, may_enable: true, enable: true, LACP status: off Signed-off-by: David Marchand <david.marchand@redhat.com> --- lib/lacp.c | 14 ++++++++++++++ lib/lacp.h | 1 + ofproto/bond.c | 39 +++++++++++++-------------------------- 3 files changed, 28 insertions(+), 26 deletions(-)