On 5/8/24 11:19, Eelco Chaudron wrote:
> The flow_reval_monitor.py script incorrectly reported the reasons for
> FDR_PURGE and FDR_TOO_EXPENSIVE, as their descriptions were swapped.
> This patch rectifies the order using a dictionary to avoid similar
> problems in the future.
>
> In addition this patch also syncs the delete reason output of the
> script, with the comments in the code.
>
> Fixes: 86b9e653ef22 ("revalidator: Add a USDT probe during flow deletion with
> purge reason.")
> Signed-off-by: Eelco Chaudron <[email protected]>
>
> ---
> v2: - Converted the list of strings to dictionary.
> - Added comment to code to keep code and script in sync.
> - Unified flow_delete reason comments and script output.
> ---
> ofproto/ofproto-dpif-upcall.c | 25 ++++++++-------
> utilities/usdt-scripts/flow_reval_monitor.py | 32 ++++++++++----------
> 2 files changed, 30 insertions(+), 27 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 73901b651..e4d348985 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -1,3 +1,4 @@
> +
> /* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016 Nicira, Inc.
> *
> * Licensed under the Apache License, Version 2.0 (the "License");
> @@ -270,18 +271,20 @@ enum ukey_state {
> };
> #define N_UKEY_STATES (UKEY_DELETED + 1)
>
> +/* Ukey delete reasons used by USDT probes. Please keep in sync with the
> + * definition in utilities/usdt-scripts/flow_reval_monitor.py. */
> enum flow_del_reason {
> - FDR_NONE = 0, /* No deletion reason for the flow. */
> - FDR_AVOID_CACHING, /* Flow deleted to avoid caching. */
> - FDR_BAD_ODP_FIT, /* The flow had a bad ODP flow fit. */
> - FDR_FLOW_IDLE, /* The flow went unused and was deleted. */
> - FDR_FLOW_LIMIT, /* All flows being killed. */
> - FDR_FLOW_WILDCARDED, /* The flow needed a narrower wildcard mask. */
> - FDR_NO_OFPROTO, /* The flow didn't have an associated ofproto. */
> - FDR_PURGE, /* User action caused flows to be killed. */
> - FDR_TOO_EXPENSIVE, /* The flow was too expensive to revalidate. */
> - FDR_UPDATE_FAIL, /* Flow state transition was unexpected. */
> - FDR_XLATION_ERROR, /* There was an error translating the flow. */
> + FDR_NONE = 0, /* No delete reason specified. */
> + FDR_AVOID_CACHING, /* Cache avoidance flag set. */
> + FDR_BAD_ODP_FIT, /* Bad ODP flow fit. */
> + FDR_FLOW_IDLE, /* Flow idle timeout. */
> + FDR_FLOW_LIMIT, /* Kill all flows condition reached. */
> + FDR_FLOW_WILDCARDED, /* Flow needs a narrower wildcard mask. */
> + FDR_NO_OFPROTO, /* Flow lacks ofproto dpif association. */
> + FDR_PURGE, /* User requested flow deletion. */
> + FDR_TOO_EXPENSIVE, /* Too expensive to revalidate. */
> + FDR_UPDATE_FAIL, /* Datapath update failed. */
> + FDR_XLATION_ERROR, /* Flow translation error. */
> };
>
> /* 'udpif_key's are responsible for tracking the little bit of state udpif
> diff --git a/utilities/usdt-scripts/flow_reval_monitor.py
> b/utilities/usdt-scripts/flow_reval_monitor.py
> index 534ba8fa2..bc3a28443 100755
> --- a/utilities/usdt-scripts/flow_reval_monitor.py
> +++ b/utilities/usdt-scripts/flow_reval_monitor.py
> @@ -254,19 +254,19 @@ FdrReasons = IntEnum(
> start=0,
> )
>
> -FdrReasonStrings = [
> - "No deletion reason",
> - "Cache avoidance flag set",
> - "Bad ODP flow fit",
> - "Idle flow timed out",
> - "Kill all flows condition detected",
> - "Mask too wide - need narrower match",
> - "No matching ofproto rules",
> - "Too expensive to revalidate",
> - "Purged with user action",
> - "Flow state inconsistent after updates",
> - "Flow translation error",
> -]
Should we also have a comment here describing from where these are
coming from?
> +FdrReasonStrings = {
> + FdrReasons.FDR_NONE: "No delete reason specified",
> + FdrReasons.FDR_AVOID_CACHING: "Cache avoidance flag set",
> + FdrReasons.FDR_BAD_ODP_FIT: "Bad ODP flow fit",
> + FdrReasons.FDR_FLOW_IDLE: "Flow idle timeout",
> + FdrReasons.FDR_FLOW_LIMIT: "Kill all flows condition reached",
> + FdrReasons.FDR_FLOW_WILDCARDED: "Flow needs a narrower wildcard mask",
> + FdrReasons.FDR_NO_OFPROTO: "Flow lacks ofproto dpif association",
Can we rename this one into "Bridge not found" maybe?
'ofproto' and 'dpif' are not user-friendly words. 'ofproto' is an entirely
internal concept.
> + FdrReasons.FDR_PURGE: "User requested flow deletion",
> + FdrReasons.FDR_TOO_EXPENSIVE: "Too expensive to revalidate",
> + FdrReasons.FDR_UPDATE_FAIL: "Datapath update failed",
> + FdrReasons.FDR_XLATION_ERROR: "Flow translation error"
> +}
>
>
> def err(msg, code=-1):
> @@ -572,10 +572,10 @@ def print_expiration(event):
> """Prints a UFID eviction with a reason."""
> ufid_str = format_ufid(event.ufid)
>
> - if event.reason > len(FdrReasons):
> - reason = f"Unknown reason '{event.reason}'"
> - else:
> + try:
> reason = FdrReasonStrings[event.reason]
> + except KeyError:
> + reason = f"Unknown reason '{event.reason}'"
>
> print(
> "{:<10} {:<18.9f} {:<36} {:<17}".format(
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
