Skip to content

CSTACKEX-367: Remove LUN-map logical unit number dead code #138

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sandeeplocharla wants to merge 1 commit into
base: main
Choose a base branch
from
+74 −186
Open

CSTACKEX-367: Remove LUN-map logical unit number dead code #138

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions ...ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1325,10 +1325,10 @@ private void grantAccessIscsi(Host host, VolumeVO volumeVO, Map<String, String>
ensureAccessGroupForHost(sanStrategy, host, storagePool, svmName, accessGroupName);

// Create or retrieve existing LUN mapping.
String lunNumber = sanStrategy.ensureLunMapped(svmName, cloudStackVolumeName, accessGroupName);
sanStrategy.ensureLunMapped(svmName, cloudStackVolumeName, accessGroupName);
String lunWwid = resolveVolumeLunWwid(sanStrategy, svmName, cloudStackVolumeName, volumeVO.getId());
logger.debug("grantAccessIscsi: LUN [{}] mapped to igroup [{}] as logical unit number [{}], WWID [{}]",
cloudStackVolumeName, accessGroupName, lunNumber, lunWwid);
logger.debug("grantAccessIscsi: LUN [{}] mapped to igroup [{}], WWID [{}]",
cloudStackVolumeName, accessGroupName, lunWwid);

// Update volume path if changed (e.g. the WWID was not yet known for a pre-existing volume)
String iscsiPath = buildIscsiPath(storagePool, lunWwid);
Expand All @@ -1343,7 +1343,7 @@ private void grantAccessIscsi(Host host, VolumeVO volumeVO, Map<String, String>
*
* <p>Called by the framework from {@code copyTemplateToManagedTemplateVolume} just before it
* issues the {@code CopyCommand}. That method reads {@code managedStoreTarget} from the pool
* details <em>before</em> this call, when the LUN number does not exist yet, so the stale
* details <em>before</em> this call, when the LUN WWID is not yet known, so the stale
* value is corrected here. The datastore details are re-read when the command is built, so the
* update lands in time.</p>
*/
Expand All @@ -1365,13 +1365,12 @@ private void grantAccessTemplate(TemplateInfo templateInfo, Host host, DataStore

boolean lunMapped = false;
try {
String lunNumber = sanStrategy.ensureLunMapped(svmName, lunName, accessGroupName);
sanStrategy.ensureLunMapped(svmName, lunName, accessGroupName);
lunMapped = true;
// The template cache LUN has no volume_details row to cache the WWID in, and this runs
// once per template per pool, so it is fetched from the array each time.
String lunWwid = fetchLunWwid(sanStrategy, svmName, lunName);
logger.debug("grantAccessTemplate: template cache LUN [{}] mapped as logical unit number [{}], WWID [{}]",
lunName, lunNumber, lunWwid);
logger.debug("grantAccessTemplate: template cache LUN [{}] mapped, WWID [{}]", lunName, lunWwid);
String iscsiPath = buildIscsiPath(storagePool, lunWwid);

templatePoolRef.setInstallPath(iscsiPath);
Expand Down
6 changes: 2 additions & 4 deletions ...storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/SANStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,15 +33,13 @@ public SANStrategy(OntapStorage ontapStorage) {

/**
* Ensures the LUN is mapped to the specified access group (igroup).
* If a mapping already exists, returns the existing LUN number.
* If not, creates a new mapping and returns the assigned LUN number.
* If a mapping already exists, nothing is done; otherwise a new mapping is created.
*
* @param svmName the SVM name
* @param lunName the LUN name
* @param accessGroupName the igroup name
* @return the logical unit number as a String
*/
public abstract String ensureLunMapped(String svmName, String lunName, String accessGroupName);
public abstract void ensureLunMapped(String svmName, String lunName, String accessGroupName);

/**
* Validates that the host initiator is present in the access group (igroup).
Expand Down
7 changes: 3 additions & 4 deletions ...age/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1035,9 +1035,8 @@ 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 String containing logical unit number for the new/existing mapping (SAN) or relevant info for NAS
*/
abstract public String enableLogicalAccess(Map<String,String> values);
abstract public void enableLogicalAccess(Map<String,String> values);

/**
* Method encapsulates the behavior based on the opted protocol in subclasses
Expand All @@ -1050,9 +1049,9 @@ 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 String containing logical unit number if mapping exists; otherwise null
* @return true if the mapping exists; otherwise false
*/
abstract public String getLogicalAccess(Map<String, String> values);
abstract public boolean getLogicalAccess(Map<String, String> values);

// ── FlexVolume Snapshot accessors ────────────────────────────────────────

Expand Down
7 changes: 3 additions & 4 deletions .../volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedNASStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -500,17 +500,16 @@ public AccessGroup getAccessGroup(Map<String, String> values) {
}

@Override
public String enableLogicalAccess(Map<String, String> values) {
return null;
public void enableLogicalAccess(Map<String, String> values) {
}

@Override
public void disableLogicalAccess(Map<String, String> values) {
}

@Override
public String getLogicalAccess(Map<String, String> values) {
return null;
public boolean getLogicalAccess(Map<String, String> values) {
return false;
}

private ExportPolicy createExportPolicy(String svmName, ExportPolicy policy) {
Expand Down
69 changes: 14 additions & 55 deletions .../volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -652,9 +652,8 @@ public AccessGroup getAccessGroup(Map<String, String> values) {
}

@Override
public String enableLogicalAccess(Map<String, String> values) {
public void enableLogicalAccess(Map<String, String> 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");
throw new CloudRuntimeException("Failed to create LunMap, invalid request");
Expand Down Expand Up @@ -693,34 +692,11 @@ public String enableLogicalAccess(Map<String, String> values) {
throw feignEx;
}
}
// Get the LunMap poolDetails
OntapResponse<LunMap> lunMapResponse = null;
try {
lunMapResponse = sanFeignClient.getLunMapResponse(authHeader,
Map.of(
OntapStorageConstants.SVM_DOT_NAME, svmName,
OntapStorageConstants.LUN_DOT_NAME, lunName,
OntapStorageConstants.IGROUP_DOT_NAME, igroupName,
OntapStorageConstants.FIELDS, OntapStorageConstants.LOGICAL_UNIT_NUMBER
));
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 poolDetails for Lun: {} and igroup: {}. LunMap response is null or empty.", lunName, igroupName);
throw new CloudRuntimeException("Failed to fetch LunMap poolDetails for Lun: " + lunName + " and igroup: " + igroupName);
}
} catch (Exception e) {
logger.error("enableLogicalAccess: Failed to fetch LunMap poolDetails for Lun: {} and igroup: {}, Exception: {}", lunName, igroupName, e);
throw new CloudRuntimeException("Failed to fetch LunMap poolDetails for Lun: " + lunName + " and igroup: " + igroupName);
}
logger.trace("enableLogicalAccess: LunMap created successfully, LunMap: {}", lunMapResponse.getRecords().get(0));
logger.trace("enableLogicalAccess: LunMap ensured for Lun: {} and igroup: {}", lunName, igroupName);
} catch (Exception e) {
logger.error("Exception occurred while creating LunMap", e);
throw new CloudRuntimeException("Failed to create LunMap: " + e.getMessage());
}
return lunNumber;
}

public void disableLogicalAccess(Map<String, String> values) {
Expand Down Expand Up @@ -752,10 +728,9 @@ public void disableLogicalAccess(Map<String, String> values) {
}
}

// GET-only helper: fetch LUN-map and return logical unit number if it exists; otherwise return null
// GET-only helper: returns true if the LUN-map exists; otherwise false
@Override
public String getLogicalAccess(Map<String, String> values) {
String lunNumber = null;
public boolean getLogicalAccess(Map<String, String> values) {
logger.trace("getLogicalAccess : Fetching LunMap with values {} ", values);
if (values == null) {
logger.error("getLogicalAccess: Invalid request values: null");
Expand All @@ -774,47 +749,31 @@ public String getLogicalAccess(Map<String, String> values) {
Map.of(
OntapStorageConstants.SVM_DOT_NAME, svmName,
OntapStorageConstants.LUN_DOT_NAME, lunName,
OntapStorageConstants.IGROUP_DOT_NAME, igroupName,
OntapStorageConstants.FIELDS, OntapStorageConstants.LOGICAL_UNIT_NUMBER
OntapStorageConstants.IGROUP_DOT_NAME, igroupName
));
if (lunMapResponse != null && lunMapResponse.getRecords() != null && !lunMapResponse.getRecords().isEmpty()) {
Integer lunLogicalUnitNum = lunMapResponse.getRecords().get(0).getLogicalUnitNumber();
lunNumber = lunLogicalUnitNum != null ? lunLogicalUnitNum.toString() : null;
}
return lunMapResponse != null && lunMapResponse.getRecords() != null && !lunMapResponse.getRecords().isEmpty();
} catch (Exception e) {
logger.warn("getLogicalAccess: LunMap not found for Lun: {} and igroup: {} ({}).", lunName, igroupName, e.getMessage());
return false;
}
return lunNumber;
}

@Override
public String ensureLunMapped(String svmName, String lunName, String accessGroupName) {
public void ensureLunMapped(String svmName, String lunName, String accessGroupName) {
logger.trace("ensureLunMapped: Ensuring LUN [{}] is mapped to igroup [{}] on SVM [{}]", lunName, accessGroupName, svmName);

// Check existing map first
Map<String, String> getMap = Map.of(
Map<String, String> mapRequest = Map.of(
OntapStorageConstants.LUN_DOT_NAME, lunName,
OntapStorageConstants.SVM_DOT_NAME, svmName,
OntapStorageConstants.IGROUP_DOT_NAME, accessGroupName
);
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;
if (getLogicalAccess(mapRequest)) {
logger.info("ensureLunMapped: Existing LunMap found for LUN [{}] in igroup [{}]", lunName, accessGroupName);
return;
}

// Create if not exists
Map<String, String> enableMap = Map.of(
OntapStorageConstants.LUN_DOT_NAME, lunName,
OntapStorageConstants.SVM_DOT_NAME, svmName,
OntapStorageConstants.IGROUP_DOT_NAME, accessGroupName
);
String response = enableLogicalAccess(enableMap);
if (response == null ) {
throw new CloudRuntimeException("Failed to map LUN [" + lunName + "] to iGroup [" + accessGroupName + "]");
}
logger.trace("ensureLunMapped: Successfully mapped LUN [{}] to igroup [{}] with LUN number [{}]", lunName, accessGroupName, response);
return response;
enableLogicalAccess(mapRequest);
logger.trace("ensureLunMapped: Successfully mapped LUN [{}] to igroup [{}]", lunName, accessGroupName);
}
/**
* Reverts a LUN to a snapshot using the ONTAP CLI-based snapshot file restore API.
Expand Down
1 change: 0 additions & 1 deletion ...volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageConstants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,6 @@ public class OntapStorageConstants {
public static final String QOS_POLICY_NAME_IOPS = "iops_";
public static final String UUID = "uuid";
public static final String QOS_POLICY_OBJECT_COUNT_FIELDS = "uuid,name,object_count";
public static final String LOGICAL_UNIT_NUMBER = "logical_unit_number";
public static final String IGROUP_DOT_NAME = "igroup.name";
public static final String IGROUP_DOT_UUID = "igroup.uuid";
public static final String UNDERSCORE = "_";
Expand Down
12 changes: 6 additions & 6 deletions ...p/src/test/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriverTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -577,7 +577,7 @@ void testGrantAccess_ClusterScope_Success() {
.thenReturn("igroup1");

when(sanStrategy.getAccessGroup(any())).thenReturn(existingAccessGroup);
when(sanStrategy.ensureLunMapped(anyString(), anyString(), anyString())).thenReturn("0");
doNothing().when(sanStrategy).ensureLunMapped(anyString(), anyString(), anyString());
when(sanStrategy.getLunSerialNumber(anyString(), anyString())).thenReturn(LUN_SERIAL_NUMBER);

// Execute
Expand Down Expand Up @@ -631,7 +631,7 @@ void testGrantAccess_IgroupNotFound_CreatesNewIgroup() {

when(sanStrategy.getAccessGroup(any())).thenReturn(null);
when(sanStrategy.createAccessGroup(any())).thenReturn(createdAccessGroup);
when(sanStrategy.ensureLunMapped(anyString(), anyString(), anyString())).thenReturn("0");
doNothing().when(sanStrategy).ensureLunMapped(anyString(), anyString(), anyString());
when(sanStrategy.getLunSerialNumber(anyString(), anyString())).thenReturn(LUN_SERIAL_NUMBER);

// Execute
Expand Down Expand Up @@ -1519,7 +1519,7 @@ void testGrantAccess_Template_WritesInstallPathAndRefreshesStoreTarget() {
utilityMock.when(() -> OntapStorageUtils.getIgroupName(anyString(), anyString())).thenReturn("igroup1");

when(sanStrategy.getAccessGroup(any())).thenReturn(existingAccessGroup);
when(sanStrategy.ensureLunMapped(eq("svm1"), eq("/vol/vol1/cs_tmpl_50"), eq("igroup1"))).thenReturn("3");
doNothing().when(sanStrategy).ensureLunMapped(eq("svm1"), eq("/vol/vol1/cs_tmpl_50"), eq("igroup1"));
when(sanStrategy.getLunSerialNumber(eq("svm1"), eq("/vol/vol1/cs_tmpl_50"))).thenReturn(LUN_SERIAL_NUMBER);

assertTrue(driver.grantAccess(templateInfo, host, primaryDataStore));
Expand Down Expand Up @@ -1575,7 +1575,7 @@ void testGrantAccess_Template_UnmapsWhenInstallPathUpdateFails() {
utilityMock.when(() -> OntapStorageUtils.getIgroupName(anyString(), anyString())).thenReturn("igroup1");

when(sanStrategy.getAccessGroup(any())).thenReturn(existingAccessGroup);
when(sanStrategy.ensureLunMapped(eq("svm1"), eq("/vol/vol1/cs_tmpl_50"), eq("igroup1"))).thenReturn("3");
doNothing().when(sanStrategy).ensureLunMapped(eq("svm1"), eq("/vol/vol1/cs_tmpl_50"), eq("igroup1"));
when(sanStrategy.getLunSerialNumber(eq("svm1"), eq("/vol/vol1/cs_tmpl_50"))).thenReturn(LUN_SERIAL_NUMBER);
when(sanStrategy.getCloudStackVolume(argThat(map ->
map != null && "/vol/vol1/cs_tmpl_50".equals(map.get("name")))))
Expand Down Expand Up @@ -2874,7 +2874,7 @@ void testGrantAccess_Template_CreatesIgroupWhenMissing() {

when(sanStrategy.getAccessGroup(any())).thenReturn(null);
when(sanStrategy.createAccessGroup(any())).thenReturn(createdAccessGroup);
when(sanStrategy.ensureLunMapped(eq("svm1"), eq("/vol/vol1/cs_tmpl_50"), eq("igroup1"))).thenReturn("3");
doNothing().when(sanStrategy).ensureLunMapped(eq("svm1"), eq("/vol/vol1/cs_tmpl_50"), eq("igroup1"));
when(sanStrategy.getLunSerialNumber(eq("svm1"), eq("/vol/vol1/cs_tmpl_50"))).thenReturn(LUN_SERIAL_NUMBER);

assertTrue(driver.grantAccess(templateInfo, hostVo, primaryDataStore));
Expand Down Expand Up @@ -3749,7 +3749,7 @@ void testGrantAccess_TemporarySnapshotCopy_SyncsIscsiPathOnInMemoryVolume() {
utilityMock.when(() -> OntapStorageUtils.getStrategyByStoragePoolDetails(storagePoolDetails)).thenReturn(sanStrategy);
utilityMock.when(() -> OntapStorageUtils.getIgroupName(anyString(), anyString())).thenReturn("igroup1");
when(sanStrategy.getAccessGroup(any())).thenReturn(existingAccessGroup);
when(sanStrategy.ensureLunMapped(anyString(), anyString(), anyString())).thenReturn("0");
doNothing().when(sanStrategy).ensureLunMapped(anyString(), anyString(), anyString());
when(sanStrategy.getLunSerialNumber(anyString(), anyString())).thenReturn(LUN_SERIAL_NUMBER);

assertTrue(driver.grantAccess(volumeInfo, host, dataStore));
Expand Down
7 changes: 3 additions & 4 deletions ...volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,17 +203,16 @@ public AccessGroup getAccessGroup(Map<String, String> values) {
}

@Override
public String enableLogicalAccess(Map<String, String> values) {
return null;
public void enableLogicalAccess(Map<String, String> values) {
}

@Override
public void disableLogicalAccess(Map<String, String> values) {
}

@Override
public String getLogicalAccess(Map<String, String> values) {
return null;
public boolean getLogicalAccess(Map<String, String> values) {
return false;
}
}

Expand Down
Loading
Loading