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

libcontainer: support set stdios for container #2961

Open abel-von opened 5 days ago

abel-von commented 5 days ago

currently the container can only inherit stdios from it's parent process, the one who create container with libcontainer can not set stdios to a different file. this restrict the use of the libcontainer that a process can only create one container by itself, because we can't imagine that every container this process creates share the same stdin/stdout/stderr.

YJDoc2 commented 5 days ago

May I ask you to take a look at discussion done in https://github.com/containers/youki/pull/2749 , as it was opened with similar intentions.

abel-von commented 4 days ago

@YJDoc2 Sorry, I also noticed that there is already a related PR after I submit this PR, but it seems it is a breaking change there, but in this PR, we just add three with_std* functions in the builder, it seems no breaking change here.

YJDoc2 commented 2 days ago

Hey @abel-von , before checking further, couple of comments -

  1. CI is failing, there are some typos and some doc comment code is not running properly , can you take a look and fix?
  2. Would it be possible to mark the builder method for setting these fds as unsafe ? Seeing your reasoning for using RawFds, maybe it might be better to set these methods to unsafe if possible, and make sure user knows what they're doing, would that make sense?
abel-von commented 2 days ago

Hi @YJDoc2, The CI failure is fixed, could you please trigger the CI again. Thank you.

YJDoc2 commented 2 days ago

Hey, I think the tests are passing, but the lint and fmt CI is failing again. We use the just commands in CI to validate, so you can also run them locally and check before pushing.

Also I'm wondering, would it be possible to have e2e or unit tests for this feature?

abel-von commented 1 day ago

Hi @YJDoc2, just lint and just test-all has been run and codes are force pushed again, CI must be ok now. I tried to add a e2e test but it seems there is no e2e tests for libcontainer only, I can only add a unit test in builder.rs to make sure the with_stdin/with_stdout/with_stderr is correctly set.

YJDoc2 commented 1 day ago

I tried to add a e2e test but it seems there is no e2e tests for libcontainer only, I can only add a unit test in builder.rs to make sure the with_stdin/with_stdout/with_stderr is correctly set.

Ok, I'll think on this. Btw, I have run the CI again, but there are conflicts with main branch due to another PR that was merged. Can you take a look and resolve them? Thanks.

abel-von commented 1 day ago

@YJDoc2 confliction is resolved. some of my just command can not succeed because my own environment. please check again. Thanks