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

ci: k8s: Fix checks used to skip confidential tests #9108

Conversation

fidencio
Copy link
Member

This has been introduced by 53bc4a4, where the condition was changed.

The correct condition is:

  • If the list of supported tees does not contain the kata hypervisor and the list of supported non tees does not contain the kata hypervisor.

The error is that we were checking whether kata-hypervisor would contain the list of supported tees, and that would almost always be false (unless in the case where the list had an one and only one element).

Fixes: #9055 -- part II

This has been introduced by 53bc4a4,
where the condition was changed.

The correct condition is:
* If the list of supported tees does not contain the kata hypervisor
  and the list of supported non tees does not contain the kata
  hypervisor.

The error is that we were checking whether kata-hypervisor would contain
the list of supported tees, and that would almost always be false
(unless in the case where the list had an one and only one element).

Fixes: kata-containers#9055 -- part II

Signed-off-by: Fabiano Fidêncio <fabiano.fidencio@intel.com>
@katacontainersbot katacontainersbot added the size/tiny Smallest and simplest task label Feb 18, 2024
@fidencio
Copy link
Member Author

/test

Copy link
Member

@ryansavino ryansavino left a comment

Choose a reason for hiding this comment

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

Approving, thanks @fidencio. All required checks and tests passing.

I spent some time trying to debug the snp failure. I will continue to investigate this, but may be a good idea to get this in so that the confidential test runs for now.

@fidencio
Copy link
Member Author

I spent some time trying to debug the snp failure. I will continue to investigate this, but may be a good idea to get this in so that the confidential test runs for now.

My understanding is that SNP shouldn't work, as the patch bringing in gmem support was not merged yet. Is this assumption valid?

Copy link
Member

@stevenhorsman stevenhorsman left a comment

Choose a reason for hiding this comment

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

My shell script interpretation logic hasn't booted yet, so I copied the if logic into a little script and tried it for all the hypervisors (and some random ones) and they all worked as I expected:

# ./test.sh qemu
KATA_HYPERVISOR: qemu
Setup proceeded
Non-tee - Need to apply image annotations
Confidential Launch Test not supported for qemu.
# ./test.sh qemu-sev
KATA_HYPERVISOR: qemu-sev
Setup proceeded
# ./test.sh qemu-se
KATA_HYPERVISOR: qemu-se
Setup proceeded
# ./test.sh qemu-snp
KATA_HYPERVISOR: qemu-snp
Setup proceeded
# ./test.sh qemu-tdx
KATA_HYPERVISOR: qemu-tdx
Setup proceeded
# ./test.sh qemu-fc
KATA_HYPERVISOR: qemu-fc
Test not supported for qemu-fc.
# ./test.sh qemu-junk
KATA_HYPERVISOR: qemu-junk
Test not supported for qemu-junk.

Thanks for the fix!

@fidencio fidencio merged commit 79dc6e9 into kata-containers:main Feb 19, 2024
293 of 309 checks passed
@ryansavino
Copy link
Member

I spent some time trying to debug the snp failure. I will continue to investigate this, but may be a good idea to get this in so that the confidential test runs for now.

My understanding is that SNP shouldn't work, as the patch bringing in gmem support was not merged yet. Is this assumption valid?

As the node kernel version has been updated, yes it shouldn't work. However, the job seemed to be failing on an earlier test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test size/tiny Smallest and simplest task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add the ability to run (some of the) confidential tests in non-TEE environments
4 participants