tests/int: cleanups #2757
Merged
tests/int: cleanups #2757
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kolyshkin
force-pushed
the
test-int-cleanups
branch
2 times, most recently
from
747d49c to
1dd1051
Compare
|
OK I guess I reached the point where I need this one and #2741 to move forward (to enable parallel bats tests execution). |
kolyshkin
force-pushed
the
test-int-cleanups
branch
from
1dd1051 to
ad2955e
Compare
kolyshkin
commented
Jan 22, 2021
kolyshkin
force-pushed
the
test-int-cleanups
branch
from
ad2955e to
aaa8480
Compare
Maybe we should error out if |
Excellent point! Added a check. |
kolyshkin
force-pushed
the
test-int-cleanups
branch
from
aaa8480 to
524c760
Compare
AkihiroSuda
previously approved these changes
Feb 5, 2021
|
Needs rebase. |
All the tests are run with cd to bundle directory, so it does not make ense to explicitly specify it. This relies on the feature of functions like set_cgroups_path or update_config to use current directory by default. In those cases where we need to change the directory (runc create/run -b), save the current one and use it later. Signed-off-by: Kir Kolyshkin <[email protected]>
Since commit abd0515 it is no longed used. Signed-off-by: Kir Kolyshkin <[email protected]>
It is only used in a couple of places and is easy to replace with ROOT=xxx teardown_running_container. Signed-off-by: Kir Kolyshkin <[email protected]>
All tests are run with cd to bundle directory, so let's just use $(pwd). Signed-off-by: Kir Kolyshkin <[email protected]>
... by using retry, to avoid code duplication. While at it, make it check the number of arguments and error out on invalid usage. Signed-off-by: Kir Kolyshkin <[email protected]>
... instead of more generic "retry", to simplify code. Signed-off-by: Kir Kolyshkin <[email protected]>
Signed-off-by: Kir Kolyshkin <[email protected]>
Overloading $HELLO_BUNDLE with a second purpose of having an alternative runc root is confusing. Instead, let's create a temp directory in setup() and clean it up in teardown(). Signed-off-by: Kir Kolyshkin <[email protected]>
It's a little known fact, but BATS_TMPDIR is just /tmp, and it is not being cleaned by bats after the test. BATS_RUN_TMPDIR (available since bats 1.2.1) is $BATS_TMPDIR/bats-run-$$ (i.e. it is per bats run and includes PID), and it is cleaned up after the test (unless --no-tmpdir-cleanup is given). Use BATS_RUN_TMPDIR for better isolation and cleanup. [v2: check for BATS_RUN_TMPDIR, effectively requiring bats >= 1.2.1] Signed-off-by: Kir Kolyshkin <[email protected]>
Use the value of $(pwd) as we cd to bundle dir before running a test. Signed-off-by: Kir Kolyshkin <[email protected]>
1. Do not depend on $DEBIAN_BUNDLE, instead use the current directory. 2. Simplify setup() by calling teardown(). 3. In teardown(), check that LIBPATH is set. 4. Move global variables into setup. Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin
force-pushed
the
test-int-cleanups
branch
from
524c760 to
21bf204
Compare
1. Get rid of fixed ROOT, *_BUNDLE, and CONSOLE_SOCKET dirs. Now they are temporary directories created in setup_bundle. 2. Automate containers cleanup: instead of having to specify all containers to be removed, list and destroy everything (which is now possible since every test case has its own unique root). 3. Randomize cgroup paths so two tests running in parallel won't use the same cgroup. Now it's theoretically possible to execute tests in parallel. Practically it's not possible yet because bats uses GNU parallel, which do not provide a terminal for whatever it executes, and many runc tests (all those that run containers with terminal: true) needs a tty. This may possibly be addressed later. Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin
force-pushed
the
test-int-cleanups
branch
from
21bf204 to
41670e2
Compare
|
Rebased, improved last commit description. I think this one is ready to go in. |
AkihiroSuda
approved these changes
Feb 12, 2021
mrunalp
approved these changes
Feb 23, 2021
|
@opencontainers/runc-maintainers can someone please merge this one (I can't merge my own PRs) |
|
GitHub refusing clicking the merge button: Probably needs rebase to retrigger Go 1.16.x tests. |
|
Wow, apparently I can merge my own PR if someone closes and reopens it 🤡 |
|
Is it a vulnerability? 😄 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The ultimate idea is to parallelize running the integration tests.
So far just some preliminary cleanups, mostly resulting in simpler setup/cleanup
and laying the ground for parallel tests (every test is run in its own set of dirs and cgroups).