Damans227 commented on code in PR #14389:
URL: https://github.com/apache/cloudstack/pull/14389#discussion_r4258631538
##########
plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/datastore/driver/StorPoolPrimaryDataStoreDriver.java:
##########
@@ -1367,6 +1377,120 @@ public Pair<Long, Long> getVolumeStats(StoragePool
storagePool, String volumeId)
return null;
}
+ /**
+ * Lists StorPool volumes available for import (globalId name, no "cs" tag
unless
+ * {@code forUnmanage}). When {@code volumePath} is given, looks up that
one volume directly
+ * instead of browsing the whole pool; when {@code forUnmanage} is also
true, an already
+ * "cs"-tagged volume is still returned, since the caller is
unmanageVolume() looking for the
+ * very volume it is about to drop from CloudStack.
+ */
+ @Override
+ public List<VolumeOnStorageTO> listVolumesForImport(StoragePool pool,
String volumePath, String keyword, boolean forUnmanage) {
+
StorPoolUtil.spLog("StorPoolPrimaryDataStoreDriver.listVolumesForImport:
pool=%s, volumePath=%s, keyword=%s, forUnmanage=%s", pool.getName(),
volumePath, keyword, forUnmanage);
+ SpConnectionDesc conn = StorPoolUtil.getSpConnection(pool.getUuid(),
pool.getId(), storagePoolDetailsDao, primaryStoreDao);
+ List<VolumeOnStorageTO> volumes = new ArrayList<>();
+
+ if (StringUtils.isNotBlank(volumePath)) {
+ return getVolumeOnStorageByPath(pool, volumePath, conn, volumes,
forUnmanage);
+ }
+
+ SpApiResponse resp = StorPoolUtil.volumesListResponse(conn);
Review Comment:
should we only list volumes on this pool's storpool template? right now the
whole cluster comes back, so with two pools on one cluster the same disk shows
under both
##########
plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/datastore/driver/StorPoolPrimaryDataStoreDriver.java:
##########
@@ -1367,6 +1377,120 @@ public Pair<Long, Long> getVolumeStats(StoragePool
storagePool, String volumeId)
return null;
}
+ /**
+ * Lists StorPool volumes available for import (globalId name, no "cs" tag
unless
+ * {@code forUnmanage}). When {@code volumePath} is given, looks up that
one volume directly
+ * instead of browsing the whole pool; when {@code forUnmanage} is also
true, an already
+ * "cs"-tagged volume is still returned, since the caller is
unmanageVolume() looking for the
+ * very volume it is about to drop from CloudStack.
+ */
+ @Override
+ public List<VolumeOnStorageTO> listVolumesForImport(StoragePool pool,
String volumePath, String keyword, boolean forUnmanage) {
+
StorPoolUtil.spLog("StorPoolPrimaryDataStoreDriver.listVolumesForImport:
pool=%s, volumePath=%s, keyword=%s, forUnmanage=%s", pool.getName(),
volumePath, keyword, forUnmanage);
+ SpConnectionDesc conn = StorPoolUtil.getSpConnection(pool.getUuid(),
pool.getId(), storagePoolDetailsDao, primaryStoreDao);
+ List<VolumeOnStorageTO> volumes = new ArrayList<>();
+
+ if (StringUtils.isNotBlank(volumePath)) {
+ return getVolumeOnStorageByPath(pool, volumePath, conn, volumes,
forUnmanage);
+ }
+
+ SpApiResponse resp = StorPoolUtil.volumesListResponse(conn);
+ if (resp.getError() != null) {
+
StorPoolUtil.spLog("StorPoolPrimaryDataStoreDriver.listVolumesForImport: could
not list volumes for pool=%s. Error: %s", pool.getName(), resp.getError());
+ return volumes;
+ }
+ return getVolumeOnStorage(pool, keyword, resp, volumes);
+ }
+
+ @NotNull
+ private List<VolumeOnStorageTO> getVolumeOnStorage(StoragePool pool,
String keyword, SpApiResponse resp, List<VolumeOnStorageTO> volumes) {
+ JsonArray volumesList =
resp.fullJson.getAsJsonObject().getAsJsonArray("data");
+
+ for (int i = 0; i < volumesList.size(); i++) {
+ JsonObject volume = volumesList.get(i).getAsJsonObject();
+ String rawName = volume.get("name").getAsString();
+ if (isVolumeDeleted(volume) || !isEligibleForBulkImport(volume,
rawName)) {
+ continue;
+ }
+
+ String name = rawName.substring(1);
+
+ if (StringUtils.isNotBlank(keyword) && !name.contains(keyword)) {
+ continue;
+ }
+ volumes.add(toVolumeOnStorageTO(volume, name));
+ }
+
StorPoolUtil.spLog("StorPoolPrimaryDataStoreDriver.listVolumesForImport: found
%s volume(s) for pool=%s", volumes.size(), pool.getName());
+ return volumes;
+ }
+
+ @NotNull
+ private List<VolumeOnStorageTO> getVolumeOnStorageByPath(StoragePool pool,
String volumePath, SpConnectionDesc conn, List<VolumeOnStorageTO> volumes,
boolean forUnmanage) {
+ String name = StorPoolStorageAdaptor.getVolumeNameFromPath(volumePath,
true);
+ SpApiResponse resp = StorPoolUtil.volumeDescribeResponse(name, conn);
+ if (resp.getError() != null) {
+
StorPoolUtil.spLog("StorPoolPrimaryDataStoreDriver.listVolumesForImport: could
not describe volume=%s on pool=%s. Error: %s", name, pool.getName(),
resp.getError());
+ return volumes;
+ }
+ JsonArray data =
resp.fullJson.getAsJsonObject().getAsJsonArray("data");
+ if (data == null || data.isEmpty()) {
+ return volumes;
+ }
+ JsonObject volume = data.get(0).getAsJsonObject();
+ String rawName = volume.get("name").getAsString();
+ if (isVolumeDeleted(volume) || !isGlobalIdVolumeName(rawName) ||
(!forUnmanage && isTaggedCs(volume))) {
+ return volumes;
+ }
+ volumes.add(toVolumeOnStorageTO(volume, rawName.substring(1)));
+ return volumes;
+ }
+
+ private static boolean isVolumeDeleted(JsonObject volume) {
+ return volume.has("deleted") && volume.get("deleted").getAsBoolean();
+ }
+
+ private static boolean isGlobalIdVolumeName(String rawName) {
+ return !rawName.startsWith("*") && !rawName.contains("@") &&
rawName.startsWith("~");
+ }
+
+ private static boolean isTaggedCs(JsonObject volume) {
+ JsonObject tags = volume.has("tags") &&
!volume.get("tags").isJsonNull() ? volume.getAsJsonObject("tags") : null;
+ return tags != null && tags.has("cs");
+ }
+
+ private static boolean isEligibleForBulkImport(JsonObject volume, String
rawName) {
+ return isGlobalIdVolumeName(rawName) && !isTaggedCs(volume);
+ }
+
+ private VolumeOnStorageTO toVolumeOnStorageTO(JsonObject volume, String
name) {
+ String path = StorPoolUtil.devPath(name);
+ long size = volume.get("size").getAsLong();
+ long spaceUsed = volume.has("spaceUsed") ?
volume.get("spaceUsed").getAsLong() : size;
+ StorPoolUtil.spLog("listVolumesForImport: found volume=%s, path=%s,
size=%s, spaceUsed=%s", name, path, size, spaceUsed);
+ return new VolumeOnStorageTO(Hypervisor.HypervisorType.KVM, path,
name, path,
+ Storage.ImageFormat.QCOW2.toString(), spaceUsed, size);
+ }
+
+ @Override
+ public void volumeManagementChanged(StoragePool pool, Volume volume,
boolean imported) {
+
StorPoolUtil.spLog("StorPoolPrimaryDataStoreDriver.volumeManagementChanged:
pool=%s, volume=%s, imported=%s", pool.getName(), volume.getPath(), imported);
+ SpConnectionDesc conn = StorPoolUtil.getSpConnection(pool.getUuid(),
pool.getId(), storagePoolDetailsDao, primaryStoreDao);
+ String name =
StorPoolStorageAdaptor.getVolumeNameFromPath(volume.getPath(), true);
+ Map<String, String> tags;
+ if (imported) {
+ String tier = volume.getDiskOfferingId() != null ?
getTierFromOfferingDetail(volume.getDiskOfferingId()) : null;
+ Long instanceId = volume.getInstanceId();
+ tags = StorPoolHelper.addStorPoolTags(volume.getUuid(),
getVMInstanceUUID(instanceId), "volume", getVcPolicyTag(instanceId), tier);
+ } else {
+ tags = StorPoolHelper.addStorPoolTags(null, null, "", null, null);
Review Comment:
should unmanage also clear the vm and snapshot policy tags? only the cs tag
is removed, so the disk keeps the old vm link and snapshot policy
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckVolumeCommandWrapper.java:
##########
@@ -63,27 +71,37 @@ public Answer execute(final CheckVolumeCommand command,
final LibvirtComputingRe
try {
if
(STORAGE_POOL_TYPES_SUPPORTED.contains(storageFilerTO.getType())) {
- final KVMPhysicalDisk vol = pool.getPhysicalDisk(srcFile);
- final String path = vol.getPath();
- final boolean isRbd =
Storage.StoragePoolType.RBD.equals(storageFilerTO.getType());
-
- Map<VolumeOnStorageTO.Detail, String> volumeDetails =
getVolumeDetails(pool, vol);
- if (MapUtils.isEmpty(volumeDetails)) {
- return new Answer(command, false, "Unable to read the
volume on the storage pool");
+ final boolean shouldAttachTheVolume =
POOL_NEEDS_TO_EXPOSE_THE_VOLUME.contains(storageFilerTO.getType());
+ if (shouldAttachTheVolume &&
!pool.connectPhysicalDisk(srcFile, new HashMap<>())) {
+ return new Answer(command, false, "Unable to attach the
volume on the storage pool");
}
+ try {
+ final KVMPhysicalDisk vol = pool.getPhysicalDisk(srcFile);
+ final String path = vol.getPath();
+ final boolean isRawBlockPool =
RAW_BLOCK_STORAGE_POOL_TYPES.contains(storageFilerTO.getType());
+
+ Map<VolumeOnStorageTO.Detail, String> volumeDetails =
getVolumeDetails(pool, vol);
+ if (MapUtils.isEmpty(volumeDetails)) {
+ return new Answer(command, false, "Unable to read the
volume on the storage pool");
+ }
- if (!isRbd) {
- try {
- KVMPhysicalDisk.checkQcow2File(path);
- } catch (final CloudRuntimeException e) {
- return new CheckVolumeAnswer(command, false, "", 0,
volumeDetails);
+ if (!isRawBlockPool) {
+ try {
+ KVMPhysicalDisk.checkQcow2File(path);
+ } catch (final CloudRuntimeException e) {
+ return new CheckVolumeAnswer(command, false, "",
0, volumeDetails);
+ }
}
- }
- // Images on RBD are raw and the path is an image name that
qemu-img cannot open
- // without the rbd: URI, so take the size libvirt already
reported for the volume.
- long size = isRbd ? vol.getVirtualSize() :
KVMPhysicalDisk.getVirtualSizeFromFile(path);
- return new CheckVolumeAnswer(command, true, "", size,
volumeDetails);
+ // Images on RBD are raw and the path is an image name
that qemu-img cannot open
+ // without the rbd: URI, so take the size libvirt already
reported for the volume
+ long size = isRawBlockPool ? vol.getVirtualSize() :
KVMPhysicalDisk.getVirtualSizeFromFile(path);
+ return new CheckVolumeAnswer(command, true, "", size,
volumeDetails);
+ } finally {
+ if (shouldAttachTheVolume) {
+ pool.disconnectPhysicalDisk(srcFile);
Review Comment:
what happens if the disk is already attached on this host before the check?
looks like the detach at the end would pull it from there, or fail the check if
its in use
--
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]
