From 8b30b4cd9dfa1c042a576a520a7093e2276f3d99 Mon Sep 17 00:00:00 2001 From: pfilipko1 Date: Mon, 24 Apr 2023 17:28:25 +0200 Subject: [PATCH 01/14] draft --- gprofiler/profilers/perf.py | 42 +++++++++++++++++++++++-------------- 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/gprofiler/profilers/perf.py b/gprofiler/profilers/perf.py index 89e6329c5..878cca3a1 100644 --- a/gprofiler/profilers/perf.py +++ b/gprofiler/profilers/perf.py @@ -7,11 +7,13 @@ import re import signal import time +import glob from collections import Counter, defaultdict from pathlib import Path from subprocess import Popen from threading import Event from typing import Any, Dict, Iterable, List, Optional +from tempfile import NamedTemporaryFile from granulate_utils.golang import get_process_golang_version, is_golang_process from granulate_utils.linux.elf import is_statically_linked @@ -322,27 +324,35 @@ def wait_and_script(self) -> str: # (unlike Popen.communicate()) assert self._process is not None and self._process.stderr is not None logger.debug(f"{self._log_name} run output", perf_stderr=self._process.stderr.read1()) # type: ignore - try: - inject_data = Path(f"{str(perf_data)}.inject") - if self._inject_jit: - run_process( - [perf_path(), "inject", "--jit", "-o", str(inject_data), "-i", str(perf_data)], + with NamedTemporaryFile(dir=os.path.dirname(self._output_path), suffix='.inject') as inject_data: + if self._inject_jit: + run_process( + [perf_path(), "inject", "--jit", "-o", str(inject_data), "-i", str(perf_data)], + ) + perf_data = inject_data + + perf_script_proc = run_process( + [perf_path(), "script", "-F", "+pid", "-i", str(perf_data)], + suppress_log=True, ) - perf_data.unlink() - perf_data = inject_data - - perf_script_proc = run_process( - [perf_path(), "script", "-F", "+pid", "-i", str(perf_data)], - suppress_log=True, + return perf_script_proc.stdout.decode("utf8") + except Exception: + assert self._process is not None and self._process.stdout is not None and self._process.stderr is not None + logger.critical( + f"{self._log_name} failed to dump output", + perf_stdout=self._process.stdout.read(), + perf_stderr=self._process.stderr.read(), + perf_running=self.is_running(), ) - return perf_script_proc.stdout.decode("utf8") + raise finally: perf_data.unlink() - if self._inject_jit: - # might be missing if it's already removed. - # might be existing if "perf inject" itself fails - remove_path(inject_data, missing_ok=True) + # always read its stderr + # using read1() which performs just a single read() call and doesn't read until EOF + # (unlike Popen.communicate()) + assert self._process is not None and self._process.stderr is not None + logger.debug(f"{self._log_name} run output", perf_stderr=self._process.stderr.read1()) # type: ignore @register_profiler( From c9e047903a35527240c444ee1c71015b059d1f73 Mon Sep 17 00:00:00 2001 From: pfilipko1 Date: Mon, 24 Apr 2023 17:36:57 +0200 Subject: [PATCH 02/14] draft --- gprofiler/profilers/perf.py | 9 --------- 1 file changed, 9 deletions(-) diff --git a/gprofiler/profilers/perf.py b/gprofiler/profilers/perf.py index 878cca3a1..120b3ac1f 100644 --- a/gprofiler/profilers/perf.py +++ b/gprofiler/profilers/perf.py @@ -337,15 +337,6 @@ def wait_and_script(self) -> str: suppress_log=True, ) return perf_script_proc.stdout.decode("utf8") - except Exception: - assert self._process is not None and self._process.stdout is not None and self._process.stderr is not None - logger.critical( - f"{self._log_name} failed to dump output", - perf_stdout=self._process.stdout.read(), - perf_stderr=self._process.stderr.read(), - perf_running=self.is_running(), - ) - raise finally: perf_data.unlink() # always read its stderr From c15bd9ceea4a916f98d1a2e333a4f9fbb40be6da Mon Sep 17 00:00:00 2001 From: pfilipko1 Date: Tue, 25 Apr 2023 12:47:02 +0200 Subject: [PATCH 03/14] linter --- gprofiler/profilers/perf.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/gprofiler/profilers/perf.py b/gprofiler/profilers/perf.py index 120b3ac1f..9df0e1935 100644 --- a/gprofiler/profilers/perf.py +++ b/gprofiler/profilers/perf.py @@ -3,17 +3,17 @@ # Licensed under the AGPL3 License. See LICENSE.md in the project root for license information. # +import glob import os import re import signal import time -import glob from collections import Counter, defaultdict from pathlib import Path from subprocess import Popen +from tempfile import NamedTemporaryFile from threading import Event from typing import Any, Dict, Iterable, List, Optional -from tempfile import NamedTemporaryFile from granulate_utils.golang import get_process_golang_version, is_golang_process from granulate_utils.linux.elf import is_statically_linked @@ -325,7 +325,7 @@ def wait_and_script(self) -> str: assert self._process is not None and self._process.stderr is not None logger.debug(f"{self._log_name} run output", perf_stderr=self._process.stderr.read1()) # type: ignore try: - with NamedTemporaryFile(dir=os.path.dirname(self._output_path), suffix='.inject') as inject_data: + with NamedTemporaryFile(dir=os.path.dirname(self._output_path), suffix=".inject") as inject_data: if self._inject_jit: run_process( [perf_path(), "inject", "--jit", "-o", str(inject_data), "-i", str(perf_data)], From 30784e4b9f5d892efa930af41481fb9072f6699d Mon Sep 17 00:00:00 2001 From: pfilipko1 Date: Tue, 25 Apr 2023 12:47:24 +0200 Subject: [PATCH 04/14] linter --- gprofiler/profilers/perf.py | 1 - 1 file changed, 1 deletion(-) diff --git a/gprofiler/profilers/perf.py b/gprofiler/profilers/perf.py index 9df0e1935..f584ca077 100644 --- a/gprofiler/profilers/perf.py +++ b/gprofiler/profilers/perf.py @@ -3,7 +3,6 @@ # Licensed under the AGPL3 License. See LICENSE.md in the project root for license information. # -import glob import os import re import signal From 78b31d8bf1478f9983dfb405eeac2bbe7202044d Mon Sep 17 00:00:00 2001 From: pfilipko1 Date: Wed, 26 Apr 2023 13:37:05 +0200 Subject: [PATCH 05/14] tempfile here too --- gprofiler/utils/fs.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/gprofiler/utils/fs.py b/gprofiler/utils/fs.py index fb96f8968..ad55f1ce2 100644 --- a/gprofiler/utils/fs.py +++ b/gprofiler/utils/fs.py @@ -8,6 +8,7 @@ import shutil from pathlib import Path from secrets import token_hex +from tempfile import NamedTemporaryFile from gprofiler.platform import is_windows from gprofiler.utils import remove_path, run_process @@ -27,19 +28,18 @@ def is_rw_exec_dir(path: str) -> bool: """ Is 'path' rw and exec? """ - # randomize the name - this function runs concurrently on paths of in same mnt namespace. - test_script = Path(path) / f"t-{token_hex(10)}.sh" # try creating & writing try: + test_script = NamedTemporaryFile(dir=path, suffix=".sh") os.makedirs(path, 0o755, exist_ok=True) - test_script.write_text("#!/bin/sh\nexit 0") - test_script.chmod(0o755) + with open(test_script.name, "w") as f: + f.write("#!/bin/sh\nexit 0") + os.chmod(test_script.name, 0o755) except OSError as e: if e.errno == errno.EROFS: # ro return False - remove_path(test_script) raise # try executing From f976906d6bc186c9d8bf801ee5042c2ccab355a6 Mon Sep 17 00:00:00 2001 From: pfilipko1 Date: Wed, 26 Apr 2023 13:41:07 +0200 Subject: [PATCH 06/14] tempfile here too --- gprofiler/utils/fs.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/gprofiler/utils/fs.py b/gprofiler/utils/fs.py index ad55f1ce2..bb8d6ac20 100644 --- a/gprofiler/utils/fs.py +++ b/gprofiler/utils/fs.py @@ -6,12 +6,10 @@ import errno import os import shutil -from pathlib import Path -from secrets import token_hex from tempfile import NamedTemporaryFile from gprofiler.platform import is_windows -from gprofiler.utils import remove_path, run_process +from gprofiler.utils import run_process def safe_copy(src: str, dst: str) -> None: From 1316bf1ecec1745c3183d605019bc9c76b340a29 Mon Sep 17 00:00:00 2001 From: pfilipko1 Date: Wed, 26 Apr 2023 13:50:34 +0200 Subject: [PATCH 07/14] linter --- gprofiler/profilers/perf.py | 1 - 1 file changed, 1 deletion(-) diff --git a/gprofiler/profilers/perf.py b/gprofiler/profilers/perf.py index f584ca077..43cea5f58 100644 --- a/gprofiler/profilers/perf.py +++ b/gprofiler/profilers/perf.py @@ -8,7 +8,6 @@ import signal import time from collections import Counter, defaultdict -from pathlib import Path from subprocess import Popen from tempfile import NamedTemporaryFile from threading import Event From bc2d75b3053fb94d3d23e8cac78b862ac20a61cc Mon Sep 17 00:00:00 2001 From: pfilipko1 Date: Thu, 27 Apr 2023 11:10:13 +0200 Subject: [PATCH 08/14] remove unlink for gods sake --- gprofiler/utils/fs.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/gprofiler/utils/fs.py b/gprofiler/utils/fs.py index bb8d6ac20..b91ae3913 100644 --- a/gprofiler/utils/fs.py +++ b/gprofiler/utils/fs.py @@ -46,8 +46,6 @@ def is_rw_exec_dir(path: str) -> bool: except PermissionError: # noexec return False - finally: - test_script.unlink() return True From 201db15fa3769f88bd243c9aa5fa30d092b6b825 Mon Sep 17 00:00:00 2001 From: pfilipko1 Date: Thu, 27 Apr 2023 17:06:14 +0200 Subject: [PATCH 09/14] types compat --- gprofiler/profilers/perf.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/gprofiler/profilers/perf.py b/gprofiler/profilers/perf.py index 43cea5f58..c9faf5318 100644 --- a/gprofiler/profilers/perf.py +++ b/gprofiler/profilers/perf.py @@ -12,6 +12,7 @@ from tempfile import NamedTemporaryFile from threading import Event from typing import Any, Dict, Iterable, List, Optional +from pathlib import Path from granulate_utils.golang import get_process_golang_version, is_golang_process from granulate_utils.linux.elf import is_statically_linked @@ -328,7 +329,7 @@ def wait_and_script(self) -> str: run_process( [perf_path(), "inject", "--jit", "-o", str(inject_data), "-i", str(perf_data)], ) - perf_data = inject_data + perf_data = Path(inject_data.name) perf_script_proc = run_process( [perf_path(), "script", "-F", "+pid", "-i", str(perf_data)], From d20158a5d495396fae78b5bc583ab6b3ba2ca03a Mon Sep 17 00:00:00 2001 From: pfilipko1 Date: Thu, 27 Apr 2023 17:11:54 +0200 Subject: [PATCH 10/14] types compat --- gprofiler/profilers/perf.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gprofiler/profilers/perf.py b/gprofiler/profilers/perf.py index c9faf5318..7a26c1806 100644 --- a/gprofiler/profilers/perf.py +++ b/gprofiler/profilers/perf.py @@ -8,11 +8,11 @@ import signal import time from collections import Counter, defaultdict +from pathlib import Path from subprocess import Popen from tempfile import NamedTemporaryFile from threading import Event from typing import Any, Dict, Iterable, List, Optional -from pathlib import Path from granulate_utils.golang import get_process_golang_version, is_golang_process from granulate_utils.linux.elf import is_statically_linked From 26fc8a090936bcb40455dd58ea4263922a7ebe0d Mon Sep 17 00:00:00 2001 From: pfilipko1 Date: Fri, 28 Apr 2023 13:57:55 +0200 Subject: [PATCH 11/14] file is free now --- gprofiler/utils/fs.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/gprofiler/utils/fs.py b/gprofiler/utils/fs.py index b91ae3913..7bf4b6347 100644 --- a/gprofiler/utils/fs.py +++ b/gprofiler/utils/fs.py @@ -29,11 +29,12 @@ def is_rw_exec_dir(path: str) -> bool: # try creating & writing try: - test_script = NamedTemporaryFile(dir=path, suffix=".sh") os.makedirs(path, 0o755, exist_ok=True) + test_script = NamedTemporaryFile(dir=path, suffix=".sh") with open(test_script.name, "w") as f: f.write("#!/bin/sh\nexit 0") os.chmod(test_script.name, 0o755) + test_script.file.close() except OSError as e: if e.errno == errno.EROFS: # ro @@ -42,7 +43,7 @@ def is_rw_exec_dir(path: str) -> bool: # try executing try: - run_process([str(test_script)], suppress_log=True) + run_process([str(test_script.name)], suppress_log=True) except PermissionError: # noexec return False From fd6d708632a7c181d0e9397025aa81b773b845ca Mon Sep 17 00:00:00 2001 From: pfilipko1 Date: Thu, 4 May 2023 15:57:42 +0200 Subject: [PATCH 12/14] addressing comments --- gprofiler/profilers/perf.py | 12 ++++-------- gprofiler/utils/fs.py | 9 ++++----- 2 files changed, 8 insertions(+), 13 deletions(-) diff --git a/gprofiler/profilers/perf.py b/gprofiler/profilers/perf.py index 7a26c1806..55ed2eb16 100644 --- a/gprofiler/profilers/perf.py +++ b/gprofiler/profilers/perf.py @@ -323,26 +323,22 @@ def wait_and_script(self) -> str: # (unlike Popen.communicate()) assert self._process is not None and self._process.stderr is not None logger.debug(f"{self._log_name} run output", perf_stderr=self._process.stderr.read1()) # type: ignore + try: with NamedTemporaryFile(dir=os.path.dirname(self._output_path), suffix=".inject") as inject_data: if self._inject_jit: run_process( - [perf_path(), "inject", "--jit", "-o", str(inject_data), "-i", str(perf_data)], + [perf_path(), "inject", "--jit", "-o", str(inject_data.name), "-i", str(perf_data)], ) - perf_data = Path(inject_data.name) + perf_script_input = Path(inject_data.name) perf_script_proc = run_process( - [perf_path(), "script", "-F", "+pid", "-i", str(perf_data)], + [perf_path(), "script", "-F", "+pid", "-i", str(perf_script_input)], suppress_log=True, ) return perf_script_proc.stdout.decode("utf8") finally: perf_data.unlink() - # always read its stderr - # using read1() which performs just a single read() call and doesn't read until EOF - # (unlike Popen.communicate()) - assert self._process is not None and self._process.stderr is not None - logger.debug(f"{self._log_name} run output", perf_stderr=self._process.stderr.read1()) # type: ignore @register_profiler( diff --git a/gprofiler/utils/fs.py b/gprofiler/utils/fs.py index 7bf4b6347..e721fc6a2 100644 --- a/gprofiler/utils/fs.py +++ b/gprofiler/utils/fs.py @@ -30,11 +30,10 @@ def is_rw_exec_dir(path: str) -> bool: # try creating & writing try: os.makedirs(path, 0o755, exist_ok=True) - test_script = NamedTemporaryFile(dir=path, suffix=".sh") - with open(test_script.name, "w") as f: - f.write("#!/bin/sh\nexit 0") - os.chmod(test_script.name, 0o755) - test_script.file.close() + with NamedTemporaryFile(dir=path, suffix=".sh") as test_script: + with open(test_script.name, "w") as f: + f.write("#!/bin/sh\nexit 0") + os.chmod(test_script.name, 0o755) except OSError as e: if e.errno == errno.EROFS: # ro From 353e34825719450935415a05c24a8aa540b55233 Mon Sep 17 00:00:00 2001 From: pfilipko1 Date: Thu, 4 May 2023 18:06:41 +0200 Subject: [PATCH 13/14] addressing comments --- gprofiler/profilers/perf.py | 1 + gprofiler/utils/fs.py | 14 ++++++++------ 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/gprofiler/profilers/perf.py b/gprofiler/profilers/perf.py index 55ed2eb16..2f0f7ab89 100644 --- a/gprofiler/profilers/perf.py +++ b/gprofiler/profilers/perf.py @@ -326,6 +326,7 @@ def wait_and_script(self) -> str: try: with NamedTemporaryFile(dir=os.path.dirname(self._output_path), suffix=".inject") as inject_data: + perf_script_input = perf_data if self._inject_jit: run_process( [perf_path(), "inject", "--jit", "-o", str(inject_data.name), "-i", str(perf_data)], diff --git a/gprofiler/utils/fs.py b/gprofiler/utils/fs.py index e721fc6a2..03e1c54b9 100644 --- a/gprofiler/utils/fs.py +++ b/gprofiler/utils/fs.py @@ -34,18 +34,20 @@ def is_rw_exec_dir(path: str) -> bool: with open(test_script.name, "w") as f: f.write("#!/bin/sh\nexit 0") os.chmod(test_script.name, 0o755) + test_script.file.close() + # try executing + try: + run_process([str(test_script.name)], suppress_log=True) + except PermissionError: + # noexec + return False except OSError as e: if e.errno == errno.EROFS: # ro return False raise - # try executing - try: - run_process([str(test_script.name)], suppress_log=True) - except PermissionError: - # noexec - return False + return True From 91089bda9530d5e6c24bd4df0f70156c973ddfae Mon Sep 17 00:00:00 2001 From: pfilipko1 Date: Thu, 4 May 2023 18:17:19 +0200 Subject: [PATCH 14/14] linter --- gprofiler/utils/fs.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/gprofiler/utils/fs.py b/gprofiler/utils/fs.py index 03e1c54b9..364a0bdf0 100644 --- a/gprofiler/utils/fs.py +++ b/gprofiler/utils/fs.py @@ -47,8 +47,6 @@ def is_rw_exec_dir(path: str) -> bool: return False raise - - return True