Copilot commented on code in PR #14379:
URL: https://github.com/apache/cloudstack/pull/14379#discussion_r4381062852
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ClvmStorageAdaptor.java:
##########
@@ -89,6 +90,53 @@ 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);
+ } else {
+ try {
+ undefineInactiveClvmPool(LibvirtConnection.getConnection(),
uuid);
+ } catch (LibvirtException e) {
+ throw new CloudRuntimeException(e.toString(), e);
Review Comment:
Wrapping with `new CloudRuntimeException(e.toString(), e)` loses important
context about what operation failed and for which pool. Prefer an explicit
message including the UUID and action (e.g., “Failed to remove CLVM/CLVM_NG
storage pool <uuid> from libvirt”) while still chaining `e` as the cause.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ClvmStorageAdaptor.java:
##########
@@ -89,6 +90,53 @@ 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);
+ } else {
+ try {
+ undefineInactiveClvmPool(LibvirtConnection.getConnection(),
uuid);
+ } catch (LibvirtException e) {
+ throw new CloudRuntimeException(e.toString(), e);
+ }
+ }
+ return true;
+ }
Review Comment:
This override changes deletion semantics in a non-obvious way (removes only
the libvirt storage pool definition; does not tear down the VG/lockspace).
Adding a short method-level comment/Javadoc explaining this intent helps
prevent future regressions (e.g., reintroducing autostart or attempting to
destroy inactive pools) and clarifies operational expectations.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ClvmStorageAdaptor.java:
##########
@@ -89,6 +90,53 @@ 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);
+ } else {
+ try {
+ undefineInactiveClvmPool(LibvirtConnection.getConnection(),
uuid);
+ } catch (LibvirtException e) {
+ throw new CloudRuntimeException(e.toString(), e);
+ }
+ }
+ return true;
+ }
+
+ private void 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;
+ }
+
+ try {
+ if (sp.isActive() == 1) {
+ sp.destroy();
+ }
+ if (sp.isPersistent() == 1) {
+ sp.undefine();
+ }
+ } finally {
+ sp.free();
+ }
+ logger.info("CLVM/CLVM_NG storage pool {} was successfully removed
from libvirt", uuid);
+ }
Review Comment:
The method name `undefineInactiveClvmPool` is misleading because it also
destroys the pool when it is active (`sp.destroy()`). Consider renaming it to
reflect the actual behavior (e.g., `removeClvmPoolFromLibvirt`,
`undefineClvmPool`, or similar), so it remains accurate if someone reads/calls
it expecting “inactive only” semantics.
--
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]
