Skip to content

Commit 199c463

Browse files
Srivastava, PiyushSrivastava, Piyush
Srivastava, Piyush
authored and
Srivastava, Piyush
committed
sync: fixed review comments without test class changes
1 parent c86d377 commit 199c463

18 files changed

Lines changed: 237 additions & 261 deletions

File tree

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -328,12 +328,12 @@ public StrategyPriority canHandle(Long vmId, Long rootPoolId, boolean snapshotMe
328328
for (VolumeVO volume : volumes) {
329329
StoragePoolVO storagePoolVO = storagePool.findById(volume.getPoolId());
330330
if (storagePoolVO.isManaged() && ONTAP_PROVIDER_NAME.equals(storagePoolVO.getStorageProviderName())) {
331-
logger.debug(String.format("%s as the VM has a volume on ONTAP managed storage pool [%s]. " +
332-
"ONTAP managed storage has its own dedicated VM snapshot strategy.", cantHandleLog, storagePoolVO.getName()));
331+
logger.debug(" {} as the VM has a volume on ONTAP managed storage pool [{}]. " +
332+
"ONTAP managed storage has its own dedicated VM snapshot strategy.", cantHandleLog, storagePoolVO.getName());
333333
return StrategyPriority.CANT_HANDLE;
334334
}
335335
if (!supportedStoragePoolTypes.contains(storagePoolVO.getPoolType())) {
336-
logger.debug(String.format("%s as the VM has a volume that is in a storage with unsupported type [%s].", cantHandleLog, storagePoolVO.getPoolType()));
336+
logger.debug("{} as the VM has a volume that is in a storage with unsupported type [{}].", cantHandleLog, storagePoolVO.getPoolType());
337337
return StrategyPriority.CANT_HANDLE;
338338
}
339339
List<SnapshotVO> snapshots = snapshotDao.listByVolumeIdAndTypeNotInAndStateNotRemoved(volume.getId(), Snapshot.Type.GROUP);

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

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,7 @@
8383
import org.apache.cloudstack.storage.to.TemplateObjectTO;
8484
import org.apache.cloudstack.storage.to.VolumeObjectTO;
8585
import org.apache.commons.collections.CollectionUtils;
86+
import org.apache.commons.lang3.ObjectUtils;
8687
import org.apache.commons.lang3.StringUtils;
8788
import org.apache.logging.log4j.Logger;
8889
import org.apache.logging.log4j.LogManager;
@@ -1342,13 +1343,9 @@ private void createManagedVolumeCopyTemplateAsync(VolumeInfo volumeInfo, Primary
13421343
grantAccess(volumeInfo, destHost, primaryDataStore);
13431344
volumeInfo = volFactory.getVolume(volumeInfo.getId(), primaryDataStore);
13441345
// For Netapp ONTAP iscsiName or Lun path is available only after grantAccess
1345-
String managedStoreTarget = volumeInfo.get_iScsiName() != null ? volumeInfo.get_iScsiName() : volumeInfo.getUuid();
1346+
String managedStoreTarget = ObjectUtils.defaultIfNull(volumeInfo.get_iScsiName(), volumeInfo.getUuid());
13461347
details.put(PrimaryDataStore.MANAGED_STORE_TARGET, managedStoreTarget);
13471348
primaryDataStore.setDetails(details);
1348-
// Update destTemplateInfo with the iSCSI path from volumeInfo
1349-
if (destTemplateInfo instanceof TemplateObject) {
1350-
((TemplateObject)destTemplateInfo).setInstallPath(volumeInfo.getPath());
1351-
}
13521349

13531350
try {
13541351
motionSrv.copyAsync(srcTemplateInfo, destTemplateInfo, destHost, caller);

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

Lines changed: 56 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
// under the License.
1717
package com.cloud.hypervisor.kvm.storage;
1818

19+
import java.io.File;
1920
import java.util.HashMap;
2021
import java.util.List;
2122
import java.util.Map;
@@ -97,19 +98,8 @@ public boolean connectPhysicalDisk(String volumeUuid, KVMStoragePool pool, Map<S
9798

9899
String result = iScsiAdmCmd.execute();
99100

100-
if (result != null) {
101-
// Node record may already exist from a previous run; accept and proceed
102-
if (isNonFatalNodeCreate(result)) {
103-
logger.debug("iSCSI node already exists for {}@{}:{}, proceeding", getIqn(volumeUuid), pool.getSourceHost(), pool.getSourcePort());
104-
} else {
105-
logger.debug("Failed to add iSCSI target " + volumeUuid);
106-
System.out.println("Failed to add iSCSI target " + volumeUuid);
107-
108-
return false;
109-
}
110-
} else {
111-
logger.debug("Successfully added iSCSI target " + volumeUuid);
112-
System.out.println("Successfully added to iSCSI target " + volumeUuid);
101+
if (!handleNodeCreateResult(result, volumeUuid)) {
102+
return false;
113103
}
114104

115105
String chapInitiatorUsername = details.get(DiskTO.CHAP_INITIATOR_USERNAME);
@@ -143,29 +133,13 @@ public boolean connectPhysicalDisk(String volumeUuid, KVMStoragePool pool, Map<S
143133

144134
result = iScsiAdmCmd.execute();
145135

146-
if (result != null) {
147-
if (isNonFatalLogin(result)) {
148-
logger.debug("iSCSI login returned benign message for {}@{}:{}: {}", iqn, host, port, result);
149-
// Session already exists — a newly mapped LUN won't be visible until
150-
// the kernel's next periodic SCSI scan (~30-60s).
151-
Script rescanCmd = new Script(true, "iscsiadm", 0, logger);
152-
rescanCmd.add("-m", "session");
153-
rescanCmd.add("--rescan");
154-
String rescanResult = rescanCmd.execute();
155-
if (rescanResult != null) {
156-
logger.warn("iSCSI session rescan returned: {}", rescanResult);
157-
} else {
158-
logger.debug("iSCSI session rescan completed successfully for {}@{}:{}", iqn, host, port);
159-
}
160-
} else {
161-
logger.debug("Failed to log in to iSCSI target " + volumeUuid + ": " + result);
162-
System.out.println("Failed to log in to iSCSI target " + volumeUuid);
136+
if (!handleLoginResult(result, volumeUuid)) {
137+
return false;
138+
}
163139

164-
return false;
165-
}
166-
} else {
167-
logger.debug("Successfully logged in to iSCSI target " + volumeUuid);
168-
System.out.println("Successfully logged in to iSCSI target " + volumeUuid);
140+
// If the session already existed, a newly mapped LUN won't be visible until a rescan.
141+
if (result != null) {
142+
rescanIscsiSessions(iqn, host, port);
169143
}
170144

171145
// There appears to be a race condition where logging in to the iSCSI volume via iscsiadm
@@ -183,19 +157,54 @@ public boolean connectPhysicalDisk(String volumeUuid, KVMStoragePool pool, Map<S
183157
return true;
184158
}
185159

186-
// Removed sessionExists() call to avoid noisy sudo/iscsiadm session queries that may fail on some setups
187-
188-
private boolean isNonFatalLogin(String result) {
189-
if (result == null) return true;
160+
/**
161+
* Checks the result of an iscsiadm node-create command.
162+
* Returns true if the node was created or already exists, false on failure.
163+
*/
164+
boolean handleNodeCreateResult(String result, String volumeUuid) {
165+
if (result == null) {
166+
logger.debug("Successfully added iSCSI node for target {}", volumeUuid);
167+
return true;
168+
}
190169
String msg = result.toLowerCase();
191-
// Accept messages where the session already exists
192-
return msg.contains("already present") || msg.contains("already logged in") || msg.contains("session exists");
170+
if (msg.contains("already exists") || msg.contains("database exists") || msg.contains("exists")) {
171+
logger.debug("iSCSI node already exists for target {}, proceeding", volumeUuid);
172+
return true;
173+
}
174+
logger.debug("Failed to add iSCSI node for target {}: {}", volumeUuid, result);
175+
return false;
193176
}
194177

195-
private boolean isNonFatalNodeCreate(String result) {
196-
if (result == null) return true;
178+
/**
179+
* Checks the result of an iscsiadm login command.
180+
* Returns true if the login succeeded or session already exists, false on failure.
181+
*/
182+
boolean handleLoginResult(String result, String volumeUuid) {
183+
if (result == null) {
184+
logger.debug("Successfully logged in to iSCSI target {}", volumeUuid);
185+
return true;
186+
}
197187
String msg = result.toLowerCase();
198-
return msg.contains("already exists") || msg.contains("database exists") || msg.contains("exists");
188+
if (msg.contains("already present") || msg.contains("already logged in") || msg.contains("session exists")) {
189+
logger.debug("iSCSI session already exists for target {}, proceeding", volumeUuid);
190+
return true;
191+
}
192+
logger.debug("Failed to log in to iSCSI target {}: {}", volumeUuid, result);
193+
return false;
194+
}
195+
196+
private void rescanIscsiSessions(String iqn, String host, int port) {
197+
Script rescanCmd = new Script(true, "iscsiadm", 0, logger);
198+
rescanCmd.add("-m", "node");
199+
rescanCmd.add("-T", iqn);
200+
rescanCmd.add("-p", host + ":" + port);
201+
rescanCmd.add("--rescan");
202+
String rescanResult = rescanCmd.execute();
203+
if (rescanResult != null) {
204+
logger.warn("iSCSI session rescan returned: {}", rescanResult);
205+
} else {
206+
logger.debug("iSCSI session rescan completed successfully for {}@{}:{}", iqn, host, port);
207+
}
199208
}
200209

201210
private void waitForDiskToBecomeAvailable(String volumeUuid, KVMStoragePool pool) {
@@ -340,15 +349,15 @@ private String getComponent(String path, int index) {
340349
*/
341350
private boolean hasOtherActiveLuns(String host, int port, String iqn, String lun) {
342351
String prefix = "ip-" + host + ":" + port + "-iscsi-" + iqn + "-lun-";
343-
java.io.File byPathDir = new java.io.File("/dev/disk/by-path");
352+
File byPathDir = new File("/dev/disk/by-path");
344353
if (!byPathDir.exists() || !byPathDir.isDirectory()) {
345354
return false;
346355
}
347-
java.io.File[] entries = byPathDir.listFiles();
356+
File[] entries = byPathDir.listFiles();
348357
if (entries == null) {
349358
return false;
350359
}
351-
for (java.io.File entry : entries) {
360+
for (File entry : entries) {
352361
String name = entry.getName();
353362
// Skip partition entries (e.g. lun-0-part1, lun-0-part2) — these are not
354363
// independent LUNs, they are partition symlinks for the same LUN disk.

‎plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java‎

Lines changed: 15 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
import com.cloud.storage.Storage;
3030
import com.cloud.storage.StoragePool;
3131
import com.cloud.storage.Volume;
32+
import com.cloud.storage.VolumeDetailVO;
3233
import com.cloud.storage.VolumeVO;
3334
import com.cloud.storage.ScopeType;
3435
import com.cloud.storage.dao.SnapshotDetailsDao;
@@ -109,7 +110,7 @@ public DataTO getTO(DataObject data) {
109110
public DataStoreTO getStoreTO(DataStore store) { return null; }
110111

111112
@Override
112-
public boolean volumesRequireGrantAccessWhenUsed(){
113+
public boolean volumesRequireGrantAccessWhenUsed() {
113114
logger.info("OntapPrimaryDatastoreDriver: volumesRequireGrantAccessWhenUsed: Called");
114115
return true;
115116
}
@@ -141,15 +142,14 @@ public void createAsync(DataStore dataStore, DataObject dataObject, AsyncComplet
141142
logger.error("createAsync: Storage Pool not found for id: " + dataStore.getId());
142143
throw new CloudRuntimeException("Storage Pool not found for id: " + dataStore.getId());
143144
}
144-
String storagePoolUuid = dataStore.getUuid();
145145

146146
Map<String, String> details = storagePoolDetailsDao.listDetailsKeyPairs(dataStore.getId());
147147

148148
if (dataObject.getType() == DataObjectType.VOLUME) {
149149
VolumeInfo volInfo = (VolumeInfo) dataObject;
150150

151151
// Create the backend storage object (LUN for iSCSI, no-op for NFS)
152-
CloudStackVolume created = createCloudStackVolume(dataStore, volInfo, details);
152+
CloudStackVolume created = createCloudStackVolume(storagePool, volInfo, details);
153153

154154
// Update CloudStack volume record with storage pool association and protocol-specific details
155155
VolumeVO volumeVO = volumeDao.findById(volInfo.getId());
@@ -201,22 +201,10 @@ public void createAsync(DataStore dataStore, DataObject dataObject, AsyncComplet
201201
/**
202202
* Creates a volume on the ONTAP backend.
203203
*/
204-
private CloudStackVolume createCloudStackVolume(DataStore dataStore, DataObject dataObject, Map<String, String> details) {
205-
StoragePoolVO storagePool = storagePoolDao.findById(dataStore.getId());
206-
if (storagePool == null) {
207-
logger.error("createCloudStackVolume: Storage Pool not found for id: {}", dataStore.getId());
208-
throw new CloudRuntimeException("Storage Pool not found for id: " + dataStore.getId());
209-
}
210-
204+
private CloudStackVolume createCloudStackVolume(StoragePoolVO storagePool, VolumeInfo volumeObject, Map<String, String> details) {
211205
StorageStrategy storageStrategy = OntapStorageUtils.getStrategyByStoragePoolDetails(details);
212-
213-
if (dataObject.getType() == DataObjectType.VOLUME) {
214-
VolumeInfo volumeObject = (VolumeInfo) dataObject;
215-
CloudStackVolume cloudStackVolumeRequest = OntapStorageUtils.createCloudStackVolumeRequestByProtocol(storagePool, details, volumeObject);
216-
return storageStrategy.createCloudStackVolume(cloudStackVolumeRequest);
217-
} else {
218-
throw new CloudRuntimeException("Unsupported DataObjectType: " + dataObject.getType());
219-
}
206+
CloudStackVolume cloudStackVolumeRequest = OntapStorageUtils.createCloudStackVolumeRequestByProtocol(storagePool, details, volumeObject);
207+
return storageStrategy.createCloudStackVolume(cloudStackVolumeRequest);
220208
}
221209

222210
/**
@@ -243,7 +231,7 @@ public void deleteAsync(DataStore store, DataObject data, AsyncCompletionCallbac
243231
StorageStrategy storageStrategy = OntapStorageUtils.getStrategyByStoragePoolDetails(details);
244232
logger.info("createCloudStackVolumeForTypeVolume: Connection to Ontap SVM [{}] successful, preparing CloudStackVolumeRequest", details.get(OntapStorageConstants.SVM_NAME));
245233
VolumeInfo volumeInfo = (VolumeInfo) data;
246-
CloudStackVolume cloudStackVolumeRequest = createDeleteCloudStackVolumeRequest(storagePool,details,volumeInfo);
234+
CloudStackVolume cloudStackVolumeRequest = createDeleteCloudStackVolumeRequest(storagePool, details, volumeInfo);
247235
storageStrategy.deleteCloudStackVolume(cloudStackVolumeRequest);
248236
logger.info("deleteAsync: Volume deleted: " + volumeInfo.getId());
249237
commandResult.setResult(null);
@@ -308,7 +296,7 @@ private void deleteOntapSnapshot(SnapshotInfo snapshotInfo, CommandResult comman
308296

309297
if (jobResponse != null && jobResponse.getJob() != null) {
310298
// Poll for job completion
311-
Boolean jobSucceeded = storageStrategy.jobPollForSuccess(jobResponse.getJob().getUuid(), 30, 2);
299+
Boolean jobSucceeded = storageStrategy.jobPollForSuccess(jobResponse.getJob().getUuid(), 30, 2000);
312300
if (!jobSucceeded) {
313301
throw new CloudRuntimeException("Delete job failed for snapshot [" +
314302
snapshotName + "] on FlexVol [" + flexVolUuid + "]");
@@ -383,7 +371,6 @@ public boolean grantAccess(DataObject dataObject, Host host, DataStore dataStore
383371
logger.error("grantAccess: Storage Pool not found for id: " + dataStore.getId());
384372
throw new CloudRuntimeException("Storage Pool not found for id: " + dataStore.getId());
385373
}
386-
String storagePoolUuid = dataStore.getUuid();
387374

388375
// ONTAP managed storage only supports cluster and zone scoped pools
389376
if (storagePool.getScope() != ScopeType.CLUSTER && storagePool.getScope() != ScopeType.ZONE) {
@@ -412,7 +399,6 @@ public boolean grantAccess(DataObject dataObject, Host host, DataStore dataStore
412399
OntapStorageConstants.NAME, accessGroupName,
413400
OntapStorageConstants.SVM_DOT_NAME, svmName
414401
);
415-
Igroup igroup = new Igroup();
416402
AccessGroup accessGroup = sanStrategy.getAccessGroup(getAccessGroupMap);
417403
if(accessGroup == null || accessGroup.getIgroup() == null) {
418404
logger.info("grantAccess: Igroup {} does not exist for the host {} : Need to create Igroup for the host ", accessGroupName, host.getName());
@@ -425,7 +411,6 @@ public boolean grantAccess(DataObject dataObject, Host host, DataStore dataStore
425411
accessGroup = sanStrategy.createAccessGroup(accessGroup);
426412
}else{
427413
logger.info("grantAccess: Igroup {} already exist for the host {}: ", accessGroup.getIgroup().getName() ,host.getName());
428-
igroup = accessGroup.getIgroup();
429414
/* TODO Below cases will be covered later, for now they will be a pre-requisite on customer side
430415
1. Igroup exist with the same name but host initiator has been rempved
431416
2. Igroup exist with the same name but host initiator has been changed may be due to new NIC or new adapter
@@ -520,8 +505,8 @@ private void revokeAccessForVolume(StoragePoolVO storagePool, VolumeVO volumeVO,
520505
String accessGroupName = OntapStorageUtils.getIgroupName(svmName, host.getName());
521506

522507
// Retrieve LUN name from volume details; if missing, volume may not have been fully created
523-
String lunName = volumeDetailsDao.findDetail(volumeVO.getId(), OntapStorageConstants.LUN_DOT_NAME) != null ?
524-
volumeDetailsDao.findDetail(volumeVO.getId(), OntapStorageConstants.LUN_DOT_NAME).getValue() : null;
508+
VolumeDetailVO lunDetail = volumeDetailsDao.findDetail(volumeVO.getId(), OntapStorageConstants.LUN_DOT_NAME);
509+
String lunName = lunDetail != null ? lunDetail.getValue() : null;
525510
if (lunName == null) {
526511
logger.warn("revokeAccessForVolume: No LUN name found for volume [{}]; skipping revoke", volumeVO.getId());
527512
return;
@@ -671,10 +656,9 @@ public void takeSnapshot(SnapshotInfo snapshot, AsyncCompletionCallback<CreateCm
671656
// For iSCSI, retrieve LUN UUID for restore operations
672657
String lunUuid = null;
673658
if (ProtocolType.ISCSI.name().equalsIgnoreCase(protocol)) {
674-
lunUuid = volumeDetailsDao.findDetail(volumeVO.getId(), OntapStorageConstants.LUN_DOT_UUID) != null
675-
? volumeDetailsDao.findDetail(volumeVO.getId(), OntapStorageConstants.LUN_DOT_UUID).getValue()
676-
: null;
677-
if (lunUuid == null) {
659+
VolumeDetailVO lunDetail = volumeDetailsDao.findDetail(volumeVO.getId(), OntapStorageConstants.LUN_DOT_UUID);
660+
String lunUUID = lunDetail != null ? lunDetail.getValue() : null;
661+
if (lunUUID == null) {
678662
throw new CloudRuntimeException("LUN UUID not found for iSCSI volume " + volumeVO.getId());
679663
}
680664
}
@@ -692,7 +676,7 @@ public void takeSnapshot(SnapshotInfo snapshot, AsyncCompletionCallback<CreateCm
692676
}
693677

694678
// Poll for job completion
695-
Boolean jobSucceeded = storageStrategy.jobPollForSuccess(jobResponse.getJob().getUuid(), 30, 2);
679+
Boolean jobSucceeded = storageStrategy.jobPollForSuccess(jobResponse.getJob().getUuid(), 30, 2000);
696680
if (!jobSucceeded) {
697681
throw new CloudRuntimeException("FlexVolume snapshot job failed on FlexVol UUID [" + flexVolUuid + "]");
698682
}
@@ -843,7 +827,7 @@ public void revertSnapshot(SnapshotInfo snapshotOnImageStore, SnapshotInfo snaps
843827
}
844828

845829
// Poll for job completion (use longer timeout for large LUNs/files)
846-
Boolean jobSucceeded = storageStrategy.jobPollForSuccess(jobResponse.getJob().getUuid(), 60, 2);
830+
Boolean jobSucceeded = storageStrategy.jobPollForSuccess(jobResponse.getJob().getUuid(), 60, 2000);
847831
if (!jobSucceeded) {
848832
throw new CloudRuntimeException("Restore job failed for snapshot [" +
849833
snapshotName + "]");

0 commit comments

Comments
 (0)