Skip to content

CLVM: remove inactive libvirt pool on storage pool delete and stop autostartign CLVM pools #14379

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Pearl1594 wants to merge 3 commits into
base: main
Choose a base branch
from
Open

CLVM: remove inactive libvirt pool on storage pool delete and stop autostartign CLVM pools #14379

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
150d800
CLVM: remove inactive libvirt pool on storage pool delete and stop au…
Pearl1594 Oct 9, 2026
4ecc286
Address review comments
Pearl1594 Oct 9, 2026
b880b67
optimize
Pearl1594 Oct 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 36 additions & 1 deletion ...ns/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ClvmStorageAdaptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,42 @@ public KVMStoragePool createStoragePool(String name, String host, int port, Stri
}
}

@Override
public boolean deleteStoragePool(String uuid) {

@DaanHoogland DaanHoogland Oct 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Pearl1594 , sonar has a good point here; this either returns true or throws an exception. I realise it is an override but is the return needed at all? (here or in undefineInactiveClvmPool(..))

@Pearl1594 Pearl1594 Oct 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the same pattern we follow for most the storageAdaptors too - LibvirtStorageAdaptor

@DaanHoogland DaanHoogland Oct 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As said the override may not be helped, but that doesn't mean the private method has to as well. Also the pattern might well need an anti-pattern.

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);
}
}
Comment on lines +109 to +127

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;
}
Comment thread
Copilot marked this conversation as resolved.
Outdated

if (sp.isActive() == 1) {
sp.destroy();
}
if (sp.isPersistent() == 1) {
sp.undefine();
}
sp.free();
Comment thread
Copilot marked this conversation as resolved.
Outdated
logger.info("CLVM/CLVM_NG storage pool {} was successfully removed from libvirt", uuid);
return true;
}

@Override
public KVMStoragePool getStoragePool(String uuid, boolean refreshInfo) {
logger.info("Fetching CLVM/CLVM_NG storage pool {} ", uuid);
Expand Down Expand Up @@ -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);
pool.setAutostart(1);
return pool;
Comment thread
Pearl1594 marked this conversation as resolved.
} catch (LibvirtException e) {
logger.warn("Failed to define CLVM/CLVM_NG pool in libvirt: {}", e.getMessage());
Expand Down
Loading