libvirt: Enable multiple PodVM image scenario #2061
Conversation
|
I think this need to be draft until #2036 which bumps us to the 3.9.0 version of the kata runtime is merged? |
ajaypvictor
force-pushed
the
multiple-podvm-images
branch
from
daa9fea
to
d1df0da
Compare
|
I missed the kata-containers PR, do we expect other providers to use the same annotations for their podvm images? e.g. in AWS, will |
Hi Snir, |
| @@ -79,6 +79,11 @@ func (p *libvirtProvider) CreateInstance(ctx context.Context, podName, sandboxID | |||
| } | |||
| logger.Printf("LaunchSecurityType: %s", vm.launchSecurityType.String()) | |||
|
|
|||
| if spec.Image != "" { | |||
| logger.Printf("Choosing %s as libvirt volume for the PodVM image", spec.Image) | |||
| p.libvirtClient.volName = spec.Image | |||
There was a problem hiding this comment.
Hi Ajay, I'd like to understand better how this change is propagated through if needed. My (very limited) understanding is that we set this value initially when we create the libvirt provider client, so it overriding that here sufficient to pick up this custom specified image, or is there any other code we need to skip to stop the previous version being used?
There was a problem hiding this comment.
Hi Steve,
Exactly, that's what my understanding also is..
With the enhancement of CreateVM() function to support getting the image from GetImageFromAnnotation(), we are able to retrieve the same on CreateInstance() and then furthermore override the default Volume Name which is set initially once the CAA comes up for the libvirt provider as you pointed.
I guess with the CreateDomain() flow starting post overriding the volume and initially the volume being picked up from the LIBVIRT_VOL_NAME which is being assigned to libvirtcfg.VolName, we might not have anywhere else to update?
Kindly suggest as I might be missing something here.
There was a problem hiding this comment.
Thanks, there isn't anything I know that is missing, I just wanted to check my understanding.
I'm assuming that you've tested this locally. Is there a way to integrate this into our e2e tests for libvirt?
There was a problem hiding this comment.
Thanks Steve..
I have posted the test results (CAA logs) for these changes on the PR description.
I will start exploring the options of adding e2e tests for this..
ajaypvictor
force-pushed
the
multiple-podvm-images
branch
2 times, most recently
from
077b833
to
4f00cce
Compare
|
e2e test results: |
ajaypvictor
force-pushed
the
multiple-podvm-images
branch
from
4f00cce
to
b18e30a
Compare
| @@ -406,6 +406,32 @@ func DoTestPodVMwithAnnotationsLargerCPU(t *testing.T, e env.Environment, assert | |||
| NewTestCase(t, e, "PodVMwithAnnotationsLargerCPU", assert, "Failed to Create PodVM with Annotations Larger CPU").WithPod(pod).WithInstanceTypes(testInstanceTypes).WithCustomPodState(v1.PodPending).Run() | |||
| } | |||
|
|
|||
| func DoTestCreatePeerPodContainerWithAlternateImage(t *testing.T, e env.Environment, assert CloudAssert) { | |||
There was a problem hiding this comment.
I think the negative test is a good start to check that the code path works. I'm wondering if we could use the provisioner.UploadPodvm logic to upload another podvmImage if a test image is provided (I guess the same image with a different name might be the best we can hope for) and then switch the annotation to try using that. I guess the problem then becomes how we check that the alternate podvm is being used might be tricky?
There was a problem hiding this comment.
yeah, that's true.. but if checking the CAA logs and ensuring that the PodVM picked up is the alternate one as per the annotation sounds good, then I guess we can try that option?
There was a problem hiding this comment.
Yeah - the CAA logs would be a good place to check - I didn't think about that. I'd say it's probably worth spending a bit of time to investigate this approach at least then
Enable support for multiple PodVM images for libvirt provider with the merge of kata-containers/kata-containers#10274 Fixes confidential-containers#1282 Signed-off-by: Ajay Victor <[email protected]>
ajaypvictor
force-pushed
the
multiple-podvm-images
branch
from
b18e30a
to
f451cd9
Compare
|
Latest e2e test results: |
| var podlist v1.PodList | ||
| var podLogString string | ||
| expectedSuccessMessage := "Choosing " + tc.alternateImageName | ||
| if err := client.Resources("confidential-containers-system").List(ctx, &podlist); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| for _, pod := range podlist.Items { | ||
| if pod.Labels["app"] == "cloud-api-adaptor" { | ||
| podLogString, _ = GetPodLog(ctx, client, pod) | ||
| break | ||
| } | ||
| } | ||
| if strings.Contains(podLogString, expectedSuccessMessage) { | ||
| t.Logf("PodVM was brought up using the alternate PodVM image %s", tc.alternateImageName) | ||
| } else { | ||
| t.Logf("cloud-api-adaptor pod logs: %s", podLogString) | ||
| yamlData, err := yaml.Marshal(tc.pod.Status) | ||
| if err != nil { | ||
| log.Errorf("Error marshaling pod.Status to JSON: %s", err.Error()) | ||
| } else { | ||
| t.Logf("Current Pod State: %v", string(yamlData)) | ||
| } | ||
| t.Fatal("failed due to unknown reason") | ||
| } |
There was a problem hiding this comment.
Optional request: Could we move this section to assessment_helper.go as a new function? I'm trying to reduce how massive the assessment_runner has got with this strategy in some WIP commits I'm working on on the side.
| @@ -68,6 +69,9 @@ func NewLibvirtProvisioner(properties map[string]string) (pv.CloudProvisioner, e | |||
| vol_name = properties["libvirt_vol_name"] | |||
| } | |||
|
|
|||
| // alternate_vol_name is used for multi-podvm testing. | |||
| alternate_vol_name := "another-podvm-base.qcow2" | |||
There was a problem hiding this comment.
Is there a possibility to extract this as a constant, so we can reference it in libvirt_test rather than having to copy the value there?
There was a problem hiding this comment.
Actually I tried to do this way, but unfortunately common.go inside the e2e folder is not extending to this provision_common.go, also thought about adding it to common.go under provisioner, which is again not referenced on libvirt_test/common_suite under e2e. if it sounds fair to import, i can try this out..
There was a problem hiding this comment.
Ah, I see. One approach we could take (which I'm not sure is even good) is create the const in libvirt/provision_common.go and then we can reference it in libvirt_test.go and pass it in as an argument to the common_test.go version? Otherwise I think we can leave it for now.
There was a problem hiding this comment.
Thanks Steve, this sounds like an idea, will test it out..
| @@ -135,6 +136,11 @@ func (tc *TestCase) WithInstanceTypes(testInstanceTypes InstanceValidatorFunctio | |||
| return tc | |||
| } | |||
|
|
|||
| func (tc *TestCase) WithMultiplePodVM(alternateImageName string) *TestCase { | |||
There was a problem hiding this comment.
I wonder if WithMultiplePodVM is the best name here? Maybe something like WithAlternateImage would be clearer?
ajaypvictor
force-pushed
the
multiple-podvm-images
branch
from
f451cd9
to
fde68ba
Compare
Negative test for multiple PodVM images on libvirt provider with a non existing volume Fixes confidential-containers#1282 Signed-off-by: Ajay Victor <[email protected]>
ajaypvictor
force-pushed
the
multiple-podvm-images
branch
from
fde68ba
to
5f5ef7e
Compare
|
@stevenhorsman Would it be fair to add |
Enable support for multiple PodVM images for libvirt provider with the merge of kata-containers/kata-containers#10274
Fixes #1282
Working Scenario:
Tried with the following busybox manifest:
Here the
pppvm-nov-vol.qcow2is a valid libvirt volume and a podvm image is uploaded there.CAA logs:
Failure Scenario:
Tried with the following busybox manifest:
Here the
rhel9-osis a dummy variable and not an actual libvirt volume.CAA logs: