Skip to content
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

Add process test #2968

Merged
merged 23 commits into from
Nov 25, 2024
Merged

Add process test #2968

merged 23 commits into from
Nov 25, 2024

Conversation

sat0ken
Copy link
Contributor

@sat0ken sat0ken commented Oct 30, 2024

This implements the process validation in #361.

Signed-off-by: sat0ken <[email protected]>
Signed-off-by: sat0ken <[email protected]>
Signed-off-by: sat0ken <[email protected]>
Signed-off-by: sat0ken <[email protected]>
@sat0ken
Copy link
Contributor Author

sat0ken commented Nov 1, 2024

@utam0k @YJDoc2

I figure out why the test failed.

I reproduced it on my PC.
If /test does not exist in the rootfs, the youki execution will fail same as the E2E tests.

root@satoken:~/work/rootfs# ls
bin  boot  config.json  dev  etc  home  lib  lib64  media  mnt  opt  proc  root  run  sbin  srv  sys  tmp  usr  var
root@satoken:~/work/rootfs# youki run test
ERROR libcontainer::process::container_init_process: failed to chdir e=ENOENT
ERROR libcontainer::process::container_intermediate_process: failed to initialize container process: failed to unix syscall
ERROR libcontainer::process::container_main_process: failed to wait for init ready: exec process failed with error error in executing process : failed to unix syscall
ERROR libcontainer::container::builder_impl: failed to run container process exec process failed with error error in executing process : failed to unix syscall
ERROR youki: error in executing command: exec process failed with error error in executing process : failed to unix syscall
run failed : exec process failed with error error in executing process : failed to unix syscall

But runc is, even if /test is not in rootfs, runc run is working.

root@satoken:~/work/rootfs# ls
bin  boot  config.json  dev  etc  home  lib  lib64  media  mnt  opt  proc  root  run  sbin  srv  sys  tmp  usr  var
root@satoken:~/work/rootfs# runc run test
/test

If we add a test directory to the rootfs you use for testing, the tests will pass.
However, to resolve the error, maybe I need to check youki's code that prepares the rootfs?

Sorry, I don't understand the detailed code of runc or youki, but I think it looks like runc is creating the cwd directory in this line.

What is the difference between the behavior of runc and youki?

@sat0ken
Copy link
Contributor Author

sat0ken commented Nov 4, 2024

@utam0k @YJDoc2 ping!
Please confirm.

#2968 (comment)

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Nov 4, 2024

Hey, thanks for the ping, I'll try to take a look at this today.

Copy link
Collaborator

@YJDoc2 YJDoc2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments, also there are several warnings from the CI check, please take a look

tests/contest/runtimetest/src/main.rs Outdated Show resolved Hide resolved
tests/contest/contest/src/tests/process/process_test.rs Outdated Show resolved Hide resolved
tests/contest/runtimetest/src/tests.rs Outdated Show resolved Hide resolved
tests/contest/runtimetest/src/tests.rs Outdated Show resolved Hide resolved
@YJDoc2
Copy link
Collaborator

YJDoc2 commented Nov 5, 2024

Hey @sat0ken , I took a look at the code and there are two things -

  1. In the test (as per my comment), you are not creating the /test dir correctly. Hence youki does not find it and fails.
  2. As for if to to follow or not runc's way of creating the cwd if not present, I think we should check and stick to the runtime spec, I'll check and get back to you. But if the failing test is fixed by correctly creating the dir, then maybe we should not copy this.

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Nov 5, 2024

Also @sat0ken a small request, please ping/comment in the tracking issue before opening a e2e test PR, so we can properly keep track. I have added your current PRs, but please comment for future ones. Thanks!

@sat0ken
Copy link
Contributor Author

sat0ken commented Nov 6, 2024

@YJDoc2
Thank you for your comments and confirmation. I will fix the code.
I am newbie to contribution, so I apologize for not following the rules. Thank you for your continued support!

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Nov 13, 2024

Hey @sat0ken , There are some conflicts and I have left a comment above on the still failing tests, please take a look.

@sat0ken
Copy link
Contributor Author

sat0ken commented Nov 13, 2024

@YJDoc2 Hi, thank you for comment and support. I fixed conflict and code. Could you please check?

@sat0ken
Copy link
Contributor Author

sat0ken commented Nov 14, 2024

@YJDoc2 Ping!

Copy link
Collaborator

@YJDoc2 YJDoc2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some nitpicks, but overall looks good.

tests/contest/contest/src/tests/process/process_test.rs Outdated Show resolved Hide resolved
tests/contest/runtimetest/src/tests.rs Outdated Show resolved Hide resolved
tests/contest/runtimetest/src/tests.rs Outdated Show resolved Hide resolved
tests/contest/runtimetest/src/tests.rs Outdated Show resolved Hide resolved
@sat0ken
Copy link
Contributor Author

sat0ken commented Nov 24, 2024

@YJDoc2 Thank you for review and comments. I've fixed the code. Could you please check the code?

Copy link
Collaborator

@YJDoc2 YJDoc2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm 👍 Thanks

@YJDoc2 YJDoc2 merged commit 3b78801 into youki-dev:main Nov 25, 2024
27 checks passed
@github-actions github-actions bot mentioned this pull request Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants