[PATCH v2 00/15] Refactor RecipeCommon.Ping

[Jan Tluka]

This is a series of patches that attempts to remove the duplicated code
in Recipes/ENRT in case of cross-vlan ping runs.

The changes include:
 * split of ping command execution and evaluation of results
 * introducing two basic Ping evaluators
 * replacing the duplicated code in Recipes/ENRT with the new reusable code

v2:

* fixed a whitespace error
 * moved Perf BaseEvaluator to BaseResultEvaluator
 * used plural for ping_confing argument in ping_test()
 * instead of overriding the do_ping_tests base class method I added few
   mixin classes that do the same thing in even more reusable way
 * be aware of additional patches compared to previous v1

Jan Tluka (15):
  RecipeCommon.Ping: rename running_ping to ping
  RecipeCommon.Ping: move prepare_job to ping_init
  RecipeCommon.Ping: remove ping_config from ping_evaluate_and_report
  Tests.Ping: update failure logic
  Tests.Ping: do not check rtt data if no packets were received
  RecipeCommon.Ping: rename ping_evaluate_and_report method
  lnst.RecipeCommon: move Ping Recipe to separate directory
  lnst.RecipeCommon: rename Perf BaseEvaluator to BaseResultEvaluator
  lnst.RecipeCommon.Ping: add Ping evaluators
  RecipeCommon.Ping.Recipe: add evaluators to PingConf object
  Recipes.ENRT.BaseEnrtRecipe: register default PingEvaluator
  RecipeCommon.Ping.Recipe: split report and evaluate
  lnst.Recipes.ENRT: add PingMixins
  lnst.Recipes.ENRT: use PingMixins to override Ping evaluators
  lnst.RecipeCommon.Ping.Recipe: use plural for ping_config in ping_test

...aseEvaluator.py => BaseResultEvaluator.py} |  2 +-
 .../Perf/Evaluators/BaselineEvaluator.py      |  4 +-
 .../Perf/Evaluators/NonzeroFlowEvaluator.py   |  4 +-
 .../Ping/Evaluators/RatePingEvaluator.py      | 59 +++++++++++++++
 .../Ping/Evaluators/ZeroPassPingEvaluator.py  | 21 ++++++
 lnst/RecipeCommon/Ping/Evaluators/__init__.py |  2 +
 lnst/RecipeCommon/{Ping.py => Ping/Recipe.py} | 55 ++++++++------
 lnst/RecipeCommon/Ping/__init__.py            |  0
 lnst/Recipes/ENRT/BaseEnrtRecipe.py           | 15 ++--
 lnst/Recipes/ENRT/BasePvPRecipe.py            |  2 +-
 lnst/Recipes/ENRT/IpsecEspAeadRecipe.py       | 16 ++---
 lnst/Recipes/ENRT/IpsecEspAhCompRecipe.py     | 26 +++----
 lnst/Recipes/ENRT/OvS_DPDK_PvP.py             |  2 +-
 lnst/Recipes/ENRT/PingFloodRecipe.py          |  4 +-
 .../ENRT/PingMixins/PingConditionMixins.py    | 34 +++++++++
 .../ENRT/PingMixins/PingEvaluatorMixins.py    | 15 ++++
 lnst/Recipes/ENRT/PingMixins/__init__.py      |  9 +++
 lnst/Recipes/ENRT/VirtOvsVxlanRecipe.py       | 69 ++----------------
 .../ENRT/VirtualBridgeVlansOverBondRecipe.py  | 71 ++-----------------
 .../VirtualOvsBridgeVlansOverBondRecipe.py    | 71 ++-----------------
 lnst/Recipes/ENRT/VlansOverBondRecipe.py      | 63 ++--------------
 lnst/Recipes/ENRT/VlansOverTeamRecipe.py      | 63 ++--------------
 lnst/Recipes/ENRT/VlansRecipe.py              | 63 ++--------------
 lnst/Tests/Ping.py                            | 20 +++---
 24 files changed, 260 insertions(+), 430 deletions(-)
 rename lnst/RecipeCommon/{Perf/Evaluators/BaseEvaluator.py => BaseResultEvaluator.py} (70%)
 create mode 100644 lnst/RecipeCommon/Ping/Evaluators/RatePingEvaluator.py
 create mode 100644 lnst/RecipeCommon/Ping/Evaluators/ZeroPassPingEvaluator.py
 create mode 100644 lnst/RecipeCommon/Ping/Evaluators/__init__.py
 rename lnst/RecipeCommon/{Ping.py => Ping/Recipe.py} (63%)
 create mode 100644 lnst/RecipeCommon/Ping/__init__.py
 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

[PATCH v2 01/15] RecipeCommon.Ping: rename running_ping to ping

[Jan Tluka]

Signed-off-by: Jan Tluka 
---
 lnst/RecipeCommon/Ping.py | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/lnst/RecipeCommon/Ping.py b/lnst/RecipeCommon/Ping.py
index 62cbc2ff..f6ad47e9 100644
--- a/lnst/RecipeCommon/Ping.py
+++ b/lnst/RecipeCommon/Ping.py
@@ -50,20 +50,20 @@ class PingTestAndEvaluate(BaseRecipe):
     def ping_test(self, ping_config):
         results = {}

-        running_ping_array = []
+        ping_array = []
         for pingconf in ping_config:
             ping, client = self.ping_init(pingconf)
-            running_ping = client.prepare_job(ping)
-            running_ping.start(bg = True)
-            running_ping_array.append((pingconf, running_ping))
+            ping = client.prepare_job(ping)
+            ping.start(bg = True)
+            ping_array.append((pingconf, ping))

-        for _, pingjob in running_ping_array:
+        for _, pingjob in ping_array:
             try:
                 pingjob.wait()
             finally:
                 pingjob.kill()

-        for pingconf, pingjob in running_ping_array:
+        for pingconf, pingjob in ping_array:
             result = pingjob.result
             results[pingconf] = result

[PATCH v2 02/15] RecipeCommon.Ping: move prepare_job to ping_init

[Jan Tluka]

Signed-off-by: Jan Tluka 
---
 lnst/RecipeCommon/Ping.py | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/lnst/RecipeCommon/Ping.py b/lnst/RecipeCommon/Ping.py
index f6ad47e9..985f4073 100644
--- a/lnst/RecipeCommon/Ping.py
+++ b/lnst/RecipeCommon/Ping.py
@@ -52,8 +52,7 @@ class PingTestAndEvaluate(BaseRecipe):

ping_array = []
         for pingconf in ping_config:
-            ping, client = self.ping_init(pingconf)
-            ping = client.prepare_job(ping)
+            ping = self.ping_init(pingconf)
             ping.start(bg = True)
             ping_array.append((pingconf, ping))

@@ -72,8 +71,8 @@ class PingTestAndEvaluate(BaseRecipe):
     def ping_init(self, ping_config):
         client = ping_config.client
         kwargs = self._generate_ping_kwargs(ping_config)
-        ping = Ping(**kwargs)
-        return (ping, client)
+        ping = client.prepare_job(Ping(**kwargs))
+        return ping

def ping_evaluate_and_report(self, ping_config, results):
         for pingconf, result in results.items():

[PATCH v2 03/15] RecipeCommon.Ping: remove ping_config from ping_evaluate_and_report

[Jan Tluka]

This parameter is never used. The value is a list of PingConf
objects and these are available in the results parameter anyway.

Signed-off-by: Jan Tluka 
---
 lnst/RecipeCommon/Ping.py                                | 2 +-
 lnst/Recipes/ENRT/BaseEnrtRecipe.py                      | 2 +-
 lnst/Recipes/ENRT/IpsecEspAeadRecipe.py                  | 2 +-
 lnst/Recipes/ENRT/IpsecEspAhCompRecipe.py                | 2 +-
 lnst/Recipes/ENRT/PingFloodRecipe.py                     | 2 +-
 lnst/Recipes/ENRT/VirtOvsVxlanRecipe.py                  | 2 +-
 lnst/Recipes/ENRT/VirtualBridgeVlansOverBondRecipe.py    | 2 +-
 lnst/Recipes/ENRT/VirtualOvsBridgeVlansOverBondRecipe.py | 2 +-
 lnst/Recipes/ENRT/VlansOverBondRecipe.py                 | 2 +-
 lnst/Recipes/ENRT/VlansOverTeamRecipe.py                 | 2 +-
 lnst/Recipes/ENRT/VlansRecipe.py                         | 2 +-
 11 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/lnst/RecipeCommon/Ping.py b/lnst/RecipeCommon/Ping.py
index 985f4073..792b0e52 100644
--- a/lnst/RecipeCommon/Ping.py
+++ b/lnst/RecipeCommon/Ping.py
@@ -74,7 +74,7 @@ class PingTestAndEvaluate(BaseRecipe):
         ping = client.prepare_job(Ping(**kwargs))
         return ping

-    def ping_evaluate_and_report(self, ping_config, results):
+    def ping_evaluate_and_report(self, results):
         for pingconf, result in results.items():
             self.single_ping_evaluate_and_report(pingconf, result)

diff --git a/lnst/Recipes/ENRT/BaseEnrtRecipe.py b/lnst/Recipes/ENRT/BaseEnrtRecipe.py
index a90c837e..edd4da70 100644
--- a/lnst/Recipes/ENRT/BaseEnrtRecipe.py
+++ b/lnst/Recipes/ENRT/BaseEnrtRecipe.py
@@ -99,7 +99,7 @@ class BaseEnrtRecipe(BaseSubConfigMixin, PingTestAndEvaluate, PerfRecipe):
     def do_ping_tests(self, recipe_config):
         for ping_config in self.generate_ping_configurations(recipe_config):
             result = self.ping_test(ping_config)
-            self.ping_evaluate_and_report(ping_config, result)
+            self.ping_evaluate_and_report(result)

def do_perf_tests(self, recipe_config):
         for perf_config in self.generate_perf_configurations(recipe_config):
diff --git a/lnst/Recipes/ENRT/IpsecEspAeadRecipe.py b/lnst/Recipes/ENRT/IpsecEspAeadRecipe.py
index fd8b1bda..6d7e1c1c 100644
--- a/lnst/Recipes/ENRT/IpsecEspAeadRecipe.py
+++ b/lnst/Recipes/ENRT/IpsecEspAeadRecipe.py
@@ -186,7 +186,7 @@ class IpsecEspAeadRecipe(CommonHWSubConfigMixin, BaseEnrtRecipe,
         return (ping_result, pa_config, pa_result)

def ping_evaluate_and_report(self, ping_config, result):
-        super().ping_evaluate_and_report(ping_config, result[0])
+        super().ping_evaluate_and_report(result[0])
         self.packet_assert_evaluate_and_report(result[1], result[2])

def get_dev_by_ip(self, netns, ip):
diff --git a/lnst/Recipes/ENRT/IpsecEspAhCompRecipe.py b/lnst/Recipes/ENRT/IpsecEspAhCompRecipe.py
index 26ecda03..7ecb5ae1 100644
--- a/lnst/Recipes/ENRT/IpsecEspAhCompRecipe.py
+++ b/lnst/Recipes/ENRT/IpsecEspAhCompRecipe.py
@@ -217,7 +217,7 @@ class IpsecEspAhCompRecipe(CommonHWSubConfigMixin, BaseEnrtRecipe,

def ping_evaluate_and_report(self, ping_config, result):
         for res in result:
-            super().ping_evaluate_and_report(ping_config, res[0])
+            super().ping_evaluate_and_report(res[0])
             self.packet_assert_evaluate_and_report(res[1], res[2])

def get_dev_by_ip(self, netns, ip):
diff --git a/lnst/Recipes/ENRT/PingFloodRecipe.py b/lnst/Recipes/ENRT/PingFloodRecipe.py
index bb04e169..1b1dec41 100644
--- a/lnst/Recipes/ENRT/PingFloodRecipe.py
+++ b/lnst/Recipes/ENRT/PingFloodRecipe.py
@@ -43,4 +43,4 @@ class PingFloodRecipe(PingTestAndEvaluate):
         pcfg=PingConf(host1, ip1, host2, ip2, count = cn, interval = iv,
             size = sz)
         result = self.ping_test([pcfg])
-        self.ping_evaluate_and_report(pcfg, result)
+        self.ping_evaluate_and_report(result)
diff --git a/lnst/Recipes/ENRT/VirtOvsVxlanRecipe.py b/lnst/Recipes/ENRT/VirtOvsVxlanRecipe.py
index f7c7511c..c1fc1478 100644
--- a/lnst/Recipes/ENRT/VirtOvsVxlanRecipe.py
+++ b/lnst/Recipes/ENRT/VirtOvsVxlanRecipe.py
@@ -154,7 +154,7 @@ class VirtOvsVxlanRecipe(CommonHWSubConfigMixin, BaseEnrtRecipe):
                     pconf.destination_address)
                 exp_fail.append(cond)
             result = self.ping_test(ping_config, exp_fail)
-            self.ping_evaluate_and_report(ping_config, result)
+            self.ping_evaluate_and_report(result)

def ping_test(self, ping_config, exp_fail):
         results = {}
diff --git a/lnst/Recipes/ENRT/VirtualBridgeVlansOverBondRecipe.py b/lnst/Recipes/ENRT/VirtualBridgeVlansOverBondRecipe.py
index c12d1945..8edf4234 100644
--- a/lnst/Recipes/ENRT/VirtualBridgeVlansOverBondRecipe.py
+++ b/lnst/Recipes/ENRT/VirtualBridgeVlansOverBondRecipe.py
@@ -190,7 +190,7 @@ class VirtualBridgeVlansOverBondRecipe(CommonHWSubConfigMixin,
                     pconf.destination_address)
                 exp_fail.append(cond)
             result = self.ping_test(ping_config, exp_fail)
-            self.ping_evaluate_and_report(ping_config, result)
+            self.ping_evaluate_and_report(result)

def ping_test(self, ping_config, exp_fail):
         results = {}
diff --git a/lnst/Recipes/ENRT/VirtualOvsBridgeVlansOverBondRecipe.py b/lnst/Recipes/ENRT/VirtualOvsBridgeVlansOverBondRecipe.py
index e8efd1d8..7cc9641f 100644
--- a/lnst/Recipes/ENRT/VirtualOvsBridgeVlansOverBondRecipe.py
+++ b/lnst/Recipes/ENRT/VirtualOvsBridgeVlansOverBondRecipe.py
@@ -194,7 +194,7 @@ class VirtualOvsBridgeVlansOverBondRecipe(CommonHWSubConfigMixin,
                     pconf.destination_address)
                 exp_fail.append(cond)
             result = self.ping_test(ping_config, exp_fail)
-            self.ping_evaluate_and_report(ping_config, result)
+            self.ping_evaluate_and_report(result)

def ping_test(self, ping_config, exp_fail):
         results = {}
diff --git a/lnst/Recipes/ENRT/VlansOverBondRecipe.py b/lnst/Recipes/ENRT/VlansOverBondRecipe.py
index f47d0a2c..12ce812d 100644
--- a/lnst/Recipes/ENRT/VlansOverBondRecipe.py
+++ b/lnst/Recipes/ENRT/VlansOverBondRecipe.py
@@ -182,7 +182,7 @@ class VlansOverBondRecipe(CommonHWSubConfigMixin, OffloadSubConfigMixin,
                     pconf.destination_address)
                 exp_fail.append(cond)
             result = self.ping_test(ping_config, exp_fail)
-            self.ping_evaluate_and_report(ping_config, result)
+            self.ping_evaluate_and_report(result)

def ping_test(self, ping_config, exp_fail):
         results = {}
diff --git a/lnst/Recipes/ENRT/VlansOverTeamRecipe.py b/lnst/Recipes/ENRT/VlansOverTeamRecipe.py
index 4b62be8d..679ab35f 100644
--- a/lnst/Recipes/ENRT/VlansOverTeamRecipe.py
+++ b/lnst/Recipes/ENRT/VlansOverTeamRecipe.py
@@ -179,7 +179,7 @@ class VlansOverTeamRecipe(CommonHWSubConfigMixin, OffloadSubConfigMixin,
                     pconf.destination_address)
                 exp_fail.append(cond)
             result = self.ping_test(ping_config, exp_fail)
-            self.ping_evaluate_and_report(ping_config, result)
+            self.ping_evaluate_and_report(result)

def ping_test(self, ping_config, exp_fail):
         results = {}
diff --git a/lnst/Recipes/ENRT/VlansRecipe.py b/lnst/Recipes/ENRT/VlansRecipe.py
index bfaa51dd..38f1f454 100644
--- a/lnst/Recipes/ENRT/VlansRecipe.py
+++ b/lnst/Recipes/ENRT/VlansRecipe.py
@@ -148,7 +148,7 @@ class VlansRecipe(CommonHWSubConfigMixin, OffloadSubConfigMixin,
                     pconf.destination_address)
                 exp_fail.append(cond)
             result = self.ping_test(ping_config, exp_fail)
-            self.ping_evaluate_and_report(ping_config, result)
+            self.ping_evaluate_and_report(result)

def ping_test(self, ping_config, exp_fail):
         results = {}

[PATCH v2 04/15] Tests.Ping: update failure logic

[Jan Tluka]

This patch modifies the code to leave the pass/fail decision to a user of the
test module.

The module will now return with failure only if exit code is higher than 1.
Exit code 1 is used to report that ping did not receive any reply packets
at all or fewer packets than expected are received. Ping exits with exit
code 2 on other errors.

Signed-off-by: Jan Tluka 
---
 lnst/Tests/Ping.py | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/lnst/Tests/Ping.py b/lnst/Tests/Ping.py
index 17de8abf..b6ab9d55 100644
--- a/lnst/Tests/Ping.py
+++ b/lnst/Tests/Ping.py
@@ -53,16 +53,13 @@ class Ping(BaseTestModule):
         except KeyboardInterrupt:
             pass

-        self._res_data["stderr"] = stderr
-
-        if stderr != "":
-            self._res_data["msg"] = "errors reported by ping"
-            logging.error(self._res_data["msg"])
-            logging.error(self._res_data["stderr"])
-
-        if ping_process.returncode > 0:
+        if ping_process.returncode > 1:
             self._res_data["msg"] = "returncode = {}".format(ping_process.returncode)
             logging.error(self._res_data["msg"])
+            if stderr != "":
+                self._res_data["stderr"] = stderr
+                logging.error("errors reported by ping")
+                logging.error(self._res_data["stderr"])
             return False

stat_pttr1 = r'(\d+) packets transmitted, (\d+) received'

[PATCH v2 05/15] Tests.Ping: do not check rtt data if no packets were received

[Jan Tluka]

If no packets are received there's no rtt data reported by ping.
Still, this could be a valid case, e.g. when we expect no packets
should pass.

Signed-off-by: Jan Tluka 
---
 lnst/Tests/Ping.py | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/lnst/Tests/Ping.py b/lnst/Tests/Ping.py
index b6ab9d55..c42e2f6a 100644
--- a/lnst/Tests/Ping.py
+++ b/lnst/Tests/Ping.py
@@ -82,9 +82,10 @@ class Ping(BaseTestModule):

match = re.search(stat_pttr2, stdout)
         if not match:
-            self._res_data = {"msg": "expected pattern not found"}
-            logging.error(self._res_data["msg"])
-            return False
+            if self._res_data['rate'] > 0:
+                self._res_data = {"msg": "expected pattern not found"}
+                logging.error(self._res_data["msg"])
+                return False
         else:
             tmin, tavg, tmax, tmdev = [float(x) for x in match.groups()]
             logging.debug("rtt min "%.3f", avg "%.3f", max "%.3f", "

[PATCH v2 06/15] RecipeCommon.Ping: rename ping_evaluate_and_report method

[Jan Tluka]

This is to match Perf pattern.

Signed-off-by: Jan Tluka 
---
 lnst/RecipeCommon/Ping.py                 | 6 +++---
 lnst/Recipes/ENRT/BaseEnrtRecipe.py       | 2 +-
 lnst/Recipes/ENRT/IpsecEspAeadRecipe.py   | 2 +-
 lnst/Recipes/ENRT/IpsecEspAhCompRecipe.py | 2 +-
 lnst/Recipes/ENRT/PingFloodRecipe.py      | 2 +-
 5 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/lnst/RecipeCommon/Ping.py b/lnst/RecipeCommon/Ping.py
index 792b0e52..320e4ac5 100644
--- a/lnst/RecipeCommon/Ping.py
+++ b/lnst/RecipeCommon/Ping.py
@@ -74,11 +74,11 @@ class PingTestAndEvaluate(BaseRecipe):
         ping = client.prepare_job(Ping(**kwargs))
         return ping

-    def ping_evaluate_and_report(self, results):
+    def ping_report_and_evaluate(self, results):
         for pingconf, result in results.items():
-            self.single_ping_evaluate_and_report(pingconf, result)
+            self.single_ping_report_and_evaluate(pingconf, result)

-    def single_ping_evaluate_and_report(self, ping_config, result):
+    def single_ping_report_and_evaluate(self, ping_config, result):
         fmt = "From: <{0.client.hostid} ({0.client_bind})> To: " \
               "<{0.destination.hostid} ({0.destination_address})>"
         description = fmt.format(ping_config)
diff --git a/lnst/Recipes/ENRT/BaseEnrtRecipe.py b/lnst/Recipes/ENRT/BaseEnrtRecipe.py
index edd4da70..08faa5c4 100644
--- a/lnst/Recipes/ENRT/BaseEnrtRecipe.py
+++ b/lnst/Recipes/ENRT/BaseEnrtRecipe.py
@@ -99,7 +99,7 @@ class BaseEnrtRecipe(BaseSubConfigMixin, PingTestAndEvaluate, PerfRecipe):
     def do_ping_tests(self, recipe_config):
         for ping_config in self.generate_ping_configurations(recipe_config):
             result = self.ping_test(ping_config)
-            self.ping_evaluate_and_report(result)
+            self.ping_report_and_evaluate(result)

def do_perf_tests(self, recipe_config):
         for perf_config in self.generate_perf_configurations(recipe_config):
diff --git a/lnst/Recipes/ENRT/IpsecEspAeadRecipe.py b/lnst/Recipes/ENRT/IpsecEspAeadRecipe.py
index 6d7e1c1c..014891c0 100644
--- a/lnst/Recipes/ENRT/IpsecEspAeadRecipe.py
+++ b/lnst/Recipes/ENRT/IpsecEspAeadRecipe.py
@@ -186,7 +186,7 @@ class IpsecEspAeadRecipe(CommonHWSubConfigMixin, BaseEnrtRecipe,
         return (ping_result, pa_config, pa_result)

def ping_evaluate_and_report(self, ping_config, result):
-        super().ping_evaluate_and_report(result[0])
+        super().ping_report_and_evaluate(result[0])
         self.packet_assert_evaluate_and_report(result[1], result[2])

def get_dev_by_ip(self, netns, ip):
diff --git a/lnst/Recipes/ENRT/IpsecEspAhCompRecipe.py b/lnst/Recipes/ENRT/IpsecEspAhCompRecipe.py
index 7ecb5ae1..eb67e236 100644
--- a/lnst/Recipes/ENRT/IpsecEspAhCompRecipe.py
+++ b/lnst/Recipes/ENRT/IpsecEspAhCompRecipe.py
@@ -217,7 +217,7 @@ class IpsecEspAhCompRecipe(CommonHWSubConfigMixin, BaseEnrtRecipe,

def ping_evaluate_and_report(self, ping_config, result):
         for res in result:
-            super().ping_evaluate_and_report(res[0])
+            super().ping_report_and_evaluate(res[0])
             self.packet_assert_evaluate_and_report(res[1], res[2])

def get_dev_by_ip(self, netns, ip):
diff --git a/lnst/Recipes/ENRT/PingFloodRecipe.py b/lnst/Recipes/ENRT/PingFloodRecipe.py
index 1b1dec41..964f956b 100644
--- a/lnst/Recipes/ENRT/PingFloodRecipe.py
+++ b/lnst/Recipes/ENRT/PingFloodRecipe.py
@@ -43,4 +43,4 @@ class PingFloodRecipe(PingTestAndEvaluate):
         pcfg=PingConf(host1, ip1, host2, ip2, count = cn, interval = iv,
             size = sz)
         result = self.ping_test([pcfg])
-        self.ping_evaluate_and_report(result)
+        self.ping_report_and_evaluate(result)

[PATCH v2 07/15] lnst.RecipeCommon: move Ping Recipe to separate directory

[Jan Tluka]

Signed-off-by: Jan Tluka 
---
 lnst/RecipeCommon/{Ping.py => Ping/Recipe.py} | 0
 lnst/RecipeCommon/Ping/__init__.py            | 0
 lnst/Recipes/ENRT/BaseEnrtRecipe.py           | 2 +-
 lnst/Recipes/ENRT/BasePvPRecipe.py            | 2 +-
 lnst/Recipes/ENRT/OvS_DPDK_PvP.py             | 2 +-
 lnst/Recipes/ENRT/PingFloodRecipe.py          | 2 +-
 6 files changed, 4 insertions(+), 4 deletions(-)
 rename lnst/RecipeCommon/{Ping.py => Ping/Recipe.py} (100%)
 create mode 100644 lnst/RecipeCommon/Ping/__init__.py

diff --git a/lnst/RecipeCommon/Ping.py b/lnst/RecipeCommon/Ping/Recipe.py
similarity index 100%
rename from lnst/RecipeCommon/Ping.py
rename to lnst/RecipeCommon/Ping/Recipe.py
diff --git a/lnst/RecipeCommon/Ping/__init__.py b/lnst/RecipeCommon/Ping/__init__.py
new file mode 100644
index 00000000..e69de29b
diff --git a/lnst/Recipes/ENRT/BaseEnrtRecipe.py b/lnst/Recipes/ENRT/BaseEnrtRecipe.py
index 08faa5c4..d5d52bb9 100644
--- a/lnst/Recipes/ENRT/BaseEnrtRecipe.py
+++ b/lnst/Recipes/ENRT/BaseEnrtRecipe.py
@@ -7,7 +7,7 @@ from lnst.Common.IpAddress import AF_INET, AF_INET6

from lnst.Recipes.ENRT.ConfigMixins.BaseSubConfigMixin import BaseSubConfigMixin

-from lnst.RecipeCommon.Ping import PingTestAndEvaluate, PingConf
+from lnst.RecipeCommon.Ping.Recipe import PingTestAndEvaluate, PingConf
 from lnst.RecipeCommon.Perf.Recipe import Recipe as PerfRecipe
 from lnst.RecipeCommon.Perf.Recipe import RecipeConf as PerfRecipeConf
 from lnst.RecipeCommon.Perf.Measurements import Flow as PerfFlow
diff --git a/lnst/Recipes/ENRT/BasePvPRecipe.py b/lnst/Recipes/ENRT/BasePvPRecipe.py
index 34fa2e3d..89e42938 100644
--- a/lnst/Recipes/ENRT/BasePvPRecipe.py
+++ b/lnst/Recipes/ENRT/BasePvPRecipe.py
@@ -4,7 +4,7 @@ from enum import Enum
 from lnst.Common.LnstError import LnstError
 from lnst.Common.Parameters import Param, IntParam, StrParam
 from lnst.Common.IpAddress import ipaddress
-from lnst.RecipeCommon.Ping import PingTestAndEvaluate
+from lnst.RecipeCommon.Ping.Recipe import PingTestAndEvaluate
 from lnst.Tests import Ping

from lnst.RecipeCommon.Perf.Recipe import Recipe as PerfRecipe
diff --git a/lnst/Recipes/ENRT/OvS_DPDK_PvP.py b/lnst/Recipes/ENRT/OvS_DPDK_PvP.py
index 63a6af45..899057d9 100644
--- a/lnst/Recipes/ENRT/OvS_DPDK_PvP.py
+++ b/lnst/Recipes/ENRT/OvS_DPDK_PvP.py
@@ -9,7 +9,7 @@ from lnst.Controller import HostReq, DeviceReq, RecipeParam
 from lnst.Common.Logs import log_exc_traceback
 from lnst.Common.Parameters import Param, IntParam, StrParam, BoolParam
 from lnst.Common.IpAddress import ipaddress
-from lnst.RecipeCommon.Ping import PingTestAndEvaluate, PingConf
+from lnst.RecipeCommon.Ping.Recipe import PingTestAndEvaluate, PingConf
 from lnst.Tests import Ping
 from lnst.Tests.TestPMD import TestPMD

diff --git a/lnst/Recipes/ENRT/PingFloodRecipe.py b/lnst/Recipes/ENRT/PingFloodRecipe.py
index 964f956b..d5c00ce1 100644
--- a/lnst/Recipes/ENRT/PingFloodRecipe.py
+++ b/lnst/Recipes/ENRT/PingFloodRecipe.py
@@ -1,7 +1,7 @@
 from lnst.Common.Parameters import Param, IntParam, StrParam
 from lnst.Common.IpAddress import ipaddress
 from lnst.Controller import HostReq, DeviceReq, RecipeParam
-from lnst.RecipeCommon.Ping import PingConf, PingTestAndEvaluate
+from lnst.RecipeCommon.Ping.Recipe import PingConf, PingTestAndEvaluate

class PingFloodRecipe(PingTestAndEvaluate):
     driver = StrParam(default='ixgbe')

[PATCH v2 08/15] lnst.RecipeCommon: rename Perf BaseEvaluator to BaseResultEvaluator

[Jan Tluka]

The base class can be later reused to implement non-Perf evaluators.

Signed-off-by: Jan Tluka 
---
 .../Evaluators/BaseEvaluator.py => BaseResultEvaluator.py}    | 2 +-
 lnst/RecipeCommon/Perf/Evaluators/BaselineEvaluator.py        | 4 ++--
 lnst/RecipeCommon/Perf/Evaluators/NonzeroFlowEvaluator.py     | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)
 rename lnst/RecipeCommon/{Perf/Evaluators/BaseEvaluator.py => BaseResultEvaluator.py} (70%)

diff --git a/lnst/RecipeCommon/Perf/Evaluators/BaseEvaluator.py b/lnst/RecipeCommon/BaseResultEvaluator.py
similarity index 70%
rename from lnst/RecipeCommon/Perf/Evaluators/BaseEvaluator.py
rename to lnst/RecipeCommon/BaseResultEvaluator.py
index be828412..f8902b9a 100644
--- a/lnst/RecipeCommon/Perf/Evaluators/BaseEvaluator.py
+++ b/lnst/RecipeCommon/BaseResultEvaluator.py
@@ -1,3 +1,3 @@
-class BaseEvaluator(object):
+class BaseResultEvaluator(object):
     def evaluate_results(self, recipe, results):
         raise NotImplementedError()
diff --git a/lnst/RecipeCommon/Perf/Evaluators/BaselineEvaluator.py b/lnst/RecipeCommon/Perf/Evaluators/BaselineEvaluator.py
index 74fb6d37..20e3887a 100644
--- a/lnst/RecipeCommon/Perf/Evaluators/BaselineEvaluator.py
+++ b/lnst/RecipeCommon/Perf/Evaluators/BaselineEvaluator.py
@@ -1,7 +1,7 @@
-from lnst.RecipeCommon.Perf.Evaluators.BaseEvaluator import BaseEvaluator
+from lnst.RecipeCommon.BaseResultEvaluator import BaseResultEvaluator

-class BaselineEvaluator(BaseEvaluator):
+class BaselineEvaluator(BaseResultEvaluator):
     def evaluate_results(self, recipe, results):
         filtered_results = self.filter_results(recipe, results)

diff --git a/lnst/RecipeCommon/Perf/Evaluators/NonzeroFlowEvaluator.py b/lnst/RecipeCommon/Perf/Evaluators/NonzeroFlowEvaluator.py
index 86dc146f..0d0921cb 100644
--- a/lnst/RecipeCommon/Perf/Evaluators/NonzeroFlowEvaluator.py
+++ b/lnst/RecipeCommon/Perf/Evaluators/NonzeroFlowEvaluator.py
@@ -1,4 +1,4 @@
-from lnst.RecipeCommon.Perf.Evaluators.BaseEvaluator import BaseEvaluator
+from lnst.RecipeCommon.BaseResultEvaluator import BaseResultEvaluator

from lnst.RecipeCommon.Perf.Measurements.BaseFlowMeasurement import (
     FlowMeasurementResults,
@@ -6,7 +6,7 @@ from lnst.RecipeCommon.Perf.Measurements.BaseFlowMeasurement import (
 )

-class NonzeroFlowEvaluator(BaseEvaluator):
+class NonzeroFlowEvaluator(BaseResultEvaluator):
     def evaluate_results(self, recipe, results):
         for flow_results in results:
             result = True

[PATCH v2 09/15] lnst.RecipeCommon.Ping: add Ping evaluators

[Jan Tluka]

This patch adds ZeroPassPingEvaluator and RatePingEvaluator
evaluators that can be used to evaluate results of Ping test
module.

ZeroPassPingEvaluator passes when no packets could be sent.
RatePingEvaluator uses the rate reported by the Ping test module
and compares with the configured min_rate, max_rate and rate
parameters.

Signed-off-by: Jan Tluka 
---
 .../Ping/Evaluators/RatePingEvaluator.py      | 59 +++++++++++++++++++
 .../Ping/Evaluators/ZeroPassPingEvaluator.py  | 21 +++++++
 lnst/RecipeCommon/Ping/Evaluators/__init__.py |  2 +
 3 files changed, 82 insertions(+)
 create mode 100644 lnst/RecipeCommon/Ping/Evaluators/RatePingEvaluator.py
 create mode 100644 lnst/RecipeCommon/Ping/Evaluators/ZeroPassPingEvaluator.py
 create mode 100644 lnst/RecipeCommon/Ping/Evaluators/__init__.py

diff --git a/lnst/RecipeCommon/Ping/Evaluators/RatePingEvaluator.py b/lnst/RecipeCommon/Ping/Evaluators/RatePingEvaluator.py
new file mode 100644
index 00000000..6760d32a
--- /dev/null
+++ b/lnst/RecipeCommon/Ping/Evaluators/RatePingEvaluator.py
@@ -0,0 +1,59 @@
+from lnst.RecipeCommon.BaseResultEvaluator import BaseResultEvaluator
+
+class RatePingEvaluator(BaseResultEvaluator):
+    def __init__(self, min_rate=None, max_rate=None, rate=None):
+        self.min_rate = min_rate
+        self.max_rate = max_rate
+        self.rate = rate
+
+        if min_rate is None and max_rate is None and rate is None:
+            raise Exception('{} requires at least one of min_rate, '
+                'max_rate and rate parameters specified'.format(
+                    self.__class__.__name__)
+                    )
+
+    def evaluate_results(self, recipe, result):
+        result_status = True
+        ping_rate = int(result['rate'])
+
+        result_text = []
+
+        if self.min_rate is not None:
+            rate_text = 'measured rate {} is {} than min_rate({})'
+            if ping_rate < int(self.min_rate):
+                result_status = False
+                result_text.append(
+                    rate_text.format(ping_rate, 'less', self.min_rate)
+                    )
+            else:
+                result_text.append(
+                    rate_text.format(ping_rate, 'more', self.min_rate)
+                    )
+
+        if self.max_rate is not None:
+            rate_text = 'measured rate {} is {} than max_rate({})'
+            if ping_rate > int(self.max_rate):
+                result_status = False
+                result_text.append(
+                    rate_text.format(ping_rate, 'more', self.max_rate)
+                    )
+            else:
+                result_text.append(
+                    rate_text.format(ping_rate, 'less', self.max_rate)
+                    )
+
+        if self.rate is not None:
+            rate_text = 'measured rate {} is {} rate({})'.format(
+                    ping_rate, self.rate)
+
+            if ping_rate != int(self.rate):
+                result_status = False
+                result_text.append(
+                    rate_text.format(ping_rate, 'different than', self.rate)
+                    )
+            else:
+                result_text.append(
+                    rate_text.format(ping_rate, 'equal to', self.rate)
+                    )
+
+        recipe.add_result(result_status, "\n".join(result_text))
diff --git a/lnst/RecipeCommon/Ping/Evaluators/ZeroPassPingEvaluator.py b/lnst/RecipeCommon/Ping/Evaluators/ZeroPassPingEvaluator.py
new file mode 100644
index 00000000..350e157e
--- /dev/null
+++ b/lnst/RecipeCommon/Ping/Evaluators/ZeroPassPingEvaluator.py
@@ -0,0 +1,21 @@
+from lnst.RecipeCommon.BaseResultEvaluator import BaseResultEvaluator
+
+class ZeroPassPingEvaluator(BaseResultEvaluator):
+    def evaluate_results(self, recipe, result):
+        result_status = True
+        trans_packets = int(result['trans_pkts'])
+        recv_packets = int(result['recv_pkts'])
+
+        if recv_packets > 0:
+            result_status = False
+            result_text = [
+                'expected zero packets but {} of {} packets '
+                'were received'.format(
+                    recv_packets, trans_packets)
+                ]
+        else:
+            result_text = ['received {} of {} packets as expected'.format(
+                recv_packets, trans_packets)
+                ]
+
+        recipe.add_result(result_status, "\n".join(result_text))
diff --git a/lnst/RecipeCommon/Ping/Evaluators/__init__.py b/lnst/RecipeCommon/Ping/Evaluators/__init__.py
new file mode 100644
index 00000000..2270cce7
--- /dev/null
+++ b/lnst/RecipeCommon/Ping/Evaluators/__init__.py
@@ -0,0 +1,2 @@
+from lnst.RecipeCommon.Ping.Evaluators.ZeroPassPingEvaluator import ZeroPassPingEvaluator
+from lnst.RecipeCommon.Ping.Evaluators.RatePingEvaluator import RatePingEvaluator

[PATCH v2 10/15] RecipeCommon.Ping.Recipe: add evaluators to PingConf object

[Jan Tluka]

Signed-off-by: Jan Tluka 
---
 lnst/RecipeCommon/Ping/Recipe.py | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/lnst/RecipeCommon/Ping/Recipe.py b/lnst/RecipeCommon/Ping/Recipe.py
index 320e4ac5..2ca86e07 100644
--- a/lnst/RecipeCommon/Ping/Recipe.py
+++ b/lnst/RecipeCommon/Ping/Recipe.py
@@ -13,6 +13,7 @@ class PingConf(object):
         self._count = count
         self._interval = interval
         self._size = size
+        self._evaluators = list()

@property
     def client(self):
@@ -46,6 +47,14 @@ class PingConf(object):
     def size(self, value):
         self._size = value

+    @property
+    def evaluators(self):
+        return self._evaluators
+
+    def register_evaluators(self, evaluators):
+        self._evaluators = list(evaluators)
+
+
 class PingTestAndEvaluate(BaseRecipe):
     def ping_test(self, ping_config):
         results = {}

[PATCH v2 11/15] Recipes.ENRT.BaseEnrtRecipe: register default PingEvaluator

[Jan Tluka]

Unless the get_ping_evaluators() method is overriden, the default Ping
evaluator will be set to RatePingEvaluator with the minimum ping rate of 50%.

v2:

I moved the registration of default Ping evaluator to separate
class method because individual classes may need specific evaluators
for specific Ping configurations.

Signed-off-by: Jan Tluka 
---
 lnst/Recipes/ENRT/BaseEnrtRecipe.py | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/lnst/Recipes/ENRT/BaseEnrtRecipe.py b/lnst/Recipes/ENRT/BaseEnrtRecipe.py
index d5d52bb9..84d192ee 100644
--- a/lnst/Recipes/ENRT/BaseEnrtRecipe.py
+++ b/lnst/Recipes/ENRT/BaseEnrtRecipe.py
@@ -14,6 +14,7 @@ from lnst.RecipeCommon.Perf.Measurements import Flow as PerfFlow
 from lnst.RecipeCommon.Perf.Measurements import IperfFlowMeasurement
 from lnst.RecipeCommon.Perf.Measurements import StatCPUMeasurement
 from lnst.RecipeCommon.Perf.Evaluators import NonzeroFlowEvaluator
+from lnst.RecipeCommon.Ping.Evaluators import RatePingEvaluator

class EnrtConfiguration(object):
     pass
@@ -133,6 +134,9 @@ class BaseEnrtRecipe(BaseSubConfigMixin, PingTestAndEvaluate, PerfRecipe):
                                      size = self.params.ping_psize,
                                      )

+                    ping_evaluators = self.get_ping_evaluators(pconf)
+                    pconf.register_evaluators(ping_evaluators)
+
                     ping_conf_list.append(pconf)

if self.params.ping_bidirect:
@@ -146,6 +150,9 @@ class BaseEnrtRecipe(BaseSubConfigMixin, PingTestAndEvaluate, PerfRecipe):
     def generate_ping_endpoints(self, config):
         return []

+    def get_ping_evaluators(self, pconf):
+        return [RatePingEvaluator(min_rate=50)]
+
     def generate_perf_configurations(self, config):
         for flows in self.generate_flow_combinations(config):
             perf_recipe_conf=dict(

[PATCH v2 11/15] Recipes.ENRT.BaseEnrtRecipe: register default PingEvaluator

[Ondrej Lichtner]

On Tue, Mar 24, 2020 at 05:49:08PM +0100, Jan Tluka wrote:
> Unless the get_ping_evaluators() method is overriden, the default Ping
> evaluator will be set to RatePingEvaluator with the minimum ping rate of 50%.
>
> v2:
>
> I moved the registration of default Ping evaluator to separate
> class method because individual classes may need specific evaluators
> for specific Ping configurations.
> Signed-off-by: Jan Tluka
>
> lnst/Recipes/ENRT/BaseEnrtRecipe.py | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/lnst/Recipes/ENRT/BaseEnrtRecipe.py b/lnst/Recipes/ENRT/BaseEnrtRecipe.py
> index d5d52bb9..84d192ee 100644
> --- a/lnst/Recipes/ENRT/BaseEnrtRecipe.py
> +++ b/lnst/Recipes/ENRT/BaseEnrtRecipe.py
> @@ -14,6 +14,7 @@ from lnst.RecipeCommon.Perf.Measurements import Flow as PerfFlow
>  from lnst.RecipeCommon.Perf.Measurements import IperfFlowMeasurement
>  from lnst.RecipeCommon.Perf.Measurements import StatCPUMeasurement
>  from lnst.RecipeCommon.Perf.Evaluators import NonzeroFlowEvaluator
> +from lnst.RecipeCommon.Ping.Evaluators import RatePingEvaluator
>
> class EnrtConfiguration(object):
>      pass
> @@ -133,6 +134,9 @@ class BaseEnrtRecipe(BaseSubConfigMixin, PingTestAndEvaluate, PerfRecipe):
>                                       size = self.params.ping_psize,
>                                       )
>                ping_evaluators = self.get_ping_evaluators(pconf)
>                pconf.register_evaluators(ping_evaluators)
>                ping_conf_list.append(pconf)
>
>                if self.params.ping_bidirect:
>
> @@ -146,6 +150,9 @@ class BaseEnrtRecipe(BaseSubConfigMixin, PingTestAndEvaluate, PerfRecipe):
>      def generate_ping_endpoints(self, config):
>          return []
> def get_ping_evaluators(self, pconf):
>    return [RatePingEvaluator(min_rate=50)]

This should jus tbe a property to be consistent with how perf evaluators
are defined:

@property
def ping_evaluators():
...

The registration can then look like this:
pconf.register_evaluators(self.ping_evaluators)

instead of being split into two parts of "get" and "register"
>  def generate_perf_configurations(self, config):
>      for flows in self.generate_flow_combinations(config):
>          perf_recipe_conf=dict(
>
> -- 
> 2.21.1
> _______________________________________________
> LNST-developers mailing list -- [e-mail]
> To unsubscribe send an email to [e-mail]
> 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/[e-mail]...

[PATCH v2 11/15] Recipes.ENRT.BaseEnrtRecipe: register default PingEvaluator

[Ondrej Lichtner]

On Thu, Mar 26, 2020 at 11:41:50AM +0100, Ondrej Lichtner wrote:
> On Tue, Mar 24, 2020 at 05:49:08PM +0100, Jan Tluka wrote:
> > Unless the get_ping_evaluators() method is overriden, the default Ping
> > evaluator will be set to RatePingEvaluator with the minimum ping rate of 50%.
> >
> > v2:
> >
> > I moved the registration of default Ping evaluator to separate
> > class method because individual classes may need specific evaluators
> > for specific Ping configurations.
> > Signed-off-by: Jan Tluka
> >
> > lnst/Recipes/ENRT/BaseEnrtRecipe.py | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/lnst/Recipes/ENRT/BaseEnrtRecipe.py b/lnst/Recipes/ENRT/BaseEnrtRecipe.py
> > index d5d52bb9..84d192ee 100644
> > --- a/lnst/Recipes/ENRT/BaseEnrtRecipe.py
> > +++ b/lnst/Recipes/ENRT/BaseEnrtRecipe.py
> > @@ -14,6 +14,7 @@ from lnst.RecipeCommon.Perf.Measurements import Flow as PerfFlow
> >  from lnst.RecipeCommon.Perf.Measurements import IperfFlowMeasurement
> >  from lnst.RecipeCommon.Perf.Measurements import StatCPUMeasurement
> >  from lnst.RecipeCommon.Perf.Evaluators import NonzeroFlowEvaluator
> > +from lnst.RecipeCommon.Ping.Evaluators import RatePingEvaluator
> >
> > class EnrtConfiguration(object):
> >      pass
> > @@ -133,6 +134,9 @@ class BaseEnrtRecipe(BaseSubConfigMixin, PingTestAndEvaluate, PerfRecipe):
> >                                       size = self.params.ping_psize,
> >                                       )
> >                ping_evaluators = self.get_ping_evaluators(pconf)
> >                pconf.register_evaluators(ping_evaluators)
> >                ping_conf_list.append(pconf)
> >
> >                if self.params.ping_bidirect:
> >
> > @@ -146,6 +150,9 @@ class BaseEnrtRecipe(BaseSubConfigMixin, PingTestAndEvaluate, PerfRecipe):
> >      def generate_ping_endpoints(self, config):
> >          return []
> > def get_ping_evaluators(self, pconf):
> >    return [RatePingEvaluator(min_rate=50)]
>
> This should jus tbe a property to be consistent with how perf evaluators
> are defined:

Nevermind... I just noticed that this takes a "pconf" parameter, in that
case, this is a "generator" method and it would, in my opinion, be more
appropriate to rename it to "generate_ping_evaluators".

-Ondrej

[PATCH v2 12/15] RecipeCommon.Ping.Recipe: split report and evaluate

[Jan Tluka]

The report part will now add result about the ping command status.
The evaluation part will be handled separately by configured Ping
evaluators.

Signed-off-by: Jan Tluka 
---
 lnst/RecipeCommon/Ping/Recipe.py | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/lnst/RecipeCommon/Ping/Recipe.py b/lnst/RecipeCommon/Ping/Recipe.py
index 2ca86e07..128d7356 100644
--- a/lnst/RecipeCommon/Ping/Recipe.py
+++ b/lnst/RecipeCommon/Ping/Recipe.py
@@ -72,7 +72,7 @@ class PingTestAndEvaluate(BaseRecipe):
                 pingjob.kill()

for pingconf, pingjob in ping_array:
-            result = pingjob.result
+            result = (pingjob.passed, pingjob.result)
             results[pingconf] = result

return results
@@ -88,15 +88,20 @@ class PingTestAndEvaluate(BaseRecipe):
             self.single_ping_report_and_evaluate(pingconf, result)

def single_ping_report_and_evaluate(self, ping_config, result):
+        self.single_ping_report(ping_config, result)
+
+        self.single_ping_evaluate(ping_config, result)
+
+    def single_ping_report(self, ping_config, result):
         fmt = "From: <{0.client.hostid} ({0.client_bind})> To: " \
               "<{0.destination.hostid} ({0.destination_address})>"
         description = fmt.format(ping_config)
-        if result["rate"] > 50:
-            message = "Ping successful --- " + description
-            self.add_result(True, message, result)
-        else:
-            message = "Ping unsuccessful --- " + description
-            self.add_result(False, message, result)
+        message = "Ping result --- " + description
+        self.add_result(result[0], message, result[1])
+
+    def single_ping_evaluate(self, ping_config, result):
+        for evaluator in ping_config.evaluators:
+            evaluator.evaluate_results(self, result[1])

def _generate_ping_kwargs(self, ping_config):
         kwargs = dict(dst=ping_config.destination_address,

[PATCH v2 13/15] lnst.Recipes.ENRT: add PingMixins

[Jan Tluka]

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 
---
 .../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)
+
+        matching_pairs = []
+        for pair in [(guest1, guest3), (guest2, guest4)]:
+            matching_pairs.extend([pair, pair[::-1]])
+
+        devs = []
+        for dev in (guest1.devices + guest2.devices + guest3.devices +
+            guest4.devices):
+            if src_addr in dev.ips or dst_addr in dev.ips:
+                devs.append(dev)
+        try:
+            return (devs[0].host, devs[1].host) not in matching_pairs
+        except IndexError:
+            return False
diff --git a/lnst/Recipes/ENRT/PingMixins/PingEvaluatorMixins.py b/lnst/Recipes/ENRT/PingMixins/PingEvaluatorMixins.py
new file mode 100644
index 00000000..67f4fa56
--- /dev/null
+++ b/lnst/Recipes/ENRT/PingMixins/PingEvaluatorMixins.py
@@ -0,0 +1,15 @@
+from lnst.RecipeCommon.Ping.Evaluators import (
+    ZeroPassPingEvaluator, RatePingEvaluator)
+
+class PingEvaluatorMixin(object):
+    def get_ping_evaluators(self, ping_config):
+        pass
+
+class VlanPingEvaluatorMixin(PingEvaluatorMixin):
+    def get_ping_evaluators(self, ping_config):
+        endpoints_match = self.ping_endpoints_match(ping_config.client_bind,
+                ping_config.destination_address)
+        if endpoints_match:
+            return [RatePingEvaluator(min_rate=50)]
+        else:
+            return [ZeroPassPingEvaluator()]
diff --git a/lnst/Recipes/ENRT/PingMixins/__init__.py b/lnst/Recipes/ENRT/PingMixins/__init__.py
new file mode 100644
index 00000000..a4f2da09
--- /dev/null
+++ b/lnst/Recipes/ENRT/PingMixins/__init__.py
@@ -0,0 +1,9 @@
+from lnst.Recipes.ENRT.PingMixins.PingEvaluatorMixins import (
+        VlanPingEvaluatorMixin,
+        )
+
+from lnst.Recipes.ENRT.PingMixins.PingConditionMixins import (
+        VlanPingConditionMixin,
+        VirtualVlanPingConditionMixin,
+        )
+

[PATCH v2 13/15] lnst.Recipes.ENRT: add PingMixins

[Jan Tluka]

Tue, Mar 24, 2020 at 05:49:10PM CET, [e-mail] 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
>
> .../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)
>    matching_pairs = []
>    for pair in [(guest1, guest3), (guest2, guest4)]:
>        matching_pairs.extend([pair, pair[::-1]])
>    devs = []
>    for dev in (guest1.devices + guest2.devices + guest3.devices +
>        guest4.devices):
>        if src_addr in dev.ips or dst_addr in dev.ips:
>            devs.append(dev)
>    try:
>        return (devs[0].host, devs[1].host) not in matching_pairs

The condition should be reversed.
>    except IndexError:
>        return False
>
> diff --git a/lnst/Recipes/ENRT/PingMixins/PingEvaluatorMixins.py b/lnst/Recipes/ENRT/PingMixins/PingEvaluatorMixins.py
> new file mode 100644
> index 00000000..67f4fa56
> --- /dev/null
> +++ b/lnst/Recipes/ENRT/PingMixins/PingEvaluatorMixins.py
> @@ -0,0 +1,15 @@
> +from lnst.RecipeCommon.Ping.Evaluators import (
> ZeroPassPingEvaluator, RatePingEvaluator)
>
> +class PingEvaluatorMixin(object):
> def get_ping_evaluators(self, ping_config):
>    pass
>
> +class VlanPingEvaluatorMixin(PingEvaluatorMixin):
> def get_ping_evaluators(self, ping_config):
>    endpoints_match = self.ping_endpoints_match(ping_config.client_bind,
>            ping_config.destination_address)
>    if endpoints_match:
>        return [RatePingEvaluator(min_rate=50)]
>    else:
>        return [ZeroPassPingEvaluator()]
>
> diff --git a/lnst/Recipes/ENRT/PingMixins/__init__.py b/lnst/Recipes/ENRT/PingMixins/__init__.py
> new file mode 100644
> index 00000000..a4f2da09
> --- /dev/null
> +++ b/lnst/Recipes/ENRT/PingMixins/__init__.py
> @@ -0,0 +1,9 @@
> +from lnst.Recipes.ENRT.PingMixins.PingEvaluatorMixins import (
>    VlanPingEvaluatorMixin,
>    )
>
> +from lnst.Recipes.ENRT.PingMixins.PingConditionMixins import (
>    VlanPingConditionMixin,
>    VirtualVlanPingConditionMixin,
>    )
>
> -- 
> 2.21.1

[PATCH v2 13/15] lnst.Recipes.ENRT: add PingMixins

[Ondrej Lichtner]

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
>
> .../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)
>    matching_pairs = []
>    for pair in [(guest1, guest3), (guest2, guest4)]:
>        matching_pairs.extend([pair, pair[::-1]])
>    devs = []
>    for dev in (guest1.devices + guest2.devices + guest3.devices +
>        guest4.devices):
>        if src_addr in dev.ips or dst_addr in dev.ips:
>            devs.append(dev)
>    try:
>        return (devs[0].host, devs[1].host) not in matching_pairs
>    except IndexError:
>        return False
>
> diff --git a/lnst/Recipes/ENRT/PingMixins/PingEvaluatorMixins.py b/lnst/Recipes/ENRT/PingMixins/PingEvaluatorMixins.py
> new file mode 100644
> index 00000000..67f4fa56
> --- /dev/null
> +++ b/lnst/Recipes/ENRT/PingMixins/PingEvaluatorMixins.py
> @@ -0,0 +1,15 @@
> +from lnst.RecipeCommon.Ping.Evaluators import (
> ZeroPassPingEvaluator, RatePingEvaluator)
>
> +class PingEvaluatorMixin(object):
> def get_ping_evaluators(self, ping_config):
>    pass
>
> +class VlanPingEvaluatorMixin(PingEvaluatorMixin):
> def get_ping_evaluators(self, ping_config):
>    endpoints_match = self.ping_endpoints_match(ping_config.client_bind,
>            ping_config.destination_address)
>    if endpoints_match:
>        return [RatePingEvaluator(min_rate=50)]
>    else:
>        return [ZeroPassPingEvaluator()]
>
> diff --git a/lnst/Recipes/ENRT/PingMixins/__init__.py b/lnst/Recipes/ENRT/PingMixins/__init__.py
> new file mode 100644
> index 00000000..a4f2da09
> --- /dev/null
> +++ b/lnst/Recipes/ENRT/PingMixins/__init__.py
> @@ -0,0 +1,9 @@
> +from lnst.Recipes.ENRT.PingMixins.PingEvaluatorMixins import (
>    VlanPingEvaluatorMixin,
>    )
>
> +from lnst.Recipes.ENRT.PingMixins.PingConditionMixins import (
>    VlanPingConditionMixin,
>    VirtualVlanPingConditionMixin,
>    )
>
> -- 
> 2.21.1
> _______________________________________________
> LNST-developers mailing list -- [e-mail]
> To unsubscribe send an email to [e-mail]
> 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/[e-mail]...

you have a whitespace error here again :)

Applying: lnst.Recipes.ENRT: add PingMixins
.git/rebase-apply/patch:85: new blank line at EOF.
+
warning: 1 line adds whitespace errors.

-Ondrej

[PATCH v2 13/15] lnst.Recipes.ENRT: add PingMixins

[Ondrej Lichtner]

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
>
> .../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?
>    matching_pairs = []
>    for pair in [(guest1, guest3), (guest2, guest4)]:
>        matching_pairs.extend([pair, pair[::-1]])
>    devs = []
>    for dev in (guest1.devices + guest2.devices + guest3.devices +
>        guest4.devices):
>        if src_addr in dev.ips or dst_addr in dev.ips:
>            devs.append(dev)
>    try:
>        return (devs[0].host, devs[1].host) not in matching_pairs
>    except IndexError:
>        return False
>
> diff --git a/lnst/Recipes/ENRT/PingMixins/PingEvaluatorMixins.py b/lnst/Recipes/ENRT/PingMixins/PingEvaluatorMixins.py
> new file mode 100644
> index 00000000..67f4fa56
> --- /dev/null
> +++ b/lnst/Recipes/ENRT/PingMixins/PingEvaluatorMixins.py
> @@ -0,0 +1,15 @@
> +from lnst.RecipeCommon.Ping.Evaluators import (
> ZeroPassPingEvaluator, RatePingEvaluator)
>
> +class PingEvaluatorMixin(object):
> def get_ping_evaluators(self, ping_config):
>    pass
>
> +class VlanPingEvaluatorMixin(PingEvaluatorMixin):
> def get_ping_evaluators(self, ping_config):
>    endpoints_match = self.ping_endpoints_match(ping_config.client_bind,
>            ping_config.destination_address)
>    if endpoints_match:
>        return [RatePingEvaluator(min_rate=50)]
>    else:
>        return [ZeroPassPingEvaluator()]
>
> diff --git a/lnst/Recipes/ENRT/PingMixins/__init__.py b/lnst/Recipes/ENRT/PingMixins/__init__.py
> new file mode 100644
> index 00000000..a4f2da09
> --- /dev/null
> +++ b/lnst/Recipes/ENRT/PingMixins/__init__.py
> @@ -0,0 +1,9 @@
> +from lnst.Recipes.ENRT.PingMixins.PingEvaluatorMixins import (
>    VlanPingEvaluatorMixin,
>    )
>
> +from lnst.Recipes.ENRT.PingMixins.PingConditionMixins import (
>    VlanPingConditionMixin,
>    VirtualVlanPingConditionMixin,
>    )
>
> -- 
> 2.21.1
> _______________________________________________
> LNST-developers mailing list -- [e-mail]
> To unsubscribe send an email to [e-mail]
> 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/[e-mail]...

[PATCH v2 13/15] lnst.Recipes.ENRT: add PingMixins

[Jan Tluka]

Thu, Mar 26, 2020 at 11:56:49AM CET, [e-mail] 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
> >
> > .../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.
> >    matching_pairs = []
> >    for pair in [(guest1, guest3), (guest2, guest4)]:
> >        matching_pairs.extend([pair, pair[::-1]])
> >    devs = []
> >    for dev in (guest1.devices + guest2.devices + guest3.devices +
> >        guest4.devices):
> >        if src_addr in dev.ips or dst_addr in dev.ips:
> >            devs.append(dev)
> >    try:
> >        return (devs[0].host, devs[1].host) not in matching_pairs
> >    except IndexError:
> >        return False
> >
> > diff --git a/lnst/Recipes/ENRT/PingMixins/PingEvaluatorMixins.py b/lnst/Recipes/ENRT/PingMixins/PingEvaluatorMixins.py
> > new file mode 100644
> > index 00000000..67f4fa56
> > --- /dev/null
> > +++ b/lnst/Recipes/ENRT/PingMixins/PingEvaluatorMixins.py
> > @@ -0,0 +1,15 @@
> > +from lnst.RecipeCommon.Ping.Evaluators import (
> > ZeroPassPingEvaluator, RatePingEvaluator)
> >
> > +class PingEvaluatorMixin(object):
> > def get_ping_evaluators(self, ping_config):
> >    pass
> >
> > +class VlanPingEvaluatorMixin(PingEvaluatorMixin):
> > def get_ping_evaluators(self, ping_config):
> >    endpoints_match = self.ping_endpoints_match(ping_config.client_bind,
> >            ping_config.destination_address)
> >    if endpoints_match:
> >        return [RatePingEvaluator(min_rate=50)]
> >    else:
> >        return [ZeroPassPingEvaluator()]
> >
> > diff --git a/lnst/Recipes/ENRT/PingMixins/__init__.py b/lnst/Recipes/ENRT/PingMixins/__init__.py
> > new file mode 100644
> > index 00000000..a4f2da09
> > --- /dev/null
> > +++ b/lnst/Recipes/ENRT/PingMixins/__init__.py
> > @@ -0,0 +1,9 @@
> > +from lnst.Recipes.ENRT.PingMixins.PingEvaluatorMixins import (
> >    VlanPingEvaluatorMixin,
> >    )
> >
> > +from lnst.Recipes.ENRT.PingMixins.PingConditionMixins import (
> >    VlanPingConditionMixin,
> >    VirtualVlanPingConditionMixin,
> >    )
> >
> > -- 
> > 2.21.1
> > _______________________________________________
> > LNST-developers mailing list -- [e-mail]
> > To unsubscribe send an email to [e-mail]
> > 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/[e-mail]...

[PATCH v2 13/15] lnst.Recipes.ENRT: add PingMixins

[Ondrej Lichtner]

On Thu, Mar 26, 2020 at 12:43:49PM +0100, Jan Tluka wrote:
> Thu, Mar 26, 2020 at 11:56:49AM CET, [e-mail] 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
> > >
> > > .../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.

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.

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.

-Ondrej

[PATCH v2 13/15] lnst.Recipes.ENRT: add PingMixins

[Jan Tluka]

Thu, Mar 26, 2020 at 04:30:51PM CET, [e-mail] wrote:
> On Thu, Mar 26, 2020 at 12:43:49PM +0100, Jan Tluka wrote:
> > Thu, Mar 26, 2020 at 11:56:49AM CET, [e-mail] 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
> > > >
> > > > .../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

[PATCH v2 13/15] lnst.Recipes.ENRT: add PingMixins

[Ondrej Lichtner]

On Thu, Mar 26, 2020 at 05:11:34PM +0100, Jan Tluka wrote:
> Thu, Mar 26, 2020 at 04:30:51PM CET, [e-mail] wrote:
> > On Thu, Mar 26, 2020 at 12:43:49PM +0100, Jan Tluka wrote:
> > > Thu, Mar 26, 2020 at 11:56:49AM CET, [e-mail] 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
> > > > >
> > > > > .../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.
> > 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

[PATCH v2 13/15] lnst.Recipes.ENRT: add PingMixins

[Jan Tluka]

Thu, Mar 26, 2020 at 05:28:45PM CET, [e-mail] wrote:
> On Thu, Mar 26, 2020 at 05:11:34PM +0100, Jan Tluka wrote:
> > Thu, Mar 26, 2020 at 04:30:51PM CET, [e-mail] wrote:
> > > On Thu, Mar 26, 2020 at 12:43:49PM +0100, Jan Tluka wrote:
> > > > Thu, Mar 26, 2020 at 11:56:49AM CET, [e-mail] 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
> > > > > >
> > > > > > .../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

[PATCH v2 13/15] lnst.Recipes.ENRT: add PingMixins

[Ondrej Lichtner]

On Fri, Mar 27, 2020 at 09:03:07AM +0100, Jan Tluka wrote:
> Thu, Mar 26, 2020 at 05:28:45PM CET, [e-mail] wrote:
> > On Thu, Mar 26, 2020 at 05:11:34PM +0100, Jan Tluka wrote:
> > > Thu, Mar 26, 2020 at 04:30:51PM CET, [e-mail] 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

[PATCH v2 13/15] lnst.Recipes.ENRT: add PingMixins

[Jan Tluka]

Fri, Mar 27, 2020 at 09:34:25AM CET, [e-mail] wrote:
> On Fri, Mar 27, 2020 at 09:03:07AM +0100, Jan Tluka wrote:
> > Thu, Mar 26, 2020 at 05:28:45PM CET, [e-mail] wrote:
> > > On Thu, Mar 26, 2020 at 05:11:34PM +0100, Jan Tluka wrote:
> > > > Thu, Mar 26, 2020 at 04:30:51PM CET, [e-mail] 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

[PATCH v2 14/15] lnst.Recipes.ENRT: use PingMixins to override Ping evaluators

[Jan Tluka]

This patch removes some code duplicity by using code from the base
and Ping mixin classes.

For pings within the vlan the RatePingEvaluator is used, for cross-vlan
ping the ZeroPassPingEvaluator is used.

Signed-off-by: Jan Tluka 
---
 lnst/Recipes/ENRT/VirtOvsVxlanRecipe.py       | 69 ++----------------
 .../ENRT/VirtualBridgeVlansOverBondRecipe.py  | 71 ++-----------------
 .../VirtualOvsBridgeVlansOverBondRecipe.py    | 71 ++-----------------
 lnst/Recipes/ENRT/VlansOverBondRecipe.py      | 63 ++--------------
 lnst/Recipes/ENRT/VlansOverTeamRecipe.py      | 63 ++--------------
 lnst/Recipes/ENRT/VlansRecipe.py              | 63 ++--------------
 6 files changed, 36 insertions(+), 364 deletions(-)

diff --git a/lnst/Recipes/ENRT/VirtOvsVxlanRecipe.py b/lnst/Recipes/ENRT/VirtOvsVxlanRecipe.py
index c1fc1478..f661c5d7 100644
--- a/lnst/Recipes/ENRT/VirtOvsVxlanRecipe.py
+++ b/lnst/Recipes/ENRT/VirtOvsVxlanRecipe.py
@@ -4,9 +4,14 @@ from lnst.Controller import HostReq, DeviceReq, RecipeParam
 from lnst.Recipes.ENRT.BaseEnrtRecipe import BaseEnrtRecipe
 from lnst.Recipes.ENRT.ConfigMixins.CommonHWSubConfigMixin import (
     CommonHWSubConfigMixin)
+from lnst.Recipes.ENRT.PingMixins import (
+        VlanPingEvaluatorMixin,
+        VirtualVlanPingConditionMixin,
+        )
 from lnst.Devices import OvsBridgeDevice

-class VirtOvsVxlanRecipe(CommonHWSubConfigMixin, BaseEnrtRecipe):
+class VirtOvsVxlanRecipe(VlanPingEvaluatorMixin, VirtualVlanPingConditionMixin,
+    CommonHWSubConfigMixin, BaseEnrtRecipe):
     host1 = HostReq()
     host1.eth0 = DeviceReq(label="to_switch", driver=RecipeParam("driver"))
     host1.tap0 = DeviceReq(label="to_guest1")
@@ -146,68 +151,6 @@ class VirtOvsVxlanRecipe(CommonHWSubConfigMixin, BaseEnrtRecipe):
     def parallel_stream_qdisc_hw_config_dev_list(self):
         return [self.matched.host1.eth0, self.matched.host2.eth0]

-    def do_ping_tests(self, recipe_config):
-        for ping_config in self.generate_ping_configurations(recipe_config):
-            exp_fail = []
-            for pconf in ping_config:
-                cond = self.tun_id_match(pconf.client_bind,
-                    pconf.destination_address)
-                exp_fail.append(cond)
-            result = self.ping_test(ping_config, exp_fail)
-            self.ping_evaluate_and_report(result)
-
-    def ping_test(self, ping_config, exp_fail):
-        results = {}
-
-        running_ping_array = []
-        for pingconf, fail in zip(ping_config, exp_fail):
-            ping, client = self.ping_init(pingconf)
-            running_ping = client.prepare_job(ping, fail=fail)
-            running_ping.start(bg = True)
-            running_ping_array.append((pingconf, running_ping))
-
-        for _, pingjob in running_ping_array:
-            try:
-                pingjob.wait()
-            finally:
-                pingjob.kill()
-
-        for pingconf, pingjob in running_ping_array:
-            result = pingjob.result
-            passed = pingjob.passed
-            results[pingconf] = (result, passed)
-
-        return results
-
-    def single_ping_evaluate_and_report(self, ping_config, result):
-        fmt = "From: <{0.client.hostid} ({0.client_bind})> To: " \
-              "<{0.destination.hostid} ({0.destination_address})>"
-        description = fmt.format(ping_config)
-        if result[0].get("rate", 0) > 50:
-            message = "Ping successful --- " + description
-            self.add_result(result[1], message, result[0])
-        else:
-            message = "Ping unsuccessful --- " + description
-            self.add_result(result[1], message, result[0])
-
-    def tun_id_match(self, src_addr, dst_addr):
-        guest1, guest2, guest3, guest4 = (self.matched.guest1,
-            self.matched.guest2, self.matched.guest3, self.matched.guest4)
-
-        matching_pairs = []
-        for pair in [(guest1, guest3), (guest2, guest4)]:
-            matching_pairs.extend([pair, pair[::-1]])
-
-        devs = []
-        for dev in (guest1.devices + guest2.devices + guest3.devices +
-            guest4.devices):
-            if src_addr in dev.ips or dst_addr in dev.ips:
-                devs.append(dev)
-        try:
-            return (devs[0].host, devs[1].host) not in matching_pairs
-        except IndexError:
-            return False
-
     def hw_config(self, config):
         host1, host2, guest1, guest2, guest3, guest4 = (self.matched.host1,
             self.matched.host2, self.matched.guest1, self.matched.guest2,
diff --git a/lnst/Recipes/ENRT/VirtualBridgeVlansOverBondRecipe.py b/lnst/Recipes/ENRT/VirtualBridgeVlansOverBondRecipe.py
index 8edf4234..16230046 100644
--- a/lnst/Recipes/ENRT/VirtualBridgeVlansOverBondRecipe.py
+++ b/lnst/Recipes/ENRT/VirtualBridgeVlansOverBondRecipe.py
@@ -7,12 +7,16 @@ from lnst.Recipes.ENRT.ConfigMixins.OffloadSubConfigMixin import (
     OffloadSubConfigMixin)
 from lnst.Recipes.ENRT.ConfigMixins.CommonHWSubConfigMixin import (
     CommonHWSubConfigMixin)
+from lnst.Recipes.ENRT.PingMixins import (
+        VlanPingEvaluatorMixin,
+        VirtualVlanPingConditionMixin,
+        )
 from lnst.Devices import VlanDevice
 from lnst.Devices import BondDevice
 from lnst.Devices import BridgeDevice

-class VirtualBridgeVlansOverBondRecipe(CommonHWSubConfigMixin,
-    OffloadSubConfigMixin, BaseEnrtRecipe):
+class VirtualBridgeVlansOverBondRecipe(VlanPingEvaluatorMixin, VirtualVlanPingConditionMixin,
+    CommonHWSubConfigMixin, OffloadSubConfigMixin, BaseEnrtRecipe):
     host1 = HostReq()
     host1.eth0 = DeviceReq(label="to_switch", driver=RecipeParam("driver"))
     host1.eth1 = DeviceReq(label="to_switch", driver=RecipeParam("driver"))
@@ -181,69 +185,6 @@ class VirtualBridgeVlansOverBondRecipe(CommonHWSubConfigMixin,

self.ctl.wait_for_condition(condition, timeout=5)

-    def do_ping_tests(self, recipe_config):
-        for ping_config in self.generate_ping_configurations(
-            recipe_config):
-            exp_fail = []
-            for pconf in ping_config:
-                cond = self.vlan_id_match(pconf.client_bind,
-                    pconf.destination_address)
-                exp_fail.append(cond)
-            result = self.ping_test(ping_config, exp_fail)
-            self.ping_evaluate_and_report(result)
-
-    def ping_test(self, ping_config, exp_fail):
-        results = {}
-
-        running_ping_array = []
-        for pingconf, fail in zip(ping_config, exp_fail):
-            ping, client = self.ping_init(pingconf)
-            running_ping = client.prepare_job(ping, fail=fail)
-            running_ping.start(bg = True)
-            running_ping_array.append((pingconf, running_ping))
-
-        for _, pingjob in running_ping_array:
-            try:
-                pingjob.wait()
-            finally:
-                pingjob.kill()
-
-        for pingconf, pingjob in running_ping_array:
-            result = pingjob.result
-            passed = pingjob.passed
-            results[pingconf] = (result, passed)
-
-        return results
-
-    def single_ping_evaluate_and_report(self, ping_config, result):
-        fmt = "From: <{0.client.hostid} ({0.client_bind})> To: " \
-              "<{0.destination.hostid} ({0.destination_address})>"
-        description = fmt.format(ping_config)
-        if result[0].get("rate", 0) > 50:
-            message = "Ping successful --- " + description
-            self.add_result(result[1], message, result[0])
-        else:
-            message = "Ping unsuccessful --- " + description
-            self.add_result(result[1], message, result[0])
-
-    def vlan_id_match(self, src_addr, dst_addr):
-        guest1, guest2, guest3, guest4 = (self.matched.guest1,
-            self.matched.guest2, self.matched.guest3, self.matched.guest4)
-
-        matching_pairs = []
-        for pair in [(guest1, guest3), (guest2, guest4)]:
-            matching_pairs.extend([pair, pair[::-1]])
-
-        devs = []
-        for dev in (guest1.devices + guest2.devices + guest3.devices +
-            guest4.devices):
-            if src_addr in dev.ips or dst_addr in dev.ips:
-                devs.append(dev)
-        try:
-            return (devs[0].host, devs[1].host) not in matching_pairs
-        except IndexError:
-            return False
-
     @property
     def offload_nics(self):
         host1, host2, guest1, guest2, guest3, guest4 = (self.matched.host1,
diff --git a/lnst/Recipes/ENRT/VirtualOvsBridgeVlansOverBondRecipe.py b/lnst/Recipes/ENRT/VirtualOvsBridgeVlansOverBondRecipe.py
index 7cc9641f..22918957 100644
--- a/lnst/Recipes/ENRT/VirtualOvsBridgeVlansOverBondRecipe.py
+++ b/lnst/Recipes/ENRT/VirtualOvsBridgeVlansOverBondRecipe.py
@@ -7,10 +7,14 @@ from lnst.Recipes.ENRT.ConfigMixins.OffloadSubConfigMixin import (
     OffloadSubConfigMixin)
 from lnst.Recipes.ENRT.ConfigMixins.CommonHWSubConfigMixin import (
     CommonHWSubConfigMixin)
+from lnst.Recipes.ENRT.PingMixins import (
+        VlanPingEvaluatorMixin,
+        VirtualVlanPingConditionMixin,
+        )
 from lnst.Devices import OvsBridgeDevice

-class VirtualOvsBridgeVlansOverBondRecipe(CommonHWSubConfigMixin,
-    OffloadSubConfigMixin, BaseEnrtRecipe):
+class VirtualOvsBridgeVlansOverBondRecipe(VlanPingEvaluatorMixin, VirtualVlanPingConditionMixin,
+    CommonHWSubConfigMixin, OffloadSubConfigMixin, BaseEnrtRecipe):
     host1 = HostReq()
     host1.eth0 = DeviceReq(label="to_switch", driver=RecipeParam("driver"))
     host1.eth1 = DeviceReq(label="to_switch", driver=RecipeParam("driver"))
@@ -184,66 +188,3 @@ class VirtualOvsBridgeVlansOverBondRecipe(CommonHWSubConfigMixin,
     def parallel_stream_qdisc_hw_config_dev_list(self):
         return [self.matched.host1.eth0, self.matched.host1.eth1,
             self.matched.host2.eth0, self.matched.host2.eth1]
-
-    def do_ping_tests(self, recipe_config):
-        for ping_config in self.generate_ping_configurations(
-            recipe_config):
-            exp_fail = []
-            for pconf in ping_config:
-                cond = self.vlan_id_match(pconf.client_bind,
-                    pconf.destination_address)
-                exp_fail.append(cond)
-            result = self.ping_test(ping_config, exp_fail)
-            self.ping_evaluate_and_report(result)
-
-    def ping_test(self, ping_config, exp_fail):
-        results = {}
-
-        running_ping_array = []
-        for pingconf, fail in zip(ping_config, exp_fail):
-            ping, client = self.ping_init(pingconf)
-            running_ping = client.prepare_job(ping, fail=fail)
-            running_ping.start(bg = True)
-            running_ping_array.append((pingconf, running_ping))
-
-        for _, pingjob in running_ping_array:
-            try:
-                pingjob.wait()
-            finally:
-                pingjob.kill()
-
-        for pingconf, pingjob in running_ping_array:
-            result = pingjob.result
-            passed = pingjob.passed
-            results[pingconf] = (result, passed)
-
-        return results
-
-    def single_ping_evaluate_and_report(self, ping_config, result):
-        fmt = "From: <{0.client.hostid} ({0.client_bind})> To: " \
-              "<{0.destination.hostid} ({0.destination_address})>"
-        description = fmt.format(ping_config)
-        if result[0].get("rate", 0) > 50:
-            message = "Ping successful --- " + description
-            self.add_result(result[1], message, result[0])
-        else:
-            message = "Ping unsuccessful --- " + description
-            self.add_result(result[1], message, result[0])
-
-    def vlan_id_match(self, src_addr, dst_addr):
-        guest1, guest2, guest3, guest4 = (self.matched.guest1,
-            self.matched.guest2, self.matched.guest3, self.matched.guest4)
-
-        matching_pairs = []
-        for pair in [(guest1, guest3), (guest2, guest4)]:
-            matching_pairs.extend([pair, pair[::-1]])
-
-        devs = []
-        for dev in (guest1.devices + guest2.devices + guest3.devices +
-                    guest4.devices):
-            if src_addr in dev.ips or dst_addr in dev.ips:
-                devs.append(dev)
-        try:
-            return (devs[0].host, devs[1].host) not in matching_pairs
-        except IndexError:
-            return False
diff --git a/lnst/Recipes/ENRT/VlansOverBondRecipe.py b/lnst/Recipes/ENRT/VlansOverBondRecipe.py
index 12ce812d..d059a104 100644
--- a/lnst/Recipes/ENRT/VlansOverBondRecipe.py
+++ b/lnst/Recipes/ENRT/VlansOverBondRecipe.py
@@ -9,8 +9,13 @@ from lnst.Recipes.ENRT.ConfigMixins.CommonHWSubConfigMixin import (
 from lnst.Devices import VlanDevice
 from lnst.Devices.VlanDevice import VlanDevice as Vlan
 from lnst.Devices import BondDevice
+from lnst.Recipes.ENRT.PingMixins import (
+        VlanPingEvaluatorMixin,
+        VlanPingConditionMixin,
+        )

-class VlansOverBondRecipe(CommonHWSubConfigMixin, OffloadSubConfigMixin,
+class VlansOverBondRecipe(VlanPingEvaluatorMixin, VlanPingConditionMixin,
+    CommonHWSubConfigMixin, OffloadSubConfigMixin,
     BaseEnrtRecipe):
     host1 = HostReq()
     host1.eth0 = DeviceReq(label="net1", driver=RecipeParam("driver"))
@@ -172,59 +177,3 @@ class VlansOverBondRecipe(CommonHWSubConfigMixin, OffloadSubConfigMixin,
     def parallel_stream_qdisc_hw_config_dev_list(self):
         host1, host2 = self.matched.host1, self.matched.host2
         return [host1.eth0, host1.eth1, host2.eth0]
-
-    def do_ping_tests(self, recipe_config):
-        for ping_config in self.generate_ping_configurations(
-            recipe_config):
-            exp_fail = []
-            for pconf in ping_config:
-                cond = self.vlan_id_same(pconf.client_bind,
-                    pconf.destination_address)
-                exp_fail.append(cond)
-            result = self.ping_test(ping_config, exp_fail)
-            self.ping_evaluate_and_report(result)
-
-    def ping_test(self, ping_config, exp_fail):
-        results = {}
-
-        running_ping_array = []
-        for pingconf, fail in zip(ping_config, exp_fail):
-            ping, client = self.ping_init(pingconf)
-            running_ping = client.prepare_job(ping, fail=fail)
-            running_ping.start(bg = True)
-            running_ping_array.append((pingconf, running_ping))
-
-        for _, pingjob in running_ping_array:
-            try:
-                pingjob.wait()
-            finally:
-                pingjob.kill()
-
-        for pingconf, pingjob in running_ping_array:
-            result = pingjob.result
-            passed = pingjob.passed
-            results[pingconf] = (result, passed)
-
-        return results
-
-    def single_ping_evaluate_and_report(self, ping_config, result):
-        fmt = "From: <{0.client.hostid} ({0.client_bind})> To: " \
-              "<{0.destination.hostid} ({0.destination_address})>"
-        description = fmt.format(ping_config)
-        if result[0].get("rate", 0) > 50:
-            message = "Ping successful --- " + description
-            self.add_result(result[1], message, result[0])
-        else:
-            message = "Ping unsuccessful --- " + description
-            self.add_result(result[1], message, result[0])
-
-    def vlan_id_same(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
diff --git a/lnst/Recipes/ENRT/VlansOverTeamRecipe.py b/lnst/Recipes/ENRT/VlansOverTeamRecipe.py
index 679ab35f..761f2959 100644
--- a/lnst/Recipes/ENRT/VlansOverTeamRecipe.py
+++ b/lnst/Recipes/ENRT/VlansOverTeamRecipe.py
@@ -9,8 +9,13 @@ from lnst.Recipes.ENRT.ConfigMixins.CommonHWSubConfigMixin import (
 from lnst.Devices import VlanDevice
 from lnst.Devices.VlanDevice import VlanDevice as Vlan
 from lnst.Devices import TeamDevice
+from lnst.Recipes.ENRT.PingMixins import (
+        VlanPingEvaluatorMixin,
+        VlanPingConditionMixin,
+        )

-class VlansOverTeamRecipe(CommonHWSubConfigMixin, OffloadSubConfigMixin,
+class VlansOverTeamRecipe(VlanPingEvaluatorMixin, VlanPingConditionMixin,
+    CommonHWSubConfigMixin, OffloadSubConfigMixin,
     BaseEnrtRecipe):
     host1 = HostReq()
     host1.eth0 = DeviceReq(label="tnet", driver=RecipeParam("driver"))
@@ -169,59 +174,3 @@ class VlansOverTeamRecipe(CommonHWSubConfigMixin, OffloadSubConfigMixin,
     def parallel_stream_qdisc_hw_config_dev_list(self):
         host1, host2 = self.matched.host1, self.matched.host2
         return [host1.eth0, host1.eth1, host2.eth0]
-
-    def do_ping_tests(self, recipe_config):
-        for ping_config in self.generate_ping_configurations(
-            recipe_config):
-            exp_fail = []
-            for pconf in ping_config:
-                cond = self.vlan_id_same(pconf.client_bind,
-                    pconf.destination_address)
-                exp_fail.append(cond)
-            result = self.ping_test(ping_config, exp_fail)
-            self.ping_evaluate_and_report(result)
-
-    def ping_test(self, ping_config, exp_fail):
-        results = {}
-
-        running_ping_array = []
-        for pingconf, fail in zip(ping_config, exp_fail):
-            ping, client = self.ping_init(pingconf)
-            running_ping = client.prepare_job(ping, fail=fail)
-            running_ping.start(bg = True)
-            running_ping_array.append((pingconf, running_ping))
-
-        for _, pingjob in running_ping_array:
-            try:
-                pingjob.wait()
-            finally:
-                pingjob.kill()
-
-        for pingconf, pingjob in running_ping_array:
-            result = pingjob.result
-            passed = pingjob.passed
-            results[pingconf] = (result, passed)
-
-        return results
-
-    def single_ping_evaluate_and_report(self, ping_config, result):
-        fmt = "From: <{0.client.hostid} ({0.client_bind})> To: " \
-              "<{0.destination.hostid} ({0.destination_address})>"
-        description = fmt.format(ping_config)
-        if result[0].get("rate", 0) > 50:
-            message = "Ping successful --- " + description
-            self.add_result(result[1], message, result[0])
-        else:
-            message = "Ping unsuccessful --- " + description
-            self.add_result(result[1], message, result[0])
-
-    def vlan_id_same(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
diff --git a/lnst/Recipes/ENRT/VlansRecipe.py b/lnst/Recipes/ENRT/VlansRecipe.py
index 38f1f454..f75745d6 100644
--- a/lnst/Recipes/ENRT/VlansRecipe.py
+++ b/lnst/Recipes/ENRT/VlansRecipe.py
@@ -6,9 +6,14 @@ from lnst.Recipes.ENRT.ConfigMixins.OffloadSubConfigMixin import (
     OffloadSubConfigMixin)
 from lnst.Recipes.ENRT.ConfigMixins.CommonHWSubConfigMixin import (
     CommonHWSubConfigMixin)
+from lnst.Recipes.ENRT.PingMixins import (
+        VlanPingEvaluatorMixin,
+        VlanPingConditionMixin,
+        )
 from lnst.Devices import VlanDevice

-class VlansRecipe(CommonHWSubConfigMixin, OffloadSubConfigMixin,
+class VlansRecipe(VlanPingEvaluatorMixin, VlanPingConditionMixin,
+    CommonHWSubConfigMixin, OffloadSubConfigMixin,
     BaseEnrtRecipe):
     host1 = HostReq()
     host1.eth0 = DeviceReq(label="net1", driver=RecipeParam("driver"))
@@ -138,59 +143,3 @@ class VlansRecipe(CommonHWSubConfigMixin, OffloadSubConfigMixin,
     @property
     def parallel_stream_qdisc_hw_config_dev_list(self):
         return [self.matched.host1.eth0, self.matched.host2.eth0]
-
-    def do_ping_tests(self, recipe_config):
-        for ping_config in self.generate_ping_configurations(
-            recipe_config):
-            exp_fail = []
-            for pconf in ping_config:
-                cond = self.vlan_id_same(pconf.client_bind,
-                    pconf.destination_address)
-                exp_fail.append(cond)
-            result = self.ping_test(ping_config, exp_fail)
-            self.ping_evaluate_and_report(result)
-
-    def ping_test(self, ping_config, exp_fail):
-        results = {}
-
-        running_ping_array = []
-        for pingconf, fail in zip(ping_config, exp_fail):
-            ping, client = self.ping_init(pingconf)
-            running_ping = client.prepare_job(ping, fail=fail)
-            running_ping.start(bg = True)
-            running_ping_array.append((pingconf, running_ping))
-
-        for _, pingjob in running_ping_array:
-            try:
-                pingjob.wait()
-            finally:
-                pingjob.kill()
-
-        for pingconf, pingjob in running_ping_array:
-            result = pingjob.result
-            passed = pingjob.passed
-            results[pingconf] = (result, passed)
-
-        return results
-
-    def single_ping_evaluate_and_report(self, ping_config, result):
-        fmt = "From: <{0.client.hostid} ({0.client_bind})> To: " \
-              "<{0.destination.hostid} ({0.destination_address})>"
-        description = fmt.format(ping_config)
-        if result[0].get("rate", 0) > 50:
-            message = "Ping successful --- " + description
-            self.add_result(result[1], message, result[0])
-        else:
-            message = "Ping unsuccessful --- " + description
-            self.add_result(result[1], message, result[0])
-
-    def vlan_id_same(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

[PATCH v2 15/15] lnst.RecipeCommon.Ping.Recipe: use plural for ping_config in ping_test

[Jan Tluka]

The ping_config argument is a list of PingConfig instances, so to avoid
confusion use plural.

I also updated the method arguments in some classes that inherit
from this base class.

Signed-off-by: Jan Tluka 
---
 lnst/RecipeCommon/Ping/Recipe.py          |  4 ++--
 lnst/Recipes/ENRT/BaseEnrtRecipe.py       |  4 ++--
 lnst/Recipes/ENRT/IpsecEspAeadRecipe.py   | 14 ++++++-------
 lnst/Recipes/ENRT/IpsecEspAhCompRecipe.py | 24 +++++++++++------------
 4 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/lnst/RecipeCommon/Ping/Recipe.py b/lnst/RecipeCommon/Ping/Recipe.py
index 128d7356..90cc0f6c 100644
--- a/lnst/RecipeCommon/Ping/Recipe.py
+++ b/lnst/RecipeCommon/Ping/Recipe.py
@@ -56,11 +56,11 @@ class PingConf(object):

class PingTestAndEvaluate(BaseRecipe):
-    def ping_test(self, ping_config):
+    def ping_test(self, ping_configs):
         results = {}

ping_array = []
-        for pingconf in ping_config:
+        for pingconf in ping_configs:
             ping = self.ping_init(pingconf)
             ping.start(bg = True)
             ping_array.append((pingconf, ping))
diff --git a/lnst/Recipes/ENRT/BaseEnrtRecipe.py b/lnst/Recipes/ENRT/BaseEnrtRecipe.py
index 84d192ee..de72cd0a 100644
--- a/lnst/Recipes/ENRT/BaseEnrtRecipe.py
+++ b/lnst/Recipes/ENRT/BaseEnrtRecipe.py
@@ -98,8 +98,8 @@ class BaseEnrtRecipe(BaseSubConfigMixin, PingTestAndEvaluate, PerfRecipe):
         self.do_perf_tests(recipe_config)

def do_ping_tests(self, recipe_config):
-        for ping_config in self.generate_ping_configurations(recipe_config):
-            result = self.ping_test(ping_config)
+        for ping_configs in self.generate_ping_configurations(recipe_config):
+            result = self.ping_test(ping_configs)
             self.ping_report_and_evaluate(result)

def do_perf_tests(self, recipe_config):
diff --git a/lnst/Recipes/ENRT/IpsecEspAeadRecipe.py b/lnst/Recipes/ENRT/IpsecEspAeadRecipe.py
index 014891c0..0bf41128 100644
--- a/lnst/Recipes/ENRT/IpsecEspAeadRecipe.py
+++ b/lnst/Recipes/ENRT/IpsecEspAeadRecipe.py
@@ -161,24 +161,24 @@ class IpsecEspAeadRecipe(CommonHWSubConfigMixin, BaseEnrtRecipe,
                     reverse_flow = self._create_reverse_flow(flow)
                     yield [reverse_flow]

-    def ping_test(self, ping_config):
-        m1, m2 = ping_config[0].client, ping_config[0].destination
-        ip1, ip2 = (ping_config[0].client_bind,
-            ping_config[0].destination_address)
+    def ping_test(self, ping_configs):
+        m1, m2 = ping_configs[0].client, ping_configs[0].destination
+        ip1, ip2 = (ping_configs[0].client_bind,
+            ping_configs[0].destination_address)
         if1_name = self.get_dev_by_ip(m1, ip1).name
         if2 = self.get_dev_by_ip(m2, ip2)

pa_kwargs = {}
         pa_kwargs["p_filter"] = "esp"
         pa_kwargs["grep_for"] = ['ESP(spi=' + self.spi_values[1]]
-        if ping_config[0].count:
-            pa_kwargs["p_min"] = ping_config[0].count
+        if ping_configs[0].count:
+            pa_kwargs["p_min"] = ping_configs[0].count
         pa_config = PacketAssertConf(m2, if2, **pa_kwargs)

dump = m1.run("tcpdump -i %s -nn -vv" % if1_name, bg=True)
         self.packet_assert_test_start(pa_config)
         self.ctl.wait(2)
-        ping_result = super().ping_test(ping_config)
+        ping_result = super().ping_test(ping_configs)
         self.ctl.wait(2)
         pa_result = self.packet_assert_test_stop()
         dump.kill(signal=signal.SIGINT)
diff --git a/lnst/Recipes/ENRT/IpsecEspAhCompRecipe.py b/lnst/Recipes/ENRT/IpsecEspAhCompRecipe.py
index eb67e236..fcb2d5fd 100644
--- a/lnst/Recipes/ENRT/IpsecEspAhCompRecipe.py
+++ b/lnst/Recipes/ENRT/IpsecEspAhCompRecipe.py
@@ -165,10 +165,10 @@ class IpsecEspAhCompRecipe(CommonHWSubConfigMixin, BaseEnrtRecipe,
                     reverse_flow = self._create_reverse_flow(flow)
                     yield [reverse_flow]

-    def ping_test(self, ping_config):
-        m1, m2 = ping_config[0].client, ping_config[0].destination
-        ip1, ip2 = (ping_config[0].client_bind,
-            ping_config[0].destination_address)
+    def ping_test(self, ping_configs):
+        m1, m2 = ping_configs[0].client, ping_configs[0].destination
+        ip1, ip2 = (ping_configs[0].client_bind,
+            ping_configs[0].destination_address)
         if1_name = self.get_dev_by_ip(m1, ip1).name
         if2 = self.get_dev_by_ip(m2, ip2)

@@ -176,14 +176,14 @@ class IpsecEspAhCompRecipe(CommonHWSubConfigMixin, BaseEnrtRecipe,
         pa_kwargs["p_filter"] = "ah"
         pa_kwargs["grep_for"] = ["AH(spi=" + self.spi_values[2],
             "ESP(spi=" + self.spi_values[1]]
-        if ping_config[0].count:
-            pa_kwargs["p_min"] = 2 * ping_config[0].count
+        if ping_configs[0].count:
+            pa_kwargs["p_min"] = 2 * ping_configs[0].count
         pa_config = PacketAssertConf(m2, if2, **pa_kwargs)

dump = m1.run("tcpdump -i %s -nn -vv" % if1_name, bg=True)
         self.packet_assert_test_start(pa_config)
         self.ctl.wait(2)
-        ping_result = super().ping_test(ping_config)
+        ping_result = super().ping_test(ping_configs)
         self.ctl.wait(2)
         pa_result = self.packet_assert_test_stop()
         dump.kill(signal=signal.SIGINT)
@@ -193,18 +193,18 @@ class IpsecEspAhCompRecipe(CommonHWSubConfigMixin, BaseEnrtRecipe,

dump2 = m1.run("tcpdump -i %s -nn -vv" % if1_name, bg=True)
         no_trans = self.params.ipsec_mode != 'transport'
-        ping_config2 = copy.copy(ping_config)
-        ping_config2[0].size = 1500
+        ping_configs2 = copy.copy(ping_configs)
+        ping_configs2[0].size = 1500
         if no_trans:
             pa_kwargs2 = copy.copy(pa_kwargs)
             pa_kwargs2["p_filter"] = ''
             pa_kwargs2["grep_for"] = ["IPComp"]
-            if ping_config2[0].count:
-                pa_kwargs2["p_min"] = ping_config2[0].count
+            if ping_configs2[0].count:
+                pa_kwargs2["p_min"] = ping_configs2[0].count
             pa_config2 = PacketAssertConf(m2, if2, **pa_kwargs2)
             self.packet_assert_test_start(pa_config2)
         self.ctl.wait(2)
-        ping_result2 = super().ping_test(ping_config2)
+        ping_result2 = super().ping_test(ping_configs2)
         self.ctl.wait(2)
         if no_trans:
             pa_result2 = self.packet_assert_test_stop()

[Jan Tluka]

Tue, Mar 24, 2020 at 05:48:57PM CET, [e-mail] wrote:
> This is a series of patches that attempts to remove the duplicated code
> in Recipes/ENRT in case of cross-vlan ping runs.
>
> The changes include:
> split of ping command execution and evaluation of results
> introducing two basic Ping evaluators
> replacing the duplicated code in Recipes/ENRT with the new reusable code
>
> v2:
> fixed a whitespace error
> moved Perf BaseEvaluator to BaseResultEvaluator
> used plural for ping_confing argument in ping_test()
> instead of overriding the do_ping_tests base class method I added few
> mixin classes that do the same thing in even more reusable way
> be aware of additional patches compared to previous v1

One more comment, the patchset must be applied on top of Ondrej's patch set
that starts with following commit:

[PATCH v2 1/4] lnst.Recipes.ENRT.BaseEnrtRecipe: ping_interval should be FloatParam

-Jan