rp- commented on code in PR #14256:
URL: https://github.com/apache/cloudstack/pull/14256#discussion_r4182142433
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckVolumeCommandWrapper.java:
##########
@@ -63,6 +65,31 @@ public Answer execute(final CheckVolumeCommand command,
final LibvirtComputingRe
try {
if
(STORAGE_POOL_TYPES_SUPPORTED.contains(storageFilerTO.getType())) {
final KVMPhysicalDisk vol = pool.getPhysicalDisk(srcFile);
+ if
(Storage.StoragePoolType.RBD.equals(storageFilerTO.getType())
+ ||
Storage.StoragePoolType.Linstor.equals(storageFilerTO.getType())) {
+ // RBD and Linstor volumes are raw block devices, not
local qcow2 files:
+ // inspect them through qemu-img (RBD by its rbd: URI,
Linstor by its
+ // /dev/drbd device path) rather than checkQcow2File,
which would reject
+ // a raw device.
+ //
+ // A Linstor volume only materialises as a local /dev/drbd
device once the
+ // resource is made available on THIS host (a diskless
DRBD assignment). RBD
+ // needs no such step — qemu-img reaches it over the
network by its rbd: URI.
+ // So for Linstor we connect the resource here before
qemu-img inspects it and
+ // release the diskless assignment afterwards; the
replicated data on the
+ // storage nodes is untouched (disconnect only drops a
local diskless copy).
+ boolean linstorConnected = false;
+ if
(Storage.StoragePoolType.Linstor.equals(storageFilerTO.getType())) {
+ linstorConnected =
poolMgr.connectPhysicalDisk(storageFilerTO.getType(), storageFilerTO.getUuid(),
srcFile, null);
+ }
+ try {
+ return checkRbdVolume(command, pool, vol);
+ } finally {
+ if (linstorConnected) {
+
poolMgr.disconnectPhysicalDisk(storageFilerTO.getType(),
storageFilerTO.getUuid(), srcFile);
Review Comment:
`LinstorStorageAdaptor.connectPhysicalDisk` returns true whenever
`resourceMakeAvailableOnNode` succeeds, including when the resource was already
on this node. linstorConnected therefore means "the call succeeded", not "we
created the local resource". The disconnect then goes through
`tryDisconnectLinstor`, which:
- deletes the local resource if it is diskless and not a tiebreaker, even
if it existed before this check (e.g. placed by an admin, or left over from
another operation);
- calls removeTwoPrimariesProps if the resource is in use on another node.
If that volume is being live-migrated, this strips allow-two-primaries and
protocol in the middle of the migration.
I suggest: before connecting, check whether a resource for this name already
exists on the local node (one `viewResources` filtered to the local node and
this resource), and only disconnect if this check created it. If the in-use
check reports another node, skip the disconnect completely so the
`allow-two-primaries properties` are left alone. Same pattern applies to
`LibvirtGetVolumesOnStorageCommandWrapper.java` L72–82 (see comment there).
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtGetVolumesOnStorageCommandWrapper.java:
##########
@@ -65,7 +65,21 @@ public Answer execute(final GetVolumesOnStorageCommand
command, final LibvirtCom
final KVMStoragePool storagePool =
storagePoolMgr.getStoragePool(pool.getType(), pool.getUuid(), true, true);
if (StringUtils.isNotBlank(volumePath)) {
- return addVolumeByVolumePath(command, storagePool, volumePath);
+ // A Linstor volume is a DRBD device that only appears on this
host once its resource
+ // is made available here (a diskless assignment); connect it
before qemu-img inspects
+ // the device and release the diskless assignment afterwards (the
replicated data on
+ // the storage nodes is untouched). RBD needs no such step.
+ boolean linstorConnected = false;
+ if (StoragePoolType.Linstor.equals(pool.getType())) {
+ linstorConnected =
storagePoolMgr.connectPhysicalDisk(pool.getType(), pool.getUuid(), volumePath,
null);
+ }
+ try {
+ return addVolumeByVolumePath(command, storagePool, volumePath);
+ } finally {
+ if (linstorConnected) {
+ storagePoolMgr.disconnectPhysicalDisk(pool.getType(),
pool.getUuid(), volumePath);
+ }
+ }
Review Comment:
The connect step is only added for the single-path case. `addAllVolumes`
(L162–174, not part of this diff) runs `getDiskFileInfo` on each disk from
`listPhysicalDisks`. For Linstor, `disk.getPath()` is
`/dev/drbd/by-res/cs-…/0`, which only exists if the resource has a replica or
diskless resource on this host. For every other volume `qemu-img` info fails,
`info == null`, and the continue skips it without any error. In the UI, the
import-data-disk list for a Linstor pool therefore shows only the volumes that
happen to be on whichever host the management server sent the command to.
The listing is also expensive on large controllers. For each resource there
are 2 API calls in getPhysicalDisk (volumeDefinitionList and viewResources), 1
in getVolumeInUseNode (resourceList), and 2 qemu-img info runs.
For Linstor you should probably, don't inspect devices when listing. The
format is always `RAW`, size and in-use state come from the controller, and
backing files and qcow2 encryption can't apply to a raw DRBD device. A single
`viewResources` call (filtered to the pool's resource group) can provide name,
size and in-use state for all resources at once. Connect and inspect only in
the single-path case, as now.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtGetVolumesOnStorageCommandWrapper.java:
##########
@@ -65,7 +65,21 @@ public Answer execute(final GetVolumesOnStorageCommand
command, final LibvirtCom
final KVMStoragePool storagePool =
storagePoolMgr.getStoragePool(pool.getType(), pool.getUuid(), true, true);
if (StringUtils.isNotBlank(volumePath)) {
- return addVolumeByVolumePath(command, storagePool, volumePath);
+ // A Linstor volume is a DRBD device that only appears on this
host once its resource
+ // is made available here (a diskless assignment); connect it
before qemu-img inspects
+ // the device and release the diskless assignment afterwards (the
replicated data on
+ // the storage nodes is untouched). RBD needs no such step.
+ boolean linstorConnected = false;
+ if (StoragePoolType.Linstor.equals(pool.getType())) {
+ linstorConnected =
storagePoolMgr.connectPhysicalDisk(pool.getType(), pool.getUuid(), volumePath,
null);
+ }
+ try {
+ return addVolumeByVolumePath(command, storagePool, volumePath);
+ } finally {
+ if (linstorConnected) {
+ storagePoolMgr.disconnectPhysicalDisk(pool.getType(),
pool.getUuid(), volumePath);
Review Comment:
Same issue as in LibvirtCheckVolumeCommandWrapper L89: this disconnect can
remove a local diskless resource that existed before the call, and can strip
allow-two-primaries from a resource that is in use elsewhere. It should only
disconnect if this call created the local resource.
##########
plugins/storage/volume/linstor/src/main/java/com/cloud/hypervisor/kvm/storage/LinstorStorageAdaptor.java:
##########
@@ -593,7 +593,30 @@ public KVMPhysicalDisk createDiskFromTemplate(
@Override
public List<KVMPhysicalDisk> listPhysicalDisks(String storagePoolUuid,
KVMStoragePool pool)
{
- throw new UnsupportedOperationException("Listing disks is not
supported for this configuration.");
+ logger.debug("Linstor: listPhysicalDisks for pool {}",
storagePoolUuid);
+ final DevelopersApi api = getLinstorAPI(pool);
+ final LinstorStoragePool linstorPool = (LinstorStoragePool) pool;
+ final String rscGroup = linstorPool.getResourceGroup();
+ List<KVMPhysicalDisk> disks = new ArrayList<>();
+ try {
+ List<ResourceDefinition> rscDfns =
LinstorUtil.getRDListStartingWith(api, LinstorUtil.RSC_PREFIX);
+ for (ResourceDefinition rscDfn : rscDfns) {
+ if (rscGroup != null &&
!rscGroup.equalsIgnoreCase(rscDfn.getResourceGroupName())) {
+ continue;
+ }
+ String name =
rscDfn.getName().substring(LinstorUtil.RSC_PREFIX.length());
Review Comment:
This check keeps the listing to resources in the pool's own resource group.
But importVolume path=… (`GetVolumesOnStorage` with a volume path) and importVm
diskpath=… (CheckVolume) go straight to `getPhysicalDisk(name)`, which accepts
any `cs-*` resource on the controller. The duplicate checks on the management
server only look within the target pool:
- `VolumeImportUnmanageManagerImpl.java`:370:
`volumeDao.findByPoolIdAndPath(pool.getId(), volumePath)`
- `UnmanagedVMsManagerImpl.java`:3237: `findByPoolIdAndPath(poolId,
diskPath)`
Resource names are unique per LINSTOR controller, and several CloudStack
pools on one controller (different resource groups, e.g. SSD and HDD) is a
common setup. Scenario:
1. Pool A (resource group rg-ssd) has volume X, i.e. resource cs-X, attached
to a stopped VM.
2. An admin runs importVolume with storageid=<pool B> (resource group
rg-hdd, same controller) and path=X.
3. All checks pass. The in-use check doesn't help because nothing is running.
4. Two CloudStack volumes now point at the same DRBD resource. Deleting
either one calls `deleteResourceDefinition(cs-X)`, and the other volume's data
is gone.
Suggested fix: on the management server, before sending `CheckVolume` or
`GetVolumesOnStorage `for a Linstor pool, look up `cs-<path>` on the controller
(the driver already builds the API client via
`LinstorUtil.getLinstorAPI(pool.getHostAddress(), …))` and reject the import if
`ResourceDefinition.getResourceGroupName()` doesn't match the pool's resource
group. That uses the controller's own data, covers resources this CloudStack
database doesn't know about, and avoids comparing controller addresses across
pools. It applies to both `importVolume` and `importVm importsource=shared`.
--
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]
