[ovs-dev] [PATCH] ofproto/bond: Improve admissibility debug readability.

Kevin Traynor ktraynor at redhat.com
Fri Oct 15 13:15:25 UTC 2021


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 at redhat.com>


> Signed-off-by: David Marchand <david.marchand at 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");
> 



More information about the dev mailing list