Skip to content

Commit b9ef8b1

Browse files
committed
CLVM: fix template LV removal and protect templates used as backing file
- Treat a template as in use when any volume on any CLVM pool of the same VG uses it, so a CLVM_NG backing file is never removed under a volume - Deactivate the template LV on all hosts of the pools sharing the VG before removing it, releasing stale shared locks ("LV locked by other host") - Log hosts that cannot deactivate it by name, uuid and state, with the lvchange command to run there - Keep the LV when another pool on the same VG still references it - DestroyCommand reports a failed delete as failure, so the template pool record is kept and eviction retried instead of leaking the LV - CLVM cleanup treats only "not found" as already deleted; lvs errors and exceptions now fail the delete - Add clvm.lock.query.threads (default 5, restart required) for the parallel host calls, replacing the hardcoded 16
1 parent 799ab92 commit b9ef8b1

8 files changed

Lines changed: 356 additions & 35 deletions

File tree

‎plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDestroyCommandWrapper.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,9 @@ public Answer execute(final DestroyCommand command, final LibvirtComputingResour
4040
try {
4141
final KVMStoragePoolManager storagePoolMgr = libvirtComputingResource.getStoragePoolMgr();
4242
final KVMStoragePool pool = storagePoolMgr.getStoragePool(vol.getPoolType(), vol.getPoolUuid());
43-
pool.deletePhysicalDisk(vol.getPath(), null);
43+
if (!pool.deletePhysicalDisk(vol.getPath(), null)) {
44+
return new Answer(command, false, "Failed to delete " + vol.getPath());
45+
}
4446
return new Answer(command, true, "Success");
4547
} catch (final CloudRuntimeException e) {
4648
logger.debug("Failed to delete volume: " + e.toString());

‎plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ClvmStorageAdaptor.java‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -962,8 +962,12 @@ private boolean cleanupCLVMVolume(String uuid, KVMStoragePool pool) {
962962
String checkResult = checkLvs.execute();
963963

964964
if (checkResult != null) {
965-
logger.info("CLVM volume {} does not exist in LVM (check returned: {}), considering it as already deleted", uuid, checkResult);
966-
return true;
965+
if (isLvNotFound(checkResult)) {
966+
logger.info("CLVM volume {} does not exist in LVM (check returned: {}), considering it as already deleted", uuid, checkResult);
967+
return true;
968+
}
969+
logger.warn("Cannot check CLVM volume {}: {}", uuid, checkResult);
970+
return false;
967971
}
968972

969973
logger.info("Volume {} exists, proceeding with cleanup", uuid);
@@ -990,18 +994,22 @@ private boolean cleanupCLVMVolume(String uuid, KVMStoragePool pool) {
990994
return true;
991995
} else {
992996
logger.warn("Command 'lvremove -f {}' returned error: {}", lvPath, removeResult);
993-
if (removeResult.contains("not found") || removeResult.contains("Failed to find")) {
997+
if (isLvNotFound(removeResult)) {
994998
logger.info("CLVM volume {} not found during cleanup, considering it as already deleted", uuid);
995999
return true;
9961000
}
9971001
return false;
9981002
}
9991003
} catch (Exception ex) {
10001004
logger.error("Exception during CLVM volume cleanup for {}: {}", uuid, ex.getMessage(), ex);
1001-
return true;
1005+
return false;
10021006
}
10031007
}
10041008

1009+
private boolean isLvNotFound(String lvmError) {
1010+
return lvmError.contains("not found") || lvmError.contains("Failed to find");
1011+
}
1012+
10051013
private boolean shouldSecureZeroFill(KVMStoragePool pool) {
10061014
Map<String, String> details = pool.getDetails();
10071015
String secureZeroFillStr = (details != null) ? details.get(KVMStoragePool.CLVM_SECURE_ZERO_FILL) : null;

‎plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2269,6 +2269,7 @@ public void testDestroyCommand() {
22692269

22702270
when(libvirtComputingResourceMock.getStoragePoolMgr()).thenReturn(poolManager);
22712271
when(poolManager.getStoragePool(vol.getPoolType(), vol.getPoolUuid())).thenReturn(primary);
2272+
when(primary.deletePhysicalDisk(vol.getPath(), null)).thenReturn(true);
22722273

22732274
final LibvirtRequestWrapper wrapper = LibvirtRequestWrapper.getInstance();
22742275
assertNotNull(wrapper);
@@ -2280,6 +2281,22 @@ public void testDestroyCommand() {
22802281
verify(poolManager, times(1)).getStoragePool(vol.getPoolType(), vol.getPoolUuid());
22812282
}
22822283

2284+
@Test
2285+
public void testDestroyCommandDeleteFailed() {
2286+
final DestroyCommand command = new DestroyCommand(Mockito.mock(StoragePool.class), Mockito.mock(Volume.class), "Test");
2287+
final KVMStoragePoolManager poolManager = Mockito.mock(KVMStoragePoolManager.class);
2288+
final KVMStoragePool primary = Mockito.mock(KVMStoragePool.class);
2289+
final VolumeTO vol = command.getVolume();
2290+
2291+
when(libvirtComputingResourceMock.getStoragePoolMgr()).thenReturn(poolManager);
2292+
when(poolManager.getStoragePool(vol.getPoolType(), vol.getPoolUuid())).thenReturn(primary);
2293+
when(primary.deletePhysicalDisk(vol.getPath(), null)).thenReturn(false);
2294+
2295+
final Answer answer = LibvirtRequestWrapper.getInstance().execute(command, libvirtComputingResourceMock);
2296+
2297+
assertFalse(answer.getResult());
2298+
}
2299+
22832300
@SuppressWarnings("unchecked")
22842301
@Test
22852302
public void testDestroyCommandError() {

‎plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ClvmStorageAdaptorTest.java‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
import java.util.Map;
3737
import java.util.UUID;
3838
import org.junit.After;
39+
import org.junit.Assert;
3940
import org.junit.Before;
4041
import org.junit.Test;
4142
import org.junit.runner.RunWith;
@@ -821,6 +822,38 @@ public void testCleanupCLVMVolume_VolumeNotFound() throws Exception {
821822
assert result : "Should return true when volume not found (already deleted)";
822823
}
823824

825+
@Test
826+
public void testCleanupCLVMVolume_LockedByOtherHost() throws Exception {
827+
Mockito.when(mockPool.getLocalPath()).thenReturn("testvg");
828+
Mockito.when(mockPool.getUuid()).thenReturn(UUID.randomUUID().toString());
829+
830+
mockScriptConstruction = Mockito.mockConstruction(Script.class, (mock, context) -> {
831+
boolean lvremove = "lvremove".equals(context.arguments().get(0));
832+
when(mock.execute()).thenReturn(lvremove ? " LV locked by other host: testvg/template-1" : null);
833+
});
834+
835+
Method method = ClvmStorageAdaptor.class.getDeclaredMethod(
836+
"cleanupCLVMVolume", String.class, KVMStoragePool.class);
837+
method.setAccessible(true);
838+
839+
Assert.assertFalse((boolean) method.invoke(clvmStorageAdaptor, "template-1", mockPool));
840+
}
841+
842+
@Test
843+
public void testCleanupCLVMVolume_CheckFailsForOtherReason() throws Exception {
844+
Mockito.when(mockPool.getLocalPath()).thenReturn("testvg");
845+
Mockito.when(mockPool.getUuid()).thenReturn(UUID.randomUUID().toString());
846+
847+
mockScriptConstruction = Mockito.mockConstruction(Script.class,
848+
(mock, context) -> when(mock.execute()).thenReturn(" Lock manager lvmlockd is not running"));
849+
850+
Method method = ClvmStorageAdaptor.class.getDeclaredMethod(
851+
"cleanupCLVMVolume", String.class, KVMStoragePool.class);
852+
method.setAccessible(true);
853+
854+
Assert.assertFalse((boolean) method.invoke(clvmStorageAdaptor, "template-1", mockPool));
855+
}
856+
824857
@Test
825858
public void testCleanupCLVMVolume_NullSourceDir() throws Exception {
826859
String volumeUuid = UUID.randomUUID().toString();

‎server/src/main/java/com/cloud/storage/clvm/ClvmPoolManager.java‎

Lines changed: 104 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,10 @@
2323
import java.util.Collections;
2424
import java.util.HashMap;
2525
import java.util.HashSet;
26+
import java.util.LinkedHashMap;
2627
import java.util.List;
2728
import java.util.Map;
29+
import java.util.Objects;
2830
import java.util.Set;
2931
import java.util.concurrent.ConcurrentHashMap;
3032
import java.util.concurrent.ExecutionException;
@@ -33,6 +35,7 @@
3335
import java.util.concurrent.LinkedBlockingQueue;
3436
import java.util.concurrent.ThreadPoolExecutor;
3537
import java.util.concurrent.TimeUnit;
38+
import java.util.function.Function;
3639
import java.util.stream.Collectors;
3740
import javax.inject.Inject;
3841

@@ -58,6 +61,8 @@
5861
import org.apache.cloudstack.managed.context.ManagedContextRunnable;
5962
import org.apache.cloudstack.storage.clvm.command.ClvmLockTransferCommand;
6063
import org.apache.cloudstack.storage.clvm.command.ClvmLockTransferAnswer;
64+
import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
65+
import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
6166
import org.apache.logging.log4j.LogManager;
6267
import org.apache.logging.log4j.Logger;
6368
import org.springframework.stereotype.Component;
@@ -72,6 +77,8 @@ public class ClvmPoolManager implements Configurable {
7277
private HostDao _hostDao;
7378
@Inject
7479
private HostDetailsDao _hostDetailsDao;
80+
@Inject
81+
private PrimaryDataStoreDao _storagePoolDao;
7582

7683
protected Logger logger = LogManager.getLogger(getClass());
7784

@@ -93,10 +100,12 @@ public class ClvmPoolManager implements Configurable {
93100
public static final ConfigKey<Boolean> CLVMSecureZeroFill = new ConfigKey<>("Advanced", Boolean.class, "clvm.secure.zero.fill", "false",
94101
"When enabled, CLVM volumes to be zero-filled at the time of deletion to prevent data from being recovered by VMs reusing the space, as thick LVM volumes write data linearly. Note: This setting is propagated to hosts when they connect to the storage pool. Changing this setting requires disconnecting and reconnecting hosts or restarting the KVM agent for it to take effect.", false, ConfigKey.Scope.StoragePool);
95102

96-
private static final int LOCK_QUERY_THREADS = 16;
103+
public static final ConfigKey<Integer> ClvmLockQueryThreads = new ConfigKey<>("Advanced", Integer.class, "clvm.lock.query.threads", "5",
104+
"Number of threads a management server uses to send CLVM lock commands to several hosts in parallel: the fallback lock holder lookup that asks every host, and the deactivation of a template LV on all hosts before it is removed. Requires a management server restart.", false);
105+
97106
private static final int MAX_LEASE_READERS = 2;
98107

99-
private final ExecutorService lockQueryExecutor = newLockQueryExecutor();
108+
private ExecutorService lockQueryExecutor;
100109

101110
/** State shared by the lock lookups of all volumes of one operation. */
102111
public static class LockLookup {
@@ -114,17 +123,86 @@ boolean isUnreachable(Long hostId) {
114123
}
115124
}
116125

117-
private static ExecutorService newLockQueryExecutor() {
118-
ThreadPoolExecutor executor = new ThreadPoolExecutor(LOCK_QUERY_THREADS, LOCK_QUERY_THREADS, 60L, TimeUnit.SECONDS,
119-
new LinkedBlockingQueue<>(), new NamedThreadFactory("ClvmLockQuery"));
120-
executor.allowCoreThreadTimeOut(true);
121-
return executor;
126+
/** Created on first use, as the config value is not available when the bean is constructed. */
127+
private synchronized ExecutorService getLockQueryExecutor() {
128+
if (lockQueryExecutor == null) {
129+
int threads = Math.max(1, ClvmLockQueryThreads.value());
130+
ThreadPoolExecutor executor = new ThreadPoolExecutor(threads, threads, 60L, TimeUnit.SECONDS,
131+
new LinkedBlockingQueue<>(), new NamedThreadFactory("ClvmLockQuery"));
132+
executor.allowCoreThreadTimeOut(true);
133+
lockQueryExecutor = executor;
134+
}
135+
return lockQueryExecutor;
122136
}
123137

124138
public static boolean isClvmPoolType(Storage.StoragePoolType poolType) {
125139
return Arrays.asList(Storage.StoragePoolType.CLVM, Storage.StoragePoolType.CLVM_NG).contains(poolType);
126140
}
127141

142+
private static String getVgName(StoragePool pool) {
143+
String vgName = pool.getPath();
144+
return vgName.startsWith("/") ? vgName.substring(1) : vgName;
145+
}
146+
147+
private static String getLvPath(StoragePool pool, String lvName) {
148+
return String.format("/dev/%s/%s", getVgName(pool), lvName);
149+
}
150+
151+
/**
152+
* Pools of the same zone backed by the same VG share its LVs, e.g. one template LV used as backing file by volumes of all of them.
153+
*
154+
* @return ids of the pools sharing the VG of {@code pool}, including {@code pool}
155+
*/
156+
public Set<Long> findPoolIdsSharingVg(StoragePool pool) {
157+
String vgName = getVgName(pool);
158+
Set<Long> poolIds = _storagePoolDao.findPoolsByStorageTypeAndZone(pool.getPoolType(), pool.getDataCenterId()).stream()
159+
.filter(p -> vgName.equals(getVgName(p)))
160+
.map(StoragePoolVO::getId)
161+
.collect(Collectors.toCollection(HashSet::new));
162+
poolIds.add(pool.getId());
163+
return poolIds;
164+
}
165+
166+
/**
167+
* Deactivates an LV on every Up host of the pools sharing its VG, releasing their shared locks so it can be removed.
168+
* Best effort: a host still using the LV refuses, and removing it then fails on the lock.
169+
*/
170+
public void deactivateOnAllHosts(StoragePool pool, String lvName) {
171+
String lvPath = getLvPath(pool, lvName);
172+
Map<Long, HostVO> hosts = new LinkedHashMap<>();
173+
findPoolIdsSharingVg(pool).stream()
174+
.map(_storagePoolDao::findById)
175+
.filter(Objects::nonNull)
176+
.flatMap(p -> findPoolHosts(p).stream())
177+
.filter(h -> h.getHypervisorType() == Hypervisor.HypervisorType.KVM)
178+
.forEach(h -> hosts.putIfAbsent(h.getId(), h));
179+
List<Long> upHostIds = hosts.values().stream().filter(this::isUpKvmRoutingHost).map(HostVO::getId).collect(Collectors.toList());
180+
Map<Long, Boolean> results = callOnHosts(upHostIds, lvName, hostId -> deactivateOnHost(hosts.get(hostId), lvPath, lvName));
181+
List<String> notDeactivated = hosts.values().stream()
182+
.filter(h -> !Boolean.TRUE.equals(results.get(h.getId())))
183+
.map(h -> String.format("%s (%s, %s)", h.getName(), h.getUuid(), h.getStatus()))
184+
.collect(Collectors.toList());
185+
if (notDeactivated.isEmpty()) {
186+
logger.info("Deactivated {} on all hosts {}", lvPath, hosts.values().stream().map(HostVO::getName).collect(Collectors.toList()));
187+
return;
188+
}
189+
logger.warn("{} could not be deactivated on hosts {}. It cannot be removed while active there; deactivate it on those hosts with 'lvchange -an {}'.",
190+
lvPath, notDeactivated, lvPath);
191+
}
192+
193+
private Boolean deactivateOnHost(HostVO host, String lvPath, String lvName) {
194+
try {
195+
Answer answer = _agentMgr.send(host.getId(), new ClvmLockTransferCommand(ClvmLockTransferCommand.Operation.DEACTIVATE, lvPath, lvName));
196+
if (answer != null && answer.getResult()) {
197+
return true;
198+
}
199+
logger.warn("Failed to deactivate {} on host {}: {}", lvPath, host.getName(), answer != null ? answer.getDetails() : "null answer");
200+
} catch (AgentUnavailableException | OperationTimedoutException e) {
201+
logger.warn("Could not deactivate {} on host {}: {}", lvPath, host.getName(), e.getMessage());
202+
}
203+
return false;
204+
}
205+
128206
/**
129207
* Gets the CLVM lock host ID for a volume, optionally querying actual LVM state.
130208
*
@@ -234,11 +312,7 @@ public Long queryCurrentLockHolder(Long volumeId, String volumeUuid, String volu
234312
return null;
235313
}
236314

237-
String vgName = pool.getPath();
238-
if (vgName.startsWith("/")) {
239-
vgName = vgName.substring(1);
240-
}
241-
String lvPath = String.format("/dev/%s/%s", vgName, volumePath);
315+
String lvPath = getLvPath(pool, volumePath);
242316
Long dbHostId = getClvmLockHostId(volumeId, volumeUuid);
243317
// Hosts that did not answer the lock-state query for this volume
244318
Set<Long> noAnswerHostIds = new HashSet<>();
@@ -533,21 +607,26 @@ private Long recoverInactiveVolume(Long volumeId, String volumeUuid, String lvPa
533607
return null;
534608
}
535609

610+
/** @return lock state per host that answered; hosts that did not answer are absent */
611+
private Map<Long, Boolean> queryHostsLockState(List<Long> hostIds, String lvPath, String volumeUuid, LockLookup lookup) {
612+
return callOnHosts(hostIds, volumeUuid, hostId -> querySingleHostLockState(hostId, lvPath, volumeUuid, lookup));
613+
}
614+
536615
/**
537-
* Queries the hosts in parallel, so one slow host does not delay the others.
616+
* Calls the hosts in parallel, so one slow host does not delay the others.
538617
*
539-
* @return lock state per host that answered; hosts that did not answer are absent
618+
* @return result per host; hosts whose call returned null are absent
540619
*/
541-
private Map<Long, Boolean> queryHostsLockState(List<Long> hostIds, String lvPath, String volumeUuid, LockLookup lookup) {
542-
Map<Long, Boolean> states = new ConcurrentHashMap<>();
620+
private <T> Map<Long, T> callOnHosts(List<Long> hostIds, String volumeUuid, Function<Long, T> call) {
621+
Map<Long, T> results = new ConcurrentHashMap<>();
543622
List<Future<?>> futures = new ArrayList<>();
544623
for (Long hostId : hostIds) {
545-
futures.add(lockQueryExecutor.submit(new ManagedContextRunnable() {
624+
futures.add(getLockQueryExecutor().submit(new ManagedContextRunnable() {
546625
@Override
547626
protected void runInContext() {
548-
Boolean active = querySingleHostLockState(hostId, lvPath, volumeUuid, lookup);
549-
if (active != null) {
550-
states.put(hostId, active);
627+
T result = call.apply(hostId);
628+
if (result != null) {
629+
results.put(hostId, result);
551630
}
552631
}
553632
}));
@@ -559,10 +638,10 @@ protected void runInContext() {
559638
Thread.currentThread().interrupt();
560639
break;
561640
} catch (ExecutionException e) {
562-
logger.warn("Lock state query for volume {} failed: {}", volumeUuid, e.getMessage());
641+
logger.warn("Host call for volume {} failed: {}", volumeUuid, e.getMessage());
563642
}
564643
}
565-
return new HashMap<>(states);
644+
return new HashMap<>(results);
566645
}
567646

568647
/**
@@ -769,12 +848,7 @@ public boolean transferClvmVolumeLock(String volumeUuid, Long volumeId, String v
769848
return false;
770849
}
771850

772-
String vgName = pool.getPath();
773-
if (vgName.startsWith("/")) {
774-
vgName = vgName.substring(1);
775-
}
776-
777-
String lvPath = String.format("/dev/%s/%s", vgName, volumePath);
851+
String lvPath = getLvPath(pool, volumePath);
778852

779853
try {
780854
// sourceHostId is trusted as pre-validated by the caller
@@ -833,7 +907,8 @@ public ConfigKey<?>[] getConfigKeys() {
833907
return new ConfigKey<?>[] {
834908
CLVMSecureZeroFill,
835909
ClvmLockRecoveryRetryTimeout,
836-
ClvmLockRecoveryRetryInterval
910+
ClvmLockRecoveryRetryInterval,
911+
ClvmLockQueryThreads
837912
};
838913
}
839914
}

0 commit comments

Comments
 (0)