winterhazel commented on code in PR #13053:
URL: https://github.com/apache/cloudstack/pull/13053#discussion_r3181612958
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCreateDiskOnlyVMSnapshotCommandWrapper.java:
##########
@@ -146,21 +150,13 @@ protected Answer
takeDiskOnlyVmSnapshotOfStoppedVm(CreateDiskOnlyVmSnapshotComma
}
} catch (LibvirtException | QemuImgException e) {
logger.error("Exception while creating disk-only VM snapshot for
VM [{}]. Deleting leftover deltas.", vmName, e);
- for (VolumeObjectTO volumeObjectTO : volumeObjectTos) {
- Pair<Long, String> volSizeAndNewPath =
mapVolumeToSnapshotSizeAndNewVolumePath.get(volumeObjectTO.getUuid());
- PrimaryDataStoreTO primaryDataStoreTO = (PrimaryDataStoreTO)
volumeObjectTO.getDataStore();
- KVMStoragePool kvmStoragePool =
storagePoolMgr.getStoragePool(primaryDataStoreTO.getPoolType(),
primaryDataStoreTO.getUuid());
-
- if (volSizeAndNewPath == null) {
- continue;
- }
- try {
-
Files.deleteIfExists(Path.of(kvmStoragePool.getLocalPathFor(volSizeAndNewPath.second())));
- } catch (IOException ex) {
- logger.warn("Tried to delete leftover snapshot at [{}]
failed.", volSizeAndNewPath.second(), ex);
- }
- }
+ cleanupLeftoverDeltas(volumeObjectTos,
mapVolumeToSnapshotSizeAndNewVolumePath, storagePoolMgr);
return new Answer(cmd, e);
+ } catch (Exception e) {
+ logger.error("Unexpected exception while creating disk-only VM
snapshot for VM [{}]. Deleting leftover deltas.", vmName, e);
+ cleanupLeftoverDeltas(volumeObjectTos,
mapVolumeToSnapshotSizeAndNewVolumePath, storagePoolMgr);
+ return new CreateDiskOnlyVmSnapshotAnswer(cmd, false,
+ String.format("Creation of disk-only VM snapshot for VM
[%s] failed due to %s.", vmName, e.getMessage()), null);
Review Comment:
Can this be unified with the catch block above?
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiAdmStorageAdaptor.java:
##########
@@ -123,21 +130,39 @@ public boolean connectPhysicalDisk(String volumeUuid,
KVMStoragePool pool, Map<S
}
}
Review Comment:
This block could be extracted into a single method to reduce indentation and
make unit testing easier
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiAdmStorageAdaptor.java:
##########
@@ -280,8 +329,96 @@ private String getComponent(String path, int index) {
return tmp[index].trim();
}
+ /**
+ * Check if there are other LUNs on the same iSCSI target (IQN) that are
still
+ * visible as block devices. This is needed because ONTAP uses a single
IQN per
+ * SVM — logging out of the target would kill ALL LUNs, not just the one
being
+ * disconnected.
+ *
+ * Checks /dev/disk/by-path/ for symlinks matching the same host:port +
IQN but
+ * with a different LUN number.
+ */
+ private boolean hasOtherActiveLuns(String host, int port, String iqn,
String lun) {
+ String prefix = "ip-" + host + ":" + port + "-iscsi-" + iqn + "-lun-";
+ java.io.File byPathDir = new java.io.File("/dev/disk/by-path");
Review Comment:
Please import this class and use only the classname
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/listener/OntapHostListener.java:
##########
@@ -63,14 +69,20 @@ public boolean hostConnect(long hostId, long poolId) {
return false;
}
+ // TODO add host type check also since we support only KVM for now,
host.getHypervisorType().equals(HypervisorType.KVM)
Review Comment:
Is this going to be addressed in this PR?
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/provider/StorageProviderFactory.java:
##########
@@ -36,23 +39,25 @@ public class StorageProviderFactory {
public static StorageStrategy getStrategy(OntapStorage ontapStorage) {
ProtocolType protocol = ontapStorage.getProtocol();
logger.info("Initializing StorageProviderFactory with protocol: " +
protocol);
+ String decodedPassword = new
String(java.util.Base64.getDecoder().decode(ontapStorage.getPassword()),
StandardCharsets.UTF_8);
+ ontapStorage = new OntapStorage(
+ ontapStorage.getUsername(),
+ decodedPassword,
+ ontapStorage.getStorageIP(),
+ ontapStorage.getSvmName(),
+ ontapStorage.getSize(),
+ protocol);
switch (protocol) {
case NFS3:
- if (!ontapStorage.getIsDisaggregated()) {
- UnifiedNASStrategy unifiedNASStrategy = new
UnifiedNASStrategy(ontapStorage);
- ComponentContext.inject(unifiedNASStrategy);
- unifiedNASStrategy.setOntapStorage(ontapStorage);
- return unifiedNASStrategy;
- }
- throw new CloudRuntimeException("Unsupported configuration:
Disaggregated ONTAP is not supported.");
+ UnifiedNASStrategy unifiedNASStrategy = new
UnifiedNASStrategy(ontapStorage);
+ ComponentContext.inject(unifiedNASStrategy);
+ unifiedNASStrategy.setOntapStorage(ontapStorage);
+ return unifiedNASStrategy;
case ISCSI:
- if (!ontapStorage.getIsDisaggregated()) {
- UnifiedSANStrategy unifiedSANStrategy = new
UnifiedSANStrategy(ontapStorage);
- ComponentContext.inject(unifiedSANStrategy);
- unifiedSANStrategy.setOntapStorage(ontapStorage);
- return unifiedSANStrategy;
- }
- throw new CloudRuntimeException("Unsupported configuration:
Disaggregated ONTAP is not supported.");
+ UnifiedSANStrategy unifiedSANStrategy = new
UnifiedSANStrategy(ontapStorage);
+ ComponentContext.inject(unifiedSANStrategy);
+ unifiedSANStrategy.setOntapStorage(ontapStorage);
Review Comment:
Is this set needed? `ontapStorage` is already passed as a parameter to
`unifiedSANStrategy`
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/listener/OntapHostListener.java:
##########
@@ -137,6 +156,7 @@ public boolean hostDisconnected(Host host, StoragePool
pool) {
logger.error("Failed to add host by HostListener as host was not
found with id : {}", host.getId());
return false;
}
+ // TODO add storage pool get validation
Review Comment:
Is this going to be addressed in this PR?
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiAdmStorageAdaptor.java:
##########
@@ -280,8 +329,96 @@ private String getComponent(String path, int index) {
return tmp[index].trim();
}
+ /**
+ * Check if there are other LUNs on the same iSCSI target (IQN) that are
still
+ * visible as block devices. This is needed because ONTAP uses a single
IQN per
+ * SVM — logging out of the target would kill ALL LUNs, not just the one
being
+ * disconnected.
+ *
+ * Checks /dev/disk/by-path/ for symlinks matching the same host:port +
IQN but
+ * with a different LUN number.
+ */
+ private boolean hasOtherActiveLuns(String host, int port, String iqn,
String lun) {
+ String prefix = "ip-" + host + ":" + port + "-iscsi-" + iqn + "-lun-";
+ java.io.File byPathDir = new java.io.File("/dev/disk/by-path");
+ if (!byPathDir.exists() || !byPathDir.isDirectory()) {
+ return false;
+ }
+ java.io.File[] entries = byPathDir.listFiles();
+ if (entries == null) {
+ return false;
+ }
+ for (java.io.File entry : entries) {
+ String name = entry.getName();
+ // Skip partition entries (e.g. lun-0-part1, lun-0-part2) — these
are not
+ // independent LUNs, they are partition symlinks for the same LUN
disk.
+ // Only count actual LUN entries (no "-part" suffix after the lun
number).
+ if (name.startsWith(prefix) && !name.equals(prefix + lun) &&
!name.contains("-part")) {
+ logger.debug("Found other active LUN on same target: " + name);
+ return true;
+ }
+ }
+ return false;
+ }
+
+ /**
+ * Removes a single stale SCSI device from the kernel using the sysfs
interface.
+ *
+ * When ONTAP unmaps a LUN from the host's igroup, the by-path symlink and
the
+ * underlying SCSI device (/dev/sdX) remain present in the kernel until
explicitly
+ * removed — the kernel does not auto-remove devices from live iSCSI
sessions.
+ *
+ * This method resolves the by-path symlink to the real block device name
(e.g. sdd),
+ * then writes "1" to /sys/block/<dev>/device/delete — the standard Linux
kernel SCSI
+ * API for removing a single device without tearing down the entire iSCSI
session.
+ * Once the kernel processes the delete, it also removes the by-path
symlink.
+ *
+ * This is used instead of iscsiadm --logout when other LUNs on the same
IQN are still
+ * active (ONTAP single-IQN-per-SVM model), since logout would tear down
ALL LUNs.
+ */
+ private void removeStaleScsiDevice(String host, int port, String iqn,
String lun) {
+ String byPath = getByPath(host, port, "/" + iqn + "/" + lun);
+ java.nio.file.Path byPathLink = java.nio.file.Paths.get(byPath);
+ if (!java.nio.file.Files.exists(byPathLink)) {
+ logger.debug("by-path entry for LUN " + lun + " already gone,
nothing to remove");
+ return;
+ }
+ try {
+ java.nio.file.Path realDevice = byPathLink.toRealPath();
+ String devName = realDevice.getFileName().toString();
+ java.io.File deleteFile = new java.io.File("/sys/block/" + devName
+ "/device/delete");
+ if (!deleteFile.exists()) {
+ logger.warn("sysfs delete entry not found for device " +
devName + " — cannot remove stale SCSI device");
+ return;
+ }
+ try (java.io.FileWriter fw = new java.io.FileWriter(deleteFile)) {
+ fw.write("1");
+ }
+ logger.info("Removed stale SCSI device " + devName + " for LUN /"
+ iqn + "/" + lun + " via sysfs");
+ } catch (Exception e) {
+ logger.warn("Failed to remove stale SCSI device for LUN /" + iqn +
"/" + lun + ": " + e.getMessage());
+ }
+ }
+
private boolean disconnectPhysicalDisk(String host, int port, String iqn,
String lun) {
- // use iscsiadm to log out of the iSCSI target and un-discover it
+ // Check if other LUNs on the same IQN target are still in use.
+ // ONTAP (and similar) uses a single IQN per SVM with multiple LUNs.
+ // Doing iscsiadm --logout tears down the ENTIRE target session,
+ // which would destroy access to ALL LUNs — not just the one being
disconnected.
+ if (hasOtherActiveLuns(host, port, iqn, lun)) {
+ logger.info("Skipping iSCSI logout for /" + iqn + "/" + lun +
+ " — other LUNs on the same target are still active.
Removing stale SCSI device for this LUN only.");
+ removeStaleScsiDevice(host, port, iqn, lun);
+ // After removing this LUN's device, re-check: if no other LUNs
remain active,
+ // If it is the last one then must logout to clean up the iSCSI
session entirely.
+ if (hasOtherActiveLuns(host, port, iqn, lun)) {
+ logger.info("Other LUNs still active after removing /" + iqn +
"/" + lun + " — session kept alive.");
+ return true;
Review Comment:
As the disconnect operation failed, I wonder if it makes more sense to throw
an exception here instead
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiAdmStorageAdaptor.java:
##########
@@ -158,8 +183,23 @@ public boolean connectPhysicalDisk(String volumeUuid,
KVMStoragePool pool, Map<S
return true;
}
Review Comment:
This block could be extracted into a single method to reduce indentation and
make unit testing easier
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/provider/StorageProviderFactory.java:
##########
@@ -36,23 +39,25 @@ public class StorageProviderFactory {
public static StorageStrategy getStrategy(OntapStorage ontapStorage) {
ProtocolType protocol = ontapStorage.getProtocol();
logger.info("Initializing StorageProviderFactory with protocol: " +
protocol);
+ String decodedPassword = new
String(java.util.Base64.getDecoder().decode(ontapStorage.getPassword()),
StandardCharsets.UTF_8);
+ ontapStorage = new OntapStorage(
+ ontapStorage.getUsername(),
+ decodedPassword,
+ ontapStorage.getStorageIP(),
+ ontapStorage.getSvmName(),
+ ontapStorage.getSize(),
+ protocol);
switch (protocol) {
case NFS3:
- if (!ontapStorage.getIsDisaggregated()) {
- UnifiedNASStrategy unifiedNASStrategy = new
UnifiedNASStrategy(ontapStorage);
- ComponentContext.inject(unifiedNASStrategy);
- unifiedNASStrategy.setOntapStorage(ontapStorage);
- return unifiedNASStrategy;
- }
- throw new CloudRuntimeException("Unsupported configuration:
Disaggregated ONTAP is not supported.");
+ UnifiedNASStrategy unifiedNASStrategy = new
UnifiedNASStrategy(ontapStorage);
+ ComponentContext.inject(unifiedNASStrategy);
+ unifiedNASStrategy.setOntapStorage(ontapStorage);
Review Comment:
Is this set needed? `ontapStorage` is already passed as a parameter to
`unifiedNASStrategy`
--
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]
