Re: [PATCH 4/6] RecipeCommon.Perf.PerfTestMixins.BasePerfTestTweakMixin: move get_flow_measurements_from_config to standalone function

[Jan Tluka]

Fri, Oct 30, 2020 at 12:59:06PM CET, olichtne(a)redhat.com wrote:
> On Fri, Oct 30, 2020 at 09:30:45AM +0100, Jan Tluka wrote:
> > The method will be used by mixin classes derived from both
> > BasePerfTestTweakMixin and BasePerfTestIterationTweakMixin.
> > 
> > Signed-off-by: Jan Tluka <jtluka(a)redhat.com&gt;
> > ---
> >  .../Perf/PerfTestMixins/BasePerfTestTweakMixin.py           | 6 ------
> >  .../ENRT/PerfTestMixins/SctpFirewallPerfTestMixin.py        | 5 +++--
> >  lnst/Recipes/ENRT/PerfTestMixins/Utils.py                   | 5 +++++
> >  3 files changed, 8 insertions(+), 8 deletions(-)
> >  create mode 100644 lnst/Recipes/ENRT/PerfTestMixins/Utils.py
> > 
> > diff --git a/lnst/RecipeCommon/Perf/PerfTestMixins/BasePerfTestTweakMixin.py
> b/lnst/RecipeCommon/Perf/PerfTestMixins/BasePerfTestTweakMixin.py
> > index 5e008a68..380fc72e 100644
> > --- a/lnst/RecipeCommon/Perf/PerfTestMixins/BasePerfTestTweakMixin.py
> > +++ b/lnst/RecipeCommon/Perf/PerfTestMixins/BasePerfTestTweakMixin.py
> > @@ -1,5 +1,3 @@
> > -from lnst.RecipeCommon.Perf.Measurements.BaseFlowMeasurement import
> BaseFlowMeasurement
> > -
> >  class BasePerfTestTweakMixin(object):
> >      """
> >      This is a base class that defines common API for specific *perf test*
> > @@ -15,7 +13,3 @@ class BasePerfTestTweakMixin(object):
> >      def remove_perf_test_tweak(self, perf_config):
> >          # TODO: check if anything left in the perf_config.perf_test_tweak_config
> >          pass
> > -
> > -    def _get_flow_measurement_from_config(self, perf_config):
> > -        flow_measurements = [ m for m in perf_config.measurements if isinstance(m,
> BaseFlowMeasurement) ]
> > -        return flow_measurements[0]
> > diff --git a/lnst/Recipes/ENRT/PerfTestMixins/SctpFirewallPerfTestMixin.py
> b/lnst/Recipes/ENRT/PerfTestMixins/SctpFirewallPerfTestMixin.py
> > index 841df3f7..42cbf492 100644
> > --- a/lnst/Recipes/ENRT/PerfTestMixins/SctpFirewallPerfTestMixin.py
> > +++ b/lnst/Recipes/ENRT/PerfTestMixins/SctpFirewallPerfTestMixin.py
> > @@ -1,11 +1,12 @@
> >  from lnst.Controller.RecipeResults import ResultLevel
> >  from lnst.RecipeCommon.Perf.PerfTestMixins import BasePerfTestTweakMixin
> > +from lnst.Recipes.ENRT.PerfTestMixins.Utils import get_flow_measurement_from_config
> >  
> >  class SctpFirewallPerfTestMixin(BasePerfTestTweakMixin):
> >      def apply_perf_test_tweak(self, perf_config):
> >          super().apply_perf_test_tweak(perf_config)
> >  
> > -        flow_measurement = self._get_flow_measurement_from_config(perf_config)
> > +        flow_measurement = get_flow_measurement_from_config(perf_config)
> >          flow = flow_measurement.conf[0]
> >          if flow.type == "sctp_stream":
> >              for nic in [flow.generator_nic, flow.receiver_nic]:
> > @@ -18,7 +19,7 @@ class SctpFirewallPerfTestMixin(BasePerfTestTweakMixin):
> >              tweak_config["iptables_sctp"] = True
> >  
> >      def remove_perf_test_tweak(self, perf_config):
> > -        flow_measurement = self._get_flow_measurement_from_config(perf_config)
> > +        flow_measurement = get_flow_measurement_from_config(perf_config)
> >          flow = flow_measurement.conf[0]
> >          if flow.type == "sctp_stream":
> >              for nic in [flow.generator_nic, flow.receiver_nic]:
> > diff --git a/lnst/Recipes/ENRT/PerfTestMixins/Utils.py
> b/lnst/Recipes/ENRT/PerfTestMixins/Utils.py
> > new file mode 100644
> > index 00000000..9f6f167e
> > --- /dev/null
> > +++ b/lnst/Recipes/ENRT/PerfTestMixins/Utils.py
> > @@ -0,0 +1,5 @@
> > +from lnst.RecipeCommon.Perf.Measurements.BaseFlowMeasurement import
> BaseFlowMeasurement
> > +
> > +def get_flow_measurement_from_config(perf_config):
> > +    flow_measurements = [ m for m in perf_config.measurements if isinstance(m,
> BaseFlowMeasurement) ]
> > +    return flow_measurements[0]
>
> I'm a bit worried about extending the use of the function and also
> burying it "deeper" into a "utils" module while the function returns
> a
> specific [0] value.
>
> I know that currently we only have a single FlowMeasurement in the list,
> but if we ever add more than one, this could become quite a complicated
> thing to find/debug.
>
> Would it make more sense to have this as a "get_...measurementS..."
> method, return all of the flow measurements and then, for now, pick the
> [0] directly the the place where it's being used?
>  
Makes sense. So, when you say to have this as a method, you mean a class
method, correct? I will need to create a base class for PerfTest and
PerfTestIteration classes that would share this method (which is ok).


> Optionally maybe renaming the method to get_first_flow_measurement...
> could also work and make it more obvious what's happening.
>
> -Ondrej
>  
> > -- 
> > 2.21.3
> > _______________________________________________
> > LNST-developers mailing list -- lnst-developers(a)lists.fedorahosted.org
> > To unsubscribe send an email to lnst-developers-leave(a)lists.fedorahosted.org
> > Fedora Code of Conduct:
> https://docs.fedoraproject.org/en-US/project/code-of-conduct/
> > List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
> > List Archives:
> https://lists.fedorahosted.org/archives/list/[e-mail]...