Feature/cstackex 183 pr

.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,19 +159,54 @@ 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) {
@@ -340,15 +351,15 @@ 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();
             // 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.
@@ -378,20 +389,20 @@ private boolean hasOtherActiveLuns(String host, int port, String iqn, String lun
      */
     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)) {
+        Path byPathLink = Paths.get(byPath);
+        if (!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();
+            Path realDevice = byPathLink.toRealPath();
             String devName = realDevice.getFileName().toString();
-            java.io.File deleteFile = new java.io.File("/sys/block/" + devName + "/device/delete");
+            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 (java.io.FileWriter fw = new java.io.FileWriter(deleteFile)) {
+            try (FileWriter fw = new FileWriter(deleteFile)) {
                 fw.write("1");
             }
             logger.info("Removed stale SCSI device " + devName + " for LUN /" + iqn + "/" + lun + " via sysfs");

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>