test: use config file rather than environment variables for the kata CI job #8166
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
openshift-ci
bot
added
release-note-none
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: littlejawa The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold This PR contains a commit that we don't want to merge. |
openshift-ci
bot
added
the
do-not-merge/hold
littlejawa
force-pushed
the
kata_ci_config_simplified
branch
from
6739436
to
53fb9f7
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8166 +/- ##
=======================================
Coverage 49.57% 49.57%
=======================================
Files 153 153
Lines 16953 16953
=======================================
Hits 8404 8404
Misses 7503 7503
Partials 1046 1046 |
littlejawa
force-pushed
the
kata_ci_config_simplified
branch
2 times, most recently
from
c31b4c7
to
961cff2
Compare
|
/test ci-fedora-kata |
littlejawa
force-pushed
the
kata_ci_config_simplified
branch
from
961cff2
to
2768cae
Compare
|
/test ci-fedora-kata |
1 similar comment
|
/test ci-fedora-kata |
littlejawa
force-pushed
the
kata_ci_config_simplified
branch
4 times, most recently
from
83a3a8f
to
51e9edd
Compare
|
/test ci-fedora-kata |
littlejawa
force-pushed
the
kata_ci_config_simplified
branch
2 times, most recently
from
ad8303f
to
7728583
Compare
|
/test ci-fedora-kata |
littlejawa
force-pushed
the
kata_ci_config_simplified
branch
2 times, most recently
from
7d08d21
to
8dfec01
Compare
|
/test ci-fedora-kata |
1 similar comment
|
/test ci-fedora-kata |
|
/test ci-fedora-integration |
littlejawa
force-pushed
the
kata_ci_config_simplified
branch
4 times, most recently
from
0ac8a72
to
57ebd2b
Compare
|
/test ci-fedora-integration |
1 similar comment
|
/test ci-fedora-integration |
|
/test ci-fedora-kata |
2 similar comments
|
/test ci-fedora-kata |
|
/test ci-fedora-kata |
littlejawa
force-pushed
the
kata_ci_config_simplified
branch
from
57ebd2b
to
c43a02f
Compare
|
/test ci-fedora-integration |
|
/test ci-fedora-kata |
1 similar comment
|
/test ci-fedora-kata |
The way our tests are working, we don't always stop/delete containers before we shut down crio. That's ok for most tests because the container processes will go away with crio, but with kata, the runtime and (most importantly) the qemu process running the VM are still active. This is not an issue for the test themselves, as the multiple processes left over by the tests do not interact with each other. But the resource consumption on the test machine is growing during the execution, and could lead to missing memory after hundreds of tests. This commit ensures that we kill the runtime, which will properly shut down the VM. Signed-off-by: Julien Ropé <jrope@redhat.com>
Adding a test that verifies we're actually running the job with the kata runtime. This is meant to validate the CI job's config for kata while working on the test scripts themselves. Signed-off-by: Julien Ropé <jrope@redhat.com>
The RUNTIME_BINARY_PATH as it is set assumes that the runtime name is also the binary name for the runtime. That's not true for kata, where the runtime name "kata" is different than the binary name in most installations. We used to use the "containerd-shim-kata-v2" name for the runtime, so that the CI would work, but that's making a difference between how we test and how it's actually used by most users. This commit allows us to set RUNTIME_BINARY_PATH as needed, and keeps the default behavior for any other runtimes. Signed-off-by: Julien Ropé <jrope@redhat.com>
The helpers.bash function "create_runtime_with_allowed_annotation()" was missing a couple settings that the kata container runtime needs, resulting in an incomplete configuration for testing with kata containers. Signed-off-by: Julien Ropé <jrope@redhat.com>
This is a shared functionality for multiple tests: create a new runtime config, set it to default, and add specific parameters to it. Can be allowed annotation, min memory, etc. These tests are failing in the kata use case because they miss some of the required settings that the kata runtime needs. Their created runtime is just not properly set for kata containers. Based on the existing "create_runtime_with_allowed_annotation()", this commit creates a generic function to have a new runtime and set it as the default. The function returns the path to the created config file so that the caller can append any needed parameter to it. Subsequent commits will try to use that new function where needed. Signed-off-by: Julien Ropé <jrope@redhat.com>
These tests create a new runtime class with only the "runtime_binary" parameter set. Kata requires additional parameters that are ignored here, which is why they fail with it. This commit uses the newly added "create_new_runtime_class" in helpers.bash to make sure that all parameters are set. Signed-off-by: Julien Ropé <jrope@redhat.com>
Apply the function "create_new_default_runtime()" to cgroups.bats and ctr.bats Signed-off-by: Julien Ropé <jrope@redhat.com>
Signed-off-by: Julien Ropé <jrope@redhat.com>
Now that the helpers.bash function is fixed, try re-enabling those tests. Signed-off-by: Julien Ropé <jrope@redhat.com>
Signed-off-by: Julien Ropé <jrope@redhat.com>
Signed-off-by: Julien Ropé <jrope@redhat.com>
Signed-off-by: Julien Ropé <jrope@redhat.com>
littlejawa
force-pushed
the
kata_ci_config_simplified
branch
from
0fbb852
to
e02f2d7
Compare
|
/test ci-fedora-kata |
|
/test ci-fedora-integration |
|
/test ci-fedora-kata |
|
@littlejawa: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
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.
What type of PR is this?
/kind ci
What this PR does / why we need it:
There are a number of integration tests that rely on creating a new runtime class with specific parameters, and make it the default before running some tests.
Theses tests fail with kata, because the kata CI job setup uses environment variables (CONTAINER_DEFAULT_RUNTIME typically), which take precedence in crio's config.
This PR tries to modify the kata job's configuration so that it relies on config files too. This way, no interaction should happen, and we should be able to enable additional tests for kata.
Which issue(s) this PR fixes:
None
Special notes for your reviewer:
Does this PR introduce a user-facing change?