Copilot commented on code in PR #14379:
URL: https://github.com/apache/cloudstack/pull/14379#discussion_r4234443577
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ClvmStorageAdaptor.java:
##########
@@ -89,6 +89,42 @@ 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;
+ try {
+ sp = conn.storagePoolLookupByUUIDString(uuid);
+ } catch (LibvirtException e) {
+ logger.warn("CLVM/CLVM_NG storage pool {} doesn't exist in
libvirt. Assuming it is already removed", uuid);
+ return true;
+ }
Review Comment:
Only a missing pool should be treated as successful deletion.
`storagePoolLookupByUUIDString` can also throw for connection, authentication,
or daemon errors; swallowing those reports success and allows the caller to
remove its pool record while the libvirt definition remains. Check for
`VIR_ERR_NO_STORAGE_POOL` and rethrow every other libvirt error.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ClvmStorageAdaptor.java:
##########
@@ -290,7 +326,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);
Review Comment:
The existing `testCreateCLVMStoragePool_Success` still verifies
`mockStoragePool.setAutostart(1)` at `ClvmStorageAdaptorTest.java:110`. Since
this call is now intentionally removed, that test will fail; update it to
verify that autostart is not enabled.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ClvmStorageAdaptor.java:
##########
@@ -89,6 +89,42 @@ 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 {
Review Comment:
The new deletion behavior is untested despite this adaptor having dedicated
unit coverage. Add cases for inactive persistent pools (undefine without
destroy), legacy active pools (destroy then undefine), missing pools, and
lookup failures so the regression fix and error handling remain enforced.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ClvmStorageAdaptor.java:
##########
@@ -89,6 +89,42 @@ 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;
+ try {
+ sp = conn.storagePoolLookupByUUIDString(uuid);
+ } catch (LibvirtException e) {
+ logger.warn("CLVM/CLVM_NG storage pool {} doesn't exist in
libvirt. Assuming it is already removed", uuid);
+ return true;
+ }
+
+ if (sp.isActive() == 1) {
+ sp.destroy();
+ }
+ if (sp.isPersistent() == 1) {
+ sp.undefine();
+ }
+ sp.free();
Review Comment:
The libvirt `StoragePool` handle is leaked whenever `isActive`, `destroy`,
`isPersistent`, or `undefine` throws. Free it in a `finally` block, as other
libvirt handles are managed in this module, so failed deletion attempts do not
accumulate native handles.
--
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]
