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
+137 −2
Open

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

Show file tree
Hide file tree
Changes from all commits
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
49 changes: 48 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 @@ -38,6 +38,7 @@
import org.apache.cloudstack.utils.qemu.QemuImg.PhysicalDiskFormat;
import org.joda.time.Duration;
import org.libvirt.Connect;
import org.libvirt.Error;
import org.libvirt.LibvirtException;
import org.libvirt.StoragePool;
import org.libvirt.StorageVol;
Expand Down Expand Up @@ -89,6 +90,53 @@ 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);
} else {
try {
undefineInactiveClvmPool(LibvirtConnection.getConnection(), uuid);
} catch (LibvirtException e) {
throw new CloudRuntimeException(e.toString(), e);
}
}
return true;
}
Comment on lines +93 to +107

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();
}
Comment thread
Pearl1594 marked this conversation as resolved.
if (sp.isPersistent() == 1) {
sp.undefine();
}
} finally {
sp.free();
}
logger.info("CLVM/CLVM_NG storage pool {} was successfully removed from libvirt", uuid);
}
Comment on lines +109 to +127

private StoragePool lookupClvmPool(Connect conn, String uuid) throws LibvirtException {
try {
return conn.storagePoolLookupByUUIDString(uuid);
} catch (LibvirtException e) {
if (e.getError() != null && e.getError().getCode() == Error.ErrorNumber.VIR_ERR_NO_STORAGE_POOL) {
return null;
}
throw e;
}
}

@Override
public KVMStoragePool getStoragePool(String uuid, boolean refreshInfo) {
logger.info("Fetching CLVM/CLVM_NG storage pool {} ", uuid);
Expand Down Expand Up @@ -290,7 +338,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
90 changes: 89 additions & 1 deletion ...ypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ClvmStorageAdaptorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@
import org.junit.Test;
import org.junit.runner.RunWith;
import org.libvirt.Connect;
import org.libvirt.Error;
import org.libvirt.LibvirtException;
import org.libvirt.StoragePool;
import org.mockito.Mock;
import org.mockito.MockedConstruction;
Expand Down Expand Up @@ -107,10 +109,96 @@
clvmStorageAdaptor, mockConn, uuid, host, vgName);

assertNotNull("Storage pool should be created", result);
Mockito.verify(mockStoragePool).setAutostart(1);
Mockito.verify(mockStoragePool, never()).setAutostart(anyInt());

Check warning on line 112 in plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ClvmStorageAdaptorTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use a static import for "verify". 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AaEisSruqGtALy3xVwe4&open=AaEisSruqGtALy3xVwe4&pullRequest=14379
Mockito.verify(mockConn).storagePoolDefineXML(anyString(), eq(0));
}

private Connect mockLibvirtConnection() throws Exception {

Check warning on line 116 in plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ClvmStorageAdaptorTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Remove the declaration of thrown exception 'java.lang.Exception', as it cannot be thrown from method's body. 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AaEisSruqGtALy3xVwe5&open=AaEisSruqGtALy3xVwe5&pullRequest=14379
Connect mockConn = Mockito.mock(Connect.class);

Check warning on line 117 in plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ClvmStorageAdaptorTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use a static import for "mock". 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AaEisSruqGtALy3xVwe6&open=AaEisSruqGtALy3xVwe6&pullRequest=14379
libvirtConnectionMockedStatic.when(LibvirtConnection::getConnection).thenReturn(mockConn);
return mockConn;
}

private LibvirtException mockLibvirtException(Error.ErrorNumber errorNumber) {
Error error = Mockito.mock(Error.class);

Check warning on line 123 in plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ClvmStorageAdaptorTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use a static import for "mock". 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AaEisSruqGtALy3xVwe7&open=AaEisSruqGtALy3xVwe7&pullRequest=14379
when(error.getCode()).thenReturn(errorNumber);
LibvirtException exception = Mockito.mock(LibvirtException.class);

Check warning on line 125 in plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ClvmStorageAdaptorTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use a static import for "mock". 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AaEisSruqGtALy3xVwe8&open=AaEisSruqGtALy3xVwe8&pullRequest=14379
when(exception.getError()).thenReturn(error);
return exception;
}

@Test
public void testDeleteStoragePool_InactivePersistentPoolIsUndefinedWithoutDestroy() throws Exception {
String uuid = UUID.randomUUID().toString();
Connect mockConn = mockLibvirtConnection();
StoragePool mockStoragePool = Mockito.mock(StoragePool.class);

Check warning on line 134 in plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ClvmStorageAdaptorTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use a static import for "mock". 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AaEisSruqGtALy3xVwe9&open=AaEisSruqGtALy3xVwe9&pullRequest=14379
when(mockConn.storagePoolLookupByUUIDString(uuid)).thenReturn(mockStoragePool);
when(mockStoragePool.isActive()).thenReturn(0);
when(mockStoragePool.isPersistent()).thenReturn(1);

assertTrue(clvmStorageAdaptor.deleteStoragePool(uuid));

Mockito.verify(mockStoragePool, never()).destroy();

Check warning on line 141 in plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ClvmStorageAdaptorTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use a static import for "verify". 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AaEisSruqGtALy3xVwe-&open=AaEisSruqGtALy3xVwe-&pullRequest=14379
Mockito.verify(mockStoragePool).undefine();

Check warning on line 142 in plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ClvmStorageAdaptorTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use a static import for "verify". 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AaEisSruqGtALy3xVwe_&open=AaEisSruqGtALy3xVwe_&pullRequest=14379
Mockito.verify(mockStoragePool).free();

Check warning on line 143 in plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ClvmStorageAdaptorTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use a static import for "verify". 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AaEisSruqGtALy3xVwfA&open=AaEisSruqGtALy3xVwfA&pullRequest=14379
}

@Test
public void testDeleteStoragePool_ActivePoolIsDestroyedThenUndefined() throws Exception {
String uuid = UUID.randomUUID().toString();
Connect mockConn = mockLibvirtConnection();
StoragePool mockStoragePool = Mockito.mock(StoragePool.class);

Check warning on line 150 in plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ClvmStorageAdaptorTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use a static import for "mock". 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AaEisSruqGtALy3xVwfB&open=AaEisSruqGtALy3xVwfB&pullRequest=14379
when(mockConn.storagePoolLookupByUUIDString(uuid)).thenReturn(mockStoragePool);
when(mockStoragePool.isActive()).thenReturn(1);
when(mockStoragePool.isPersistent()).thenReturn(1);

assertTrue(clvmStorageAdaptor.deleteStoragePool(uuid));

org.mockito.InOrder inOrder = Mockito.inOrder(mockStoragePool);
inOrder.verify(mockStoragePool).destroy();
inOrder.verify(mockStoragePool).undefine();
inOrder.verify(mockStoragePool).free();
}

@Test
public void testDeleteStoragePool_MissingPoolIsTreatedAsRemoved() throws Exception {
String uuid = UUID.randomUUID().toString();
Connect mockConn = mockLibvirtConnection();
LibvirtException lookupError = mockLibvirtException(Error.ErrorNumber.VIR_ERR_NO_STORAGE_POOL);
when(mockConn.storagePoolLookupByUUIDString(uuid)).thenThrow(lookupError);

assertTrue(clvmStorageAdaptor.deleteStoragePool(uuid));
}

@Test(expected = CloudRuntimeException.class)
public void testDeleteStoragePool_OtherLookupErrorIsNotSwallowed() throws Exception {
String uuid = UUID.randomUUID().toString();
Connect mockConn = mockLibvirtConnection();
LibvirtException lookupError = mockLibvirtException(Error.ErrorNumber.VIR_ERR_INTERNAL_ERROR);
when(mockConn.storagePoolLookupByUUIDString(uuid)).thenThrow(lookupError);

clvmStorageAdaptor.deleteStoragePool(uuid);
}

@Test
public void testDeleteStoragePool_PoolIsFreedWhenUndefineFails() throws Exception {
String uuid = UUID.randomUUID().toString();
Connect mockConn = mockLibvirtConnection();
StoragePool mockStoragePool = Mockito.mock(StoragePool.class);

Check warning on line 187 in plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ClvmStorageAdaptorTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use a static import for "mock". 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AaEisSruqGtALy3xVwfC&open=AaEisSruqGtALy3xVwfC&pullRequest=14379
when(mockConn.storagePoolLookupByUUIDString(uuid)).thenReturn(mockStoragePool);
when(mockStoragePool.isActive()).thenReturn(0);
when(mockStoragePool.isPersistent()).thenReturn(1);
LibvirtException undefineError = mockLibvirtException(Error.ErrorNumber.VIR_ERR_INTERNAL_ERROR);
Mockito.doThrow(undefineError).when(mockStoragePool).undefine();

Check warning on line 192 in plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ClvmStorageAdaptorTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use a static import for "doThrow". 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AaEisSruqGtALy3xVwfD&open=AaEisSruqGtALy3xVwfD&pullRequest=14379

try {
clvmStorageAdaptor.deleteStoragePool(uuid);
org.junit.Assert.fail("Expected CloudRuntimeException");
} catch (CloudRuntimeException expected) {
Mockito.verify(mockStoragePool).free();

Check warning on line 198 in plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ClvmStorageAdaptorTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use a static import for "verify". 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AaEisSruqGtALy3xVwfE&open=AaEisSruqGtALy3xVwfE&pullRequest=14379
}
}

@Test
public void testCreateCLVMStoragePool_VGNotFound() throws Exception {
String uuid = UUID.randomUUID().toString();
Expand Down
Loading