rp- commented on code in PR #14388:
URL: https://github.com/apache/cloudstack/pull/14388#discussion_r4281583464
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckVolumeCommandWrapper.java:
##########
@@ -61,35 +62,47 @@ public Answer execute(final CheckVolumeCommand command,
final LibvirtComputingRe
KVMStoragePoolManager poolMgr =
libvirtComputingResource.getStoragePoolMgr();
KVMStoragePool pool = poolMgr.getStoragePool(storageFilerTO.getType(),
storageFilerTO.getUuid());
+ boolean connected = false;
try {
if
(STORAGE_POOL_TYPES_SUPPORTED.contains(storageFilerTO.getType())) {
+ if (Boolean.TRUE.equals(pool.isPhysicalDiskInUse(srcFile))) {
+ Map<VolumeOnStorageTO.Detail, String> volumeDetails = new
HashMap<>();
+ volumeDetails.put(VolumeOnStorageTO.Detail.IS_LOCKED,
String.valueOf(Boolean.TRUE));
+ return new CheckVolumeAnswer(command, false, "", 0,
volumeDetails);
+ }
+ pool.checkPhysicalDiskForImport(srcFile);
Review Comment:
If `checkPhysicalDiskForImport` throws, the catch at line 101 returns `new
Answer(command, false, result)` with `result == null`. The server then only
reports "Disk not found or is invalid", and the "belongs to resource group X,
not to Y" message ends up in the agent log only. Could the wrapper return the
exception message here, and could `importKvmVirtualMachineFromDisk` include
`answer.getDetails()` in its error?
##########
plugins/storage/volume/linstor/src/main/java/com/cloud/hypervisor/kvm/storage/LinstorStorageAdaptor.java:
##########
@@ -357,6 +357,51 @@ public boolean connectPhysicalDisk(
return true;
}
+ /**
+ * Checks that a resource which is not managed by CloudStack yet can be
imported into this pool. It must
+ * belong to the resource group of the pool, otherwise a resource owned by
a volume of another pool on the
+ * same controller could be imported a second time.
+ */
+ public void checkPhysicalDiskForImport(String name, LinstorStoragePool
pool) {
+ final DevelopersApi api = getLinstorAPI(pool);
+ final String rscName = getLinstorRscName(name);
+ try {
+ List<ResourceDefinition> rscDfns = api.resourceDefinitionList(
+ Collections.singletonList(rscName), null, null, null,
null);
+ if (rscDfns == null || rscDfns.isEmpty()) {
+ throw new CloudRuntimeException(String.format("Linstor:
resource %s not found", rscName));
+ }
+ final String rscGrpName = rscDfns.get(0).getResourceGroupName();
+ if (rscGrpName == null ||
!rscGrpName.equalsIgnoreCase(pool.getResourceGroup())) {
Review Comment:
The resource group check is good, but this should also refuse resources with
a LUKS layer.
LINSTOR encrypts with a LUKS layer below DRBD, so the DRBD device the import
reads is plaintext. `qemu-img` info never reports encrypted: yes, and the
`IS_ENCRYPTED` checks in `checkVolume/checkIfVolumeIsEncrypted` never trigger.
The imported volume then has no `passphrase_id`.
`LinstorPrimaryDataStoreDriverImpl#copySnapshot`decides whether a volume is
encrypted from `getPassphraseId() != null`. For such a volume it backs up the
storage-layer snapshot device directly, and that device holds the LUKS
ciphertext. The snapshot backup, and any template or volume created from it, is
silently corrupt.
The easiest way in is unmanaging a VM with an encrypted LINSTOR root disk
and importing it again. As far as I can see, unmanaging a VM doesn't block
encrypted volumes. A resource created outside CloudStack in a LUKS-layered
stack has the same problem.
Could you refuse resources whose layer stack contains `LayerType.LUKS`, e.g.
by checking the layer data of the resource definition / resource we already
fetch here? The error message should say why. Importing them as encrypted isn't
possible without the passphrase, so refusing seems like the only safe option.
--
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]
