Copilot commented on code in PR #14379:
URL: https://github.com/apache/cloudstack/pull/14379#discussion_r4234509786
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ClvmStorageAdaptor.java:
##########
@@ -89,6 +90,54 @@ public KVMStoragePool createStoragePool(String name, String
host, int port, Stri
}
}
+ @Override
+ public boolean deleteStoragePool(String uuid) {
+ logger.info("Attempting to remove CLVM/CLVM_NG storage pool {} from
libvirt", uuid);
+
+ if (decStoragePoolRefCount(uuid)) {
+ logger.info("deleteStoragePool: CLVM/CLVM_NG storage pool {} still
in use", uuid);
+ return true;
+ }
+
+ try {
+ return undefineInactiveClvmPool(LibvirtConnection.getConnection(),
uuid);
+ } catch (LibvirtException e) {
+ throw new CloudRuntimeException(e.toString(), e);
+ }
+ }
+
+ private boolean undefineInactiveClvmPool(Connect conn, String uuid) throws
LibvirtException {
+ StoragePool sp = lookupClvmPool(conn, uuid);
+ if (sp == null) {
+ logger.warn("CLVM/CLVM_NG storage pool {} doesn't exist in
libvirt. Assuming it is already removed", uuid);
+ return true;
+ }
+
+ try {
+ if (sp.isActive() == 1) {
+ sp.destroy();
+ }
Review Comment:
`destroy()` on a libvirt logical pool deactivates its backing volume group,
so this path takes the storage-side action that the PR description explicitly
says deletion must leave to the operator. A legacy/autostarted CLVM pool could
therefore lose shared LV availability during CloudStack deletion. Only undefine
inactive pools; if the pool is active, fail with an actionable message (or use
a separately designed safe migration), and update the active-pool test
accordingly.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ClvmStorageAdaptor.java:
##########
@@ -290,7 +339,6 @@ private StoragePool createCLVMStoragePool(Connect conn,
String uuid, String host
try {
StoragePool pool = conn.storagePoolDefineXML(poolDef.toString(),
0);
logger.info("Created libvirt pool definition for CLVM/CLVM_NG VG:
{} (pool will remain inactive)", volgroupName);
- pool.setAutostart(1);
return pool;
Review Comment:
Removing the setter only changes newly defined pools; libvirt preserves the
independent autostart flag when an existing pool with the same UUID is
redefined. Pools created by earlier CloudStack versions were set to autostart,
so after upgrade they will continue autostarting. Explicitly call
`pool.setAutostart(0)` and change the test to verify that value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
