-
Notifications
You must be signed in to change notification settings - Fork 996
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
tests: pull-image: Only skip tests for TEEs #9613
tests: pull-image: Only skip tests for TEEs #9613
Conversation
41e31f9
to
c2615cd
Compare
@@ -9,7 +9,7 @@ load "${BATS_TEST_DIRNAME}/lib.sh" | |||
load "${BATS_TEST_DIRNAME}/confidential_common.sh" | |||
|
|||
setup() { | |||
confidential_setup && skip "Due to issues related to pull-image integration skip tests for ${KATA_HYPERVISOR}." | |||
confidential_setup_non_tees || skip "Due to issues related to pull-image integration skip tests for ${KATA_HYPERVISOR}." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is maybe slightly outside of the scope of this PR, but it feels like the confidential_setup_non_tees
/ confidential_setup
logic is pretty confusing with various negation and requiring ||
vs. &&
. This lead to the original mistake that at least three people overlooked.
This might be controversial, but I wonder if we would have better clarity (and avoid the issue we had here) with some renaming and separation and a bit more code. So this code would look something like:
if [ ! is_confidential_runtime_class ] then;
skip "Test not supported for ${KATA_HYPERVISOR}."
fi
if [ is_confidential_hardware ]; then
skip "Due to issues related to pull-image integration skip tests for ${KATA_HYPERVISOR}."
fi
It isn't as succinct coding wise, but I'd argue that the best code is the easier to understand rather than the shortest.
We could create a confidential_common function that wraps this and was called something like skip_for_non_confidential_and_tee_hardware
that would add clarity as well, to avoid the duplication of this code in the bats files?
What do others thinks?
c2615cd
to
56d7aad
Compare
Looking much clearer - Thank you! |
56d7aad
to
16a22b5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking much clearer. Thanks for the updates!
16a22b5
to
be25683
Compare
cb5ef4f
to
a74c399
Compare
a74c399
to
3dd6057
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is much better now! Thanks @fidencio @stevenhorsman !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
/test |
3dd6057
to
9d8bc54
Compare
We are getting a failure here where the pause image rootfs which we still expect on the host isn't found in the standard place
We have seen this before in kata-containers/tests#5781, but we thought that was due to stale processes on bare-metal machines. With the impending move to |
b625265
to
dfbc2e7
Compare
Let's do this, as this whole test will basically be dropped in the very short future. |
Already done! @fidencio @wainersm - I also took the opportunity to un skip the tests that check for runc then guest-pull and vice versa. They pass, but we're hitting failures in the attestation tests, but the output isn't useful. Any ideas?:
|
This kind of issue usually means unset variable being used. Let me look at the test's code. |
Ok, k8s-confidential-attestation.bats calls |
Thanks for the made bats knowledge! I'll work on that tomorrow morning. |
dfbc2e7
to
3451411
Compare
Let's rename it to `is_confidential_runtime_class`, and adapt all the places where it's called. The new name provides a better description, leading to a better understanding of what the function really does. Signed-off-by: Fabiano Fidêncio <fabiano.fidencio@intel.com>
This function is a helper to check whether the KATA_HYPERVISOR being used is a confidential hardware (TEE) or not, and we can use it to skip or only run tests on those platforms when needed. Signed-off-by: Fabiano Fidêncio <fabiano.fidencio@intel.com>
On 1423420, I've mistakenly disabled the tests entirely, for both non-TEEs and TEEs. This happened as I didn't realise that `confidential_setup` would take non-TEEs into consideration. :-/ Now, let me follow-up on that and make sure that the tests will be running on non-TEEs. Signed-off-by: Fabiano Fidêncio <fabiano.fidencio@intel.com>
Running those with the non-TEE runtime classes will simply fail. Signed-off-by: Fabiano Fidêncio <fabiano.fidencio@intel.com>
This test is called from `tests/integration/run_kuberentes_tests.sh`, which already ensures that yq is installed. Signed-off-by: Fabiano Fidêncio <fabiano.fidencio@intel.com>
- We previously have an expectation for the pause rootfs to be pull on the host when we did a guest pull. We weren't really clear why, but it is plausible related to the issues we had with containerd and nydus caching. Now that is fixed we can begin to address this with setting shared_fs=none, but let's start with updating the rootfs host check to be not higher than expected Signed-off-by: stevenhorsman <steven@uk.ibm.com>
Given that we think the containerd -> snapshotter image cache problems have been resolved by bumping to nydus-snapshotter v0.3.13 we can try removing the skips to test this out Signed-off-by: stevenhorsman <steven@uk.ibm.com>
3451411
to
a92defd
Compare
/test |
This is done in order to work around Azure/azure-cli#28984, following a suggestion on the very same issue. Signed-off-by: Fabiano Fidêncio <fabiano.fidencio@intel.com>
/test |
On 1423420, I've mistakenly disabled the tests entirely, for both non-TEEs and TEEs.
This happened as I didn't realise that
confidential_setup
would take non-TEEs into consideration. :-/Now, let me follow-up on that and make sure that the tests will be running on non-TEEs.