8:57 a.m.
Thu, Apr 22, 2021 at 01:57:40PM CEST, olichtne(a)redhat.com wrote:
On Wed, Apr 21, 2021 at 05:48:30PM +0200, Jan Tluka wrote:
> Signed-off-by: Jan Tluka <jtluka(a)redhat.com>
> ---
> docs/source/base_enrt_tunnel_class.rst | 6 +
> docs/source/enrt_recipes.rst | 1 +
> lnst/Recipes/ENRT/BaseTunnelRecipe.py | 157 +++++++++++++++++++++++++
> lnst/Recipes/ENRT/__init__.py | 1 +
> 4 files changed, 165 insertions(+)
> create mode 100644 docs/source/base_enrt_tunnel_class.rst
> create mode 100644 lnst/Recipes/ENRT/BaseTunnelRecipe.py
>
> diff --git a/docs/source/base_enrt_tunnel_class.rst
b/docs/source/base_enrt_tunnel_class.rst
> new file mode 100644
> index 00000000..b3acddfd
> --- /dev/null
> +++ b/docs/source/base_enrt_tunnel_class.rst
> @@ -0,0 +1,6 @@
> +BaseTunnelRecipe class
> +====================
> +
> +.. autoclass:: lnst.Recipes.ENRT.BaseTunnelRecipe.BaseTunnelRecipe
> + :members:
> + :show-inheritance:
> diff --git a/docs/source/enrt_recipes.rst b/docs/source/enrt_recipes.rst
> index af8935ab..da6a330e 100644
> --- a/docs/source/enrt_recipes.rst
> +++ b/docs/source/enrt_recipes.rst
> @@ -8,5 +8,6 @@ Early Network Regression Testing (ENRT)
> :caption: ENRT Components
>
> base_enrt_class
> + base_enrt_tunnel_class
> config_mixins
> specific_scenarios
> diff --git a/lnst/Recipes/ENRT/BaseTunnelRecipe.py
b/lnst/Recipes/ENRT/BaseTunnelRecipe.py
> new file mode 100644
> index 00000000..6f2176c2
> --- /dev/null
> +++ b/lnst/Recipes/ENRT/BaseTunnelRecipe.py
> @@ -0,0 +1,157 @@
> +from lnst.Common.Parameters import Param
> +from lnst.Common.IpAddress import ipaddress
> +from lnst.Recipes.ENRT.BaremetalEnrtRecipe import BaremetalEnrtRecipe
> +from lnst.Recipes.ENRT.ConfigMixins.OffloadSubConfigMixin import (
> + OffloadSubConfigMixin,
> +)
> +from lnst.Recipes.ENRT.ConfigMixins.CommonHWSubConfigMixin import (
> + CommonHWSubConfigMixin,
> +)
> +from lnst.RecipeCommon.PacketAssert import PacketAssertConf,
PacketAssertTestAndEvaluate
> +
> +
> +class BaseTunnelRecipe(
> + PacketAssertTestAndEvaluate,
> + CommonHWSubConfigMixin,
> + OffloadSubConfigMixin,
I'm not sure if we should include these config mixin classes into a base
class, I generally tried to avoid that in case a specific derived class
doesn't have a use for e.g. offload configuration - ipsec recipes come
to mind.
If this is not an issue for tunnel recipes (or at least as far as we
know) we can probably keep it like this and potentially update this
later if needed.
No problem, it makes sense and I can fix that.
> + BaremetalEnrtRecipe,
> +):
> + """
> + This base class extends the :any:`BaseEnrtRecipe` class and defines
> + a common API for implementation of baremetal tunnel recipes.
> +
> + The :meth:`ping_test` method is overridden to include a test
> + that the ping packets are properly tunnelled. This is implemented with
> + :any:`PacketAssert` test module. Each child class needs to implement
> + :meth:`get_packet_assert_config` so that the correct filters are passed
> + to the module.
> +
> + The class inheriting from this base class need to implement following
> + methods:
> +
> + * :meth:`configure_underlying_network`
> + * :meth:`create_tunnel`
> + * :meth:`get_packet_assert_config`
> + """
> +
> + offload_combinations = Param(
> + default=(
> + dict(gro="on", gso="on", tso="on"),
> + dict(gro="off", gso="on", tso="on"),
> + dict(gro="on", gso="off", tso="off"),
> + dict(gro="on", gso="on", tso="off"),
> + )
> + )
> +
> + def test_wide_configuration(self):
> + """
> + The base class defines common steps to create the test wide
> + configuration of a tunnel recipe.
> +
> + First, an underlying network that will be the transport layer for
> + the tunnel is configured in :meth:`configure_underlying_network`,
> + then the tunnel is created between the specified endpoints in
> + :meth:`create_tunnel`.
> + """
> +
> + configuration = super().test_wide_configuration()
> + configuration.test_wide_devices = []
> + configuration.tunnel_devices = []
> +
> + endpoint1, endpoint2 = self.configure_underlying_network(configuration)
> + self.wait_tentative_ips(configuration.test_wide_devices)
> + self.create_tunnel(configuration, (endpoint1, endpoint2))
> + self.wait_tentative_ips(configuration.tunnel_devices)
Shouldn't these wait_tentative_ips be part of the respective configure
methods? In case these create something more complicated and the wait is
relevant for more than just the `test_wide_devices` and
`tunnel_devices`?
It does feel a little bit like duplicating code but at the same time I'd
say it fits the specific config method more.
Makes sense, I'll update the patchset.
> +
> + return configuration
> +
> + def configure_underlying_network(self, configuration):
> + """
> + This method must be implemented by the child class.
> +
> + The child class should configure the network stack that will be
> + the transport layer for the tunnel configured in :meth:`create_tunnel`.
> +
> + That usually includes configuring the IP addresses, creating additional
> + network devices such as bonding or vlans, creating network namespaces,
> + etc.
> +
> + :param configuration:
> + An EnrtConfiguration instance that will be populated with
> + configured network devices.
> + :type configuration: :any:`EnrtConfiguration`
> +
> + :return: The method must return a tuple of two Device objects that
> + will be used as tunnel endpoints in :meth:`create_tunnel`
> + """
> + raise NotImplementedError
> +
> + def create_tunnel(self, configuration, endpoints):
Shouldn't the "endpoints" be somehow part of the "configuration"
object
and so passing this as a separate parameter is redundant?
Yes, makes sense. And I see that I have silently omitted the parameter
in the class docstring.
> + """
> + This method must be implemented by the child class.
> +
> + The child class should create a network tunnel between provided
> + endpoints. That usually includes creating tunnel devices such as
> + :any:`GreDevice`, :any:`VxlanDevice`, etc. and configuring the IP
> + addresses.
> +
> + :param endpoints:
> + A tuple of two Device objects that represent the tunnel endpoints
> + :type endpoints: tuple(:any:`lnst.Devices.Device.Device`)
> + """
> + raise NotImplementedError
> +
> + def generate_test_wide_description(self, config):
> + """
> + Test wide description is extended with the configured addresses
> + of the underlying network devices and the configured tunnel devices
> + """
> + desc = super().generate_test_wide_description(config)
> + desc += [
> + "Configured {}.{}.ips = {}".format(dev.host.hostid, dev.name,
dev.ips)
> + for dev in config.test_wide_devices
> + ]
> + desc += [
> + "Configured tunnel endpoint {}.{}.ips = {}".format(
> + dev.host.hostid, dev.name, dev.ips
> + )
> + for dev in config.tunnel_devices
> + ]
> + return desc
> +
> + def test_wide_deconfiguration(self, config):
> + "" # overriding the parent docstring
> + del config.test_wide_devices
> + del config.tunnel_devices
> +
> + super().test_wide_deconfiguration(config)
I think this should be in reverse order - first call the super
test_wide_deconfiguration and then delete the config attributes.
Are you sure? The BaseEnrtRecipe states that the super() call should be
the last.
However, I'd reverse the deletion of tunnel_devices and
test_wide_devices here, that would match the reversed configuration flow.
> +
> + def ping_test(self, ping_configs):
> + pa_config = self.get_packet_assert_config(ping_configs[0])
> + self.packet_assert_test_start(pa_config)
> + self.ctl.wait(2)
> + ping_result = super().ping_test(ping_configs)
> + self.ctl.wait(2)
> + pa_result = self.packet_assert_test_stop()
> +
> + result = ((ping_result, pa_config, pa_result),)
> +
> + return result
> +
> + def ping_report_and_evaluate(self, results):
> + for res in results:
> + super().ping_report_and_evaluate(res[0])
> + self.packet_assert_evaluate_and_report(res[1], res[2])
> +
> + def get_packet_assert_config(self, ping_config):
> + """
> + This method must be implemented by the child class.
> +
> + :param ping_config:
> + Ping configuration generated by
:meth:`BaseEnrtRecipe.generate_ping_configurations`
> + :type ping_config: :any:`PingConfig`
> +
> + :return: returns a PacketAssert configuration
> + :rtype: :any:`PacketAssertConf`
> + """
> + raise NotImplementedError
> diff --git a/lnst/Recipes/ENRT/__init__.py b/lnst/Recipes/ENRT/__init__.py
> index e3c7d1e6..f08c6375 100644
> --- a/lnst/Recipes/ENRT/__init__.py
> +++ b/lnst/Recipes/ENRT/__init__.py
> @@ -82,3 +82,4 @@ from lnst.Recipes.ENRT.VxlanMulticastRecipe import
VxlanMulticastRecipe
> from lnst.Recipes.ENRT.VxlanRemoteRecipe import VxlanRemoteRecipe
>
> from lnst.Recipes.ENRT.BaseEnrtRecipe import BaseEnrtRecipe
> +from lnst.Recipes.ENRT.BaseTunnelRecipe import BaseTunnelRecipe
> --
> 2.26.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...
> Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure