copilot review comments incorporation

.github/CODEOWNERS

@@ -17,7 +17,7 @@
 
 /plugins/storage/volume/linstor @rp-
 /plugins/storage/volume/storpool @slavkap
-/plugins/storage/volume/ontap @rajiv1 @sandeeplocharla @piyush5 @suryag
+/plugins/storage/volume/ontap @rajiv-jain-netapp @sandeeplocharla @piyush5netapp @suryag1201
 
 .pre-commit-config.yaml @jbampton
 /.github/linters/ @jbampton

engine/api/src/main/java/org/apache/cloudstack/engine/subsystem/api/storage/DataStoreProvider.java

@@ -29,6 +29,11 @@ public interface DataStoreProvider {
     String SAMPLE_IMAGE = "Sample";
     String SMB = "NFS";
     String DEFAULT_PRIMARY = "DefaultPrimary";
+    /**
+     * Primary storage provider name for NetApp ONTAP ({@code OntapPrimaryDatastoreProvider#getName}).
+     * Single canonical value; use this instead of duplicating the string across modules.
+     */
+    String ONTAP_PLUGIN_NAME = "NetApp ONTAP";
 
     enum DataStoreProviderType {
         PRIMARY, IMAGE, ImageCache, OBJECT

engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategy.java

@@ -51,6 +51,7 @@
 import com.cloud.vm.snapshot.VMSnapshotVO;
 import org.apache.cloudstack.backup.BackupOfferingVO;
 import org.apache.cloudstack.backup.dao.BackupOfferingDao;
+import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreProvider;
 import org.apache.cloudstack.engine.subsystem.api.storage.ObjectInDataStoreStateMachine;
 import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotInfo;
 import org.apache.cloudstack.engine.subsystem.api.storage.StrategyPriority;
@@ -77,8 +78,6 @@ public class KvmFileBasedStorageVmSnapshotStrategy extends StorageVMSnapshotStra
 
     private static final List<Storage.StoragePoolType> supportedStoragePoolTypes = List.of(Storage.StoragePoolType.Filesystem, Storage.StoragePoolType.NetworkFilesystem, Storage.StoragePoolType.SharedMountPoint);
 
-    private static final String ONTAP_PROVIDER_NAME = "NetApp ONTAP";
-
     @Inject
     protected SnapshotDataStoreDao snapshotDataStoreDao;
 
@@ -327,13 +326,13 @@ public StrategyPriority canHandle(Long vmId, Long rootPoolId, boolean snapshotMe
         List<VolumeVO> volumes = volumeDao.findByInstance(vmId);
         for (VolumeVO volume : volumes) {
             StoragePoolVO storagePoolVO = storagePool.findById(volume.getPoolId());
-            if (storagePoolVO.isManaged() && ONTAP_PROVIDER_NAME.equals(storagePoolVO.getStorageProviderName())) {
-                logger.debug(String.format("%s as the VM has a volume on ONTAP managed storage pool [%s]. " +
-                        "ONTAP managed storage has its own dedicated VM snapshot strategy.", cantHandleLog, storagePoolVO.getName()));
+            if (storagePoolVO.isManaged() && DataStoreProvider.ONTAP_PLUGIN_NAME.equals(storagePoolVO.getStorageProviderName())) {
+                logger.debug("{} as the VM has a volume on ONTAP managed storage pool [{}]. " +
+                        "ONTAP managed storage has its own dedicated VM snapshot strategy.", cantHandleLog, storagePoolVO.getName());
                 return StrategyPriority.CANT_HANDLE;
             }
             if (!supportedStoragePoolTypes.contains(storagePoolVO.getPoolType())) {
-                logger.debug(String.format("%s as the VM has a volume that is in a storage with unsupported type [%s].", cantHandleLog, storagePoolVO.getPoolType()));
+                logger.debug("{} as the VM has a volume that is in a storage with unsupported type [{}].", cantHandleLog, storagePoolVO.getPoolType());
                 return StrategyPriority.CANT_HANDLE;
             }
             List<SnapshotVO> snapshots = snapshotDao.listByVolumeIdAndTypeNotInAndStateNotRemoved(volume.getId(), Snapshot.Type.GROUP);

engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java

@@ -83,6 +83,7 @@
 import org.apache.cloudstack.storage.to.TemplateObjectTO;
 import org.apache.cloudstack.storage.to.VolumeObjectTO;
 import org.apache.commons.collections.CollectionUtils;
+import org.apache.commons.lang3.ObjectUtils;
 import org.apache.commons.lang3.StringUtils;
 import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.LogManager;
@@ -1342,13 +1343,9 @@ private void createManagedVolumeCopyTemplateAsync(VolumeInfo volumeInfo, Primary
             grantAccess(volumeInfo, destHost, primaryDataStore);
             volumeInfo = volFactory.getVolume(volumeInfo.getId(), primaryDataStore);
             // For Netapp ONTAP iscsiName or Lun path  is available only after grantAccess
-            String managedStoreTarget = volumeInfo.get_iScsiName() != null ? volumeInfo.get_iScsiName() : volumeInfo.getUuid();
+            String managedStoreTarget = ObjectUtils.defaultIfNull(volumeInfo.get_iScsiName(), volumeInfo.getUuid());
             details.put(PrimaryDataStore.MANAGED_STORE_TARGET, managedStoreTarget);
             primaryDataStore.setDetails(details);
-            // Update destTemplateInfo with the iSCSI path from volumeInfo
-            if (destTemplateInfo instanceof TemplateObject) {
-                ((TemplateObject)destTemplateInfo).setInstallPath(volumeInfo.getPath());
-            }
 
             try {
                 motionSrv.copyAsync(srcTemplateInfo, destTemplateInfo, destHost, caller);

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiAdmStorageAdaptor.java

@@ -16,6 +16,9 @@
 // under the License.
 package com.cloud.hypervisor.kvm.storage;
 
+import java.io.File;
+import java.io.FileWriter;
+import java.nio.file.Path;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -97,19 +100,8 @@ public boolean connectPhysicalDisk(String volumeUuid, KVMStoragePool pool, Map<S
 
         String result = iScsiAdmCmd.execute();
 
-        if (result != null) {
-            // Node record may already exist from a previous run; accept and proceed
-            if (isNonFatalNodeCreate(result)) {
-                logger.debug("iSCSI node already exists for {}@{}:{}, proceeding", getIqn(volumeUuid), pool.getSourceHost(), pool.getSourcePort());
-            } else {
-                logger.debug("Failed to add iSCSI target " + volumeUuid);
-                System.out.println("Failed to add iSCSI target " + volumeUuid);
-
-                return false;
-            }
-        } else {
-            logger.debug("Successfully added iSCSI target " + volumeUuid);
-            System.out.println("Successfully added to iSCSI target " + volumeUuid);
+        if (!handleNodeCreateResult(result, volumeUuid)) {
+            return false;
         }
 
         String chapInitiatorUsername = details.get(DiskTO.CHAP_INITIATOR_USERNAME);
@@ -143,29 +135,13 @@ public boolean connectPhysicalDisk(String volumeUuid, KVMStoragePool pool, Map<S
 
         result = iScsiAdmCmd.execute();
 
-        if (result != null) {
-            if (isNonFatalLogin(result)) {
-                logger.debug("iSCSI login returned benign message for {}@{}:{}: {}", iqn, host, port, result);
-                // Session already exists — a newly mapped LUN won't be visible until
-                // the kernel's next periodic SCSI scan (~30-60s).
-                Script rescanCmd = new Script(true, "iscsiadm", 0, logger);
-                rescanCmd.add("-m", "session");
-                rescanCmd.add("--rescan");
-                String rescanResult = rescanCmd.execute();
-                if (rescanResult != null) {
-                    logger.warn("iSCSI session rescan returned: {}", rescanResult);
-                } else {
-                    logger.debug("iSCSI session rescan completed successfully for {}@{}:{}", iqn, host, port);
-                }
-            } else {
-                logger.debug("Failed to log in to iSCSI target " + volumeUuid + ": " + result);
-                System.out.println("Failed to log in to iSCSI target " + volumeUuid);
+        if (!handleLoginResult(result, volumeUuid)) {
+            return false;
+        }
 
-                return false;
-            }
-        } else {
-            logger.debug("Successfully logged in to iSCSI target " + volumeUuid);
-            System.out.println("Successfully logged in to iSCSI target " + volumeUuid);
+        // If the session already existed, a newly mapped LUN won't be visible until a rescan.
+        if (result != null) {
+            rescanIscsiSessions(iqn, host, port);
         }
 
         // There appears to be a race condition where logging in to the iSCSI volume via iscsiadm
@@ -183,23 +159,58 @@ public boolean connectPhysicalDisk(String volumeUuid, KVMStoragePool pool, Map<S
         return true;
     }
 
-    // Removed sessionExists() call to avoid noisy sudo/iscsiadm session queries that may fail on some setups
-
-    private boolean isNonFatalLogin(String result) {
-        if (result == null) return true;
+    /**
+     * Checks the result of an iscsiadm node-create command.
+     * Returns true if the node was created or already exists, false on failure.
+     */
+    boolean handleNodeCreateResult(String result, String volumeUuid) {
+        if (result == null) {
+            logger.debug("Successfully added iSCSI node for target {}", volumeUuid);
+            return true;
+        }
         String msg = result.toLowerCase();
-        // Accept messages where the session already exists
-        return msg.contains("already present") || msg.contains("already logged in") || msg.contains("session exists");
+        if (msg.contains("already exists") || msg.contains("database exists") || msg.contains("exists")) {
+            logger.debug("iSCSI node already exists for target {}, proceeding", volumeUuid);
+            return true;
+        }
+        logger.debug("Failed to add iSCSI node for target {}: {}", volumeUuid, result);
+        return false;
     }
 
-    private boolean isNonFatalNodeCreate(String result) {
-        if (result == null) return true;
+    /**
+     * Checks the result of an iscsiadm login command.
+     * Returns true if the login succeeded or session already exists, false on failure.
+     */
+    boolean handleLoginResult(String result, String volumeUuid) {
+        if (result == null) {
+            logger.debug("Successfully logged in to iSCSI target {}", volumeUuid);
+            return true;
+        }
         String msg = result.toLowerCase();
-        return msg.contains("already exists") || msg.contains("database exists") || msg.contains("exists");
+        if (msg.contains("already present") || msg.contains("already logged in") || msg.contains("session exists")) {
+            logger.debug("iSCSI session already exists for target {}, proceeding", volumeUuid);
+            return true;
+        }
+        logger.debug("Failed to log in to iSCSI target {}: {}", volumeUuid, result);
+        return false;
+    }
+
+    private void rescanIscsiSessions(String iqn, String host, int port) {
+        Script rescanCmd = new Script(true, "iscsiadm", 0, logger);
+        rescanCmd.add("-m", "node");
+        rescanCmd.add("-T", iqn);
+        rescanCmd.add("-p", host + ":" + port);
+        rescanCmd.add("--rescan");
+        String rescanResult = rescanCmd.execute();
+        if (rescanResult != null) {
+            logger.warn("iSCSI session rescan returned: {}", rescanResult);
+        } else {
+            logger.debug("iSCSI session rescan completed successfully for {}@{}:{}", iqn, host, port);
+        }
     }
 
     private void waitForDiskToBecomeAvailable(String volumeUuid, KVMStoragePool pool) {
-        int numberOfTries = 30;
+        int numberOfTries = 10;
         int timeBetweenTries = 1000;
 
         while (getPhysicalDisk(volumeUuid, pool).getSize() == 0 && numberOfTries > 0) {
@@ -283,8 +294,9 @@ private long getDeviceSize(String deviceByPath) {
                 logger.debug("Device by-path does not exist yet: " + deviceByPath);
                 return 0L;
             }
-        } catch (Exception ignore) {
+        } catch (Exception ex) {
             // If FS check fails for any reason, fall back to blockdev call
+            logger.error("Error fetching device size for {}", deviceByPath, ex);
         }
 
         Script iScsiAdmCmd = new Script(true, "blockdev", 0, logger);
@@ -340,33 +352,82 @@ private String getComponent(String path, int index) {
      */
     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");
+        File byPathDir = new File("/dev/disk/by-path");
         if (!byPathDir.exists() || !byPathDir.isDirectory()) {
             return false;
         }
-        java.io.File[] entries = byPathDir.listFiles();
+        File[] entries = byPathDir.listFiles();
         if (entries == null) {
             return false;
         }
-        for (java.io.File entry : entries) {
+        for (File entry : entries) {
             String name = entry.getName();
-            if (name.startsWith(prefix) && !name.equals(prefix + lun)) {
+            // 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);
+        Path byPathLink = Paths.get(byPath);
+        if (!Files.exists(byPathLink)) {
+            logger.debug("by-path entry for LUN " + lun + " already gone, nothing to remove");
+            return;
+        }
+        try {
+            Path realDevice = byPathLink.toRealPath();
+            String devName = realDevice.getFileName().toString();
+            File deleteFile = new 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 (FileWriter fw = new 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) {
         // 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");
-            return true;
+                    " — 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;
+            }
+            logger.info("No more active LUNs on target after removing /" + iqn + "/" + lun + " — proceeding with iSCSI logout.");
         }
 
         // No other LUNs active on this target — safe to logout and delete the node record.

plugins/storage/volume/ontap/pom.xml

@@ -137,7 +137,7 @@
         <dependency>
             <groupId>org.apache.cloudstack</groupId>
             <artifactId>cloud-engine-storage-snapshot</artifactId>
-            <version>4.23.0.0-SNAPSHOT</version>
+            <version>${project.version}</version>
             <scope>compile</scope>
         </dependency>
     </dependencies>
@@ -164,7 +164,6 @@
                 <version>${maven-surefire-plugin.version}</version>
                 <configuration>
                     <skipTests>false</skipTests>
-                    <argLine>-javaagent:${settings.localRepository}/net/bytebuddy/byte-buddy-agent/${byte-buddy-agent.version}/byte-buddy-agent-${byte-buddy-agent.version}.jar</argLine>
                     <includes>
                         <include>**/*Test.java</include>
                     </includes>