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

Fix TestContainerNoBinaryExists test, by making create behaviour similar to runc #1347

Merged
merged 3 commits into from
Nov 28, 2022

Conversation

YJDoc2
Copy link
Collaborator

@YJDoc2 YJDoc2 commented Nov 15, 2022

When creating, this will check if the binary to run as container process exists and if it has proper permissions in the init process.
If it doesn't, then generate an error in create.

Signed-off-by: Yashodhan Joshi yjdoc2@gmail.com

…has incorrect permissions

Signed-off-by: Yashodhan Joshi <yjdoc2@gmail.com>
@codecov-commenter
Copy link

Codecov Report

Merging #1347 (88e7430) into main (489e94d) will decrease coverage by 0.25%.
The diff coverage is 0.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1347      +/-   ##
==========================================
- Coverage   68.73%   68.48%   -0.26%     
==========================================
  Files         119      119              
  Lines       12601    12646      +45     
==========================================
- Hits         8661     8660       -1     
- Misses       3940     3986      +46     

@YJDoc2
Copy link
Collaborator Author

YJDoc2 commented Nov 15, 2022

@utam0k I validated it with runc, and that too fails the test. The problem behind it is that we are validating the binary to be run in the init process exists, but the tests does not setup the binary at all. If we want to follow runc, we can skip this test, or we can choose to ignore the test in containerd validation. Unfortunately we have to make a call here. 😢 😭

@utam0k
Copy link
Member

utam0k commented Nov 17, 2022

@YJDoc2 Thanks for the great investigation. I vote for skipping it

@utam0k
Copy link
Member

utam0k commented Nov 17, 2022

@kzys May I ask you to help us? Is make integration not used in containerd? It appears that there is a test that fails even with runc. Is there something we missed?
https://containers.github.io/youki/developer/containerd_integration_test_using_youki.html

Signed-off-by: Yashodhan Joshi <yjdoc2@gmail.com>
@kzys
Copy link

kzys commented Nov 17, 2022

Let me take a look this week. We are at least using make integration.

@utam0k
Copy link
Member

utam0k commented Nov 21, 2022

Let me take a look this week. We are at least using make integration.

🙏 Thanks.

@YJDoc2
Copy link
Collaborator Author

YJDoc2 commented Nov 22, 2022

@utam0k after checking runc repo, I don't think they are using oci-integration tests anywhere in their CI or makefile. they seem to be using their self-written tests for integration.

@utam0k
Copy link
Member

utam0k commented Nov 22, 2022

Let me take a look this week. We are at least using make integration.

@kzys Sorry, I thought we were talking about containerd, but we were talking about something else. I was mistaken🙇 Never mind.

@YJDoc2 YJDoc2 force-pushed the fix_create_no_bin_test branch 2 times, most recently from 7f17c27 to 08dda37 Compare November 28, 2022 05:58
Signed-off-by: Yashodhan Joshi <yjdoc2@gmail.com>
Copy link
Member

@utam0k utam0k left a comment

Choose a reason for hiding this comment

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

Thanks!

@utam0k utam0k merged commit 7c2c61e into youki-dev:main Nov 28, 2022
@YJDoc2 YJDoc2 deleted the fix_create_no_bin_test branch December 1, 2022 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants