Thu, Mar 26, 2020 at 05:28:45PM CET, olichtne(a)redhat.com wrote:
On Thu, Mar 26, 2020 at 05:11:34PM +0100, Jan Tluka wrote:
> 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 so, it would mean that you'd have to override the
generate_ping_configurations method either in the mixin classes or the
Vlan/Vxlan recipe classes. And that is what we started with wanting to
remove.
It's actually much more simpler and I don't think I break the code the
way you see it.
It's true that I need to modify generate_ping_configurations
so that it works with additional value returned by generate_ping_endpoints
defined by each class. But that change is only in BaseEnrtRecipe class.
- for endpoint1, endpoint2 in self.generate_ping_endpoints(config):
+ for endpoint1, endpoint2, match in self.generate_ping_endpoints(config):
Then, in the same method I'd just pass the 'match' value to
get_ping_evaluators:
- ping_evaluators = self.get_ping_evaluators(pconf)
+ ping_evaluators = self.get_ping_evaluators(pconf, match)
I just need to update the generate_ping_endpoints in individual classes
to return also the 'match' value. Note, I left the pconf parameter in
case it's needed anytime in the future.
The get_ping_evaluators is the only method that PingEvaluatorMixin needs
to override. PingConditionMixins can be completely removed.
>
> >
> >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.
I see.
>
> >
> >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
-Ondrej