-
Notifications
You must be signed in to change notification settings - Fork 344
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(libcontainer) no_pivot args is not used #2923
Conversation
972e813
to
fb2a3d9
Compare
@udzura This PR addresses the issue where the no_pivot argument is not recognized. This argument is crucial in environments where the pivot_root syscall is not permitted, such as when running containers within containers. |
I understand what this PR aim to do 👍 But I have no privilege to merge this. I guess you've mistaken me for someone! |
@udzura sorry for bother you, could you help to involve someone that is able to have a look at this PR? Really appreciate. |
I'll check this PR. Thanks for your first contribution! |
@xujihui1985 May I ask you to add an integration test for this PR? |
sure, I'd like to learn how to add integrate test |
I can help you if you need ;) |
Hey @xujihui1985 Thanks for your contribution! May I ask you to also take a look at #2597 , which also deals with |
Hi @YJDoc2 I just noticed a similar pull request that addresses this issue and is almost identical to my approach. However, the only problem is that the chroot implementation is incorrect. I can build upon his PR, fix the chroot issue, and include credit for his work. |
Signed-off-by: Vanient <xiadanni1@huawei.com>
Move the rootfs to the root of the host filesystem before chrooting, this is equivalent to pivot_root, if don't move mount first, we will not see the new rootfs when exec into the container Signed-off-by: xujihui1985 <xujihui1985@gmail.com>
fb2a3d9
to
65d6b54
Compare
…havior Move the mount operation to occur before calling chroot to better simulate the effect of pivot_root. Add a check to confirm if the current process is running inside an isolated mount namespace, ensuring proper mount handling. Signed-off-by: xujihui1985 <xujihui1985@gmail.com>
Signed-off-by: xujihui1985 <xujihui1985@gmail.com>
Feat/implement intergration test
@udzura, @YJDoc2 Hi, I have implemented the integration test in commit c82496b. For now, I copied and reused some utility functions, like test_inside_container -> test_inside_container_with_no_pivot, since create_container currently doesn't allow setting the necessary arguments. and change this function will cause lots of change in current testcase. This was done to verify if the test works as expected. Once the test is reviewed and approved, I'll refactor the functions accordingly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I expected. But I want @YJDoc2 to review it, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, let me know if any improvements are possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll take a look at this soon. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, the overall implementation looks good, I have a couple of nitpick comments, please take a look at them.
As for the tests, I feel that the added test is ok, but we need to refactor the test_inside...
function to avoid duplication. I'll think on a way for more "ergonomic" imple, but if you can think of something, go ahead.
I'm also thinking we should have this functionality in test_outside..
so we can also test for container create,run,delete with no_pivot. WDYT? Would that be useful?
/cc @Gekko0114 |
Signed-off-by: xujihui1985 <xujihui1985@gmail.com>
612be88
to
6aa2ce5
Compare
@YJDoc2 I believe the test_outside_container with the no-pivot option doesn't behave any differently from the test with the option enabled. It's unclear what to assert when testing outside the container with the no-pivot option because, outside of a container mount namespace, chroot and pivot_root have no noticeable effect. Any idea? |
Signed-off-by: xujihui1985 <xujihui1985@gmail.com>
@YJDoc2 Do you think it would be better to refactor the test_inside... function in a separate PR? It will involve a lot of changes as the change of the signature |
I mentioned
I think the refactoring should be done in separate PR as that will be a big change, I'll do a final review, hopefully today, and if all's ok then merge, after which you can start with that refactoring. |
@YJDoc2 Hi, hope everything goes well with you, if there is anything I can improve for this PR, please let me know as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, I have a couple of nits, but nothing that blocks the merge. I'm approving this PR, and you can address the nits in the PR you'll make to refactor the test function. I'm not merging this right away, in case you want to reply something, but if no reply, I'll merge it by tomorrow.
if namespaces.get(LinuxNamespaceType::Mount)?.is_some() { | ||
// change the root of filesystem of the process to the rootfs | ||
syscall.pivot_rootfs(rootfs_path).map_err(|err| { | ||
tracing::error!(?err, ?rootfs_path, "failed to pivot root"); | ||
InitProcessError::SyscallOther(err) | ||
})?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we should move the comment above this to the function where we are doing this
let res = Command::new(get_runtime_path()) | ||
// set stdio so that we can get o/p of runtimetest | ||
// in test_inside_container function | ||
fn create_container_command<P: AsRef<Path>>(id: &str, dir: P, with_pivot_root: bool) -> Command { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I feel a better name for argument would be no_pivot
or pivot_root
. with_pivot_root
feels like a path for pivoting a root to. wdyt?
Hey, sorry, I was busy with work and some other stuff and couldn't get to any of this. This is good to merge, and I have approved this. I will wait till tom if you have any replies to my comments, otherwise I'll merge this as-is, and you can address the nits in your next PR. |
thanks for your reply. I will continue working on the refactoring. Cheers |
Thank you! Please address the nitpicks in your next PR. |
fix the problem that no_pivot args is not used when create container when we prepare rootfs with chroot, we should move_mount the rootfs before chroot, otherwise we will not able to use the new rootfs when exec into the container