slavkap commented on code in PR #14389:
URL: https://github.com/apache/cloudstack/pull/14389#discussion_r4247489530
##########
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.split("~")[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.split("~")[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);
+ }
Review Comment:
Not a bug — StorPool's VolumeUpdate API treats an empty-string tag value as
tag removal, not as setting the tag to "". This is an existing convention in
this codebase: see `StorPoolUtil.volumeRemoveTags()`, which predates this PR
and explicitly sends "" for tags it wants removed. So when
`volumeManagementChanged(..., imported=false)` sends `cs=""`, the StorPool side
drops the cs key entirely — a subsequent list/describe call returns a volume
with no cs tag at all, not `cs="". isTaggedCs()`'s presence-only check is
therefore correct as written.
This exact scenario is already covered end-to-end against a real StorPool
pool by t`est_01_import_volume_tags_storpool_volume_with_cs_and_uuid` in
t`est/integration/plugins/storpool/TestStorPoolImportVolume.py`: it creates a
volume via CloudStack, unmanages it, confirms the StorPool-side cs tag is
cleared, then calls the bulk `listVolumesForImport` (no path filter) and
asserts the unmanaged volume reappears in the list. That's precisely the
regression this finding was concerned about, and it's green.
--
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]
