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

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

@@ -350,23 +350,72 @@ private boolean hasOtherActiveLuns(String host, int port, String iqn, String lun
         }
         for (java.io.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);
+        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) {
         // 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>

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/NASFeignClient.java

@@ -21,9 +21,7 @@
 
 import feign.QueryMap;
 import org.apache.cloudstack.storage.feign.model.ExportPolicy;
-import org.apache.cloudstack.storage.feign.model.FileClone;
 import org.apache.cloudstack.storage.feign.model.FileInfo;
-import org.apache.cloudstack.storage.feign.model.response.JobResponse;
 import org.apache.cloudstack.storage.feign.model.response.OntapResponse;
 import feign.Headers;
 import feign.Param;
@@ -60,11 +58,6 @@ void createFile(@Param("authHeader") String authHeader,
                     @Param("path") String filePath,
                     FileInfo file);
 
-    @RequestLine("POST /api/storage/file/clone")
-    @Headers({"Authorization: {authHeader}"})
-    JobResponse cloneFile(@Param("authHeader") String authHeader,
-                          FileClone fileClone);
-
     // Export Policy Operations
     @RequestLine("POST /api/protocols/nfs/export-policies")
     @Headers({"Authorization: {authHeader}"})

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/VolumeConcise.java

@@ -39,5 +39,5 @@ public void setUuid(String uuid) {
     public String getName() {
         return name;
     }
-    public void setName(String name) {}
+    public void setName(String name) { this.name = name;  }
 }

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java

@@ -54,6 +54,7 @@
 import org.apache.cloudstack.storage.utils.OntapStorageConstants;
 import org.apache.cloudstack.storage.utils.OntapStorageUtils;
 import org.apache.cloudstack.storage.volume.datastore.PrimaryDataStoreHelper;
+import org.apache.commons.lang3.StringUtils;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 
@@ -420,7 +421,7 @@ private boolean validateProtocolSupportAndFetchHostsIdentifier(List<HostVO> host
                 for (HostVO host : hosts) {
                     if (host != null) {
                         ip =  host.getStorageIpAddress() != null ? host.getStorageIpAddress().trim() : "";
-                        if (ip.isEmpty() && host.getPrivateIpAddress() != null || host.getPrivateIpAddress().trim().isEmpty()) {
+                        if (ip.isEmpty() && StringUtils.isBlank(host.getPrivateIpAddress() )) {
                             // TODO we will inform customer through alert for excluded host because of protocol enabled on host
                             continue;
                         } else {

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/listener/OntapHostListener.java

@@ -100,6 +100,13 @@ public boolean hostConnect(long hostId, long poolId)  {
             }
 
             // Get the mount path from the answer
+
+            if (!(answer instanceof ModifyStoragePoolAnswer)) {
+                throw new CloudRuntimeException(String.format(
+                        "Unexpected answer type %s returned for modify storage pool command for pool %s on host %d",
+                        answer.getClass().getName(), pool, hostId));
+            }
+
             ModifyStoragePoolAnswer mspAnswer = (ModifyStoragePoolAnswer) answer;
             StoragePoolInfo poolInfo = mspAnswer.getPoolInfo();
             if (poolInfo == null) {

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java

@@ -37,6 +37,7 @@
 import org.apache.cloudstack.storage.service.model.ProtocolType;
 import org.apache.cloudstack.storage.utils.OntapStorageConstants;
 import org.apache.cloudstack.storage.utils.OntapStorageUtils;
+import org.apache.commons.collections.CollectionUtils;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import javax.inject.Inject;
@@ -274,47 +275,49 @@ public void deleteAccessGroup(AccessGroup accessGroup) {
             String authHeader = OntapStorageUtils.generateAuthHeader(storage.getUsername(), storage.getPassword());
             String svmName = storage.getSvmName();
             //Get iGroup name per host
-            for(HostVO host : accessGroup.getHostsToConnect()) {
-                String igroupName = OntapStorageUtils.getIgroupName(svmName, host.getName());
-                logger.info("deleteAccessGroup: iGroup name '{}'", igroupName);
-
-                // Get the iGroup to retrieve its UUID
-                Map<String, Object> igroupParams = Map.of(
-                        OntapStorageConstants.SVM_DOT_NAME, svmName,
-                        OntapStorageConstants.NAME, igroupName
-                );
-
-                try {
-                    OntapResponse<Igroup> igroupResponse = sanFeignClient.getIgroupResponse(authHeader, igroupParams);
-                    if (igroupResponse == null || igroupResponse.getRecords() == null || igroupResponse.getRecords().isEmpty()) {
-                        logger.warn("deleteAccessGroup: iGroup '{}' not found, may have been already deleted", igroupName);
-                        return;
-                    }
-
-                    Igroup igroup = igroupResponse.getRecords().get(0);
-                    String igroupUuid = igroup.getUuid();
-
-                    if (igroupUuid == null || igroupUuid.isEmpty()) {
-                        throw new CloudRuntimeException(" iGroup UUID is null or empty for iGroup: " + igroupName);
-                    }
-
-                    logger.info("deleteAccessGroup: Deleting iGroup '{}' with UUID '{}'", igroupName, igroupUuid);
-
-                    // Delete the iGroup using the UUID
-                    sanFeignClient.deleteIgroup(authHeader, igroupUuid);
+            if(!CollectionUtils.isEmpty(accessGroup.getHostsToConnect())) {
+                for (HostVO host : accessGroup.getHostsToConnect()) {
+                    String igroupName = OntapStorageUtils.getIgroupName(svmName, host.getName());
+                    logger.info("deleteAccessGroup: iGroup name '{}'", igroupName);
 
-                    logger.info("deleteAccessGroup: Successfully deleted iGroup '{}'", igroupName);
-
-                } catch (FeignException e) {
-                    if (e.status() == 404) {
-                        logger.warn("deleteAccessGroup: iGroup '{}' does not exist (status 404), skipping deletion", igroupName);
-                    } else {
-                        logger.error("deleteAccessGroup: FeignException occurred: Status: {}, Exception: {}", e.status(), e.getMessage(), e);
+                    // Get the iGroup to retrieve its UUID
+                    Map<String, Object> igroupParams = Map.of(
+                            OntapStorageConstants.SVM_DOT_NAME, svmName,
+                            OntapStorageConstants.NAME, igroupName
+                    );
+
+                    try {
+                        OntapResponse<Igroup> igroupResponse = sanFeignClient.getIgroupResponse(authHeader, igroupParams);
+                        if (igroupResponse == null || igroupResponse.getRecords() == null || igroupResponse.getRecords().isEmpty()) {
+                            logger.warn("deleteAccessGroup: iGroup '{}' not found, may have been already deleted", igroupName);
+                            return;
+                        }
+
+                        Igroup igroup = igroupResponse.getRecords().get(0);
+                        String igroupUuid = igroup.getUuid();
+
+                        if (igroupUuid == null || igroupUuid.isEmpty()) {
+                            throw new CloudRuntimeException(" iGroup UUID is null or empty for iGroup: " + igroupName);
+                        }
+
+                        logger.info("deleteAccessGroup: Deleting iGroup '{}' with UUID '{}'", igroupName, igroupUuid);
+
+                        // Delete the iGroup using the UUID
+                        sanFeignClient.deleteIgroup(authHeader, igroupUuid);
+
+                        logger.info("deleteAccessGroup: Successfully deleted iGroup '{}'", igroupName);
+
+                    } catch (FeignException e) {
+                        if (e.status() == 404) {
+                            logger.warn("deleteAccessGroup: iGroup '{}' does not exist (status 404), skipping deletion", igroupName);
+                        } else {
+                            logger.error("deleteAccessGroup: FeignException occurred: Status: {}, Exception: {}", e.status(), e.getMessage(), e);
+                            throw e;
+                        }
+                    } catch (Exception e) {
+                        logger.error("deleteAccessGroup: Exception occurred: {}", e.getMessage(), e);
                         throw e;
                     }
-                } catch (Exception e) {
-                    logger.error("deleteAccessGroup: Exception occurred: {}", e.getMessage(), e);
-                    throw e;
                 }
             }
         } catch (FeignException e) {

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java

@@ -35,6 +35,8 @@
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.springframework.util.Base64Utils;
+
+import java.nio.charset.StandardCharsets;
 import java.util.Map;
 
 public class OntapStorageUtils {
@@ -51,7 +53,7 @@ public class OntapStorageUtils {
      * @return
      */
     public static String generateAuthHeader (String username, String password) {
-        byte[] encodedBytes = Base64Utils.encode((username + AUTH_HEADER_COLON + password).getBytes());
+        byte[] encodedBytes = Base64Utils.encode((username + AUTH_HEADER_COLON + password).getBytes(StandardCharsets.UTF_8));
         return BASIC + StringUtils.SPACE + new String(encodedBytes);
     }