tests: pull-image: Only skip tests for TEEs

[fidencio]

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.

[stevenhorsman]

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?

[stevenhorsman]

Looking much clearer - Thank you!

[stevenhorsman]

Looking much clearer. Thanks for the updates!

[wainersm]

The code is much better now! Thanks @fidencio @stevenhorsman !

[stevenhorsman]

LGTM. Thanks!

[fidencio]

/test

[stevenhorsman]

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

 count of container rootfs in host is: 0, expect count is: 1

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 shared_fs=none we shouldn't rely on this any more, so I'm tempted to relax the check to ensure that the rootfs count is no higher than we expect, but I've rebased this to pick up the latest nydus-snapshotter version changes first, and will see if that makes a difference.

[fidencio]

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

 count of container rootfs in host is: 0, expect count is: 1

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 shared_fs=none we shouldn't rely on this any more, so I'm tempted to relax the check to ensure that the rootfs count is no higher than we expect, but I've rebased this to pick up the latest nydus-snapshotter version changes first, and will see if that makes a difference.

Let's do this, as this whole test will basically be dropped in the very short future.

[stevenhorsman]

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?:

[run_kubernetes_tests.sh:110] INFO: Executing k8s-confidential-attestation.bats
1..2
# bats warning: Executed 0 instead of expected 2 tests
[run_kubernetes_tests.sh:111] ERROR: bats --show-output-of-passing-tests k8s-confidential-attestation.bats

[wainersm]

Executed 0 instead of expected 2 tests

This kind of issue usually means unset variable being used. Let me look at the test's code.

[wainersm]

Executed 0 instead of expected 2 tests

This kind of issue usually means unset variable being used. Let me look at the test's code.

Ok, k8s-confidential-attestation.bats calls confidential_setup() that no longer exist. We should adjust all tests that are still using that function.

[stevenhorsman]

Executed 0 instead of expected 2 tests

This kind of issue usually means unset variable being used. Let me look at the test's code.

Ok, k8s-confidential-attestation.bats calls confidential_setup() that no longer exist. We should adjust all tests that are still using that function.

Thanks for the made bats knowledge! I'll work on that tomorrow morning.

[stevenhorsman]

/test

[fidencio]

/test