From 0407d2b6e6d70715eeb11439ea0cf6e0a64eebd7 Mon Sep 17 00:00:00 2001 From: "Jain, Rajiv" Date: Fri, 22 May 2026 17:23:48 +0530 Subject: [PATCH 1/4] CSTACKEX-191: logical access methods can have return type as primitive --- .../storage/feign/model/VolumeConcise.java | 4 +-- .../OntapPrimaryDatastoreLifecycle.java | 2 +- .../storage/service/StorageStrategy.java | 4 +-- .../storage/service/UnifiedNASStrategy.java | 6 ++-- .../storage/service/UnifiedSANStrategy.java | 31 +++++++++---------- .../storage/service/StorageStrategyTest.java | 4 +-- .../service/UnifiedSANStrategyTest.java | 17 +++++----- 7 files changed, 31 insertions(+), 37 deletions(-) diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/VolumeConcise.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/VolumeConcise.java index b76fb4f03d6b..82cea1260e10 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/VolumeConcise.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/VolumeConcise.java @@ -30,7 +30,6 @@ public class VolumeConcise { private String uuid; @JsonProperty("name") - private String name; public String getUuid() { @@ -46,5 +45,6 @@ public String getName() { } public void setName(String name) { - this.name = name; } + this.name = name; + } } diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java index 1e44ef3aaa83..d7b89de25461 100755 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java @@ -231,7 +231,7 @@ private long validateInitializeInputs(Long capacityBytes, Long podId, Long clust throw new CloudRuntimeException("Storage pool name is null or empty, cannot create primary storage"); } - if (StringUtils.isBlank(providerName )) { + if (StringUtils.isBlank(providerName)) { throw new CloudRuntimeException("Provider name is null or empty, cannot create primary storage"); } diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java index 02c201adaa13..16aa5ec3b7ec 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java @@ -595,7 +595,7 @@ public abstract JobResponse revertSnapshotForCloudStackVolume(String snapshotNam * @param values map including SVM name, LUN name, and igroup name (for SAN) or equivalent for NAS * @return map containing logical unit number for the new/existing mapping (SAN) or relevant info for NAS */ - abstract public Map enableLogicalAccess(Map values); + abstract public String enableLogicalAccess(Map values); /** * Method encapsulates the behavior based on the opted protocol in subclasses @@ -610,7 +610,7 @@ public abstract JobResponse revertSnapshotForCloudStackVolume(String snapshotNam * @param values map with SVM name, LUN name, and igroup name (for SAN) or equivalent for NAS * @return map containing logical unit number if mapping exists; otherwise null */ - abstract public Map getLogicalAccess(Map values); + abstract public String getLogicalAccess(Map values); // ── FlexVolume Snapshot accessors ──────────────────────────────────────── diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedNASStrategy.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedNASStrategy.java index f305dd6d070d..477e92630387 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedNASStrategy.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedNASStrategy.java @@ -202,7 +202,7 @@ public AccessGroup getAccessGroup(Map values) { } @Override - public Map enableLogicalAccess(Map values) { + public String enableLogicalAccess(Map values) { return null; } @@ -211,8 +211,8 @@ public void disableLogicalAccess(Map values) { } @Override - public Map getLogicalAccess(Map values) { - return Map.of(); + public String getLogicalAccess(Map values) { + return null; } private ExportPolicy createExportPolicy(String svmName, ExportPolicy policy) { diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java index 02661909d7e6..46d6a275fe04 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java @@ -383,10 +383,10 @@ public AccessGroup getAccessGroup(Map values) { } } - public Map enableLogicalAccess(Map values) { + public String enableLogicalAccess(Map values) { logger.info("enableLogicalAccess : Create LunMap"); logger.debug("enableLogicalAccess : Creating LunMap with values {} ", values); - Map response = null; + String lunNumber = null; if (values == null) { logger.error("enableLogicalAccess: LunMap creation failed. Invalid request values: null"); throw new CloudRuntimeException("Failed to create LunMap, invalid request"); @@ -435,9 +435,7 @@ public Map enableLogicalAccess(Map values) { OntapStorageConstants.IGROUP_DOT_NAME, igroupName, OntapStorageConstants.FIELDS, OntapStorageConstants.LOGICAL_UNIT_NUMBER )); - response = Map.of( - OntapStorageConstants.LOGICAL_UNIT_NUMBER, lunMapResponse.getRecords().get(0).getLogicalUnitNumber().toString() - ); + lunNumber = lunMapResponse.getRecords().get(0).getLogicalUnitNumber().toString(); } catch (Exception e) { logger.error("enableLogicalAccess: Failed to fetch LunMap details for Lun: {} and igroup: {}, Exception: {}", lunName, igroupName, e); throw new CloudRuntimeException("Failed to fetch LunMap details for Lun: " + lunName + " and igroup: " + igroupName); @@ -448,7 +446,7 @@ public Map enableLogicalAccess(Map values) { logger.error("Exception occurred while creating LunMap", e); throw new CloudRuntimeException("Failed to create LunMap: " + e.getMessage()); } - return response; + return lunNumber; } public void disableLogicalAccess(Map values) { @@ -482,8 +480,9 @@ public void disableLogicalAccess(Map values) { } // GET-only helper: fetch LUN-map and return logical unit number if it exists; otherwise return null - public Map getLogicalAccess(Map values) { + public String getLogicalAccess(Map values) { logger.info("getLogicalAccess : Fetch LunMap"); + String lunNumber = null; logger.debug("getLogicalAccess : Fetching LunMap with values {} ", values); if (values == null) { logger.error("getLogicalAccess: Invalid request values: null"); @@ -507,13 +506,12 @@ public Map getLogicalAccess(Map values) { )); if (lunMapResponse != null && lunMapResponse.getRecords() != null && !lunMapResponse.getRecords().isEmpty()) { Integer lunLogicalUnitNum = lunMapResponse.getRecords().get(0).getLogicalUnitNumber(); - String lunNumber = lunLogicalUnitNum != null ? lunLogicalUnitNum.toString() : null; - return lunNumber != null ? Map.of(OntapStorageConstants.LOGICAL_UNIT_NUMBER, lunNumber) : null; + lunNumber = lunLogicalUnitNum != null ? lunLogicalUnitNum.toString() : null; } } catch (Exception e) { logger.warn("getLogicalAccess: LunMap not found for Lun: {} and igroup: {} ({}).", lunName, igroupName, e.getMessage()); } - return null; + return lunNumber; } @Override @@ -526,9 +524,8 @@ public String ensureLunMapped(String svmName, String lunName, String accessGroup OntapStorageConstants.SVM_DOT_NAME, svmName, OntapStorageConstants.IGROUP_DOT_NAME, accessGroupName ); - Map mapResp = getLogicalAccess(getMap); - if (mapResp != null && mapResp.containsKey(OntapStorageConstants.LOGICAL_UNIT_NUMBER)) { - String lunNumber = mapResp.get(OntapStorageConstants.LOGICAL_UNIT_NUMBER); + String lunNumber = getLogicalAccess(getMap); + if (lunNumber != null) { logger.info("ensureLunMapped: Existing LunMap found for LUN [{}] in igroup [{}] with LUN number [{}]", lunName, accessGroupName, lunNumber); return lunNumber; } @@ -539,12 +536,12 @@ public String ensureLunMapped(String svmName, String lunName, String accessGroup OntapStorageConstants.SVM_DOT_NAME, svmName, OntapStorageConstants.IGROUP_DOT_NAME, accessGroupName ); - Map response = enableLogicalAccess(enableMap); - if (response == null || !response.containsKey(OntapStorageConstants.LOGICAL_UNIT_NUMBER)) { + String response = enableLogicalAccess(enableMap); + if (response == null ) { throw new CloudRuntimeException("Failed to map LUN [" + lunName + "] to iGroup [" + accessGroupName + "]"); } - logger.info("ensureLunMapped: Successfully mapped LUN [{}] to igroup [{}] with LUN number [{}]", lunName, accessGroupName, response.get(OntapStorageConstants.LOGICAL_UNIT_NUMBER)); - return response.get(OntapStorageConstants.LOGICAL_UNIT_NUMBER); + logger.info("ensureLunMapped: Successfully mapped LUN [{}] to igroup [{}] with LUN number [{}]", lunName, accessGroupName, response); + return response; } /** * Reverts a LUN to a snapshot using the ONTAP CLI-based snapshot file restore API. diff --git a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java index b859f57b37b1..41b5aac40321 100644 --- a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java +++ b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java @@ -169,7 +169,7 @@ public AccessGroup getAccessGroup(Map values) { } @Override - public Map enableLogicalAccess(Map values) { + public String enableLogicalAccess(Map values) { return null; } @@ -178,7 +178,7 @@ public void disableLogicalAccess(Map values) { } @Override - public Map getLogicalAccess(Map values) { + public String getLogicalAccess(Map values) { return null; } } diff --git a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/UnifiedSANStrategyTest.java b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/UnifiedSANStrategyTest.java index b3f2364656a7..d0f84ab0e455 100644 --- a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/UnifiedSANStrategyTest.java +++ b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/UnifiedSANStrategyTest.java @@ -516,12 +516,10 @@ void testEnableLogicalAccess_Success() { when(sanFeignClient.getLunMapResponse(eq(authHeader), anyMap())).thenReturn(response); // Execute - Map result = unifiedSANStrategy.enableLogicalAccess(values); + String result = unifiedSANStrategy.enableLogicalAccess(values); // Verify assertNotNull(result); - assertTrue(result.containsKey(OntapStorageConstants.LOGICAL_UNIT_NUMBER)); - assertEquals("0", result.get(OntapStorageConstants.LOGICAL_UNIT_NUMBER)); verify(sanFeignClient).createLunMap(eq(authHeader), eq(true), any(LunMap.class)); } @@ -550,11 +548,10 @@ void testEnableLogicalAccess_AlreadyMapped_ReturnsLunNumber() { when(sanFeignClient.getLunMapResponse(eq(authHeader), anyMap())).thenReturn(response); // Execute - Map result = unifiedSANStrategy.enableLogicalAccess(values); + String result = unifiedSANStrategy.enableLogicalAccess(values); // Verify assertNotNull(result); - assertEquals("5", result.get(OntapStorageConstants.LOGICAL_UNIT_NUMBER)); } } @@ -621,11 +618,11 @@ void testGetLogicalAccess_Success() { when(sanFeignClient.getLunMapResponse(eq(authHeader), anyMap())).thenReturn(response); // Execute - Map result = unifiedSANStrategy.getLogicalAccess(values); + String result = unifiedSANStrategy.getLogicalAccess(values); // Verify assertNotNull(result); - assertEquals("3", result.get(OntapStorageConstants.LOGICAL_UNIT_NUMBER)); + assertEquals("3", result); } } @@ -645,7 +642,7 @@ void testGetLogicalAccess_NotFound_ReturnsNull() { .thenThrow(new RuntimeException("Not found")); // Execute - Map result = unifiedSANStrategy.getLogicalAccess(values); + String result = unifiedSANStrategy.getLogicalAccess(values); // Verify assertNull(result); @@ -1671,7 +1668,7 @@ void testGetLogicalAccess_EmptyResponse_ReturnsNull() { when(sanFeignClient.getLunMapResponse(eq(authHeader), anyMap())).thenReturn(emptyResponse); - Map result = unifiedSANStrategy.getLogicalAccess(values); + String result = unifiedSANStrategy.getLogicalAccess(values); assertNull(result); } @@ -1691,7 +1688,7 @@ void testGetLogicalAccess_ExceptionThrown_ReturnsNull() { when(sanFeignClient.getLunMapResponse(eq(authHeader), anyMap())) .thenThrow(new RuntimeException("Connection failed")); - Map result = unifiedSANStrategy.getLogicalAccess(values); + String result = unifiedSANStrategy.getLogicalAccess(values); assertNull(result); } From 3e0bc4db999cc30a2747e9f28c50405dc0e4a0f7 Mon Sep 17 00:00:00 2001 From: "Jain, Rajiv" Date: Mon, 8 Jun 2026 09:51:27 +0530 Subject: [PATCH 2/4] CSTACKEX-191: adding null check for the lunResponse --- .../cloudstack/storage/service/UnifiedSANStrategy.java | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java index 46d6a275fe04..bc6c2e4d969b 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java @@ -435,7 +435,14 @@ public String enableLogicalAccess(Map values) { OntapStorageConstants.IGROUP_DOT_NAME, igroupName, OntapStorageConstants.FIELDS, OntapStorageConstants.LOGICAL_UNIT_NUMBER )); - lunNumber = lunMapResponse.getRecords().get(0).getLogicalUnitNumber().toString(); + if(lunMapResponse != null && lunMapResponse.getRecords() != null && !lunMapResponse.getRecords().isEmpty() + && lunMapResponse.getRecords().get(0).getLogicalUnitNumber() != null) { + lunNumber = lunMapResponse.getRecords().get(0).getLogicalUnitNumber().toString(); + + } else { + logger.error("enableLogicalAccess: Failed to fetch LunMap details for Lun: {} and igroup: {}. LunMap response is null or empty.", lunName, igroupName); + throw new CloudRuntimeException("Failed to fetch LunMap details for Lun: " + lunName + " and igroup: " + igroupName); + } } catch (Exception e) { logger.error("enableLogicalAccess: Failed to fetch LunMap details for Lun: {} and igroup: {}, Exception: {}", lunName, igroupName, e); throw new CloudRuntimeException("Failed to fetch LunMap details for Lun: " + lunName + " and igroup: " + igroupName); From db8f1af095bf7570e0f27adbf8288d2510799ec6 Mon Sep 17 00:00:00 2001 From: "Jain, Rajiv" Date: Tue, 9 Jun 2026 09:03:00 +0530 Subject: [PATCH 3/4] CSTACKEX-191: review comments incorporated --- .../kvm/storage/IscsiAdmStorageAdaptor.java | 5 +- .../driver/OntapPrimaryDatastoreDriver.java | 162 ++++++++++-------- .../storage/service/UnifiedSANStrategy.java | 46 ++--- 3 files changed, 114 insertions(+), 99 deletions(-) diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiAdmStorageAdaptor.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiAdmStorageAdaptor.java index 7b22c123cc78..495ae45a2896 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiAdmStorageAdaptor.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiAdmStorageAdaptor.java @@ -210,7 +210,7 @@ private void rescanIscsiSessions(String iqn, String host, int 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) { @@ -294,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); diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java index 9034a136b647..fc6db8636ac3 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java @@ -70,6 +70,7 @@ import org.apache.commons.lang3.StringUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import org.jetbrains.annotations.Nullable; import javax.inject.Inject; import java.util.ArrayList; @@ -110,7 +111,7 @@ public DataTO getTO(DataObject data) { @Override public boolean volumesRequireGrantAccessWhenUsed() { - logger.info("OntapPrimaryDatastoreDriver: volumesRequireGrantAccessWhenUsed: Called"); + logger.trace("volumesRequireGrantAccessWhenUsed invoked"); return true; } @@ -389,44 +390,7 @@ public boolean grantAccess(DataObject dataObject, Host host, DataStore dataStore if (ProtocolType.ISCSI.name().equalsIgnoreCase(details.get(OntapStorageConstants.PROTOCOL))) { // Only retrieve LUN name for iSCSI volumes - String cloudStackVolumeName = volumeDetailsDao.findDetail(volumeVO.getId(), OntapStorageConstants.LUN_DOT_NAME).getValue(); - UnifiedSANStrategy sanStrategy = (UnifiedSANStrategy) OntapStorageUtils.getStrategyByStoragePoolDetails(details); - String accessGroupName = OntapStorageUtils.getIgroupName(svmName, host.getName()); - - // Validate if Igroup exist ONTAP for this host as we may be using delete_on_unmap= true and igroup may be deleted by ONTAP automatically - Map getAccessGroupMap = Map.of( - OntapStorageConstants.NAME, accessGroupName, - OntapStorageConstants.SVM_DOT_NAME, svmName - ); - AccessGroup accessGroup = sanStrategy.getAccessGroup(getAccessGroupMap); - if(accessGroup == null || accessGroup.getIgroup() == null) { - logger.info("grantAccess: Igroup {} does not exist for the host {} : Need to create Igroup for the host ", accessGroupName, host.getName()); - // create the igroup for the host and perform lun-mapping - accessGroup = new AccessGroup(); - List hosts = new ArrayList<>(); - hosts.add((HostVO) host); - accessGroup.setHostsToConnect(hosts); - accessGroup.setStoragePoolId(storagePool.getId()); - accessGroup = sanStrategy.createAccessGroup(accessGroup); - }else{ - logger.info("grantAccess: Igroup {} already exist for the host {}: ", accessGroup.getIgroup().getName() ,host.getName()); - /* TODO Below cases will be covered later, for now they will be a pre-requisite on customer side - 1. Igroup exist with the same name but host initiator has been rempved - 2. Igroup exist with the same name but host initiator has been changed may be due to new NIC or new adapter - In both cases we need to verify current host initiator is registered in the igroup before allowing access - Incase it is not , add it and proceed for lun-mapping - */ - } - logger.info("grantAccess: Igroup {} is present now with initiators {} ", accessGroup.getIgroup().getName(), accessGroup.getIgroup().getInitiators()); - // Create or retrieve existing LUN mapping - String lunNumber = sanStrategy.ensureLunMapped(svmName, cloudStackVolumeName, accessGroupName); - - // Update volume path if changed (e.g., after migration or re-mapping) - String iscsiPath = OntapStorageConstants.SLASH + storagePool.getPath() + OntapStorageConstants.SLASH + lunNumber; - if (volumeVO.getPath() == null || !volumeVO.getPath().equals(iscsiPath)) { - volumeVO.set_iScsiName(iscsiPath); - volumeVO.setPath(iscsiPath); - } + grantAccessIscsi(host, volumeVO, details, svmName, storagePool); } else if (ProtocolType.NFS3.name().equalsIgnoreCase(details.get(OntapStorageConstants.PROTOCOL))) { // For NFS, no access grant needed - file is accessible via mount logger.debug("grantAccess: NFS volume [{}], no igroup mapping required", volumeVO.getUuid()); @@ -446,6 +410,47 @@ public boolean grantAccess(DataObject dataObject, Host host, DataStore dataStore } } + private void grantAccessIscsi(Host host, VolumeVO volumeVO, Map details, String svmName, StoragePoolVO storagePool) { + String cloudStackVolumeName = volumeDetailsDao.findDetail(volumeVO.getId(), OntapStorageConstants.LUN_DOT_NAME).getValue(); + UnifiedSANStrategy sanStrategy = (UnifiedSANStrategy) OntapStorageUtils.getStrategyByStoragePoolDetails(details); + String accessGroupName = OntapStorageUtils.getIgroupName(svmName, host.getName()); + + // Validate if Igroup exist ONTAP for this host as we may be using delete_on_unmap= true and igroup may be deleted by ONTAP automatically + Map getAccessGroupMap = Map.of( + OntapStorageConstants.NAME, accessGroupName, + OntapStorageConstants.SVM_DOT_NAME, svmName + ); + AccessGroup accessGroup = sanStrategy.getAccessGroup(getAccessGroupMap); + if(accessGroup == null || accessGroup.getIgroup() == null) { + logger.info("grantAccess: Igroup {} does not exist for the host {} : Need to create Igroup for the host ", accessGroupName, host.getName()); + // create the igroup for the host and perform lun-mapping + accessGroup = new AccessGroup(); + List hosts = new ArrayList<>(); + hosts.add((HostVO) host); + accessGroup.setHostsToConnect(hosts); + accessGroup.setStoragePoolId(storagePool.getId()); + accessGroup = sanStrategy.createAccessGroup(accessGroup); + }else{ + logger.info("grantAccess: Igroup {} already exist for the host {}: ", accessGroup.getIgroup().getName() , host.getName()); + /* TODO Below cases will be covered later, for now they will be a pre-requisite on customer side + 1. Igroup exist with the same name but host initiator has been rempved + 2. Igroup exist with the same name but host initiator has been changed may be due to new NIC or new adapter + In both cases we need to verify current host initiator is registered in the igroup before allowing access + Incase it is not , add it and proceed for lun-mapping + */ + } + logger.info("grantAccess: Igroup {} is present now with initiators {} ", accessGroup.getIgroup().getName(), accessGroup.getIgroup().getInitiators()); + // Create or retrieve existing LUN mapping + String lunNumber = sanStrategy.ensureLunMapped(svmName, cloudStackVolumeName, accessGroupName); + + // Update volume path if changed (e.g., after migration or re-mapping) + String iscsiPath = OntapStorageConstants.SLASH + storagePool.getPath() + OntapStorageConstants.SLASH + lunNumber; + if (volumeVO.getPath() == null || !volumeVO.getPath().equals(iscsiPath)) { + volumeVO.set_iScsiName(iscsiPath); + volumeVO.setPath(iscsiPath); + } + } + /** * Revokes a host's access to a volume. */ @@ -505,42 +510,61 @@ private void revokeAccessForVolume(StoragePoolVO storagePool, VolumeVO volumeVO, // Retrieve LUN name from volume details; if missing, volume may not have been fully created VolumeDetailVO lunDetail = volumeDetailsDao.findDetail(volumeVO.getId(), OntapStorageConstants.LUN_DOT_NAME); - String lunName = lunDetail != null ? lunDetail.getValue() : null; - if (lunName == null) { - logger.warn("revokeAccessForVolume: No LUN name found for volume [{}]; skipping revoke", volumeVO.getId()); - return; - } - - // Verify LUN still exists on ONTAP (may have been manually deleted) - CloudStackVolume cloudStackVolume = getCloudStackVolumeByName(storageStrategy, svmName, lunName); - if (cloudStackVolume == null || cloudStackVolume.getLun() == null || cloudStackVolume.getLun().getUuid() == null) { - logger.warn("revokeAccessForVolume: LUN for volume [{}] not found on ONTAP, skipping revoke", volumeVO.getId()); - return; - } - - // Verify igroup still exists on ONTAP - AccessGroup accessGroup = getAccessGroupByName(storageStrategy, svmName, accessGroupName); - if (accessGroup == null || accessGroup.getIgroup() == null || accessGroup.getIgroup().getUuid() == null) { - logger.warn("revokeAccessForVolume: iGroup [{}] not found on ONTAP, skipping revoke", accessGroupName); - return; - } - - // Verify host initiator is in the igroup before attempting to remove mapping - SANStrategy sanStrategy = (UnifiedSANStrategy) storageStrategy; - if (!sanStrategy.validateInitiatorInAccessGroup(host.getStorageUrl(), svmName, accessGroup.getIgroup())) { - logger.warn("revokeAccessForVolume: Initiator [{}] is not in iGroup [{}], skipping revoke", - host.getStorageUrl(), accessGroupName); - return; - } + ValidateRevoke result = getValidateRevoke(volumeVO, host, lunDetail, storageStrategy, svmName, accessGroupName); + if (result == null) return; // Remove the LUN mapping from the igroup Map disableLogicalAccessMap = new HashMap<>(); - disableLogicalAccessMap.put(OntapStorageConstants.LUN_DOT_UUID, cloudStackVolume.getLun().getUuid()); - disableLogicalAccessMap.put(OntapStorageConstants.IGROUP_DOT_UUID, accessGroup.getIgroup().getUuid()); + disableLogicalAccessMap.put(OntapStorageConstants.LUN_DOT_UUID, result.cloudStackVolume.getLun().getUuid()); + disableLogicalAccessMap.put(OntapStorageConstants.IGROUP_DOT_UUID, result.accessGroup.getIgroup().getUuid()); storageStrategy.disableLogicalAccess(disableLogicalAccessMap); logger.info("revokeAccessForVolume: Successfully revoked access to LUN [{}] for host [{}]", - lunName, host.getName()); + result.lunName, host.getName()); + } + } + + @Nullable + private ValidateRevoke getValidateRevoke(VolumeVO volumeVO, Host host, VolumeDetailVO lunDetail, StorageStrategy storageStrategy, String svmName, String accessGroupName) { + String lunName = lunDetail != null ? lunDetail.getValue() : null; + if (lunName == null) { + logger.warn("revokeAccessForVolume: No LUN name found for volume [{}]; skipping revoke", volumeVO.getId()); + return null; + } + + // Verify LUN still exists on ONTAP (may have been manually deleted) + CloudStackVolume cloudStackVolume = getCloudStackVolumeByName(storageStrategy, svmName, lunName); + if (cloudStackVolume == null || cloudStackVolume.getLun() == null || cloudStackVolume.getLun().getUuid() == null) { + logger.warn("revokeAccessForVolume: LUN for volume [{}] not found on ONTAP, skipping revoke", volumeVO.getId()); + return null; + } + + // Verify igroup still exists on ONTAP + AccessGroup accessGroup = getAccessGroupByName(storageStrategy, svmName, accessGroupName); + if (accessGroup == null || accessGroup.getIgroup() == null || accessGroup.getIgroup().getUuid() == null) { + logger.warn("revokeAccessForVolume: iGroup [{}] not found on ONTAP, skipping revoke", accessGroupName); + return null; + } + + // Verify host initiator is in the igroup before attempting to remove mapping + SANStrategy sanStrategy = (UnifiedSANStrategy) storageStrategy; + if (!sanStrategy.validateInitiatorInAccessGroup(host.getStorageUrl(), svmName, accessGroup.getIgroup())) { + logger.warn("revokeAccessForVolume: Initiator [{}] is not in iGroup [{}], skipping revoke", + host.getStorageUrl(), accessGroupName); + return null; + } + return new ValidateRevoke(lunName, cloudStackVolume, accessGroup); + } + + private static class ValidateRevoke { + public final String lunName; + public final CloudStackVolume cloudStackVolume; + public final AccessGroup accessGroup; + + public ValidateRevoke(String lunName, CloudStackVolume cloudStackVolume, AccessGroup accessGroup) { + this.lunName = lunName; + this.cloudStackVolume = cloudStackVolume; + this.accessGroup = accessGroup; } } diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java index bc6c2e4d969b..25741452428a 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java @@ -62,7 +62,7 @@ public void setOntapStorage(OntapStorage ontapStorage) { @Override public CloudStackVolume createCloudStackVolume(CloudStackVolume cloudstackVolume) { - logger.info("createCloudStackVolume : Creating Lun with cloudstackVolume request {} ", cloudstackVolume); + logger.trace("createCloudStackVolume : Creating Lun with cloudstackVolume request {} ", cloudstackVolume); if (cloudstackVolume == null || cloudstackVolume.getLun() == null) { logger.error("createCloudStackVolume: LUN creation failed. Invalid request: {}", cloudstackVolume); throw new CloudRuntimeException(" Failed to create Lun, invalid request"); @@ -79,7 +79,6 @@ public CloudStackVolume createCloudStackVolume(CloudStackVolume cloudstackVolume } Lun lun = createdLun.getRecords().get(0); logger.debug("createCloudStackVolume: LUN created successfully. Lun: {}", lun); - logger.info("createCloudStackVolume: LUN created successfully. LunName: {}", lun.getName()); CloudStackVolume createdCloudStackVolume = new CloudStackVolume(); createdCloudStackVolume.setLun(lun); @@ -105,7 +104,7 @@ public void deleteCloudStackVolume(CloudStackVolume cloudstackVolume) { logger.error("deleteCloudStackVolume: Lun deletion failed. Invalid request: {}", cloudstackVolume); throw new CloudRuntimeException(" Failed to delete Lun, invalid request"); } - logger.info("deleteCloudStackVolume : Deleting Lun: {}", cloudstackVolume.getLun().getName()); + logger.trace("deleteCloudStackVolume : Deleting Lun: {}", cloudstackVolume.getLun().getName()); try { String authHeader = OntapStorageUtils.generateAuthHeader(storage.getUsername(), storage.getPassword()); Map queryParams = Map.of("allow_delete_while_mapped", "true"); @@ -118,7 +117,7 @@ public void deleteCloudStackVolume(CloudStackVolume cloudstackVolume) { } throw feignEx; } - logger.info("deleteCloudStackVolume: Lun deleted successfully. LunName: {}", cloudstackVolume.getLun().getName()); + logger.trace("deleteCloudStackVolume: Lun deleted successfully. LunName: {}", cloudstackVolume.getLun().getName()); } catch (Exception e) { logger.error("Exception occurred while deleting Lun: {}, Exception: {}", cloudstackVolume.getLun().getName(), e.getMessage()); throw new CloudRuntimeException("Failed to delete Lun: " + e.getMessage()); @@ -130,8 +129,7 @@ public void copyCloudStackVolume(CloudStackVolume cloudstackVolume) {} @Override public CloudStackVolume getCloudStackVolume(Map values) { - logger.info("getCloudStackVolume : fetching Lun"); - logger.debug("getCloudStackVolume : fetching Lun with params {} ", values); + logger.trace("getCloudStackVolume : fetching Lun with params {} ", values); if (values == null || values.isEmpty()) { logger.error("getCloudStackVolume: get Lun failed. Invalid request: {}", values); throw new CloudRuntimeException(" get Lun Failed, invalid request"); @@ -152,7 +150,6 @@ public CloudStackVolume getCloudStackVolume(Map values) { } Lun lun = lunResponse.getRecords().get(0); logger.debug("getCloudStackVolume: Lun Details : {}", lun); - logger.info("getCloudStackVolume: Fetched the Lun successfully. LunName: {}", lun.getName()); CloudStackVolume cloudStackVolume = new CloudStackVolume(); cloudStackVolume.setLun(lun); @@ -172,7 +169,7 @@ public CloudStackVolume getCloudStackVolume(Map values) { @Override public AccessGroup createAccessGroup(AccessGroup accessGroup) { - logger.debug("createAccessGroup : Creating Igroup with access group request {} ", accessGroup); + logger.trace("createAccessGroup : Creating Igroup with access group request {} ", accessGroup); if (accessGroup == null) { logger.error("createAccessGroup: Igroup creation failed. Invalid request: {}", accessGroup); throw new CloudRuntimeException("Failed to create Igroup, invalid request"); @@ -188,7 +185,7 @@ public AccessGroup createAccessGroup(AccessGroup accessGroup) { String igroupName = null; try { Map dataStoreDetails = storagePoolDetailsDao.listDetailsKeyPairs(accessGroup.getStoragePoolId()); - logger.debug("createAccessGroup: Successfully fetched datastore details."); + logger.trace("createAccessGroup: Successfully fetched datastore details."); // Generate Igroup request Igroup igroupRequest = new Igroup(); @@ -239,7 +236,6 @@ public AccessGroup createAccessGroup(AccessGroup accessGroup) { throw feignEx; } - logger.info("createAccessGroup: createdIgroup: {}", createdIgroup); logger.debug("createAccessGroup: createdIgroup Records: {}", createdIgroup.getRecords()); if (createdIgroup.getRecords() == null || createdIgroup.getRecords().isEmpty()) { logger.error("createAccessGroup: Igroup creation failed for Igroup Name {}", igroupName); @@ -247,10 +243,9 @@ public AccessGroup createAccessGroup(AccessGroup accessGroup) { } Igroup igroup = createdIgroup.getRecords().get(0); logger.debug("createAccessGroup: Successfully extracted igroup from response: {}", igroup); - logger.info("createAccessGroup: Igroup created successfully. IgroupName: {}", igroup.getName()); createdAccessGroup.setIgroup(igroup); - logger.debug("createAccessGroup: Returning createdAccessGroup"); + logger.trace("createAccessGroup: Returning createdAccessGroup"); return createdAccessGroup; } catch (Exception e) { logger.error("Exception occurred while creating Igroup: {}, Exception: {}", igroupName, e.getMessage(), e); @@ -260,7 +255,7 @@ public AccessGroup createAccessGroup(AccessGroup accessGroup) { @Override public void deleteAccessGroup(AccessGroup accessGroup) { - logger.info("deleteAccessGroup: Deleting iGroup"); + logger.trace("deleteAccessGroup: Deleting iGroup"); if (accessGroup == null) { logger.error("deleteAccessGroup: Igroup deletion failed. Invalid request: {}", accessGroup); @@ -335,7 +330,7 @@ private boolean validateProtocolSupport(List hosts, ProtocolType protoco return false; } } - logger.info("validateProtocolSupportAndFetchHostsIdentifier: All hosts support the protocol: " + protocolType.name()); + logger.trace("validateProtocolSupportAndFetchHostsIdentifier: All hosts support the protocol: " + protocolType.name()); return true; } @@ -346,8 +341,7 @@ public AccessGroup updateAccessGroup(AccessGroup accessGroup) { @Override public AccessGroup getAccessGroup(Map values) { - logger.info("getAccessGroup : fetch Igroup"); - logger.debug("getAccessGroup : fetching Igroup with params {} ", values); + logger.trace("getAccessGroup : fetching Igroup with params {} ", values); if (values == null || values.isEmpty()) { logger.error("getAccessGroup: get Igroup failed. Invalid request: {}", values); throw new CloudRuntimeException("get Igroup Failed, invalid request"); @@ -384,8 +378,7 @@ public AccessGroup getAccessGroup(Map values) { } public String enableLogicalAccess(Map values) { - logger.info("enableLogicalAccess : Create LunMap"); - logger.debug("enableLogicalAccess : Creating LunMap with values {} ", values); + logger.trace("enableLogicalAccess : Creating LunMap with values {} ", values); String lunNumber = null; if (values == null) { logger.error("enableLogicalAccess: LunMap creation failed. Invalid request values: null"); @@ -447,8 +440,7 @@ public String enableLogicalAccess(Map values) { logger.error("enableLogicalAccess: Failed to fetch LunMap details for Lun: {} and igroup: {}, Exception: {}", lunName, igroupName, e); throw new CloudRuntimeException("Failed to fetch LunMap details for Lun: " + lunName + " and igroup: " + igroupName); } - logger.debug("enableLogicalAccess: LunMap created successfully, LunMap: {}", lunMapResponse.getRecords().get(0)); - logger.info("enableLogicalAccess: LunMap created successfully."); + logger.trace("enableLogicalAccess: LunMap created successfully, LunMap: {}", lunMapResponse.getRecords().get(0)); } catch (Exception e) { logger.error("Exception occurred while creating LunMap", e); throw new CloudRuntimeException("Failed to create LunMap: " + e.getMessage()); @@ -457,8 +449,7 @@ public String enableLogicalAccess(Map values) { } public void disableLogicalAccess(Map values) { - logger.info("disableLogicalAccess : Delete LunMap"); - logger.debug("disableLogicalAccess : Deleting LunMap with values {} ", values); + logger.trace("disableLogicalAccess : Deleting LunMap with values {} ", values); if (values == null) { logger.error("disableLogicalAccess: LunMap deletion failed. Invalid request values: null"); throw new CloudRuntimeException(" Failed to delete LunMap, invalid request"); @@ -488,9 +479,8 @@ public void disableLogicalAccess(Map values) { // GET-only helper: fetch LUN-map and return logical unit number if it exists; otherwise return null public String getLogicalAccess(Map values) { - logger.info("getLogicalAccess : Fetch LunMap"); String lunNumber = null; - logger.debug("getLogicalAccess : Fetching LunMap with values {} ", values); + logger.trace("getLogicalAccess : Fetching LunMap with values {} ", values); if (values == null) { logger.error("getLogicalAccess: Invalid request values: null"); throw new CloudRuntimeException(" Invalid request"); @@ -523,7 +513,7 @@ public String getLogicalAccess(Map values) { @Override public String ensureLunMapped(String svmName, String lunName, String accessGroupName) { - logger.info("ensureLunMapped: Ensuring LUN [{}] is mapped to igroup [{}] on SVM [{}]", lunName, accessGroupName, svmName); + logger.trace("ensureLunMapped: Ensuring LUN [{}] is mapped to igroup [{}] on SVM [{}]", lunName, accessGroupName, svmName); // Check existing map first Map getMap = Map.of( @@ -547,7 +537,7 @@ public String ensureLunMapped(String svmName, String lunName, String accessGroup if (response == null ) { throw new CloudRuntimeException("Failed to map LUN [" + lunName + "] to iGroup [" + accessGroupName + "]"); } - logger.info("ensureLunMapped: Successfully mapped LUN [{}] to igroup [{}] with LUN number [{}]", lunName, accessGroupName, response); + logger.trace("ensureLunMapped: Successfully mapped LUN [{}] to igroup [{}] with LUN number [{}]", lunName, accessGroupName, response); return response; } /** @@ -571,7 +561,7 @@ public String ensureLunMapped(String svmName, String lunName, String accessGroup public JobResponse revertSnapshotForCloudStackVolume(String snapshotName, String flexVolUuid, String snapshotUuid, String volumePath, String lunUuid, String flexVolName) { - logger.info("revertSnapshotForCloudStackVolume [iSCSI]: Restoring LUN [{}] from snapshot [{}] on FlexVol [{}]", + logger.trace("revertSnapshotForCloudStackVolume [iSCSI]: Restoring LUN [{}] from snapshot [{}] on FlexVol [{}]", volumePath, snapshotName, flexVolName); if (snapshotName == null || snapshotName.isEmpty()) { @@ -594,7 +584,7 @@ public JobResponse revertSnapshotForCloudStackVolume(String snapshotName, String CliSnapshotRestoreRequest restoreRequest = new CliSnapshotRestoreRequest( svmName, flexVolName, snapshotName, ontapLunPath); - logger.info("revertSnapshotForCloudStackVolume: Calling CLI file restore API with vserver={}, volume={}, snapshot={}, path={}", + logger.trace("revertSnapshotForCloudStackVolume: Calling CLI file restore API with vserver={}, volume={}, snapshot={}, path={}", svmName, flexVolName, snapshotName, ontapLunPath); return getSnapshotFeignClient().restoreFileFromSnapshotCli(authHeader, restoreRequest); From d1a795b9b2b0fbf8e2bb0e13cef0233e55e036cf Mon Sep 17 00:00:00 2001 From: "Jain, Rajiv" Date: Thu, 11 Jun 2026 10:43:12 +0530 Subject: [PATCH 4/4] CSTACKEX-191: addressing copilot comments --- .../storage/driver/OntapPrimaryDatastoreDriver.java | 2 +- .../apache/cloudstack/storage/service/StorageStrategy.java | 4 ++-- .../apache/cloudstack/storage/service/UnifiedSANStrategy.java | 2 ++ .../cloudstack/storage/service/UnifiedSANStrategyTest.java | 3 ++- 4 files changed, 7 insertions(+), 4 deletions(-) diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java index fc6db8636ac3..04996b74d2a5 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java @@ -433,7 +433,7 @@ private void grantAccessIscsi(Host host, VolumeVO volumeVO, Map }else{ logger.info("grantAccess: Igroup {} already exist for the host {}: ", accessGroup.getIgroup().getName() , host.getName()); /* TODO Below cases will be covered later, for now they will be a pre-requisite on customer side - 1. Igroup exist with the same name but host initiator has been rempved + 1. Igroup exist with the same name but host initiator has been removed 2. Igroup exist with the same name but host initiator has been changed may be due to new NIC or new adapter In both cases we need to verify current host initiator is registered in the igroup before allowing access Incase it is not , add it and proceed for lun-mapping diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java index 16aa5ec3b7ec..8fdde0029127 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java @@ -593,7 +593,7 @@ public abstract JobResponse revertSnapshotForCloudStackVolume(String snapshotNam * //TODO for NFS 3.0 and NFS 4.1 protocols (e.g., export rule management) * //TODO for Nvme/TCP and Nvme/FC protocols * @param values map including SVM name, LUN name, and igroup name (for SAN) or equivalent for NAS - * @return map containing logical unit number for the new/existing mapping (SAN) or relevant info for NAS + * @return String containing logical unit number for the new/existing mapping (SAN) or relevant info for NAS */ abstract public String enableLogicalAccess(Map values); @@ -608,7 +608,7 @@ public abstract JobResponse revertSnapshotForCloudStackVolume(String snapshotNam * Method encapsulates the behavior based on the opted protocol in subclasses * lunMap lookup for iSCSI/FC protocols (GET-only, no side-effects) * @param values map with SVM name, LUN name, and igroup name (for SAN) or equivalent for NAS - * @return map containing logical unit number if mapping exists; otherwise null + * @return String containing logical unit number if mapping exists; otherwise null */ abstract public String getLogicalAccess(Map values); diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java index 25741452428a..5f1ac265fc50 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java @@ -377,6 +377,7 @@ public AccessGroup getAccessGroup(Map values) { } } + @Override public String enableLogicalAccess(Map values) { logger.trace("enableLogicalAccess : Creating LunMap with values {} ", values); String lunNumber = null; @@ -478,6 +479,7 @@ public void disableLogicalAccess(Map values) { } // GET-only helper: fetch LUN-map and return logical unit number if it exists; otherwise return null + @Override public String getLogicalAccess(Map values) { String lunNumber = null; logger.trace("getLogicalAccess : Fetching LunMap with values {} ", values); diff --git a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/UnifiedSANStrategyTest.java b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/UnifiedSANStrategyTest.java index d0f84ab0e455..1c0c84ef91dd 100644 --- a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/UnifiedSANStrategyTest.java +++ b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/UnifiedSANStrategyTest.java @@ -520,7 +520,7 @@ void testEnableLogicalAccess_Success() { // Verify assertNotNull(result); - + assertEquals("0", result); verify(sanFeignClient).createLunMap(eq(authHeader), eq(true), any(LunMap.class)); } } @@ -552,6 +552,7 @@ void testEnableLogicalAccess_AlreadyMapped_ReturnsLunNumber() { // Verify assertNotNull(result); + assertEquals("5", result); } }