Create libvirt pool and volume on KVM host & libvirt config map update #499

Saripalli-lavanya · 2025-01-22T06:53:00Z

Description of the problem which is fixed/What is the use case
Added script to Automatically create libvirt pool and volume on KVM host - Enhance the usability of the Libvirt provider for confidential container administrators #435

Adding some test log results :

Checking existence of libvirt pool 'sl-pool-1' and volume 'sl-volume-1'...
virsh -d 0 -c 'qemu+ssh://[email protected]/system?no_verify=1' pool-info 'sl-pool-1'
pool-info: pool(optdata): sl-pool-1
pool-info: found option <pool>: sl-pool-1
pool-info: <pool> trying as pool NAME
error: failed to get pool 'sl-pool-1'
error: Storage pool not found: no storage pool with matching name 'sl-pool-1'
vol-info: pool(optdata): sl-pool-1
vol-info: vol(optdata): sl-volume-1
vol-info: found option <pool>: sl-pool-1
vol-info: <pool> trying as pool NAME
error: failed to get pool 'sl-pool-1'
error: Storage pool not found: no storage pool with matching name 'sl-pool-1'
Libvirt pool 'sl-pool-1' or volume 'sl-volume-1' does not exist. Proceeding to create...
pool-define-as: name(optdata): sl-pool-1
pool-define-as: type(optdata): dir
pool-define-as: target(optdata): /var/lib/libvirt/images
Pool sl-pool-1 defined
pool-start: pool(optdata): sl-pool-1
pool-start: found option <pool>: sl-pool-1
pool-start: <pool> trying as pool NAME
Pool sl-pool-1 started
vol-create-as: pool(optdata): sl-pool-1
vol-create-as: name(optdata): sl-volume-1
vol-create-as: capacity(optdata): 20G
vol-create-as: allocation(optdata): 2G
vol-create-as: prealloc-metadata(bool)
vol-create-as: format(optdata): qcow2
vol-create-as: found option <pool>: sl-pool-1
vol-create-as: <pool> trying as pool NAME
Vol sl-volume-1 created
Created libvirt pool and volume successfully.
Creating qcow2 image for libvirt provider from scratch

If pool and volume already exsist:

Checking existence of libvirt pool 'sl-pool-1' and volume 'sl-volume-1'...
virsh -d 0 -c 'qemu+ssh://[email protected]/system?no_verify=1' pool-info 'sl-pool-1'
pool-info: pool(optdata): sl-pool-1
pool-info: found option <pool>: sl-pool-1
pool-info: <pool> trying as pool NAME
Name:      sl-pool-1
UUID:      d9dc84d3-9a18-4833-8d66-95c2dc26aa9b
State:     running
Pool persistent flag value: 1
Persistent:   yes
Autostart:   no
Capacity:    2.00 TiB
Allocation:   1.32 TiB
Available:   699.47 GiB
vol-info: pool(optdata): sl-pool-1
vol-info: vol(optdata): sl-volume-1
vol-info: found option <pool>: sl-pool-1
vol-info: <pool> trying as pool NAME
vol-info: found option <vol>: sl-volume-1
vol-info: <vol> trying as vol name
Name:      sl-volume-1
Type:      file
Capacity:    100.00 GiB
Allocation:   1.13 GiB
Disclaimer: A Libvirt pool named 'sl-pool-1' with volume 'sl-volume-1' already exists on the KVM host. Image will be uploaded to same volume.
Creating qcow2 image for libvirt provider from scratch

Deleting test log result :

Deleting Libvirt image id
Deleting Libvirt image
vol-delete: pool(optdata): sl-pool-1
vol-delete: vol(optdata): sl-volume-1
vol-delete: found option <pool>: sl-pool-1
vol-delete: <pool> trying as pool NAME
vol-delete: found option <vol>: sl-volume-1
vol-delete: <vol> trying as vol name
Vol sl-volume-1 deleted
Volume 'sl-volume-1' deleted successfully.
Other volumes are present in the pool. Pool will not be deleted.
Deleted libvirt image successfully
Deleting libvirt volume from peer-pods-cm configmap
configmap/peer-pods-cm patched
libvirt image id deleted from peer-pods-cm configmap successfully
configmap/peer-pods-cm patched (no change)

openshift-ci · 2025-01-22T06:53:23Z

Hi @Saripalli-lavanya. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

bpradipt · 2025-02-05T11:14:26Z

config/peerpods/peerpodssecret.yaml

@@ -8,7 +8,6 @@ stringData:
  #CLOUD_PROVIDER: "libvirt"
  #LIBVIRT_URI: "qemu+ssh://[email protected]/system?no_verify=1"
  #LIBVIRT_NET: "default


The LIBVIRT_NET also can be moved to peer-pods-cm right ?

Thank you for the review @bpradipt, Moved LIBVIRT_NET also into peer-pods-cm. Also updated description with some test outputs.

config/peerpods/podvm/libvirt-podvm-image-handler.sh

Saripalli-lavanya · 2025-02-11T03:39:34Z

/retest

bpradipt · 2025-02-11T05:49:21Z

controllers/image_generator.go

 	// libvirt ConfigMap Keys
-	libvirtConfigMapKeys := []string{"CLOUD_PROVIDER"}
+	libvirtConfigMapKeys := []string{"CLOUD_PROVIDER", "LIBVIRT_POOL", "LIBVIRT_VOL_NAME", "LIBVIRT_DIR_NAME"}


/lgtm
nit: typo in commit message s/udpate/update

config/peerpods/podvm/libvirt-podvm-image-handler.sh

bpradipt · 2025-02-11T05:55:10Z

Looks like the prow bot takes lgtm for the whole PR and not just for individual commit. I need to remove it so that it accidentally doesn't get merged before all reviews are done

bpradipt

/lgtm

snir911

Hi, thanks, sorry for the late review
LGTM overall, I added a small logic suggestion, would be nice if considered IMHO :)

snir911 · 2025-02-12T08:32:44Z

config/peerpods/podvm/libvirt-podvm-image-handler.sh

+            || error_exit "Failed to start libvirt pool '${LIBVIRT_POOL}'."
+    fi
+
+    virsh -d 0 -c "${LIBVIRT_URI}" vol-create-as --pool "${LIBVIRT_POOL}" \


Is failing at this stage (as example, also the pool-start), means there will be a leftover pool which is not cleaned (and unusable) ?

Hi @snir911, Thank you for review & suggestions, have updated code to clean up pool upon volume creation failure.

snir911 · 2025-02-12T08:43:02Z

config/peerpods/podvm/libvirt-podvm-image-handler.sh

-    virsh -d 0 -c "${LIBVIRT_URI}" pool-destroy "${LIBVIRT_POOL}" ||
+    # Delete the libvirt volume
+    virsh -d 0 -c "${LIBVIRT_URI}" vol-delete --pool "${LIBVIRT_POOL}" "${LIBVIRT_VOL_NAME}" || 
+    error_exit "Failed to delete volume '${LIBVIRT_VOL_NAME}' from pool '${LIBVIRT_POOL}'"


should it fail here? or should it keep and try to delete the pool?

Added clean up

snir911 · 2025-02-12T08:50:46Z

config/peerpods/podvm/libvirt-podvm-image-handler.sh

-    virsh -d 0 -c "${LIBVIRT_URI}" pool-undefine "${LIBVIRT_POOL}" ||
+        echo "Pool '${LIBVIRT_POOL}' destroyed successfully."
+
+        virsh -d 0 -c "${LIBVIRT_URI}" pool-undefine "${LIBVIRT_POOL}" ||


If pool exist we may destroy pool & volumes that was not created by the code.
Would it make sense to do the following:

If user didn't set LIBVIRT_VOL & LIBVIRT_POOL -> create pool & vol named based on the cluster id e.g. vol_ab12ac -> set the values to the configMap

This is similar to the approach we take when we create images

If the user misses setting LIBVIRT_VOL and LIBVIRT_POOL, the controller will give an error and will not proceed further. However, as a preventive measure, we have added default values.

I guess the defaults can be removed if we will create the pool/volume with specific naming, doing that will prevent accidental modification of unrelated volume/pools that are in use by other libvirtd clients

openshift-ci · 2025-02-17T10:50:04Z

New changes are detected. LGTM label has been removed.

Moving LIBVIRT_POOL, LIBVIRT_VOL_NAME & LIBVIRT_DIR_NAME variables into config map instead of secrets Signed-off-by: Saripalli Lavanya <[email protected]>

Added code to check if Libvirt pool and volume exists if not creating them through handler Signed-off-by: Saripalli Lavanya <[email protected]>

Saripalli-lavanya · 2025-02-17T11:10:07Z

Hi @snir911 and @bpradipt, could you please take a re-review of the PR? I have updated code as per suggestions. Thank you so much.

openshift-ci · 2025-02-17T13:24:48Z

@Saripalli-lavanya: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

snir911

LGTM , thanks, also pls see my comment for consideration as future change

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 22, 2025

Saripalli-lavanya force-pushed the sl-libvirt-auto branch 3 times, most recently from 95a4394 to f7d6a28 Compare January 30, 2025 14:24

Saripalli-lavanya marked this pull request as ready for review January 30, 2025 14:26

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 30, 2025

openshift-ci bot requested review from tbuskey and vvoronko January 30, 2025 14:27

Saripalli-lavanya force-pushed the sl-libvirt-auto branch from f7d6a28 to 6756335 Compare January 31, 2025 11:00

bpradipt reviewed Feb 5, 2025

View reviewed changes

config/peerpods/podvm/libvirt-podvm-image-handler.sh Show resolved Hide resolved

bpradipt added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Feb 5, 2025

bpradipt requested a review from snir911 February 5, 2025 12:33

Saripalli-lavanya force-pushed the sl-libvirt-auto branch from 6756335 to a62b470 Compare February 5, 2025 13:04

openshift-ci bot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 5, 2025

bpradipt reviewed Feb 11, 2025

View reviewed changes

openshift-ci bot assigned bpradipt Feb 11, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 11, 2025

bpradipt reviewed Feb 11, 2025

View reviewed changes

config/peerpods/podvm/libvirt-podvm-image-handler.sh Outdated Show resolved Hide resolved

bpradipt reviewed Feb 11, 2025

View reviewed changes

config/peerpods/podvm/libvirt-podvm-image-handler.sh Outdated Show resolved Hide resolved

bpradipt removed the lgtm Indicates that a PR is ready to be merged. label Feb 11, 2025

Saripalli-lavanya changed the title ~~Create libvirt pool and volume on KVM host & libvirt config map udpate~~ Create libvirt pool and volume on KVM host & libvirt config map update Feb 11, 2025

Saripalli-lavanya force-pushed the sl-libvirt-auto branch from a62b470 to b6927d4 Compare February 11, 2025 06:18

bpradipt approved these changes Feb 11, 2025

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 11, 2025

snir911 reviewed Feb 12, 2025

View reviewed changes

Saripalli-lavanya force-pushed the sl-libvirt-auto branch from b6927d4 to a22af99 Compare February 17, 2025 10:50

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 17, 2025

Saripalli-lavanya force-pushed the sl-libvirt-auto branch from a22af99 to ba5e6d1 Compare February 17, 2025 10:55

Saripalli-lavanya added 2 commits February 17, 2025 16:27

Libvirt provider config map updates

a8c1ca2

Moving LIBVIRT_POOL, LIBVIRT_VOL_NAME & LIBVIRT_DIR_NAME variables into config map instead of secrets Signed-off-by: Saripalli Lavanya <[email protected]>

Extending libvirt image handler to create pool and volume

78a163c

Added code to check if Libvirt pool and volume exists if not creating them through handler Signed-off-by: Saripalli Lavanya <[email protected]>

Saripalli-lavanya force-pushed the sl-libvirt-auto branch from ba5e6d1 to 78a163c Compare February 17, 2025 10:57

snir911 approved these changes Feb 18, 2025

View reviewed changes

bpradipt merged commit 55d18c9 into openshift:devel Feb 20, 2025
4 of 5 checks passed

Create libvirt pool and volume on KVM host & libvirt config map update #499

Create libvirt pool and volume on KVM host & libvirt config map update #499

Uh oh!

Conversation

Saripalli-lavanya commented Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Jan 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Saripalli-lavanya commented Feb 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bpradipt commented Feb 11, 2025

Uh oh!

bpradipt left a comment

Choose a reason for hiding this comment

Uh oh!

snir911 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Feb 17, 2025

Uh oh!

Saripalli-lavanya commented Feb 17, 2025

Uh oh!

openshift-ci bot commented Feb 17, 2025

Uh oh!

snir911 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Saripalli-lavanya commented Jan 22, 2025 •

edited

Loading

snir911 left a comment •

edited

Loading