From b7c849a198227adf07fcc08d6c23dd2562925a1e Mon Sep 17 00:00:00 2001 From: Dominic Lavery Date: Thu, 21 Nov 2024 12:37:58 +0000 Subject: [PATCH 1/3] Calculate linter.config.jobs in cgroupsv2 environments. Fixes #10103 --- custom_dict.txt | 1 + doc/whatsnew/fragments/10103.bugfix | 3 + pylint/lint/run.py | 54 ++++++++++------- tests/test_pylint_runners.py | 90 +++++++++++++++++++++++++++++ 4 files changed, 128 insertions(+), 20 deletions(-) create mode 100644 doc/whatsnew/fragments/10103.bugfix diff --git a/custom_dict.txt b/custom_dict.txt index 576ef703e0..265c7cb881 100644 --- a/custom_dict.txt +++ b/custom_dict.txt @@ -68,6 +68,7 @@ contextlib contextmanager contravariance contravariant +cgroup CPython cpython csv diff --git a/doc/whatsnew/fragments/10103.bugfix b/doc/whatsnew/fragments/10103.bugfix new file mode 100644 index 0000000000..cf72ce8240 --- /dev/null +++ b/doc/whatsnew/fragments/10103.bugfix @@ -0,0 +1,3 @@ +Fixes a crash that occurred when pylint was run in a container on a host with cgroupsv2 and restrictions on CPU usage. + +Closes #10103 diff --git a/pylint/lint/run.py b/pylint/lint/run.py index 2bbbb337b9..68d50a8e7e 100644 --- a/pylint/lint/run.py +++ b/pylint/lint/run.py @@ -45,26 +45,40 @@ def _query_cpu() -> int | None: """ cpu_quota, avail_cpu = None, None - if Path("/sys/fs/cgroup/cpu/cpu.cfs_quota_us").is_file(): - with open("/sys/fs/cgroup/cpu/cpu.cfs_quota_us", encoding="utf-8") as file: - # Not useful for AWS Batch based jobs as result is -1, but works on local linux systems - cpu_quota = int(file.read().rstrip()) - - if ( - cpu_quota - and cpu_quota != -1 - and Path("/sys/fs/cgroup/cpu/cpu.cfs_period_us").is_file() - ): - with open("/sys/fs/cgroup/cpu/cpu.cfs_period_us", encoding="utf-8") as file: - cpu_period = int(file.read().rstrip()) - # Divide quota by period and you should get num of allotted CPU to the container, - # rounded down if fractional. - avail_cpu = int(cpu_quota / cpu_period) - elif Path("/sys/fs/cgroup/cpu/cpu.shares").is_file(): - with open("/sys/fs/cgroup/cpu/cpu.shares", encoding="utf-8") as file: - cpu_shares = int(file.read().rstrip()) - # For AWS, gives correct value * 1024. - avail_cpu = int(cpu_shares / 1024) + if Path("/sys/fs/cgroup/cpu.max").is_file(): + # Cgroupv2 systems + with open("/sys/fs/cgroup/cpu.max", encoding="utf-8") as file: + line = file.read().rstrip() + fields = line.split() + if len(fields) == 2: + str_cpu_quota = fields[0] + cpu_period = int(fields[1]) + # Make sure this is not in an unconstrained cgroup + if str_cpu_quota != "max": + cpu_quota = int(str_cpu_quota) + avail_cpu = int(cpu_quota / cpu_period) + else: + # Cgroupv1 systems + if Path("/sys/fs/cgroup/cpu/cpu.cfs_quota_us").is_file(): + with open("/sys/fs/cgroup/cpu/cpu.cfs_quota_us", encoding="utf-8") as file: + # Not useful for AWS Batch based jobs as result is -1, but works on local linux systems + cpu_quota = int(file.read().rstrip()) + + if ( + cpu_quota + and cpu_quota != -1 + and Path("/sys/fs/cgroup/cpu/cpu.cfs_period_us").is_file() + ): + with open("/sys/fs/cgroup/cpu/cpu.cfs_period_us", encoding="utf-8") as file: + cpu_period = int(file.read().rstrip()) + # Divide quota by period and you should get num of allotted CPU to the container, + # rounded down if fractional. + avail_cpu = int(cpu_quota / cpu_period) + elif Path("/sys/fs/cgroup/cpu/cpu.shares").is_file(): + with open("/sys/fs/cgroup/cpu/cpu.shares", encoding="utf-8") as file: + cpu_shares = int(file.read().rstrip()) + # For AWS, gives correct value * 1024. + avail_cpu = int(cpu_shares / 1024) # In K8s Pods also a fraction of a single core could be available # As multiprocessing is not able to run only a "fraction" of process diff --git a/tests/test_pylint_runners.py b/tests/test_pylint_runners.py index 3ffceda4cf..d334b08a49 100644 --- a/tests/test_pylint_runners.py +++ b/tests/test_pylint_runners.py @@ -18,6 +18,7 @@ import pytest from pylint import run_pylint, run_pyreverse, run_symilar +from pylint.lint.run import _query_cpu from pylint.testutils import GenericTestReporter as Reporter from pylint.testutils._run import _Run as Run from pylint.testutils.utils import _test_cwd @@ -90,6 +91,8 @@ def _mock_open(*args: Any, **kwargs: Any) -> BufferedReader: def _mock_path(*args: str, **kwargs: Any) -> pathlib.Path: if args[0] == "/sys/fs/cgroup/cpu/cpu.shares": return MagicMock(is_file=lambda: True) + if args[0] == "/sys/fs/cgroup/cpu.max": + return MagicMock(is_file=lambda: False) return pathlib_path(*args, **kwargs) filepath = os.path.abspath(__file__) @@ -100,3 +103,90 @@ def _mock_path(*args: str, **kwargs: Any) -> pathlib.Path: with patch("pylint.lint.run.Path", _mock_path): Run(testargs, reporter=Reporter()) assert err.value.code == 0 + + +@pytest.mark.parametrize( + "contents", + [ + "1 2", + "max 100000", + ], +) +def test_pylint_run_jobs_equal_zero_dont_crash_with_cgroupv2( + tmp_path: pathlib.Path, + contents: str, +) -> None: + """Check that the pylint runner does not crash if `pylint.lint.run._query_cpu` + determines only a fraction of a CPU core to be available. + """ + builtin_open = open + + def _mock_open(*args: Any, **kwargs: Any) -> BufferedReader: + if args[0] == "/sys/fs/cgroup/cpu.max": + return mock_open(read_data=contents)(*args, **kwargs) # type: ignore[no-any-return] + return builtin_open(*args, **kwargs) # type: ignore[no-any-return] + + pathlib_path = pathlib.Path + + def _mock_path(*args: str, **kwargs: Any) -> pathlib.Path: + if args[0] == "/sys/fs/cgroup/cpu/cpu.shares": + return MagicMock(is_file=lambda: False) + if args[0] == "/sys/fs/cgroup/cpu/cfs_quota_us": + return MagicMock(is_file=lambda: False) + if args[0] == "/sys/fs/cgroup/cpu.max": + return MagicMock(is_file=lambda: True) + return pathlib_path(*args, **kwargs) + + filepath = os.path.abspath(__file__) + testargs = [filepath, "--jobs=0"] + with _test_cwd(tmp_path): + with pytest.raises(SystemExit) as err: + with patch("builtins.open", _mock_open): + with patch("pylint.lint.run.Path", _mock_path): + Run(testargs, reporter=Reporter()) + assert err.value.code == 0 + + +@pytest.mark.parametrize( + "contents,expected", + [ + ("50000 100000", 1), + ("100000 100000", 1), + ("200000 100000", 2), + ("299999 100000", 2), + ("300000 100000", 3), + # Unconstrained cgroup + ("max 100000", None), + ], +) +def test_query_cpu_cgroupv2( + tmp_path: pathlib.Path, + contents: str, + expected: int, +) -> None: + """Check that `pylint.lint.run._query_cpu` generates realistic values in cgroupsv2 + systems. + """ + builtin_open = open + + def _mock_open(*args: Any, **kwargs: Any) -> BufferedReader: + if args[0] == "/sys/fs/cgroup/cpu.max": + return mock_open(read_data=contents)(*args, **kwargs) # type: ignore[no-any-return] + return builtin_open(*args, **kwargs) # type: ignore[no-any-return] + + pathlib_path = pathlib.Path + + def _mock_path(*args: str, **kwargs: Any) -> pathlib.Path: + if args[0] == "/sys/fs/cgroup/cpu/cpu.shares": + return MagicMock(is_file=lambda: False) + if args[0] == "/sys/fs/cgroup/cpu/cfs_quota_us": + return MagicMock(is_file=lambda: False) + if args[0] == "/sys/fs/cgroup/cpu.max": + return MagicMock(is_file=lambda: True) + return pathlib_path(*args, **kwargs) + + with _test_cwd(tmp_path): + with patch("builtins.open", _mock_open): + with patch("pylint.lint.run.Path", _mock_path): + cpus = _query_cpu() + assert cpus == expected From 972f45e69a5daea9c90cee345134627e6909c407 Mon Sep 17 00:00:00 2001 From: Pierre Sassoulas Date: Mon, 2 Dec 2024 22:38:06 +0100 Subject: [PATCH 2/3] Make the diff easier to read in pylint/lint/run.py --- pylint/lint/run.py | 79 ++++++++++++++++++++++++++-------------------- 1 file changed, 45 insertions(+), 34 deletions(-) diff --git a/pylint/lint/run.py b/pylint/lint/run.py index 68d50a8e7e..dc595fda96 100644 --- a/pylint/lint/run.py +++ b/pylint/lint/run.py @@ -43,43 +43,54 @@ def _query_cpu() -> int | None: This is based on discussion and copied from suggestions in https://bugs.python.org/issue36054. """ - cpu_quota, avail_cpu = None, None - if Path("/sys/fs/cgroup/cpu.max").is_file(): - # Cgroupv2 systems - with open("/sys/fs/cgroup/cpu.max", encoding="utf-8") as file: - line = file.read().rstrip() - fields = line.split() - if len(fields) == 2: - str_cpu_quota = fields[0] - cpu_period = int(fields[1]) - # Make sure this is not in an unconstrained cgroup - if str_cpu_quota != "max": - cpu_quota = int(str_cpu_quota) - avail_cpu = int(cpu_quota / cpu_period) + avail_cpu = _query_cpu_cgroupv2() else: - # Cgroupv1 systems - if Path("/sys/fs/cgroup/cpu/cpu.cfs_quota_us").is_file(): - with open("/sys/fs/cgroup/cpu/cpu.cfs_quota_us", encoding="utf-8") as file: - # Not useful for AWS Batch based jobs as result is -1, but works on local linux systems - cpu_quota = int(file.read().rstrip()) - - if ( - cpu_quota - and cpu_quota != -1 - and Path("/sys/fs/cgroup/cpu/cpu.cfs_period_us").is_file() - ): - with open("/sys/fs/cgroup/cpu/cpu.cfs_period_us", encoding="utf-8") as file: - cpu_period = int(file.read().rstrip()) - # Divide quota by period and you should get num of allotted CPU to the container, - # rounded down if fractional. - avail_cpu = int(cpu_quota / cpu_period) - elif Path("/sys/fs/cgroup/cpu/cpu.shares").is_file(): - with open("/sys/fs/cgroup/cpu/cpu.shares", encoding="utf-8") as file: - cpu_shares = int(file.read().rstrip()) - # For AWS, gives correct value * 1024. - avail_cpu = int(cpu_shares / 1024) + avail_cpu = _query_cpu_cgroupsv1() + return _query_cpu_handle_k8s_pods(avail_cpu) + + +def _query_cpu_cgroupv2() -> int | None: + avail_cpu = None + with open("/sys/fs/cgroup/cpu.max", encoding="utf-8") as file: + line = file.read().rstrip() + fields = line.split() + if len(fields) == 2: + str_cpu_quota = fields[0] + cpu_period = int(fields[1]) + # Make sure this is not in an unconstrained cgroup + if str_cpu_quota != "max": + cpu_quota = int(str_cpu_quota) + avail_cpu = int(cpu_quota / cpu_period) + return avail_cpu + + +def _query_cpu_cgroupsv1() -> int | None: + cpu_quota, avail_cpu = None, None + if Path("/sys/fs/cgroup/cpu/cpu.cfs_quota_us").is_file(): + with open("/sys/fs/cgroup/cpu/cpu.cfs_quota_us", encoding="utf-8") as file: + # Not useful for AWS Batch based jobs as result is -1, but works on local linux systems + cpu_quota = int(file.read().rstrip()) + + if ( + cpu_quota + and cpu_quota != -1 + and Path("/sys/fs/cgroup/cpu/cpu.cfs_period_us").is_file() + ): + with open("/sys/fs/cgroup/cpu/cpu.cfs_period_us", encoding="utf-8") as file: + cpu_period = int(file.read().rstrip()) + # Divide quota by period and you should get num of allotted CPU to the container, + # rounded down if fractional. + avail_cpu = int(cpu_quota / cpu_period) + elif Path("/sys/fs/cgroup/cpu/cpu.shares").is_file(): + with open("/sys/fs/cgroup/cpu/cpu.shares", encoding="utf-8") as file: + cpu_shares = int(file.read().rstrip()) + # For AWS, gives correct value * 1024. + avail_cpu = int(cpu_shares / 1024) + return avail_cpu + +def _query_cpu_handle_k8s_pods(avail_cpu: int | None) -> int | None: # In K8s Pods also a fraction of a single core could be available # As multiprocessing is not able to run only a "fraction" of process # assume we have 1 CPU available From d1eb6a601af2459f943695b500f4e8ee2ea0b48b Mon Sep 17 00:00:00 2001 From: Dominic Lavery Date: Wed, 4 Dec 2024 14:45:49 +0000 Subject: [PATCH 3/3] Add extra cgroup tests --- tests/test_pylint_runners.py | 151 +++++++++++++++++++++-------------- 1 file changed, 93 insertions(+), 58 deletions(-) diff --git a/tests/test_pylint_runners.py b/tests/test_pylint_runners.py index d334b08a49..5fc2da73f3 100644 --- a/tests/test_pylint_runners.py +++ b/tests/test_pylint_runners.py @@ -71,36 +71,38 @@ def test_pylint_argument_deduplication( assert err.value.code == 0 -def test_pylint_run_jobs_equal_zero_dont_crash_with_cpu_fraction( +@pytest.mark.parametrize( + "quota,shares,period", + [ + # Shares path + ("-1", "2", ""), + ("-1", "1023", ""), + ("-1", "1024", ""), + # Periods path + ("100", "", "200"), + ("999", "", "1000"), + ("1000", "", "1000"), + ], +) +def test_pylint_run_dont_crash_with_cgroupv1( tmp_path: pathlib.Path, + quota: str, + shares: str, + period: str, ) -> None: """Check that the pylint runner does not crash if `pylint.lint.run._query_cpu` determines only a fraction of a CPU core to be available. """ - builtin_open = open - - def _mock_open(*args: Any, **kwargs: Any) -> BufferedReader: - if args[0] == "/sys/fs/cgroup/cpu/cpu.cfs_quota_us": - return mock_open(read_data=b"-1")(*args, **kwargs) # type: ignore[no-any-return] - if args[0] == "/sys/fs/cgroup/cpu/cpu.shares": - return mock_open(read_data=b"2")(*args, **kwargs) # type: ignore[no-any-return] - return builtin_open(*args, **kwargs) # type: ignore[no-any-return] - - pathlib_path = pathlib.Path - - def _mock_path(*args: str, **kwargs: Any) -> pathlib.Path: - if args[0] == "/sys/fs/cgroup/cpu/cpu.shares": - return MagicMock(is_file=lambda: True) - if args[0] == "/sys/fs/cgroup/cpu.max": - return MagicMock(is_file=lambda: False) - return pathlib_path(*args, **kwargs) - filepath = os.path.abspath(__file__) testargs = [filepath, "--jobs=0"] + with _test_cwd(tmp_path): with pytest.raises(SystemExit) as err: - with patch("builtins.open", _mock_open): - with patch("pylint.lint.run.Path", _mock_path): + with patch( + "builtins.open", + mock_cgroup_fs(quota=quota, shares=shares, period=period), + ): + with patch("pylint.lint.run.Path", mock_cgroup_path(v2=False)): Run(testargs, reporter=Reporter()) assert err.value.code == 0 @@ -112,37 +114,20 @@ def _mock_path(*args: str, **kwargs: Any) -> pathlib.Path: "max 100000", ], ) -def test_pylint_run_jobs_equal_zero_dont_crash_with_cgroupv2( +def test_pylint_run_dont_crash_with_cgroupv2( tmp_path: pathlib.Path, contents: str, ) -> None: """Check that the pylint runner does not crash if `pylint.lint.run._query_cpu` determines only a fraction of a CPU core to be available. """ - builtin_open = open - - def _mock_open(*args: Any, **kwargs: Any) -> BufferedReader: - if args[0] == "/sys/fs/cgroup/cpu.max": - return mock_open(read_data=contents)(*args, **kwargs) # type: ignore[no-any-return] - return builtin_open(*args, **kwargs) # type: ignore[no-any-return] - - pathlib_path = pathlib.Path - - def _mock_path(*args: str, **kwargs: Any) -> pathlib.Path: - if args[0] == "/sys/fs/cgroup/cpu/cpu.shares": - return MagicMock(is_file=lambda: False) - if args[0] == "/sys/fs/cgroup/cpu/cfs_quota_us": - return MagicMock(is_file=lambda: False) - if args[0] == "/sys/fs/cgroup/cpu.max": - return MagicMock(is_file=lambda: True) - return pathlib_path(*args, **kwargs) - filepath = os.path.abspath(__file__) testargs = [filepath, "--jobs=0"] + with _test_cwd(tmp_path): with pytest.raises(SystemExit) as err: - with patch("builtins.open", _mock_open): - with patch("pylint.lint.run.Path", _mock_path): + with patch("builtins.open", mock_cgroup_fs(max_v2=contents)): + with patch("pylint.lint.run.Path", mock_cgroup_path(v2=True)): Run(testargs, reporter=Reporter()) assert err.value.code == 0 @@ -167,26 +152,76 @@ def test_query_cpu_cgroupv2( """Check that `pylint.lint.run._query_cpu` generates realistic values in cgroupsv2 systems. """ - builtin_open = open + with _test_cwd(tmp_path): + with patch("builtins.open", mock_cgroup_fs(max_v2=contents)): + with patch("pylint.lint.run.Path", mock_cgroup_path(v2=True)): + cpus = _query_cpu() + assert cpus == expected - def _mock_open(*args: Any, **kwargs: Any) -> BufferedReader: - if args[0] == "/sys/fs/cgroup/cpu.max": - return mock_open(read_data=contents)(*args, **kwargs) # type: ignore[no-any-return] - return builtin_open(*args, **kwargs) # type: ignore[no-any-return] - pathlib_path = pathlib.Path +@pytest.mark.parametrize( + "quota,shares,period,expected", + [ + # Shares path + ("-1", "2", "", 1), + ("-1", "1023", "", 1), + ("-1", "1024", "", 1), + ("-1", "2048", "", 2), + # Periods path + ("100", "", "200", 1), + ("999", "", "1000", 1), + ("1000", "", "1000", 1), + ("2000", "", "1000", 2), + ], +) +def test_query_cpu_cgroupv1( + tmp_path: pathlib.Path, + quota: str, + shares: str, + period: str, + expected: int, +) -> None: + """Check that `pylint.lint.run._query_cpu` generates realistic values in cgroupsv1 + systems. + """ + with _test_cwd(tmp_path): + with patch( + "builtins.open", mock_cgroup_fs(quota=quota, shares=shares, period=period) + ): + with patch("pylint.lint.run.Path", mock_cgroup_path(v2=False)): + cpus = _query_cpu() + assert cpus == expected + +def mock_cgroup_path(v2: bool) -> Any: def _mock_path(*args: str, **kwargs: Any) -> pathlib.Path: + if args[0] == "/sys/fs/cgroup/cpu/cpu.cfs_period_us": + return MagicMock(is_file=lambda: not v2) if args[0] == "/sys/fs/cgroup/cpu/cpu.shares": - return MagicMock(is_file=lambda: False) - if args[0] == "/sys/fs/cgroup/cpu/cfs_quota_us": - return MagicMock(is_file=lambda: False) + return MagicMock(is_file=lambda: not v2) + if args[0] == "/sys/fs/cgroup/cpu/cpu.cfs_quota_us": + return MagicMock(is_file=lambda: not v2) if args[0] == "/sys/fs/cgroup/cpu.max": - return MagicMock(is_file=lambda: True) - return pathlib_path(*args, **kwargs) + return MagicMock(is_file=lambda: v2) + return pathlib.Path(*args, **kwargs) - with _test_cwd(tmp_path): - with patch("builtins.open", _mock_open): - with patch("pylint.lint.run.Path", _mock_path): - cpus = _query_cpu() - assert cpus == expected + return _mock_path + + +def mock_cgroup_fs( + quota: str = "", shares: str = "", period: str = "", max_v2: str = "" +) -> Any: + builtin_open = open + + def _mock_open(*args: Any, **kwargs: Any) -> BufferedReader: + if args[0] == "/sys/fs/cgroup/cpu/cpu.cfs_quota_us": + return mock_open(read_data=quota)(*args, **kwargs) # type: ignore[no-any-return] + if args[0] == "/sys/fs/cgroup/cpu/cpu.shares": + return mock_open(read_data=shares)(*args, **kwargs) # type: ignore[no-any-return] + if args[0] == "/sys/fs/cgroup/cpu/cpu.cfs_period_us": + return mock_open(read_data=period)(*args, **kwargs) # type: ignore[no-any-return] + if args[0] == "/sys/fs/cgroup/cpu.max": + return mock_open(read_data=max_v2)(*args, **kwargs) # type: ignore[no-any-return] + return builtin_open(*args, **kwargs) # type: ignore[no-any-return] + + return _mock_open