cpython
cpython copied to clipboard
python
gh-95023: Added os.setns and os.unshare to easily switch between namespaces on Linux
Most changes to Python require a NEWS entry.
Please add it using the blurb_it web app or the blurb command-line tool.
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.
@MaxwellDupre:
Just providing the bear functions with out examples and descriptions seems like half a job.
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.
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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
And if you don't make the requested changes, you will be poked with soft cushions!
Thanks for making the requested changes!
@erlend-aasland, @tiran: please review the changes made to this pull request.
It looks like you need a clinic regen. Also, you may consider to summon the @python/proofreaders to scrutinize the added docs.
summon the
@python/proofreadersHow do you do that?
👉🏻 @python/proofreaders: would you mind taking a look at the docs changes in this PR?
It won't work when I do it. I cant even access https://github.com/orgs/python/teams/proofreaders, getting a 404 from GitHub @python/proofreaders
Yeah, there's definitely an access problem of some sort. @erlend-aasland's ping rendered in my email notification; @noamcohen97's didn't.
~High level question, coming from a completely naive perspective on this: are os.setns and os.unshare opposites? That is, say, does os.unshare undo what os.setns does? If so, why aren't the names more obviously opposites of one another? Are they aligning with something in the underlying OS?~
Never mind, became clear after starting review.
unshare moves the calling thread / process into a new namespace. It creates a new namespace.
The setns syscall moves a process into an existing namespace of another process.
The functions are low level wrappers around Linux syscalls. You may be familiar with them from Linux container runtimes like Docker.
![cpython-cla-bot[bot] avatar](https://avatars.githubusercontent.com/in/171517?v=4 "Published by a pub.dev verified publisher"){kind=small}
Review completed, @noamcohen97 -- my apologies if I got any of the technical details wrong in my comments. Feel free to accept/modify/reject as you prefer!
It looks like all conversations are resolved. I'll wait a couple of days before landing this.
BTW, you could add yourself in Misc/ACKS, and also, please regenerate clinic.
I'll also wait for @tiran's final approve, since he requested changes.
I'm waiting for Christian, since he requested changes. For me, the PR is good to go.
:robot: New build scheduled with the buildbot fleet by @encukou for commit b14396e342d156ca620c25c5aed8e2e7520e747c :robot:
If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.
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.
@encukou Looks like failures are related to #98216 I have updated the branch, can you started the buildbot check again please?
:robot: New build scheduled with the buildbot fleet by @CAM-Gerlach for commit 50c48098f21be3e2648c55935fad5adb82319ea7 :robot:
If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.
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 ;-)
