[PATCH 0/2] Fixes to remove tcp_crr server side stats

From: Perry Gagne <pgagne(a)redhat.com&gt;

We decided to ignore tcp_crr stats for now due to a few reasons:
1. It was removed from neper upstream due to memory issues
2. The resulting LRC files where big
3. We were probably not caluclating stats in the first place in the crr
case
4. Neper's stats collection probably needs work to properly collect
stats. 

For now we decided to record the server side stats as PerfIntervals of
0. 

These patches contain some things to enable that. 

Perry Gagne (2):
  Utils.py: Added 3.6 backport of contextlib.nullcontext
  NeperFlowMeasurement.py: Don't collect tcp_crr server side metric

 lnst/Common/Utils.py                          | 25 ++++++++++++++++
 .../Perf/Measurements/NeperFlowMeasurement.py |  8 +++++
 lnst/Tests/Neper.py                           | 29 ++++++++++++++-----
 3 files changed, 54 insertions(+), 8 deletions(-)

-- 
2.30.2

[PATCH 1/2] Utils.py: Added 3.6 backport of contextlib.nullcontext

From: Perry Gagne <pgagne(a)redhat.com&gt;

When rewriting Neper code to handle tcp_crr server not having stats, I found this made the
code flow simpler to adapt.

contextlib.nullcontext was added in python 3.7, but we need it in 3.6. The code was simple
enough so I just copied it from CPython.

Signed-off-by: Perry Gagne <pgagne(a)redhat.com&gt;
---
 lnst/Common/Utils.py | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/lnst/Common/Utils.py b/lnst/Common/Utils.py
index 477ef8db..c276a78f 100644
--- a/lnst/Common/Utils.py
+++ b/lnst/Common/Utils.py
@@ -23,6 +23,7 @@ import collections
 import math
 import itertools
 from collections.abc import Iterable
+from contextlib import AbstractContextManager
 from _ast import Call, Attribute
 from lnst.Common.ExecCmd import exec_cmd
 
@@ -329,3 +330,27 @@ def pairwise(iterable: Iterable) -> Iterable:
     a, b = itertools.tee(iterable)
     next(b, None)
     return zip(a, b)
+
+
+class nullcontext(AbstractContextManager):
+    """
+    Context manager that does no additional processing.
+    Used as a stand-in for a normal context manager, when a particular
+    block of code is only sometimes used with a normal context manager:
+    cm = optional_cm if condition else nullcontext()
+    with cm:
+        # Perform operation, using optional_cm if condition is True
+
+    Copied from
+        https://github.com/python/cpython/blob/3.9/Lib/contextlib.py
+    To backport it for python 3.6 (it was added in 3.7).
+    """
+
+    def __init__(self, enter_result=None):
+        self.enter_result = enter_result
+
+    def __enter__(self):
+        return self.enter_result
+
+    def __exit__(self, *excinfo):
+        pass
\ No newline at end of file
-- 
2.30.2

[PATCH 2/2] NeperFlowMeasurement.py: Don't collect tcp_crr server side metric

From: Perry Gagne <pgagne(a)redhat.com&gt;

Neper tcp_crr removed sample stats from the server due to memory issues.

We decided to not look at the server side stats for now, and just record it
as a PerfInterval of 0.

Signed-off-by: Perry Gagne <pgagne(a)redhat.com&gt;
---
 .../Perf/Measurements/NeperFlowMeasurement.py |  8 +++++
 lnst/Tests/Neper.py                           | 29 ++++++++++++++-----
 2 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/lnst/RecipeCommon/Perf/Measurements/NeperFlowMeasurement.py
b/lnst/RecipeCommon/Perf/Measurements/NeperFlowMeasurement.py
index 1f667fb5..f271e1b3 100644
--- a/lnst/RecipeCommon/Perf/Measurements/NeperFlowMeasurement.py
+++ b/lnst/RecipeCommon/Perf/Measurements/NeperFlowMeasurement.py
@@ -171,6 +171,14 @@ class NeperFlowMeasurement(BaseFlowMeasurement):
         if not job.passed:
             results.append(PerfInterval(0, 0, "transactions", time.time()))
             cpu_results.append(PerfInterval(0, 0, "cpu_percent", time.time()))
+        elif job.what.is_crr_server():
+            # Neper doesn't support server stats for tcp_crr due to memory issues.
+            # Use perf_interval of 0.
+            # For duration neper doesn't have time_start/time_end for tcp_crr
+            # So just use the value of test length
+            d = float(job.what.params.test_length)
+            results.append(PerfInterval(0, d, "transactions", time.time()))
+            cpu_results.append(PerfInterval(0, d, "cpu_percent", time.time()))
         else:
             job_start = job.result['start_time']
             samples = job.result['samples']
diff --git a/lnst/Tests/Neper.py b/lnst/Tests/Neper.py
index 3c2b6445..1cdadf5d 100644
--- a/lnst/Tests/Neper.py
+++ b/lnst/Tests/Neper.py
@@ -1,3 +1,4 @@
+import contextlib
 import csv
 import logging
 import os
@@ -6,9 +7,10 @@ import re
 import subprocess
 import time
 import tempfile
-from typing import List, Dict
+from typing import List, Dict, TextIO, Union
 
 from lnst.Common.Parameters import HostnameOrIpParam, StrParam, IntParam, IpParam,
ChoiceParam
+from lnst.Common.Utils import nullcontext
 from lnst.Tests.BaseTestModule import BaseTestModule, TestModuleError
 
 NEPER_OUT_RE = re.compile(r"^(?P<key>.*)=(?P<value>.*)$",
flags=re.M)
@@ -43,6 +45,9 @@ class NeperBase(BaseTestModule):
                 data[k] = v
         return data
 
+    def is_crr_server(self):
+        return self._role == "server" and self.params.workload ==
"tcp_crr"
+
     def run(self):
         self._res_data = {}
         if not NEPER_PATH.joinpath(self.params.workload).exists():
@@ -51,10 +56,15 @@ class NeperBase(BaseTestModule):
             logging.error(self._res_data['msg'])
             return False
 
-        with tempfile.NamedTemporaryFile('r', prefix='neper-samples-',
-                                         suffix='.csv', newline='') as
sf:
+        if self.is_crr_server():
+            cm = nullcontext()
+        else:
+            cm = tempfile.NamedTemporaryFile('r',
prefix='neper-samples-',
+                                         suffix='.csv', newline='')
+
+        with cm as sf:
 
-            cmd = self._compose_cmd(sf.name)
+            cmd = self._compose_cmd(sf)
             logging.debug(f"compiled command: {cmd}")
             logging.debug(f"running as {self._role}")
 
@@ -78,13 +88,16 @@ class NeperBase(BaseTestModule):
                 logging.error(self._res_data["msg"])
                 return False
 
-            self._res_data["samples"] = [r for r in csv.DictReader(sf)]
+            if not self.is_crr_server():
+                self._res_data["samples"] = [r for r in csv.DictReader(sf)]
 
         return True
 
-    def _compose_cmd(self, samples_path:str) -> str:
-        cmd = [f"./{self.params.workload}",
-               f"--all-samples={samples_path}"]
+    def _compose_cmd(self, sample_file: Union[TextIO, None]) -> str:
+        cmd = [f"./{self.params.workload}"]
+
+        if not self.is_crr_server():
+            cmd.append(f"--all-samples={sample_file.name}")
 
         if self._role == "client":
             cmd.append("-c")
-- 
2.30.2

[PATCH 2/2] NeperFlowMeasurement.py: Don't collect tcp_crr server side metric

[Jan Tluka]

Thu, Apr 01, 2021 at 09:13:37PM CEST, pgagne(a)redhat.com wrote:
> From: Perry Gagne <pgagne(a)redhat.com&gt;
>
> Neper tcp_crr removed sample stats from the server due to memory issues.
>
> We decided to not look at the server side stats for now, and just record it
> as a PerfInterval of 0.
>
> Signed-off-by: Perry Gagne <pgagne(a)redhat.com&gt;
> ---
>  .../Perf/Measurements/NeperFlowMeasurement.py |  8 +++++
>  lnst/Tests/Neper.py                           | 29 ++++++++++++++-----
>  2 files changed, 29 insertions(+), 8 deletions(-)
>
> diff --git a/lnst/RecipeCommon/Perf/Measurements/NeperFlowMeasurement.py
> b/lnst/RecipeCommon/Perf/Measurements/NeperFlowMeasurement.py
> index 1f667fb5..f271e1b3 100644
> --- a/lnst/RecipeCommon/Perf/Measurements/NeperFlowMeasurement.py
> +++ b/lnst/RecipeCommon/Perf/Measurements/NeperFlowMeasurement.py
> @@ -171,6 +171,14 @@ class NeperFlowMeasurement(BaseFlowMeasurement):
>          if not job.passed:
>              results.append(PerfInterval(0, 0, "transactions", time.time()))
>              cpu_results.append(PerfInterval(0, 0, "cpu_percent", time.time()))
> +        elif job.what.is_crr_server():
> +            # Neper doesn't support server stats for tcp_crr due to memory issues.
> +            # Use perf_interval of 0.
> +            # For duration neper doesn't have time_start/time_end for tcp_crr
> +            # So just use the value of test length
> +            d = float(job.what.params.test_length)
> +            results.append(PerfInterval(0, d, "transactions", time.time()))
> +            cpu_results.append(PerfInterval(0, d, "cpu_percent", time.time()))
>          else:
>              job_start = job.result['start_time']
>              samples = job.result['samples']
> diff --git a/lnst/Tests/Neper.py b/lnst/Tests/Neper.py
> index 3c2b6445..1cdadf5d 100644
> --- a/lnst/Tests/Neper.py
> +++ b/lnst/Tests/Neper.py
> @@ -1,3 +1,4 @@
> +import contextlib
>  import csv
>  import logging
>  import os
> @@ -6,9 +7,10 @@ import re
>  import subprocess
>  import time
>  import tempfile
> -from typing import List, Dict
> +from typing import List, Dict, TextIO, Union
>  
>  from lnst.Common.Parameters import HostnameOrIpParam, StrParam, IntParam, IpParam,
> ChoiceParam
> +from lnst.Common.Utils import nullcontext
>  from lnst.Tests.BaseTestModule import BaseTestModule, TestModuleError
>  
>  NEPER_OUT_RE = re.compile(r"^(?P<key>.*)=(?P<value>.*)$",
> flags=re.M)
> @@ -43,6 +45,9 @@ class NeperBase(BaseTestModule):
>                  data[k] = v
>          return data
>  
> +    def is_crr_server(self):
> +        return self._role == "server" and self.params.workload ==
> "tcp_crr"
> +
>      def run(self):
>          self._res_data = {}
>          if not NEPER_PATH.joinpath(self.params.workload).exists():
> @@ -51,10 +56,15 @@ class NeperBase(BaseTestModule):
>              logging.error(self._res_data['msg'])
>              return False
>  
> -        with tempfile.NamedTemporaryFile('r', prefix='neper-samples-',
> -                                         suffix='.csv', newline='') as
> sf:
> +        if self.is_crr_server():
> +            cm = nullcontext()
> +        else:
> +            cm = tempfile.NamedTemporaryFile('r',
> prefix='neper-samples-',
> +                                         suffix='.csv', newline='')
> +
> +        with cm as sf:
>  
> -            cmd = self._compose_cmd(sf.name)
> +            cmd = self._compose_cmd(sf)
>              logging.debug(f"compiled command: {cmd}")
>              logging.debug(f"running as {self._role}")
>  
> @@ -78,13 +88,16 @@ class NeperBase(BaseTestModule):
>                  logging.error(self._res_data["msg"])
>                  return False
>  
> -            self._res_data["samples"] = [r for r in csv.DictReader(sf)]
> +            if not self.is_crr_server():
> +                self._res_data["samples"] = [r for r in csv.DictReader(sf)]
Please correct me if I'm wrong, but if I understand this correctly, the
only reason to use nullcontext manager is to avoid dumping the samples?
Is there any other reason why to use it?

Eventually when upstream neper issue is fixed we would use the file so
maybe this is unnecessary.

I'm quite unsure about copying code for feature that is not present in
python3.6 just because of such change.

But I may be missing something.

-Jan

>  
>          return True
>  
> -    def _compose_cmd(self, samples_path:str) -> str:
> -        cmd = [f"./{self.params.workload}",
> -               f"--all-samples={samples_path}"]
> +    def _compose_cmd(self, sample_file: Union[TextIO, None]) -> str:
> +        cmd = [f"./{self.params.workload}"]
> +
> +        if not self.is_crr_server():
> +            cmd.append(f"--all-samples={sample_file.name}")
>  
>          if self._role == "client":
>              cmd.append("-c")
> -- 
> 2.30.2
> _______________________________________________
> LNST-developers mailing list -- lnst-developers(a)lists.fedorahosted.org
> To unsubscribe send an email to lnst-developers-leave(a)lists.fedorahosted.org
> Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/
> List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
> List Archives:
> https://lists.fedorahosted.org/archives/list/[e-mail]...
> Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure

[PATCH 2/2] NeperFlowMeasurement.py: Don't collect tcp_crr server side metric

[Perry Gagne]

Its not just that it avoids dumping the samples, it prevents creating the
sample file in the first place.

I used the null context here mostly because it just makes the code flow
simpler, due to the fact you can't really wrap a "with" in a conditional. I
challenge you to rewrite that code block another way, and have it come out
nicely.

I considered a couple alternatives, like using this contextlib2 (
https://github.com/jazzband/contextlib2). But since the code was so simple
it was easier to just copy it instead of adding a new dependency.

On Tue, Apr 6, 2021 at 3:41 AM Jan Tluka <jtluka(a)redhat.com&gt; wrote:

>  Thu, Apr 01, 2021 at 09:13:37PM CEST, pgagne(a)redhat.com wrote:
>  >From: Perry Gagne <pgagne(a)redhat.com&gt;
>  >
>  >Neper tcp_crr removed sample stats from the server due to memory issues.
>  >
>  >We decided to not look at the server side stats for now, and just record
>  it
>  >as a PerfInterval of 0.
>  >
>  >Signed-off-by: Perry Gagne <pgagne(a)redhat.com&gt;
>  >---
>  > .../Perf/Measurements/NeperFlowMeasurement.py |  8 +++++
>  > lnst/Tests/Neper.py                           | 29 ++++++++++++++-----
>  > 2 files changed, 29 insertions(+), 8 deletions(-)
>  >
>  >diff --git a/lnst/RecipeCommon/Perf/Measurements/NeperFlowMeasurement.py
>  b/lnst/RecipeCommon/Perf/Measurements/NeperFlowMeasurement.py
>  >index 1f667fb5..f271e1b3 100644
>  >--- a/lnst/RecipeCommon/Perf/Measurements/NeperFlowMeasurement.py
>  >+++ b/lnst/RecipeCommon/Perf/Measurements/NeperFlowMeasurement.py
>  >@@ -171,6 +171,14 @@ class NeperFlowMeasurement(BaseFlowMeasurement):
>  >         if not job.passed:
>  >             results.append(PerfInterval(0, 0, "transactions",
>  time.time()))
>  >             cpu_results.append(PerfInterval(0, 0, "cpu_percent",
>  time.time()))
>  >+        elif job.what.is_crr_server():
>  >+            # Neper doesn't support server stats for tcp_crr due to
>  memory issues.
>  >+            # Use perf_interval of 0.
>  >+            # For duration neper doesn't have time_start/time_end for
>  tcp_crr
>  >+            # So just use the value of test length
>  >+            d = float(job.what.params.test_length)
>  >+            results.append(PerfInterval(0, d, "transactions",
>  time.time()))
>  >+            cpu_results.append(PerfInterval(0, d, "cpu_percent",
>  time.time()))
>  >         else:
>  >             job_start = job.result['start_time']
>  >             samples = job.result['samples']
>  >diff --git a/lnst/Tests/Neper.py b/lnst/Tests/Neper.py
>  >index 3c2b6445..1cdadf5d 100644
>  >--- a/lnst/Tests/Neper.py
>  >+++ b/lnst/Tests/Neper.py
>  >@@ -1,3 +1,4 @@
>  >+import contextlib
>  > import csv
>  > import logging
>  > import os
>  >@@ -6,9 +7,10 @@ import re
>  > import subprocess
>  > import time
>  > import tempfile
>  >-from typing import List, Dict
>  >+from typing import List, Dict, TextIO, Union
>  >
>  > from lnst.Common.Parameters import HostnameOrIpParam, StrParam,
>  IntParam, IpParam, ChoiceParam
>  >+from lnst.Common.Utils import nullcontext
>  > from lnst.Tests.BaseTestModule import BaseTestModule, TestModuleError
>  >
>  > NEPER_OUT_RE = re.compile(r"^(?P<key>.*)=(?P<value>.*)$",
> flags=re.M)
>  >@@ -43,6 +45,9 @@ class NeperBase(BaseTestModule):
>  >                 data[k] = v
>  >         return data
>  >
>  >+    def is_crr_server(self):
>  >+        return self._role == "server" and self.params.workload ==
>  "tcp_crr"
>  >+
>  >     def run(self):
>  >         self._res_data = {}
>  >         if not NEPER_PATH.joinpath(self.params.workload).exists():
>  >@@ -51,10 +56,15 @@ class NeperBase(BaseTestModule):
>  >             logging.error(self._res_data['msg'])
>  >             return False
>  >
>  >-        with tempfile.NamedTemporaryFile('r',
> prefix='neper-samples-',
>  >-                                         suffix='.csv', newline='')
> as
>  sf:
>  >+        if self.is_crr_server():
>  >+            cm = nullcontext()
>  >+        else:
>  >+            cm = tempfile.NamedTemporaryFile('r',
>  prefix='neper-samples-',
>  >+                                         suffix='.csv', newline='')
>  >+
>  >+        with cm as sf:
>  >
>  >-            cmd = self._compose_cmd(sf.name)
>  >+            cmd = self._compose_cmd(sf)
>  >             logging.debug(f"compiled command: {cmd}")
>  >             logging.debug(f"running as {self._role}")
>  >
>  >@@ -78,13 +88,16 @@ class NeperBase(BaseTestModule):
>  >                 logging.error(self._res_data["msg"])
>  >                 return False
>  >
>  >-            self._res_data["samples"] = [r for r in csv.DictReader(sf)]
>  >+            if not self.is_crr_server():
>  >+                self._res_data["samples"] = [r for r in
>  csv.DictReader(sf)]
>
>  Please correct me if I'm wrong, but if I understand this correctly, the
>  only reason to use nullcontext manager is to avoid dumping the samples?
>  Is there any other reason why to use it?
>
>  Eventually when upstream neper issue is fixed we would use the file so
>  maybe this is unnecessary.
>
>  I'm quite unsure about copying code for feature that is not present in
>  python3.6 just because of such change.
>
>  But I may be missing something.
>
>  -Jan
>
>  >
>  >         return True
>  >
>  >-    def _compose_cmd(self, samples_path:str) -> str:
>  >-        cmd = [f"./{self.params.workload}",
>  >-               f"--all-samples={samples_path}"]
>  >+    def _compose_cmd(self, sample_file: Union[TextIO, None]) -> str:
>  >+        cmd = [f"./{self.params.workload}"]
>  >+
>  >+        if not self.is_crr_server():
>  >+            cmd.append(f"--all-samples={sample_file.name}")
>  >
>  >         if self._role == "client":
>  >             cmd.append("-c")
>  >--
>  >2.30.2
>  >_______________________________________________
>  >LNST-developers mailing list -- lnst-developers(a)lists.fedorahosted.org
>  >To unsubscribe send an email to
>  lnst-developers-leave(a)lists.fedorahosted.org
>  >Fedora Code of Conduct:
>  https://docs.fedoraproject.org/en-US/project/code-of-conduct/
>  >List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
>  >List Archives:
>  https://lists.fedorahosted.org/archives/list/[e-mail]...
>  >Do not reply to spam on the list, report it:
>  https://pagure.io/fedora-infrastructure
>
>

[Ondrej Lichtner]

On Thu, Apr 01, 2021 at 03:13:35PM -0400, pgagne(a)redhat.com wrote:
>  From: Perry Gagne <pgagne(a)redhat.com&gt;
>  
>  We decided to ignore tcp_crr stats for now due to a few reasons:
>  1. It was removed from neper upstream due to memory issues
>  2. The resulting LRC files where big
>  3. We were probably not caluclating stats in the first place in the crr
>  case
>  4. Neper's stats collection probably needs work to properly collect
>  stats. 
>  
>  For now we decided to record the server side stats as PerfIntervals of
>  0. 
>  
>  These patches contain some things to enable that. 
>  
>  Perry Gagne (2):
>    Utils.py: Added 3.6 backport of contextlib.nullcontext
>    NeperFlowMeasurement.py: Don't collect tcp_crr server side metric
>  
>   lnst/Common/Utils.py                          | 25 ++++++++++++++++
>   .../Perf/Measurements/NeperFlowMeasurement.py |  8 +++++
>   lnst/Tests/Neper.py                           | 29 ++++++++++++++-----
>   3 files changed, 54 insertions(+), 8 deletions(-)
>  
>  -- 
>  2.30.2
>  _______________________________________________
>  LNST-developers mailing list -- lnst-developers(a)lists.fedorahosted.org
>  To unsubscribe send an email to lnst-developers-leave(a)lists.fedorahosted.org
>  Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/
>  List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
>  List Archives:
> https://lists.fedorahosted.org/archives/list/[e-mail]...
>  Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
after discussion in a videocall we agreed that the remaining issue with
adding the nullcontext is mostly a cosmetic one and we can merge this.

With that I have pushed the patchset to the upstream master.

-Ondrej