| Message ID | 6b48b2255b07f6ea3e81b78b8802aaa1ef106a34.1715159996.git.echaudro@redhat.com |
|---|---|
| State | Superseded |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | [ovs-dev,v2] utilities: Correct deletion reason in flow_reval_monitor.py. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On Wed, May 08, 2024 at 11:19:56AM +0200, 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 <echaudro@redhat.com> > > --- > 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"); Nit: this hunk seems unrelated to the rest of the patch. Otherwise, this looks good to me. Acked-by: Simon Horman <horms@ovn.org>
On 9 May 2024, at 15:25, Simon Horman wrote: > On Wed, May 08, 2024 at 11:19:56AM +0200, 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 <echaudro@redhat.com> >> >> --- >> 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"); > > Nit: this hunk seems unrelated to the rest of the patch. Yes that slipped in unintentionally :( > Otherwise, this looks good to me. > > Acked-by: Simon Horman <horms@ovn.org> Thanks for the review.
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 <echaudro@redhat.com> > > --- > 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(
On 13 May 2024, at 17:02, Ilya Maximets wrote: > 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 <echaudro@redhat.com> >> >> --- >> 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. ACK on both. Just sent out a v3. //Eelco >> + 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(
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", -] +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", + 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(
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 <echaudro@redhat.com> --- 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(-)