Libvirt: Add CVM Nested Virtualization feature #3519
Conversation
This feature is required to enable verify_nested_cvm_attestation_report test on libvirt platform.
| node_capabilities.features = search_space.SetSpace[schema.FeatureSettings]( | ||
| is_allow_set=True, | ||
| items=[ | ||
| cvm_nested_virtualization, |
There was a problem hiding this comment.
Can we add this feature only when the guest is a CVM guest?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
- 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.
- 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.
There was a problem hiding this comment.
Thanks for clarifying! In that case, we don't need this PR. The testcase requirement is incorrect. We will fix that.
| class CVMNestedVirtualization(features.CVMNestedVirtualization): | ||
| @classmethod | ||
| def create_setting(cls, *args: Any, **kwargs: Any) -> schema.FeatureSettings: | ||
| return schema.FeatureSettings.create(cls.name()) |
There was a problem hiding this comment.
Instead of returning the settings right away, can we have a check on host_node if it supports nested cvm ?
There was a problem hiding this comment.
We will have to pass the host node sku from runbook in that case.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ref:
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
For returning
Nonefor the case when CVM is not supported, we will have to set the return type as Optional[schema.FeatureSettings]. Whereasnode_capabilities.featuresexpects value of typeschema.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.
This feature is required to enable verify_nested_cvm_attestation_report test on libvirt platform.