Fri, Mar 27, 2020 at 09:34:25AM CET, olichtne(a)redhat.com wrote:
On Fri, Mar 27, 2020 at 09:03:07AM +0100, Jan Tluka wrote:
> 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:
> >> >> 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.
Ah, I see. Yeah that's a simpler way to do what you propose, but it means
you have to update generate_ping_endpoints in _ALL_ the classes, even
those where the match parameter logically doesn't really make a lot of
sense, for example SimpleNetworkRecipe. Then you're kind of stuck with
explaining why the parameter is there at all for more recipes than the
number of classes where it immediatelly makes sense.
I think that could be more readable or more easily explainable if this
weren't just _tuples_ of three values. Maybe if we introduced a
PingEndpoints class to store this information it would become more
clear:
class PingEndpoints:
def __init__(self, endpoint1, endpoint2, reachable=True):
...
Then you get generate_ping_endpoints:
def generate_ping_endpoints():
return [
PingEndpoints(host1.eth0, host2.eth0),
PingEndpoints(host1.vlan0, host2.vlan0, reachable=True),
PingEndpoints(host1.vlan0, host2.vlan1, reachable=False),
]
Maybe this could even be generalized to just
class ConnectionEndpoints():
...
and reused for Perf as well... using plain tuples as an API is nice as
long as it is very simple and very obvious what they mean, but once you
start adding more values to it and assigning special meanings to
specific fields of the tuple, what you really want is a "structure"
where you can add names to the fields.
-Ondrej
Yes, that makes sense. I think it's probably better to start with
PingEndpoints rather than doing this generic for both Ping and Perf.
Thanks
-Jan