From: Perry Gagne pgagne@redhat.com
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(-)
From: Perry Gagne pgagne@redhat.com
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@redhat.com --- 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
From: Perry Gagne pgagne@redhat.com
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@redhat.com --- .../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")
Thu, Apr 01, 2021 at 09:13:37PM CEST, pgagne@redhat.com wrote:
From: Perry Gagne pgagne@redhat.com
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@redhat.com
.../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@lists.fedorahosted.org To unsubscribe send an email to lnst-developers-leave@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/lnst-developers@lists.fedorahos... Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
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@redhat.com wrote:
Thu, Apr 01, 2021 at 09:13:37PM CEST, pgagne@redhat.com wrote:
From: Perry Gagne pgagne@redhat.com
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@redhat.com
.../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@lists.fedorahosted.org To unsubscribe send an email to
lnst-developers-leave@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/lnst-developers@lists.fedorahos...
Do not reply to spam on the list, report it:
On Thu, Apr 01, 2021 at 03:13:35PM -0400, pgagne@redhat.com wrote:
From: Perry Gagne pgagne@redhat.com
We decided to ignore tcp_crr stats for now due to a few reasons:
- It was removed from neper upstream due to memory issues
- The resulting LRC files where big
- 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@lists.fedorahosted.org To unsubscribe send an email to lnst-developers-leave@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/lnst-developers@lists.fedorahos... 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
lnst-developers@lists.fedorahosted.org