Copilot commented on code in PR #12617:
URL: https://github.com/apache/cloudstack/pull/12617#discussion_r2962444971
##########
engine/storage/volume/src/main/java/org/apache/cloudstack/storage/datastore/provider/DefaultHostListener.java:
##########
@@ -139,6 +141,18 @@ public boolean hostConnect(long hostId, long poolId)
throws StorageConflictExcep
Map<String, String> nfsMountOpts =
storageManager.getStoragePoolNFSMountOpts(pool, null).first();
Optional.ofNullable(nfsMountOpts).ifPresent(detailsMap::putAll);
+
+ // Propagate CLVM secure zero-fill setting to the host
+ // Note: This is done during host connection (agent start, MS restart,
host reconnection)
+ // so the setting is non-dynamic. Changes require host reconnection to
take effect.
+ if (ClvmLockManager.isClvmPoolType(pool.getPoolType())) {
+ Boolean clvmSecureZeroFill =
VolumeApiServiceImpl.CLVMSecureZeroFill.valueIn(poolId);
+ if (clvmSecureZeroFill != null) {
+ detailsMap.put("clvmsecurezerofill",
String.valueOf(clvmSecureZeroFill));
+ logger.debug("Added CLVM secure zero-fill setting: {} for
storage pool: {}", clvmSecureZeroFill, pool);
+ }
Review Comment:
This code hard-codes the detail key string `"clvmsecurezerofill"`. The same
key is also defined as `KVMStoragePool.CLVM_SECURE_ZERO_FILL`; having two
sources of truth risks drift/typos between management server and agent.
Consider moving this key to a shared constant accessible from both modules (or
otherwise centralizing it) and reference that constant here.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java:
##########
@@ -693,11 +848,248 @@ public KVMPhysicalDisk getPhysicalDisk(String
volumeUuid, KVMStoragePool pool) {
}
return disk;
} catch (LibvirtException e) {
- logger.debug("Failed to get physical disk:", e);
+ logger.debug("Failed to get volume from libvirt: " +
e.getMessage());
+ if (isClvmPool) {
+ return getPhysicalDiskWithClvmFallback(volumeUuid, pool,
libvirtPool);
+ }
+
throw new CloudRuntimeException(e.toString());
}
}
+ /**
+ * CLVM fallback: First tries to refresh libvirt pool to make volume
visible,
+ * if that fails, accesses volume directly via block device path.
+ */
+ private KVMPhysicalDisk getPhysicalDiskWithClvmFallback(String volumeUuid,
KVMStoragePool pool, LibvirtStoragePool libvirtPool) {
+ logger.info("CLVM volume not visible to libvirt, attempting pool
refresh for volume: {}", volumeUuid);
+
+ try {
+ logger.debug("Refreshing libvirt storage pool: {}",
pool.getUuid());
+ libvirtPool.getPool().refresh(0);
+
+ StorageVol vol = getVolume(libvirtPool.getPool(), volumeUuid);
+ if (vol != null) {
+ logger.info("Volume found after pool refresh: {}", volumeUuid);
+ KVMPhysicalDisk disk;
+ LibvirtStorageVolumeDef voldef =
getStorageVolumeDef(libvirtPool.getPool().getConnect(), vol);
+ disk = new KVMPhysicalDisk(vol.getPath(), vol.getName(), pool);
+ disk.setSize(vol.getInfo().allocation);
+ disk.setVirtualSize(vol.getInfo().capacity);
+ disk.setFormat(voldef.getFormat() ==
LibvirtStorageVolumeDef.VolumeFormat.QCOW2 ?
+ PhysicalDiskFormat.QCOW2 : PhysicalDiskFormat.RAW);
+ return disk;
+ }
+ } catch (LibvirtException refreshEx) {
+ logger.debug("Pool refresh failed or volume still not found: {}",
refreshEx.getMessage());
+ }
+
+ logger.info("Falling back to direct block device access for volume:
{}", volumeUuid);
+ return getPhysicalDiskViaDirectBlockDevice(volumeUuid, pool);
+ }
+
+ private String getVgName(String sourceDir) {
+ String vgName = sourceDir;
+ if (vgName.startsWith("/")) {
+ String[] parts = vgName.split("/");
+ List<String> tokens = Arrays.stream(parts)
+ .filter(s -> !s.isEmpty()).collect(Collectors.toList());
+
+ vgName = tokens.size() > 1 ? tokens.get(1)
+ : tokens.size() == 1 ? tokens.get(0)
+ : "";
+ }
+ return vgName;
+ }
+
+ /**
+ * For CLVM volumes that exist in LVM but are not visible to libvirt,
+ * access them directly via block device path.
+ */
+ private KVMPhysicalDisk getPhysicalDiskViaDirectBlockDevice(String
volumeUuid, KVMStoragePool pool) {
+ try {
+ String vgName = extractVgNameFromPool(pool);
+ verifyLvExistsInVg(volumeUuid, vgName);
+
+ logger.info("Volume {} exists in LVM but not visible to libvirt,
accessing directly", volumeUuid);
+
+ String lvPath = findAccessibleDeviceNode(volumeUuid, vgName, pool);
+ long size = getClvmVolumeSize(lvPath);
+
+ KVMPhysicalDisk disk = createPhysicalDiskFromClvmLv(lvPath,
volumeUuid, pool, size);
+
+ ensureTemplateAccessibility(volumeUuid, lvPath, pool);
+
+ return disk;
+
+ } catch (CloudRuntimeException ex) {
+ throw ex;
+ } catch (Exception ex) {
+ logger.error("Failed to access CLVM volume via direct block
device: {}", volumeUuid, ex);
+ throw new CloudRuntimeException(String.format("Could not find
volume %s: %s ", volumeUuid, ex.getMessage()));
+ }
+ }
+
+ private String extractVgNameFromPool(KVMStoragePool pool) {
+ String sourceDir = pool.getLocalPath();
+ if (sourceDir == null || sourceDir.isEmpty()) {
+ throw new CloudRuntimeException("CLVM pool sourceDir is not set,
cannot determine VG name");
+ }
+ String vgName = getVgName(sourceDir);
+ logger.debug("Using VG name: {} (from sourceDir: {})", vgName,
sourceDir);
+ return vgName;
+ }
+
+ private void verifyLvExistsInVg(String volumeUuid, String vgName) {
+ logger.debug("Checking if volume {} exists in VG {}", volumeUuid,
vgName);
+ Script checkLvCmd = new Script("/usr/sbin/lvs", 5000, logger);
+ checkLvCmd.add("--noheadings");
+ checkLvCmd.add("--unbuffered");
+ checkLvCmd.add(vgName + "/" + volumeUuid);
+
+ String checkResult = checkLvCmd.execute();
+ if (checkResult != null) {
+ logger.debug("Volume {} does not exist in VG {}: {}", volumeUuid,
vgName, checkResult);
+ throw new CloudRuntimeException(String.format("Storage volume not
found: no storage vol with matching name '%s'", volumeUuid));
+ }
+ }
+
+ private String findAccessibleDeviceNode(String volumeUuid, String vgName,
KVMStoragePool pool) {
+ String lvPath = "/dev/" + vgName + "/" + volumeUuid;
+ File lvDevice = new File(lvPath);
+
+ if (!lvDevice.exists()) {
+ lvPath = tryDeviceMapperPath(volumeUuid, vgName, lvDevice);
+
+ if (!lvDevice.exists()) {
+ lvPath = handleMissingDeviceNode(volumeUuid, vgName, pool);
+ }
+ }
+
+ return lvPath;
+ }
+
+ private String tryDeviceMapperPath(String volumeUuid, String vgName, File
lvDevice) {
+ String vgNameEscaped = vgName.replace("-", "--");
+ String volumeUuidEscaped = volumeUuid.replace("-", "--");
+ String mapperPath = "/dev/mapper/" + vgNameEscaped + "-" +
volumeUuidEscaped;
+ File mapperDevice = new File(mapperPath);
+
+ if (!mapperDevice.exists()) {
+ lvDevice = mapperDevice;
+ }
+
+ return mapperPath;
+ }
Review Comment:
`findAccessibleDeviceNode` attempts to fall back to `/dev/mapper/...` if
`/dev/<vg>/<lv>` doesn’t exist, but the logic in `tryDeviceMapperPath` is
broken: it checks `if (!mapperDevice.exists())` (reversed) and assigning to
`lvDevice` has no effect on the caller. As a result, even if the mapper node
exists, the method will still think it’s missing and may throw incorrectly.
Rework this to check the mapper path existence and return the correct path (or
return a `File`/boolean) so the fallback actually works.
##########
server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java:
##########
@@ -2631,6 +2662,351 @@ private Volume orchestrateAttachVolumeToVM(Long vmId,
Long volumeId, Long device
return newVol;
}
+ /**
+ * Helper method to get storage pools for volume and VM.
+ *
+ * @param volumeToAttach The volume being attached
+ * @param vmExistingVolume The VM's existing volume
+ * @return Pair of StoragePoolVO objects (volumePool, vmPool), or null if
either pool is missing
+ */
+ private Pair<StoragePoolVO, StoragePoolVO>
getStoragePoolsForVolumeAttachment(VolumeInfo volumeToAttach, VolumeVO
vmExistingVolume) {
+ if (volumeToAttach == null || vmExistingVolume == null) {
+ return null;
+ }
+
+ StoragePoolVO volumePool =
_storagePoolDao.findById(volumeToAttach.getPoolId());
+ StoragePoolVO vmPool =
_storagePoolDao.findById(vmExistingVolume.getPoolId());
+
+ if (volumePool == null || vmPool == null) {
+ return null;
+ }
+
+ return new Pair<>(volumePool, vmPool);
+ }
+
+ /**
+ * Checks if both storage pools are CLVM type.
+ *
+ * @param volumePool Storage pool for the volume
+ * @param vmPool Storage pool for the VM
+ * @return true if both pools are CLVM type
+ */
+ private boolean areBothPoolsClvmType(StoragePoolVO volumePool,
StoragePoolVO vmPool) {
+ return volumePool.getPoolType() == StoragePoolType.CLVM &&
+ vmPool.getPoolType() == StoragePoolType.CLVM;
+ }
+
+ /**
+ * Checks if a storage pool is CLVM type.
+ *
+ * @param pool Storage pool to check
+ * @return true if pool is CLVM type
+ */
+ private boolean isClvmPool(StoragePoolVO pool) {
+ return pool != null && pool.getPoolType() == StoragePoolType.CLVM;
+ }
Review Comment:
The new CLVM attachment/migration helpers only recognize
`StoragePoolType.CLVM` (e.g., `areBothPoolsClvmType` and `isClvmPool`). Since
this PR introduces `CLVM_NG` and `ClvmLockManager.isClvmPoolType` already
treats both as CLVM-like, these helpers will silently skip the lightweight
lock-transfer path for CLVM_NG volumes. Update the helper predicates to include
`CLVM_NG` (or reuse a shared helper) so behavior is consistent for both pool
types.
##########
pom.xml:
##########
@@ -55,6 +55,10 @@
<sonar.host.url>https://sonarcloud.io</sonar.host.url>
<sonar.exclusions>engine/schema/src/main/java/org/apache/cloudstack/backup/BackupOfferingDetailsVO.java</sonar.exclusions>
<sonar.exclusions>api/src/main/java/org/apache/cloudstack/api/response/BackupOfferingResponse.java</sonar.exclusions>
+
<sonar.exclusions>core/src/main/java/com/cloud/agent/api/PostMigrationAnswer.java</sonar.exclusions>
+
<sonar.exclusions>core/src/main/java/com/cloud/agent/api/PostMigrationCommand.java</sonar.exclusions>
+
<sonar.exclusions>core/src/main/java/com/cloud/agent/api/PreMigrationCommand.java</sonar.exclusions>
+
<sonar.exclusions>core/src/main/java/org/apache/cloudstack/storage/command/ClvmLockTransferCommand.java</sonar.exclusions>
Review Comment:
`sonar.exclusions` is declared multiple times. In Maven, later property
declarations override earlier ones, so only the last exclusion will actually be
used. Combine all exclusions into a single `sonar.exclusions` property
(comma-separated) so none of the earlier exclusions are lost.
##########
engine/storage/src/main/java/org/apache/cloudstack/storage/endpoint/DefaultEndPointSelector.java:
##########
@@ -32,8 +32,13 @@
import com.cloud.dc.DedicatedResourceVO;
import com.cloud.dc.dao.DedicatedResourceDao;
+import com.cloud.storage.ClvmLockManager;
+import com.cloud.storage.Storage;
+import com.cloud.storage.VolumeDetailVO;
+import com.cloud.storage.dao.VolumeDetailsDao;
Review Comment:
This module (`cloud-engine-storage`) does not depend on `cloud-server` (see
the commented-out dependency in `engine/storage/pom.xml`), but this file
imports `com.cloud.storage.ClvmLockManager` which is defined under `server/`.
That will break compilation for this module. Consider removing this dependency
(e.g., inline the pool-type check here), or move `ClvmLockManager` (or at least
its `isClvmPoolType` helper) into a module that `cloud-engine-storage` depends
on (api/core/engine).
##########
engine/storage/src/main/java/org/apache/cloudstack/storage/endpoint/DefaultEndPointSelector.java:
##########
@@ -388,18 +406,91 @@ private List<HostVO>
listUpAndConnectingSecondaryStorageVmHost(Long dcId) {
return sc.list();
}
+ /**
+ * Selects endpoint for CLVM volumes with destination host hint.
+ * This ensures volumes are created on the correct host with exclusive
locks.
+ *
+ * @param volume The volume to check for CLVM routing
+ * @param operation Description of the operation (for logging)
+ * @return EndPoint for the destination host if CLVM routing applies, null
otherwise
+ */
+ private EndPoint selectClvmEndpointIfApplicable(VolumeInfo volume, String
operation) {
+ DataStore store = volume.getDataStore();
+
+ if (store.getRole() != DataStoreRole.Primary) {
+ return null;
+ }
+
+ // Check if this is a CLVM pool
+ StoragePoolVO pool = _storagePoolDao.findById(store.getId());
+ if (pool == null ||
+ (pool.getPoolType() != Storage.StoragePoolType.CLVM ||
Review Comment:
The CLVM pool type check is logically incorrect: `(poolType != CLVM ||
poolType != CLVM_NG)` is always true, so this method will always return `null`
and CLVM routing will never be applied. Change this to an AND check (or use a
helper like `isClvmPoolType`) so CLVM/CLVM_NG volumes can actually select the
intended endpoint.
```suggestion
(pool.getPoolType() != Storage.StoragePoolType.CLVM &&
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtClvmLockTransferCommandWrapper.java:
##########
@@ -0,0 +1,90 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package com.cloud.hypervisor.kvm.resource.wrapper;
+
+import org.apache.logging.log4j.Logger;
+import org.apache.logging.log4j.LogManager;
+
+import com.cloud.agent.api.Answer;
+import org.apache.cloudstack.storage.command.ClvmLockTransferCommand;
+import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
+import com.cloud.resource.CommandWrapper;
+import com.cloud.resource.ResourceWrapper;
+import com.cloud.utils.script.Script;
+
+@ResourceWrapper(handles = ClvmLockTransferCommand.class)
+public class LibvirtClvmLockTransferCommandWrapper
+ extends CommandWrapper<ClvmLockTransferCommand, Answer,
LibvirtComputingResource> {
+
+ protected Logger logger = LogManager.getLogger(getClass());
+
+ @Override
+ public Answer execute(ClvmLockTransferCommand cmd,
LibvirtComputingResource serverResource) {
+ String lvPath = cmd.getLvPath();
+ ClvmLockTransferCommand.Operation operation = cmd.getOperation();
+ String volumeUuid = cmd.getVolumeUuid();
+
+ logger.info(String.format("Executing CLVM lock transfer: operation=%s,
lv=%s, volume=%s",
+ operation, lvPath, volumeUuid));
+
+ try {
+ String lvchangeOpt;
+ String operationDesc;
+ switch (operation) {
+ case DEACTIVATE:
+ lvchangeOpt = "-an";
+ operationDesc = "deactivated";
+ break;
+ case ACTIVATE_EXCLUSIVE:
+ lvchangeOpt = "-aey";
+ operationDesc = "activated exclusively";
+ break;
+ case ACTIVATE_SHARED:
+ lvchangeOpt = "-asy";
+ operationDesc = "activated in shared mode";
+ break;
+ default:
+ return new Answer(cmd, false, "Unknown operation: " +
operation);
+ }
+
+ Script script = new Script("/usr/sbin/lvchange", 30000, logger);
+ script.add(lvchangeOpt);
+ script.add(lvPath);
+
+ String result = script.execute();
+
+ if (result != null) {
+ logger.error("CLVM lock transfer failed for volume {}: {}}",
+ volumeUuid, result);
+ return new Answer(cmd, false,
+ String.format("lvchange %s %s failed: %s", lvchangeOpt,
lvPath, result));
+ }
+
+ logger.info("Successfully executed CLVM lock transfer: {} {}} for
volume {}}",
+ lvchangeOpt, lvPath, volumeUuid);
+
+ return new Answer(cmd, true,
+ String.format("Successfully %s CLVM volume %s", operationDesc,
volumeUuid));
+
+ } catch (Exception e) {
+ logger.error("Exception during CLVM lock transfer for volume {}:
{}}",
Review Comment:
Log message placeholder has an extra `}` (`{}}`), resulting in malformed
output. Replace `{}}` with `{}` (and similarly fix the other affected log lines
in this class).
```suggestion
logger.error("CLVM lock transfer failed for volume {}: {}",
volumeUuid, result);
return new Answer(cmd, false,
String.format("lvchange %s %s failed: %s", lvchangeOpt,
lvPath, result));
}
logger.info("Successfully executed CLVM lock transfer: {} {} for
volume {}",
lvchangeOpt, lvPath, volumeUuid);
return new Answer(cmd, true,
String.format("Successfully %s CLVM volume %s",
operationDesc, volumeUuid));
} catch (Exception e) {
logger.error("Exception during CLVM lock transfer for volume {}:
{}",
```
##########
engine/storage/volume/src/main/java/org/apache/cloudstack/storage/datastore/provider/DefaultHostListener.java:
##########
@@ -37,12 +37,14 @@
import com.cloud.network.dao.NetworkVO;
import com.cloud.offerings.NetworkOfferingVO;
import com.cloud.offerings.dao.NetworkOfferingDao;
+import com.cloud.storage.ClvmLockManager;
import com.cloud.storage.DataStoreRole;
import com.cloud.storage.Storage;
import com.cloud.storage.StorageManager;
import com.cloud.storage.StoragePool;
import com.cloud.storage.StoragePoolHostVO;
import com.cloud.storage.StorageService;
+import com.cloud.storage.VolumeApiServiceImpl;
import com.cloud.storage.dao.StoragePoolHostDao;
Review Comment:
`cloud-engine-storage-volume` does not have a dependency on `cloud-server`,
but this class imports `com.cloud.storage.ClvmLockManager` and
`com.cloud.storage.VolumeApiServiceImpl` (both live under `server/`). This will
fail compilation for the engine module. To fix, avoid referencing server-layer
classes here (e.g., move `CLVMSecureZeroFill` ConfigKey and the CLVM pool-type
helper into an api/core module, or query the config via configuration DAOs
available in this module).
--
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]
