Wed, May 13, 2020 at 03:31:49PM CEST, olichtne(a)redhat.com wrote:
On Mon, May 11, 2020 at 12:58:19PM +0200, Jan Tluka wrote:
> The current implementation of the vlan isolation test is not accurate.
> In particular for the VlansRecipe, VlansOverBondRecipe and VlansOverTeamRecipe,
> where under certain conditions (e.g. rp_filter=2 or IPv6) the vlans are
> expected to be reachable.
>
> Before we find a better way to test this I removed the relevant PingEndpoints
> in these tests.
>
> I kept the similar test in virtual tests because in these topologies
> the test makes sense. The virtual guests are isolated at the hypervisor
> level and must not be be able to access the other network.
So I wanted to ask about this... Didn't we do the cross vlan ping that
expects to fail in the VlansRecipe to basically simulate the vm
scenario?
Kind of, but the fundamental difference is that in case of VlansRecipe
each of the test machine has access to all three subnets/vlans simply
because all of them are configured on these machines and so the machines
are directly connected to each other.
I understand that "under certain conditions" this can work... but so
can anything, if you configure firewall rules in a very specific way or
just straight up routing you can make "any ip ping any other ip". I
understand there's more to that, I'm just trying to illustrate that I'm
not sure about the "under certain conditions" is a relevant argument
here.
Modifying firewall is far from the default setting.
The rp_filter became kind of special. Fedora changed the default value to
value 2 (just recently) and RHEL still configures it to 1. But I consider
this as part of default setting.
When I worked on this recipe in the past I simply thought it is
impossible to do the following:
ping 192.168.20.2 -c 100 -i 0.1 -I p5p1.10_0
but it turned out that it's only because of rp_filter setting.
The problem with the current implementation is that test expects the
system is in certain configuration (e.g. rp_filter). We could be more
explicit and add configuration of the rp_filter to the test, sure,
that's what you're proposing in the comment below I guess.
But I doubt the purpose of this test a bit. I'm not even sure what
this test tests. Is it check if rp_filter=1 works as expected?
What's the value of this test for customers? Not quite sure.
So I guess to rephrase - based on what the Goal of a recipe is, it
should implement the expected configuration for that goal and then
define the ping/perf endpoints in such a way that these verify this
Goal. And the goal should probably be part of the documentation for the
Recipe in case it's not "obvious" from the name of the recipe.
The patch itself is fine, just asking about the semantics...
-Ondrej
>
> Signed-off-by: Jan Tluka <jtluka(a)redhat.com>
> ---
> lnst/Recipes/ENRT/VlansOverBondRecipe.py | 10 ++++------
> lnst/Recipes/ENRT/VlansOverTeamRecipe.py | 10 ++++------
> lnst/Recipes/ENRT/VlansRecipe.py | 21 +++++++++------------
> 3 files changed, 17 insertions(+), 24 deletions(-)
>
> diff --git a/lnst/Recipes/ENRT/VlansOverBondRecipe.py
b/lnst/Recipes/ENRT/VlansOverBondRecipe.py
> index 45c3e185..7ca8d63d 100644
> --- a/lnst/Recipes/ENRT/VlansOverBondRecipe.py
> +++ b/lnst/Recipes/ENRT/VlansOverBondRecipe.py
> @@ -129,12 +129,10 @@ class VlansOverBondRecipe(VlanPingEvaluatorMixin,
>
> def generate_ping_endpoints(self, config):
> host1, host2 = self.matched.host1, self.matched.host2
> - result = []
> - for src in [host1.vlan0, host1.vlan1, host1.vlan2]:
> - for dst in [host2.vlan0, host2.vlan1, host2.vlan2]:
> - result += [PingEndpoints(src, dst,
> - reachable=(src.vlan_id == dst.vlan_id))]
> - return result
> +
> + return [PingEndpoints(host1.vlan0, host2.vlan0),
> + PingEndpoints(host1.vlan1, host2.vlan1),
> + PingEndpoints(host1.vlan2, host2.vlan2)]
>
> def generate_perf_endpoints(self, config):
> return [(self.matched.host1.vlan0, self.matched.host2.vlan0)]
> diff --git a/lnst/Recipes/ENRT/VlansOverTeamRecipe.py
b/lnst/Recipes/ENRT/VlansOverTeamRecipe.py
> index 5ef430bf..cc00c3cd 100644
> --- a/lnst/Recipes/ENRT/VlansOverTeamRecipe.py
> +++ b/lnst/Recipes/ENRT/VlansOverTeamRecipe.py
> @@ -127,12 +127,10 @@ class VlansOverTeamRecipe(VlanPingEvaluatorMixin,
>
> def generate_ping_endpoints(self, config):
> host1, host2 = self.matched.host1, self.matched.host2
> - result = []
> - for src in [host1.vlan0, host1.vlan1, host1.vlan2]:
> - for dst in [host2.vlan0, host2.vlan1, host2.vlan2]:
> - result += [PingEndpoints(src, dst,
> - reachable=(src.vlan_id == dst.vlan_id))]
> - return result
> +
> + return [PingEndpoints(host1.vlan0, host2.vlan0),
> + PingEndpoints(host1.vlan1, host2.vlan1),
> + PingEndpoints(host1.vlan2, host2.vlan2)]
>
> def generate_perf_endpoints(self, config):
> return [(self.matched.host1.vlan0, self.matched.host2.vlan0)]
> diff --git a/lnst/Recipes/ENRT/VlansRecipe.py b/lnst/Recipes/ENRT/VlansRecipe.py
> index eef60ef1..1f822451 100644
> --- a/lnst/Recipes/ENRT/VlansRecipe.py
> +++ b/lnst/Recipes/ENRT/VlansRecipe.py
> @@ -145,22 +145,19 @@ class VlansRecipe(VlanPingEvaluatorMixin,
>
> def generate_ping_endpoints(self, config):
> """
> - The ping endpoints for this recipe are all combinations of the VLAN
> - tunnel endpoints of the hosts. Depending on the VLAN id match of each
> - tunnel endpoint combination the *reachable* flag is set.
> + The ping endpoints for this recipe are the matching VLAN tunnel
> + endpoints of the hosts.
>
> Returned as::
> -
> - # list of PingEndpoints with the following pattern
> - [PingEndpoints(src, dst, reachable=(src.vlan_id == dst.vlan_id)), ...]
> + [PingEndpoints(host1.vlan0, host2.vlan0),
> + PingEndpoints(host1.vlan1, host2.vlan1),
> + PingEndpoints(host1.vlan2, host2.vlan2)]
> """
> host1, host2 = self.matched.host1, self.matched.host2
> - result = []
> - for src in [host1.vlan0, host1.vlan1, host1.vlan2]:
> - for dst in [host2.vlan0, host2.vlan1, host2.vlan2]:
> - result += [PingEndpoints(src, dst,
> - reachable=(src.vlan_id == dst.vlan_id))]
> - return result
> +
> + return [PingEndpoints(host1.vlan0, host2.vlan0),
> + PingEndpoints(host1.vlan1, host2.vlan1),
> + PingEndpoints(host1.vlan2, host2.vlan2)]
>
> def generate_perf_endpoints(self, config):
> """
> --
> 2.21.1
> _______________________________________________
> 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...