Mon, Jul 27, 2020 at 09:19:12AM CEST, olichtne(a)redhat.com wrote:
Two comments inline.
On Fri, Jul 24, 2020 at 07:00:47PM +0200, Jan Tluka wrote:
> This patch adds a PerfTestMixins module that contains mixins to perform
> additional tasks required by the BaseEnrtRecipe.do_perf_tests() method.
> This is similar to ConfigMixin classes but at a deeper code level.
>
> The module provides a base class BasePerfTestTweakMixin that defines
> two methods, apply_perf_test_tweak() and remove_perf_test_tweak(), to
> do additional configuration steps before and after the performance test.
>
> The module also contains SctpFirewallPerfTestMixin that provides additional
> tweak of the network stack for SCTP performance test. As an optimization,
> the mixin applies iptables rules to the OUTPUT chain to drop any packets
> on other than the tested interface. This is to prevent SCTP going through
> unintended path.
>
> Signed-off-by: Jan Tluka <jtluka(a)redhat.com>
> ---
> .../PerfTestMixins/BasePerfTestTweakMixin.py | 11 +++++++
> .../SctpFirewallPerfTestMixin.py | 33 +++++++++++++++++++
> lnst/Recipes/ENRT/PerfTestMixins/__init__.py | 2 ++
> 3 files changed, 46 insertions(+)
> create mode 100644 lnst/Recipes/ENRT/PerfTestMixins/BasePerfTestTweakMixin.py
> create mode 100644 lnst/Recipes/ENRT/PerfTestMixins/SctpFirewallPerfTestMixin.py
> create mode 100644 lnst/Recipes/ENRT/PerfTestMixins/__init__.py
>
> diff --git a/lnst/Recipes/ENRT/PerfTestMixins/BasePerfTestTweakMixin.py
b/lnst/Recipes/ENRT/PerfTestMixins/BasePerfTestTweakMixin.py
> new file mode 100644
> index 00000000..a444384d
> --- /dev/null
> +++ b/lnst/Recipes/ENRT/PerfTestMixins/BasePerfTestTweakMixin.py
> @@ -0,0 +1,11 @@
> +class BasePerfTestTweakMixin(object):
> + """
> + This is a base class that defines common API for specific *perf test*
> + mixin classes.
> + """
> +
> + def apply_perf_test_tweak(self, perf_config):
> + pass
> +
> + def remove_perf_test_tweak(self, perf_config):
> + pass
The other mixin classes also define a "describe" method here - to
generate a human readable TestResult describing what configuration was
just applied. The reason being that we want to have all the important
configuration steps be explicitly visible to users.
The host.run() method will generate lower level result objects for the
specific commands that were executed, but a higher level descriptor for
users is IMO also important.
What do you think? Could that be added easily or does it not fit this
mixin class hierarchy?
I'll take a look if this can be done.
> diff --git
a/lnst/Recipes/ENRT/PerfTestMixins/SctpFirewallPerfTestMixin.py
b/lnst/Recipes/ENRT/PerfTestMixins/SctpFirewallPerfTestMixin.py
> new file mode 100644
> index 00000000..46963dd2
> --- /dev/null
> +++ b/lnst/Recipes/ENRT/PerfTestMixins/SctpFirewallPerfTestMixin.py
> @@ -0,0 +1,33 @@
> +from lnst.Controller.RecipeResults import ResultLevel
> +from lnst.Recipes.ENRT.PerfTestMixins import BasePerfTestTweakMixin
> +from lnst.RecipeCommon.Perf.Measurements.BaseFlowMeasurement import
BaseFlowMeasurement
> +
> +class SctpFirewallPerfTestMixin(BasePerfTestTweakMixin):
> +
> + 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]
> +
> + 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 = flow_measurement.conf[0]
I see you're using [0] here as well as in the previous method, it's
probably ok for the current use case of the SCTP tests but what would
happen if this was combined with a "multistream" sctp test? Is that
still handled?
It depends if the multistream contains a mix of different protocols to
test at once. With the current implementation of ENRT recipes this is not
possible IMO.
If this comment is just about test of multiple streams of the same
protocol, I'm not sure if this work. I'll check that.
Thanks for review!
-Jan
-Ondrej
> + if flow.type == "sctp_stream":
> + for nic in [flow.generator_nic, flow.receiver_nic]:
> + nic.netns.run(
> + "iptables -I OUTPUT ! -o %s -p sctp -j DROP" %
nic.name,
> + job_level=ResultLevel.NORMAL,
> + )
> +
> + def remove_perf_test_tweak(self, perf_config):
> + flow_measurement = self._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]:
> + nic.netns.run(
> + "iptables -D OUTPUT ! -o %s -p sctp -j DROP" %
nic.name,
> + job_level=ResultLevel.NORMAL,
> + )
> +
> + super().remove_perf_test_tweak(perf_config)
> diff --git a/lnst/Recipes/ENRT/PerfTestMixins/__init__.py
b/lnst/Recipes/ENRT/PerfTestMixins/__init__.py
> new file mode 100644
> index 00000000..9f320118
> --- /dev/null
> +++ b/lnst/Recipes/ENRT/PerfTestMixins/__init__.py
> @@ -0,0 +1,2 @@
> +from lnst.Recipes.ENRT.PerfTestMixins.BasePerfTestTweakMixin import
BasePerfTestTweakMixin
> +from lnst.Recipes.ENRT.PerfTestMixins.SctpFirewallPerfTestMixin import
SctpFirewallPerfTestMixin
> --
> 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/lnst-developers@lists.fedora...