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.

Sign up for GitHub

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

Jump to bottom

Libvirt: Add CVM Nested Virtualization feature #3519

Closed
wants to merge 1 commit into from
Closed

Libvirt: Add CVM Nested Virtualization feature #3519

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions lisa/sut_orchestrator/libvirt/features.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,3 +55,9 @@ def on_before_deployment(cls, *args: Any, **kwargs: Any) -> None:
node_context.guest_vm_type = cls._security_profile_mapping[
settings.security_profile
]


class CVMNestedVirtualization(features.CVMNestedVirtualization):
@classmethod
def create_setting(cls, *args: Any, **kwargs: Any) -> schema.FeatureSettings:
return schema.FeatureSettings.create(cls.name())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of returning the settings right away, can we have a check on host_node if it supports nested cvm ?

Copy link
Contributor Author

@gamora12 gamora12 Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We will have to pass the host node sku from runbook in that case.

Copy link
Contributor

Choose a reason for hiding this comment

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

can we have a check on host_node if it supports nested cvm ?

Is that something a feature is supposed to do? I would imagine the code that adds this feature to a node should do those checks (if at all).

Copy link
Collaborator

@pupacha pupacha Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

From this ref: https://github.com/microsoft/lisa/blob/main/lisa/sut_orchestrator/azure/features.py#L2746, I understand the return from this function would say if the actual feature (here in this case Nested CVM) is supported or not. Not all the host_node s support nested CVM.
We could also do it as you suggested, but the checks to support feature should be necessary.
A check like https://github.com/microsoft/lisa/blob/main/microsoft/testsuites/cvm/cvm_azure_host.py#L78 would suffice.

Copy link
Contributor Author

@gamora12 gamora12 Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ref:

lisa/lisa/sut_orchestrator/libvirt/platform.py

Line 353 in ceac54d

node_capabilities.features = search_space.SetSpace[schema.FeatureSettings](

We can add a check that would add the setting to node_capabilities.features when it's not NONE, but then we need a validation logic in the feature.

Copy link
Contributor Author

@gamora12 gamora12 Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Also, for checking whether the host node supports nested CVM or not, we already have verify_azure_vm_snp_enablement test as part of host functional test job, can't we use the existing testcase to validate? To add the validation in the feature, we will have to add the same logic here again.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For returning None for the case when CVM is not supported, we will have to set the return type as Optional[schema.FeatureSettings]. Whereas node_capabilities.features expects value of type schema.FeatureSettings.

You can see how Azure platform handles this and do something similar. I see Azure platform also returning Optional[schema.FeatureSettings].
ref: https://github.com/microsoft/lisa/blob/main/lisa/sut_orchestrator/azure/features.py#L2740

I'm fine with Anirudh's suggestion to have the check before adding the feature to supported list.

I believe we have a testcase- verify_azure_vm_snp_enablement as part of host functional test job, can't we use the existing testcase to validate?

Testcase and Features are different entities altogether. We cannot use testcase as check if a feature is supported or not.

5 changes: 4 additions & 1 deletion lisa/sut_orchestrator/libvirt/platform.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@
get_environment_context,
get_node_context,
)
from .features import SecurityProfile, SecurityProfileSettings
from .features import CVMNestedVirtualization, SecurityProfile, SecurityProfileSettings
from .platform_interface import IBaseLibvirtPlatform
from .schema import (
FIRMWARE_TYPE_BIOS,
Expand Down Expand Up @@ -93,6 +93,7 @@ class BaseLibvirtPlatform(Platform, IBaseLibvirtPlatform):
CONFIG_FILE_MARKER = "lisa-libvirt-platform"

_supported_features: List[Type[Feature]] = [
CVMNestedVirtualization,
SerialConsole,
StartStop,
SecurityProfile,
Expand Down Expand Up @@ -350,9 +351,11 @@ def _create_node_capabilities(
node_capabilities.network_interface.nic_count = 1
node_capabilities.gpu_count = 0
security_profile_setting = SecurityProfileSettings()
cvm_nested_virtualization = CVMNestedVirtualization.create_setting()
node_capabilities.features = search_space.SetSpace[schema.FeatureSettings](
is_allow_set=True,
items=[
cvm_nested_virtualization,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add this feature only when the guest is a CVM guest?

Copy link
Contributor Author

@gamora12 gamora12 Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The node is configured after this step, so I don't think a check can be added at this exact point to check guest vm type.

Copy link
Member

Choose a reason for hiding this comment

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

Can we add this feature only when the guest is a CVM guest?

What means "guest" here? If it means the vm image, it cannot be checked here. If it means the host supports CVM or not, it should be set here. If it's not easy to know the host support CVM or not, a switch can be added in schema to control from the runbook.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it means the vm image

Yeah, I mean the VM that is being created by this platform.

If it means the host supports CVM or not, it should be set here.

I guess I was a bit confused about what the feature CVMNestedVirtualization indicates. Based on the name I thought it is used to indicate if a node is a nested CVM.

But if it is used to indicate whether a host supports CVM or not, then it shouldn't be added as a requirement for the verify_nested_cvm_attestation_report test case in the first place. @smit-gardhariya ?

Copy link
Member

Choose a reason for hiding this comment

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

A feature means the capability of the platform, what kind of VM is capable to create. It doesn't consider if the image is CVM or not, but only consider if the host is capable to create a CVM.

Copy link
Contributor

@anirudhrb anirudhrb Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

A feature means the capability of the platform

Platform or node? At the end of the day, a feature is a property of a node right? We access it with node.features[FeatureName].

what kind of VM is capable to create

Libvirt platform is not capable of creating nodes that are in turn capable of creating CVMs.

Copy link
Member

Choose a reason for hiding this comment

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

  1. The feature is to abstract the operation of a platform on nodes. It goes with nodes, so you can use features on nodes in test cases.
  2. CVM comes with two side: 1) the image supports CVM, 2) the virtualization stack supports CVM. The feature is to tell the supportability of the virtualization stack, not the image. In LISA, "capability" means the platform is capable or not, "requirement" means the test case needs something.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for clarifying! In that case, we don't need this PR. The testcase requirement is incorrect. We will fix that.

schema.FeatureSettings.create(SerialConsole.name()),
security_profile_setting,
],
Expand Down
Loading