Please see my comments inline.
Wed, Oct 07, 2020 at 03:48:06PM CEST, jurbanov(a)redhat.com wrote:
From: Jozef Urbanovsky <jurbanov(a)redhat.com>
Added PerfReversibleFlowMixin in the recipe as it did not exist before
and functionality was included in the recipe itself.
Signed-off-by: Jozef Urbanovsky <jurbanov(a)redhat.com>
---
lnst/Recipes/ENRT/IpsecEspAeadRecipe.py | 131 +++++++++++++++++++-----
1 file changed, 107 insertions(+), 24 deletions(-)
diff --git a/lnst/Recipes/ENRT/IpsecEspAeadRecipe.py
b/lnst/Recipes/ENRT/IpsecEspAeadRecipe.py
index 015db74..e8705d5 100644
--- a/lnst/Recipes/ENRT/IpsecEspAeadRecipe.py
+++ b/lnst/Recipes/ENRT/IpsecEspAeadRecipe.py
@@ -7,6 +7,8 @@ from lnst.Common.Parameters import StrParam
from lnst.Common.LnstError import LnstError
from lnst.Controller import HostReq, DeviceReq, RecipeParam
from lnst.Recipes.ENRT.BaseEnrtRecipe import BaseEnrtRecipe
+from lnst.Recipes.ENRT.ConfigMixins.PerfReversibleFlowMixin import (
+ PerfReversibleFlowMixin)
from lnst.Recipes.ENRT.ConfigMixins.BaseSubConfigMixin import (
BaseSubConfigMixin as ConfMixin)
from lnst.Recipes.ENRT.ConfigMixins.CommonHWSubConfigMixin import (
@@ -19,11 +21,11 @@ from lnst.Recipes.ENRT.XfrmTools import (configure_ipsec_esp_aead,
generate_key)
-class IpsecEspAeadRecipe(CommonHWSubConfigMixin, BaseEnrtRecipe,
+class IpsecEspAeadRecipe(PerfReversibleFlowMixin, CommonHWSubConfigMixin,
BaseEnrtRecipe,
PacketAssertTestAndEvaluate):
This should be split to separate patches, one adding reversible mixin,
the other adding docs.
One more note on PerfReversibleFlowMixin. Are you sure it works? This
class overrides the generate_flow_combinations() of BaseEnrtRecipe but
compared to it it does not call the _create_perf_flow() that PerfReversible
overrides, so this will not work IMO.
I'm also wondering if it's possible to reuse the
generate_flow_combinations() of the BaseEnrtRecipe, so it can be
completely removed from this class.
"""
- This recipe implements Enrt testing for a simple IPsec scenario that looks
- as follows
+ This recipe implements Enrt testing for a simple IPsec tunnel
+ scenario that looks as follows
.. code-block:: none
@@ -36,12 +38,16 @@ class IpsecEspAeadRecipe(CommonHWSubConfigMixin, BaseEnrtRecipe,
| host1 | | host2 |
+--------+ +--------+
- The recipe provides additional recipe parameter to configure IPsec tunel.
+ The recipe provides additional recipe parameter to configure IPsec mode.
+ As of now recipe defines the exact algorithm to be used in ESP statically.
:param ipsec_mode:
- mode of the ipsec tunnel
+ (mandatory test parameter) the IPsec mode to be used
+ in ESP (ex. `tunnel` or `transport`)
- All sub configurations are included via Mixin classes.
+
+ All sub configurations are included via Mixin classes,
+ except for the IPsec tunnel that is configured through the subconfiguration.
I don't understand. Which subconfiguration?
The actual test machinery is implemented in the :any:`BaseEnrtRecipe` class.
"""
@@ -58,11 +64,11 @@ class IpsecEspAeadRecipe(CommonHWSubConfigMixin, BaseEnrtRecipe,
def test_wide_configuration(self):
"""
Test wide configuration for this recipe involves just adding an IPv4 and
- IPv6 address to the matched eth0 nics on both hosts and route between them.
-
- host1.eth0 = 192.168.101.1/24 and fc00::1/64
+ IPv6 address to the matched eth0 nics on both hosts and
+ creating a route between configured endpoints.
- host2.eth0 = 192.168.101.2/24 and fc00::2/64
+ | host1.eth0 = 192.168.101.1/24 and fc00::1/64
+ | host2.eth0 = 192.168.101.2/24 and fc00::2/64
"""
host1, host2 = self.matched.host1, self.matched.host2
@@ -97,7 +103,7 @@ class IpsecEspAeadRecipe(CommonHWSubConfigMixin, BaseEnrtRecipe,
def generate_test_wide_description(self, config):
"""
Test wide description is extended with the configured IP addresses,
- specified IPsec algorithm, key length and integrity check value length.
+ specified IPsec mode, key length and integrity check value length.
"""
desc = super().generate_test_wide_description(config)
desc += [
@@ -118,8 +124,15 @@ class IpsecEspAeadRecipe(CommonHWSubConfigMixin, BaseEnrtRecipe,
def generate_sub_configurations(self, config):
"""
+ This method completely overrides default method
+ from :any:`BaseSubConfigMixin` class due to not having
+ a specific IPsec tunnel configuration mixin class.
+ Therefore the method serves a similar purpose as a mixin class would.
+
Test wide configuration is extended with subconfiguration containing
IPsec tunnel with predefined parameters for both IP versions.
+
+ New copy of the config is created for each algorithm variant defined.
"""
ipsec_mode = self.params.ipsec_mode
spi_values = self.spi_values
@@ -143,8 +156,11 @@ class IpsecEspAeadRecipe(CommonHWSubConfigMixin, BaseEnrtRecipe,
def apply_sub_configuration(self, config):
"""
- Subconfiguration containing IPsec tunnel is applied through
- XfrmTools class.
+ This method applies newly defined IPsec tunnel subconfig atop
+ of previously defined :meth:`test_wide_configuration`.
+
+ IPsec tunnel configuration is applied through
+ :py:mod:`lnst.Recipes.ENRT.XfrmTools`.
Why not to reference the configure_ipsec_esp_aead method directly?
"""
super().apply_sub_configuration(config)
ns1, ns2 = config.endpoint1.netns, config.endpoint2.netns
@@ -161,8 +177,13 @@ class IpsecEspAeadRecipe(CommonHWSubConfigMixin, BaseEnrtRecipe,
def generate_ping_configurations(self, config):
"""
- The ping endpoints for this recipe are the configured endpoints of
- the IPsec tunnel on both hosts.
+ Generating ping configuration through this method is required due
+ to the later need of ping configuration for the
+ :py:mod:`lnst.RecipeCommon.PacketAssert`.
+
+ Method overrides default method of :any:`BaseEnrtRecipe` and adds
+ additional parameters to specify IPsec tunnel endpoints as well as
+ information to be able to run :py:mod:`lnst.RecipeCommon.PacketAssert`
"""
ns1, ns2 = config.endpoint1.netns, config.endpoint2.netns
ip1, ip2 = config.ips
@@ -182,6 +203,11 @@ class IpsecEspAeadRecipe(CommonHWSubConfigMixin, BaseEnrtRecipe,
"""
Flow combinations are generated based on the tunnel endpoints
and test parameters.
+
+ Method overrides the default :any:`BaseEnrtRecipe` and manually
+ creates :any:`PerfFlow` due to the IP addresses and
+ :any:`Device.ips_filter` being already handled in the IPsec
+ configuration in the :any:`generate_sub_configurations`.
"""
nic1, nic2 = config.endpoint1, config.endpoint2
ns1, ns2 = config.endpoint1.netns, config.endpoint2.netns
@@ -204,16 +230,20 @@ class IpsecEspAeadRecipe(CommonHWSubConfigMixin, BaseEnrtRecipe,
)
yield [flow]
- if ("perf_reverse" in self.params and
- self.params.perf_reverse):
- reverse_flow = self._create_reverse_flow(flow)
- yield [reverse_flow]
-
As mentioned previously, either check if this overriden method can be
completely removed or include _create_perf_flow().
def ping_test(self, ping_configs):
"""
- Ping test is utilizing PacketAssert class to search
- for the appropriate ESP IP packet. Result of ping
- test is handed to the super class' method.
+ This method is a special case of ***ping_test*** and directly
+ overrides method from :class:`PingTestAndEvaluate` instead
+ of using the default method of :any:`BaseEnrtRecipe`.
This is a bit confusing. BaseEnrtRecipe does not have ping_test()
method.
+
+ This is the case due to the integration of the
+ :py:mod:`lnst.RecipeCommon.PacketAssert` class to also search
+ for the appropriate ESP IP packet specified.
+
+ Packets are captured with tcpdump, inspected and correct ones
+ are counted.
+
+ Result of ping test is handed to the super class' method.
Returned as::
@@ -240,13 +270,31 @@ class IpsecEspAeadRecipe(CommonHWSubConfigMixin, BaseEnrtRecipe,
pa_result = self.packet_assert_test_stop()
dump.kill(signal=signal.SIGINT)
- return (ping_result, pa_config, pa_result)
+ return ping_result, pa_config, pa_result
def ping_report_and_evaluate(self, results):
+ """
+ Method reports and evaluates the results of the ping tests,
+ as well as the :py:mod:`lnst.RecipeCommon.PacketAssert` job,
+ where results are appended to the ***results*** object.
+
+ Due to this reason base method of the :any:`PingTestAndEvaluate`
+ is overrriden.
+ """
Could you make this simpler?
"""
This method extends :any:`PingTestAndEvaluate.ping_report_and_evaluate` with
evaluation and reporting the result of the :py:mod:`lnst.RecipeCommon.PacketAssert` test.
"""
super().ping_report_and_evaluate(results[0])
self.packet_assert_evaluate_and_report(results[1], results[2])
def get_dev_by_ip(self, netns, ip):
+ """
+ Auxiliary method to locate the device by IP adress in a given namespace.
+ This method is to be used in the :meth:`ping_test` due to the tcpdump
+ requiring interface name, as well as :py:mod:`lnst.RecipeCommon.PacketAssert`
+ job.
+
+ :param netns: Namespace to be searched through
+ :param ip: IP adress to be found
+ :return: dev: :any:`Device` object with desired device
+ """
for dev in netns.device_database:
if ip in dev.ips:
return dev
@@ -255,12 +303,47 @@ class IpsecEspAeadRecipe(CommonHWSubConfigMixin, BaseEnrtRecipe,
@property
def mtu_hw_config_dev_list(self):
+ """
+ The `mtu_hw_config_dev_list` property value for this scenario
+ is a list of the physical devices carrying data of the configured
+ IPsec tunnel:
+
+ | host1.eth0
+ | host2.eth0
+
+ For detailed explanation of this property see :any:`MTUHWConfigMixin`
+ class and :any:`MTUHWConfigMixin.mtu_hw_config_dev_list`.
+ """
return [self.matched.host1.eth0, self.matched.host2.eth0]
@property
def dev_interrupt_hw_config_dev_list(self):
+ """
+ The `dev_interrupt_hw_config_dev_list` property value for this scenario
+ is a list of the physical devices carrying data of the configured
+ IPsec tunnel:
+
+ | host1.eth0
+ | host2.eth0
+
+ For detailed explanation of this property see
+ :any:`DevInterruptHWConfigMixin` class and
+ :any:`DevInterruptHWConfigMixin.dev_interrupt_hw_config_dev_list`.
+ """
return [self.matched.host1.eth0, self.matched.host2.eth0]
@property
def parallel_stream_qdisc_hw_config_dev_list(self):
+ """
+ The `parallel_stream_qdisc_hw_config_dev_list` property value for this scenario
+ is a list of the physical devices carrying data of the configured
+ IPsec tunnel:
+
+ | host1.eth0
+ | host2.eth0
+
+ For detailed explanation of this property see
+ :any:`ParallelStreamQDiscHWConfigMixin` class and
+
:any:`ParallelStreamQDiscHWConfigMixin.parallel_stream_qdisc_hw_config_dev_list`.
+ """
return [self.matched.host1.eth0, self.matched.host2.eth0]
--
2.25.4
_______________________________________________
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...