search

youki-dev / youki

A container runtime written in Rust
https://youki-dev.github.io/youki/
Apache License 2.0
6.28k stars 344 forks source link

fix(libcontainer) no_pivot args is not used #2923

Closed xujihui1985 closed 1 day ago

xujihui1985 commented 1 month ago

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

xujihui1985 commented 1 month ago

@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.

udzura commented 1 month ago

I understand what this PR aim to do 👍 But I have no privilege to merge this. I guess you've mistaken me for someone!

xujihui1985 commented 1 month ago

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.

utam0k commented 1 month ago

I'll check this PR. Thanks for your first contribution!

utam0k commented 1 month ago

@xujihui1985 May I ask you to add an integration test for this PR? https://containers.github.io/youki/developer/e2e/rust_oci_test.html

xujihui1985 commented 1 month ago

@xujihui1985 May I ask you to add an integration test for this PR?

https://containers.github.io/youki/developer/e2e/rust_oci_test.html

sure, I'd like to learn how to add integrate test

utam0k commented 1 month ago

sure, I'd like to learn how to add integrate test

I can help you if you need ;)

YJDoc2 commented 1 month ago

Hey @xujihui1985 Thanks for your contribution! May I ask you to also take a look at https://github.com/containers/youki/pull/2597 , which also deals with no_pivot and the create and run commands. Can you check if you need to pick up any changes from there to here? Thanks.

xujihui1985 commented 1 month ago

Hey @xujihui1985 Thanks for your contribution! May I ask you to also take a look at #2597 , which also deals with no_pivot and the create and run commands. Can you check if you need to pick up any changes from there to here? Thanks.

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.

xujihui1985 commented 1 month ago

@YJDoc2 I have pick the change from commit https://github.com/containers/youki/pull/2597, I will continue work on integration test

xujihui1985 commented 3 weeks ago

@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.

Gekko0114 commented 2 weeks ago

/cc @Gekko0114

xujihui1985 commented 2 weeks ago

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?

@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?

xujihui1985 commented 1 week ago

@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

xujihui1985 commented 1 week ago

@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

@YJDoc2 Hi, would you like to proceed the PR or wait until the function been refactored?

YJDoc2 commented 1 week ago

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.

I mentioned test_outside_container as that is another test fn we use, but I hadn't thought this in detail. My main intention here was that to verify via e2e that basic commands such as start, delete, run etc work properly with the no_pivot, so whatever does the trick, works.

@YJDoc2 Hi, would you like to proceed the PR or wait until the function been refactored?

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.

xujihui1985 commented 2 days ago

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.

I mentioned test_outside_container as that is another test fn we use, but I hadn't thought this in detail. My main intention here was that to verify via e2e that basic commands such as start, delete, run etc work properly with the no_pivot, so whatever does the trick, works.

@YJDoc2 Hi, would you like to proceed the PR or wait until the function been refactored?

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

YJDoc2 commented 2 days ago

@YJDoc2 Hi, hope everything goes well with you, if there is anything I can improve for this PR, please let me know as

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.

xujihui1985 commented 1 day ago

@YJDoc2 Hi, hope everything goes well with you, if there is anything I can improve for this PR, please let me know as

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

YJDoc2 commented 1 day ago

I will continue working on the refactoring.

Thank you! Please address the nitpicks in your next PR.