From dac11435156721c1e25affc0cd3bf1743098faa3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20G=C3=B3rny?= Date: Wed, 18 Sep 2024 18:39:34 +0200 Subject: [PATCH 1/8] gh-124213: Skip tests failing with ``--suppress-sync=true`` when inside systemd-nspawn container Add a helper functon that checks whether the test suite is running inside a systemd-nspawn container, and skip the few tests failing with `--suppress-sync=true` in that case. The tests are failing because `--suppress-sync=true` stubs out `fsync()`, `fdatasync()` and `msync()` calls, and therefore they always return success without checking for invalid arguments. Technically, this means that the tests are also skipped when running inside systemd-nspawn container without `--suppress-sync=true`. However, to the best of my knowledge the only way to detect this option is to actually reproduce one of the unexpectedly-succeeding syscalls, and since this is precisely what we're testing for, the skip-test and the actual test would be doing the same thing. --- Lib/test/support/__init__.py | 10 ++++++++++ Lib/test/test_mmap.py | 3 ++- Lib/test/test_os.py | 10 +++++++--- .../2024-09-18-18-39-21.gh-issue-124213.AQq_xg.rst | 2 ++ 4 files changed, 21 insertions(+), 4 deletions(-) create mode 100644 Misc/NEWS.d/next/Tests/2024-09-18-18-39-21.gh-issue-124213.AQq_xg.rst diff --git a/Lib/test/support/__init__.py b/Lib/test/support/__init__.py index dbf479dddff7b3..6fd1f2c5042590 100644 --- a/Lib/test/support/__init__.py +++ b/Lib/test/support/__init__.py @@ -17,6 +17,7 @@ import types import unittest import warnings +from pathlib import Path __all__ = [ @@ -61,6 +62,7 @@ "without_optimizer", "force_not_colorized", "BrokenIter", + "in_systemd_nspawn", ] @@ -2873,3 +2875,11 @@ def __iter__(self): if self.iter_raises: 1/0 return self + + +def in_systemd_nspawn() -> bool: + try: + return (Path("/run/systemd/container").read_bytes().rstrip() == + b"systemd-nspawn") + except FileNotFoundError: + return False diff --git a/Lib/test/test_mmap.py b/Lib/test/test_mmap.py index a1cf5384ada5b5..3316a7276db82c 100644 --- a/Lib/test/test_mmap.py +++ b/Lib/test/test_mmap.py @@ -1,5 +1,6 @@ from test.support import ( requires, _2G, _4G, gc_collect, cpython_only, is_emscripten, is_apple, + in_systemd_nspawn, ) from test.support.import_helper import import_module from test.support.os_helper import TESTFN, unlink @@ -839,7 +840,7 @@ def test_flush_return_value(self): mm.write(b'python') result = mm.flush() self.assertIsNone(result) - if sys.platform.startswith(('linux', 'android')): + if sys.platform.startswith(('linux', 'android')) and not in_systemd_nspawn(): # 'offset' must be a multiple of mmap.PAGESIZE on Linux. # See bpo-34754 for details. self.assertRaises(OSError, mm.flush, 1, len(b'python')) diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index 1e570c757fccbc..e7c8ef6a29be76 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -2351,9 +2351,13 @@ def test_chmod(self): @unittest.skipIf(support.is_wasi, "Cannot create invalid FD on WASI.") class TestInvalidFD(unittest.TestCase): - singles = ["fchdir", "dup", "fdatasync", "fstat", - "fstatvfs", "fsync", "tcgetpgrp", "ttyname"] - singles_fildes = {"fchdir", "fdatasync", "fsync"} + singles = ["fchdir", "dup", "fstat", "fstatvfs", "tcgetpgrp", "ttyname"] + singles_fildes = {"fchdir"} + # systemd-nspawn --suppress-sync=true does not verify fd passed + # fdatasync() and fsync(), and always returns success + if not support.in_systemd_nspawn(): + singles += ["fdatasync", "fsync"] + singles_fildes |= {"fdatasync", "fsync"} #singles.append("close") #We omit close because it doesn't raise an exception on some platforms def get_single(f): diff --git a/Misc/NEWS.d/next/Tests/2024-09-18-18-39-21.gh-issue-124213.AQq_xg.rst b/Misc/NEWS.d/next/Tests/2024-09-18-18-39-21.gh-issue-124213.AQq_xg.rst new file mode 100644 index 00000000000000..8ac2ac24e7363c --- /dev/null +++ b/Misc/NEWS.d/next/Tests/2024-09-18-18-39-21.gh-issue-124213.AQq_xg.rst @@ -0,0 +1,2 @@ +Skip tests failing with ``--suppress-sync=true`` when running inside +systemd-nspawn container. From c4dbdeba4af41f9c1332bc16a3dd1e0c8dcd2708 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20G=C3=B3rny?= Date: Fri, 20 Sep 2024 12:57:28 +0200 Subject: [PATCH 2/8] Implement review comments from @vstinner Add a docstring explaining the function in detail. Switch from pathlib to plain `open().read()`. Update the news entry. --- Lib/test/support/__init__.py | 15 ++++++++++++--- ...2024-09-18-18-39-21.gh-issue-124213.AQq_xg.rst | 5 +++-- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/Lib/test/support/__init__.py b/Lib/test/support/__init__.py index 6fd1f2c5042590..7d9f63c33799be 100644 --- a/Lib/test/support/__init__.py +++ b/Lib/test/support/__init__.py @@ -17,7 +17,6 @@ import types import unittest import warnings -from pathlib import Path __all__ = [ @@ -2878,8 +2877,18 @@ def __iter__(self): def in_systemd_nspawn() -> bool: + """ + Test whether the test suite is running inside of systemd-nspawn container + + Return True if the test suite is being run inside a systemd-nspawn + container, False otherwise. This can be used to skip tests that are + known to be reliable inside this kind of virtualization, for example + tests that are relying on fsync() not being stubbed out + (as ``systemd-nspawn --suppress-sync=true` does). + """ + try: - return (Path("/run/systemd/container").read_bytes().rstrip() == - b"systemd-nspawn") + with open("/run/systemd/container", "rb") as fp: + return fp.read().rstrip() == b"systemd-nspawn" except FileNotFoundError: return False diff --git a/Misc/NEWS.d/next/Tests/2024-09-18-18-39-21.gh-issue-124213.AQq_xg.rst b/Misc/NEWS.d/next/Tests/2024-09-18-18-39-21.gh-issue-124213.AQq_xg.rst index 8ac2ac24e7363c..98376a25ffdc4a 100644 --- a/Misc/NEWS.d/next/Tests/2024-09-18-18-39-21.gh-issue-124213.AQq_xg.rst +++ b/Misc/NEWS.d/next/Tests/2024-09-18-18-39-21.gh-issue-124213.AQq_xg.rst @@ -1,2 +1,3 @@ -Skip tests failing with ``--suppress-sync=true`` when running inside -systemd-nspawn container. +Skip ``test_os`` and ``test_mmap`` tests that are failing +with ``--suppress-sync=true`` when running inside a systemd-nspawn +container. From e580bb5f9394ed06c777809dd5fc86b5cd673a9a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20G=C3=B3rny?= Date: Fri, 20 Sep 2024 13:10:46 +0200 Subject: [PATCH 3/8] Detect `--suppress-sync=true` and limit the skips to that Call `os.open("", os.O_RDONLY | os.O_SYNC)` and check the errno to detect whether `--suppress-sync=true` is actually used, and skip the tests only in that scenario. --- Lib/test/support/__init__.py | 33 +++++++++++++++---- Lib/test/test_mmap.py | 4 +-- Lib/test/test_os.py | 2 +- ...-09-18-18-39-21.gh-issue-124213.AQq_xg.rst | 6 ++-- 4 files changed, 32 insertions(+), 13 deletions(-) diff --git a/Lib/test/support/__init__.py b/Lib/test/support/__init__.py index 7d9f63c33799be..5ab64345c888e7 100644 --- a/Lib/test/support/__init__.py +++ b/Lib/test/support/__init__.py @@ -61,7 +61,7 @@ "without_optimizer", "force_not_colorized", "BrokenIter", - "in_systemd_nspawn", + "in_systemd_nspawn", "in_systemd_nspawn_sync_suppressed", ] @@ -2879,12 +2879,6 @@ def __iter__(self): def in_systemd_nspawn() -> bool: """ Test whether the test suite is running inside of systemd-nspawn container - - Return True if the test suite is being run inside a systemd-nspawn - container, False otherwise. This can be used to skip tests that are - known to be reliable inside this kind of virtualization, for example - tests that are relying on fsync() not being stubbed out - (as ``systemd-nspawn --suppress-sync=true` does). """ try: @@ -2892,3 +2886,28 @@ def in_systemd_nspawn() -> bool: return fp.read().rstrip() == b"systemd-nspawn" except FileNotFoundError: return False + + +def in_systemd_nspawn_sync_suppressed() -> bool: + """ + Test whether the test suite is runing in systemd-nspawn with ``--suppress-sync=true`` + + Return True if the test suite is being run inside a systemd-nspawn + container and ``--suppress-sync=true`` option is detected to be used, + False otherwise. This can be used to skip tests that rely + on ``fsync()`` calls and similar not being intercepted. + """ + + if hasattr(os, "O_SYNC") and in_systemd_nspawn(): + import errno + + # If systemd-nspawn is used, O_SYNC flag will immediately trigger + # EINVAL. Otherwise, ENOENT will be given instead. + try: + with os.open("", os.O_RDONLY | os.O_SYNC): + pass + except OSError as err: + if err.errno == errno.EINVAL: + return True + + return False diff --git a/Lib/test/test_mmap.py b/Lib/test/test_mmap.py index 3316a7276db82c..5997e9a688d9b8 100644 --- a/Lib/test/test_mmap.py +++ b/Lib/test/test_mmap.py @@ -1,6 +1,6 @@ from test.support import ( requires, _2G, _4G, gc_collect, cpython_only, is_emscripten, is_apple, - in_systemd_nspawn, + in_systemd_nspawn_sync_suppressed, ) from test.support.import_helper import import_module from test.support.os_helper import TESTFN, unlink @@ -840,7 +840,7 @@ def test_flush_return_value(self): mm.write(b'python') result = mm.flush() self.assertIsNone(result) - if sys.platform.startswith(('linux', 'android')) and not in_systemd_nspawn(): + if sys.platform.startswith(('linux', 'android')) and not in_systemd_nspawn_sync_suppressed(): # 'offset' must be a multiple of mmap.PAGESIZE on Linux. # See bpo-34754 for details. self.assertRaises(OSError, mm.flush, 1, len(b'python')) diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index e7c8ef6a29be76..9fa4e406bd2468 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -2355,7 +2355,7 @@ class TestInvalidFD(unittest.TestCase): singles_fildes = {"fchdir"} # systemd-nspawn --suppress-sync=true does not verify fd passed # fdatasync() and fsync(), and always returns success - if not support.in_systemd_nspawn(): + if not support.in_systemd_nspawn_sync_suppressed(): singles += ["fdatasync", "fsync"] singles_fildes |= {"fdatasync", "fsync"} #singles.append("close") diff --git a/Misc/NEWS.d/next/Tests/2024-09-18-18-39-21.gh-issue-124213.AQq_xg.rst b/Misc/NEWS.d/next/Tests/2024-09-18-18-39-21.gh-issue-124213.AQq_xg.rst index 98376a25ffdc4a..4f4db91e8fac8f 100644 --- a/Misc/NEWS.d/next/Tests/2024-09-18-18-39-21.gh-issue-124213.AQq_xg.rst +++ b/Misc/NEWS.d/next/Tests/2024-09-18-18-39-21.gh-issue-124213.AQq_xg.rst @@ -1,3 +1,3 @@ -Skip ``test_os`` and ``test_mmap`` tests that are failing -with ``--suppress-sync=true`` when running inside a systemd-nspawn -container. +Detect whether the test suite is running inside a systemd-nspawn container +with ``--suppress-sync=true`` option, and skip the `test_os`` and ``test_mmap`` +tests that are failing in this scenario. From ce85e7526aae07dcf44e9d0d352b7972e0e170aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20G=C3=B3rny?= Date: Fri, 20 Sep 2024 13:14:06 +0200 Subject: [PATCH 4/8] Fix formatting (typo) in the news entry --- .../next/Tests/2024-09-18-18-39-21.gh-issue-124213.AQq_xg.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Misc/NEWS.d/next/Tests/2024-09-18-18-39-21.gh-issue-124213.AQq_xg.rst b/Misc/NEWS.d/next/Tests/2024-09-18-18-39-21.gh-issue-124213.AQq_xg.rst index 4f4db91e8fac8f..021fbefb635af1 100644 --- a/Misc/NEWS.d/next/Tests/2024-09-18-18-39-21.gh-issue-124213.AQq_xg.rst +++ b/Misc/NEWS.d/next/Tests/2024-09-18-18-39-21.gh-issue-124213.AQq_xg.rst @@ -1,3 +1,3 @@ Detect whether the test suite is running inside a systemd-nspawn container -with ``--suppress-sync=true`` option, and skip the `test_os`` and ``test_mmap`` -tests that are failing in this scenario. +with ``--suppress-sync=true`` option, and skip the ``test_os`` +and ``test_mmap`` tests that are failing in this scenario. From e016a82f84834a77b0861d88f46028676d153d44 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20G=C3=B3rny?= Date: Fri, 20 Sep 2024 12:54:06 +0000 Subject: [PATCH 5/8] Apply suggestions from @vstinner Co-authored-by: Victor Stinner --- Lib/test/support/__init__.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/Lib/test/support/__init__.py b/Lib/test/support/__init__.py index 5ab64345c888e7..e6ec9437aa3d89 100644 --- a/Lib/test/support/__init__.py +++ b/Lib/test/support/__init__.py @@ -2890,12 +2890,11 @@ def in_systemd_nspawn() -> bool: def in_systemd_nspawn_sync_suppressed() -> bool: """ - Test whether the test suite is runing in systemd-nspawn with ``--suppress-sync=true`` + Test whether the test suite is runing in systemd-nspawn + with ``--suppress-sync=true``. - Return True if the test suite is being run inside a systemd-nspawn - container and ``--suppress-sync=true`` option is detected to be used, - False otherwise. This can be used to skip tests that rely - on ``fsync()`` calls and similar not being intercepted. + This can be used to skip tests that rely on ``fsync()`` calls + and similar not being intercepted. """ if hasattr(os, "O_SYNC") and in_systemd_nspawn(): @@ -2904,7 +2903,7 @@ def in_systemd_nspawn_sync_suppressed() -> bool: # If systemd-nspawn is used, O_SYNC flag will immediately trigger # EINVAL. Otherwise, ENOENT will be given instead. try: - with os.open("", os.O_RDONLY | os.O_SYNC): + with os.open(__file__, os.O_RDONLY | os.O_SYNC): pass except OSError as err: if err.errno == errno.EINVAL: From 68802f8af37aae64f823bdb048ab6dce0d124540 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20G=C3=B3rny?= Date: Fri, 20 Sep 2024 14:59:06 +0200 Subject: [PATCH 6/8] Inline in_systemd_nspawn() in *_sync_suppressed() --- Lib/test/support/__init__.py | 42 +++++++++++++++++------------------- 1 file changed, 20 insertions(+), 22 deletions(-) diff --git a/Lib/test/support/__init__.py b/Lib/test/support/__init__.py index e6ec9437aa3d89..4e84a150a91a71 100644 --- a/Lib/test/support/__init__.py +++ b/Lib/test/support/__init__.py @@ -61,7 +61,7 @@ "without_optimizer", "force_not_colorized", "BrokenIter", - "in_systemd_nspawn", "in_systemd_nspawn_sync_suppressed", + "in_systemd_nspawn_sync_suppressed", ] @@ -2876,17 +2876,6 @@ def __iter__(self): return self -def in_systemd_nspawn() -> bool: - """ - Test whether the test suite is running inside of systemd-nspawn container - """ - - try: - with open("/run/systemd/container", "rb") as fp: - return fp.read().rstrip() == b"systemd-nspawn" - except FileNotFoundError: - return False - def in_systemd_nspawn_sync_suppressed() -> bool: """ @@ -2897,16 +2886,25 @@ def in_systemd_nspawn_sync_suppressed() -> bool: and similar not being intercepted. """ - if hasattr(os, "O_SYNC") and in_systemd_nspawn(): - import errno + if not hasattr(os, "O_SYNC"): + return False - # If systemd-nspawn is used, O_SYNC flag will immediately trigger - # EINVAL. Otherwise, ENOENT will be given instead. - try: - with os.open(__file__, os.O_RDONLY | os.O_SYNC): - pass - except OSError as err: - if err.errno == errno.EINVAL: - return True + try: + with open("/run/systemd/container", "rb") as fp: + if fp.read().rstrip() != b"systemd-nspawn": + return False + except FileNotFoundError: + return False + + import errno + + # If systemd-nspawn is used, O_SYNC flag will immediately + # trigger EINVAL. Otherwise, ENOENT will be given instead. + try: + with os.open(__file__, os.O_RDONLY | os.O_SYNC): + pass + except OSError as err: + if err.errno == errno.EINVAL: + return True return False From ed0fc4286470329b67f1b8c666d8634834dba455 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Fri, 20 Sep 2024 15:11:10 +0200 Subject: [PATCH 7/8] Apply suggestions from code review --- Lib/test/support/__init__.py | 4 +--- Lib/test/test_mmap.py | 3 ++- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/Lib/test/support/__init__.py b/Lib/test/support/__init__.py index 4e84a150a91a71..628529b8664c77 100644 --- a/Lib/test/support/__init__.py +++ b/Lib/test/support/__init__.py @@ -2876,7 +2876,6 @@ def __iter__(self): return self - def in_systemd_nspawn_sync_suppressed() -> bool: """ Test whether the test suite is runing in systemd-nspawn @@ -2896,10 +2895,9 @@ def in_systemd_nspawn_sync_suppressed() -> bool: except FileNotFoundError: return False - import errno - # If systemd-nspawn is used, O_SYNC flag will immediately # trigger EINVAL. Otherwise, ENOENT will be given instead. + import errno try: with os.open(__file__, os.O_RDONLY | os.O_SYNC): pass diff --git a/Lib/test/test_mmap.py b/Lib/test/test_mmap.py index 5997e9a688d9b8..638dc2f8d976e8 100644 --- a/Lib/test/test_mmap.py +++ b/Lib/test/test_mmap.py @@ -840,7 +840,8 @@ def test_flush_return_value(self): mm.write(b'python') result = mm.flush() self.assertIsNone(result) - if sys.platform.startswith(('linux', 'android')) and not in_systemd_nspawn_sync_suppressed(): + if (sys.platform.startswith(('linux', 'android')) + and not in_systemd_nspawn_sync_suppressed()): # 'offset' must be a multiple of mmap.PAGESIZE on Linux. # See bpo-34754 for details. self.assertRaises(OSError, mm.flush, 1, len(b'python')) From 222fcc0d6703899628f28751805f8bc31bc401cf Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Fri, 20 Sep 2024 15:12:50 +0200 Subject: [PATCH 8/8] Update Lib/test/test_mmap.py --- Lib/test/test_mmap.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/test_mmap.py b/Lib/test/test_mmap.py index 638dc2f8d976e8..b2a299ed172967 100644 --- a/Lib/test/test_mmap.py +++ b/Lib/test/test_mmap.py @@ -840,7 +840,7 @@ def test_flush_return_value(self): mm.write(b'python') result = mm.flush() self.assertIsNone(result) - if (sys.platform.startswith(('linux', 'android')) + if (sys.platform.startswith(('linux', 'android')) and not in_systemd_nspawn_sync_suppressed()): # 'offset' must be a multiple of mmap.PAGESIZE on Linux. # See bpo-34754 for details.