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 integration test utils necessary for implementing rest integration tests #310

Merged
merged 5 commits into from
Sep 20, 2021

Conversation

YJDoc2
Copy link
Collaborator

@YJDoc2 YJDoc2 commented Sep 19, 2021

ref #282
Part 1 for closing the issue.

This changes hard-coded youki to more generic runtime
This also Sets up utils required for integration tests, which will be used for implementing rest of integration tests. Currently these function are not used, so are marked #[allow(dead_code)] so clippy doesn't flag them, this will be removed later.
This also switched using local directory in youki folder for setting up workspace to using temporary folders, taken after cgroups/temp_dir. This will prevent possible clashes between tests, and possibly allow running tests in parallel in future.

This is a bit big PR 😅 , but many of changes are not in implementation, but to accommodate the temp_dir.

Sleeping is necessary only when running all lifecycle tests,
as otherwise container is still running when we try to delete, and delete test fails.
But keeping sleep in kill function slows all other test where we do not try deleting immidiately afterwards.
Thus moved the sleep in the kill test itself, so only happend when testing lifecycle functions.
@codecov-commenter
Copy link

Codecov Report

Merging #310 (afd546b) into main (4392293) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #310      +/-   ##
==========================================
+ Coverage   77.17%   77.19%   +0.01%     
==========================================
  Files          43       43              
  Lines        6139     6139              
==========================================
+ Hits         4738     4739       +1     
+ Misses       1401     1400       -1     

@YJDoc2 YJDoc2 requested a review from utam0k September 19, 2021 15:48
@YJDoc2
Copy link
Collaborator Author

YJDoc2 commented Sep 19, 2021

@utam0k Please take a look.

// By experimenting, somewhere around 50 is enough for youki process
// to get the kill signal and shut down
// here we add a little buffer time as well
const SLEEP_TIME: u64 = 75;
Copy link
Member

Choose a reason for hiding this comment

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

How about using std::time::Duration insted ot u64.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A duration is created later from this u64, and then given to sleep because I thought we could not invoke functions for defining consts, but I checked ans we can use duration::from_millis directly in constant definition. I will change that along with other required changes in additional commit 👍

@YJDoc2
Copy link
Collaborator Author

YJDoc2 commented Sep 20, 2021

Changed the type of SLEEP_TIME from u64->Duration

@utam0k
Copy link
Member

utam0k commented Sep 20, 2021

@YJDoc2 How about searching for runtime in the $PATH variable as well?

@YJDoc2
Copy link
Collaborator Author

YJDoc2 commented Sep 20, 2021

I personally feel it is better to accept the path as a argument than env var, as with env var, we will need make sure it is set, and when testing multiple runtime, the variable will need to be updated ,thinking from this integration test can be their own standalone program used to test multiple runtimes.

Personally I feel taking the path as an argument is more self contained and preferable than an env var.

@utam0k
Copy link
Member

utam0k commented Sep 20, 2021

I personally feel it is better to accept the path as a argument than env var, as with env var, we will need make sure it is set, and when testing multiple runtime, the variable will need to be updated ,thinking from this integration test can be their own standalone program used to test multiple runtimes.

Personally I feel taking the path as an argument is more self contained and preferable than an env var.

The PATH variable refers to a commonly used environment variable. In other words, I thought it would be more convenient to run the program when just passing runc instead of the full path, but what do you think?
I think we need to pass /usr/bin/runc now.

@YJDoc2
Copy link
Collaborator Author

YJDoc2 commented Sep 20, 2021

Oh, extremely sorry, I misunderstood your comment 😓 😓
Yes currently we need to pass the full path right now, and indeed it would be much more convenient to search in $PATH
I will make the appropriate changes. Sorry for the misunderstanding 😓

Now it is not required to give complete path of the runtime, if it exists in $PATH
previously needed to provide /usr/bin/run, now only runc resolves to it
@utam0k
Copy link
Member

utam0k commented Sep 20, 2021

Oh, extremely sorry, I misunderstood your comment
Yes currently we need to pass the full path right now, and indeed it would be much more convenient to search in $PATH
I will make the appropriate changes. Sorry for the misunderstanding

My explanation was also inadequate. Please don't worry about it.

@YJDoc2
Copy link
Collaborator Author

YJDoc2 commented Sep 20, 2021

@utam0k Now have added the path resolution using which crate which takes care of underlying platform to resolve the path.
Now

sudo youki_integration_test -r runc

also works as expected.

@utam0k utam0k merged commit 2af5216 into containers:main Sep 20, 2021
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.

None yet

3 participants