gh-95023: Added os.setns and os.unshare to easily switch between namespaces on Linux #95046
Conversation
noamcohen97
commented
Jul 20, 2022
•
noamcohen97
commented
- Issue: Implement os.setns, os.unshare to allow changing namespaces in Linux #95023
|
Most changes to Python require a NEWS entry. Please add it using the blurb_it web app or the blurb command-line tool. |
noamcohen97
force-pushed
the
namespaces
branch
2 times, most recently
from
868fe76 to
92a6d73
Compare
|
I would like to understand more about why I would want to use these functions, how to use and caveats. Just providing the bear functions with out examples and descriptions seems like half a job. Due to the current excellent documentation and current direction of the docs. |
There is no need for such a negative tone; please try to be more encouraging when leaving review comments. Noam has followed the dev flow by creating an issue, explaining the need for this change, and has provided a very well written patch, complete with docs, tests, and a NEWS entry. I think he has done a great job, and I welcome his interest in making CPython better. I welcome your interest in helping review PRs. Reviewing PRs is a difficult task; often more difficult than writing the code (or the docs) itself. I know the devguide is lacking when it comes to "how to review PRs". For now, the best thing to do is IMO to observe how others, more experienced reviewers, perform this task, and learn from them. Personally, I often find reviewing PRs difficult, and I often use far more time when reviewing a PR than I would use to write one. |
|
Understand, I did think about how to phrase and just went with the shortest. I realise how it maybe seen as negative and will try to improve. |
Misc/NEWS.d/next/Core and Builtins/2022-07-20-09-04-55.gh-issue-95023.bs-xd7.rst
Outdated
Show resolved
Hide resolved
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase And if you don't make the requested changes, you will be poked with soft cushions! |
Co-authored-by: Christian Heimes <christian@python.org>
This reverts commit 3685a27.
also, alphabetically order consts in doc
|
I have made the requested changes; please review again. |
|
@tiran @erlend-aasland ping |
|
I'm waiting for Christian, since he requested changes. For me, the PR is good to go. |
|
@tiran ? |
|
#98056 fixed the failing tests |
|
I've pinged @tiran again out of band |
encukou
added
the
🔨 test-with-buildbots
bedevere-bot
removed
the
🔨 test-with-buildbots
|
As far as I know, Christian is quite busy these days. Don't wait for him too long if he didn't request that you do. He can always do a post-merge review. I started a buildbot check, since this looks like it might have platform-specific issues. |
|
I've triggered a re-run of the buildbots |
CAM-Gerlach
added
the
🔨 test-with-buildbots
|
🤖 New build scheduled with the buildbot fleet by @CAM-Gerlach for commit 50c4809 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
bedevere-bot
removed
the
🔨 test-with-buildbots
Lib/test/test_posix.py
Outdated
|
|
||
| if rc == 2: | ||
| e = int(err[0]) | ||
| self.assertIn(e, (errno.EPERM, errno.EINVAL, errno.ENOSPC, errno.ENOSYS)) |
There was a problem hiding this comment.
I would prefer to move these checks inside the child process code. I don't think that skipping the test if unshare() or setns() fails matters. Just exit the child process early with a success (exit code 0, well, just exit) if you get one of these errors, no? If the child process must always succeed, you can use test.suppot.script_helper.assert_python_ok() which captures stdout and stderr and raises an exception with all data if the process fails.
Maybe it would make sense to add a except PermissionError: instead. Would you mind to add a comment explaining why/how PermissionError can happen?
ENOSPC is expected on unshare() if we exceed some limits on namespaces? If it can only happen on unshare(), maybe add a try/except only on this function call?
Why is EINVAL expected? Can it be raised if there is a bug in the test? Or can it be raised depending on the kernel configuration?
There was a problem hiding this comment.
Cool! did not know about assert_python_ok
There was a problem hiding this comment.
EINVAL could be raised the kernel was not configured with the CONFIG_UTS_NS option.
Co-authored-by: Victor Stinner <vstinner@python.org>
|
Ok, enough nitpicking. This version is good enough to be merged. We can always enhance tests and doc later if needed. Thanks a lot @noamcohen97 for updating your PR many times. I like it and merged your PR. unshare() and setns() are important functions nowadays on Linux. By the way, I like the "unshare" command line tool ;-) |
* main: (40 commits) pythongh-98461: Fix source location in comprehensions bytecode (pythonGH-98464) pythongh-98421: Clean Up PyObject_Print (pythonGH-98422) pythongh-98360: multiprocessing now spawns children on Windows with correct argv[0] in virtual environments (pythonGH-98462) CODEOWNERS: Become a typing code owner (python#98480) [doc] Improve logging cookbook example. (pythonGH-98481) Add more tkinter.Canvas tests (pythonGH-98475) pythongh-95023: Added os.setns and os.unshare functions (python#95046) pythonGH-98363: Presize the list for batched() (pythonGH-98419) pythongh-98374: Suppress ImportError for invalid query for help() command. (pythongh-98450) typing tests: `_overload_dummy` raises `NotImplementedError`, not `RuntimeError` (python#98351) pythongh-98354: Add unicode check for 'name' attribute in _imp_create_builtin (pythonGH-98412) pythongh-98257: Make _PyEval_SetTrace() reentrant (python#98258) pythongh-98414: py.exe launcher does not use defaults for -V:company/ option (pythonGH-98460) pythongh-98417: Store int_max_str_digits on the Interpreter State (pythonGH-98418) Doc: Remove title text from internal links (python#98409) [doc] Refresh the venv introduction documentation, and correct the statement about VIRTUAL_ENV (pythonGH-98350) Docs: Bump sphinx-lint and fix unbalanced inline literal markup (python#98441) pythongh-92886: Replace assertion statements in `handlers.BaseHandler` to support running with optimizations (`-O`) (pythonGH-93231) pythongh-92886: Fix tests that fail when running with optimizations (`-O`) in `_test_multiprocessing.py` (pythonGH-93233) pythongh-92886: Fix tests that fail when running with optimizations (`-O`) in `test_py_compile.py` (pythonGH-93235) ...
