Thu, Mar 26, 2020 at 04:30:51PM CET, olichtne(a)redhat.com wrote:
On Thu, Mar 26, 2020 at 12:43:49PM +0100, Jan Tluka wrote:
> Thu, Mar 26, 2020 at 11:56:49AM CET, olichtne(a)redhat.com wrote:
> >On Tue, Mar 24, 2020 at 05:49:10PM +0100, Jan Tluka wrote:
> >> These mixins will be used to override the default Ping evaluator of the
> >> BaseEnrtRecipe class.
> >>
> >> The module provides VlanPingEvaluator mixin that will be used in all
> >> vlan-like (e.g. VLAN, VXLAN) tests where we try to ping from a vlan to
> >> another one that is meant to be unreachable.
> >>
> >> The module also defines PingCondition mixins that define test specific
> >> ping_endpoints_match() method. This method is used by VlanPingEvaluator
> >> to determine whether the ping should succeed or fail.
> >>
> >> Signed-off-by: Jan Tluka <jtluka(a)redhat.com>
> >> ---
> >> .../ENRT/PingMixins/PingConditionMixins.py | 34 +++++++++++++++++++
> >> .../ENRT/PingMixins/PingEvaluatorMixins.py | 15 ++++++++
> >> lnst/Recipes/ENRT/PingMixins/__init__.py | 9 +++++
> >> 3 files changed, 58 insertions(+)
> >> create mode 100644 lnst/Recipes/ENRT/PingMixins/PingConditionMixins.py
> >> create mode 100644 lnst/Recipes/ENRT/PingMixins/PingEvaluatorMixins.py
> >> create mode 100644 lnst/Recipes/ENRT/PingMixins/__init__.py
> >>
> >> diff --git a/lnst/Recipes/ENRT/PingMixins/PingConditionMixins.py
b/lnst/Recipes/ENRT/PingMixins/PingConditionMixins.py
> >> new file mode 100644
> >> index 00000000..2adb07bd
> >> --- /dev/null
> >> +++ b/lnst/Recipes/ENRT/PingMixins/PingConditionMixins.py
> >> @@ -0,0 +1,34 @@
> >> +class PingConditionMixin(object):
> >> + def ping_endpoints_match(self, src_addr, dst_addr):
> >> + pass
> >> +
> >> +class VlanPingConditionMixin(PingConditionMixin):
> >> + def ping_endpoints_match(self, src_addr, dst_addr):
> >> + host1, host2 = self.matched.host1, self.matched.host2
> >> + devs = []
> >> + for dev in (host1.devices + host2.devices):
> >> + if src_addr in dev.ips or dst_addr in dev.ips:
> >> + devs.append(dev)
> >> + try:
> >> + return devs[0].vlan_id == devs[1].vlan_id
> >> + except (IndexError, AttributeError):
> >> + return False
> >> +
> >> +class VirtualVlanPingConditionMixin(PingConditionMixin):
> >> + def ping_endpoints_match(self, src_addr, dst_addr):
> >> + guest1, guest2, guest3, guest4 = (self.matched.guest1,
> >> + self.matched.guest2, self.matched.guest3, self.matched.guest4)
> >
> >I'm not sure I like this hard coded list of host references based on
> >where the class is defined. The number and names of the hosts is defined
> >by the recipe, but this is a mixin class that doesn't know what recipe
> >it will be mixed into - you could mix it into a recipe that just calls
> >the hosts something different and it won't work.
> >
> >The method is named ping_ENDPOINTS_match, but the parameter names are
> >{src, dst}_address, maybe if we used some more generic concept of an
> >"Endpoint" we could pass in more than just the information about the
ip
> >addresses but also about the configuration and whether or not the
> >endpoints are in the same vlan?
> >
> >OR, if we can't do that, then that means that these conditions are
> >specific to the recipes that they are used in and they should therefore
> >be defined with their respective recipe, either as a helper class in the
> >same module, or just directly in the recipe itself...
> >
> >
> >What do you think? Does my thought process make sense or am I
> >misunderstanding something?
> >
>
> It makes sense.
>
> Another approach could be following. We could add additional value that
> is returned by generate_ping_endpoints() method along with the (src, dst)
> to indicate whether they "match".
>
> E.g. in case of VlansRecipe, that would be:
>
> for src in [host1.vlan0, host1.vlan1, host1.vlan2]:
> for dst in [host2.vlan0, host2.vlan1, host2.vlan2]:
> match = (src.vlan_id == dst.vlan_id) # <-----------
> result += [(src, dst, match)]
>
> This way we can remove PingConditionMixins completely and just keep
> the PingEvaluatorMixins that would override the get_ping_evaluators() and
> use the 'match' value to decide which evaluator to return.
That would break the BaseEnrtRecipe.generate_ping_configurations method.
Yes, it would. Would this be an issue?
I think I like the first approach with the condition mixin, we just need
to figure out how to do the condition "nicely" :).
One option is to detect the match based on the IP network - the ip
addresses for different vlans are from different ip networks.
That is not always the case. In case of VirtOvsVxlan the same network is
used for all 4 guests. That's what vxlan is used for anyway.
Another option would be to somehow get the device that owns the ip
address and look at it's vlanid.
Not sure which one is easier or nicer though... maybe we can extend the
PingConf object so that in addition to the Host and IpAddress it also
contains the actual endpoint that was provided by the
generate_ping_endpoints method. Then the vlanid check should be trivial.
Ok, will check this.
-Ondrej