Skip to content

Commit be4c241

Browse files
Merge pull request #55 from NetApp/feature/CSTACKEX-183-pr
Feature/cstackex 183 pr
2 parents c5bc600 + b625b0a commit be4c241

28 files changed

Lines changed: 285 additions & 389 deletions

File tree

.github/CODEOWNERS

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717

1818
/plugins/storage/volume/linstor @rp-
1919
/plugins/storage/volume/storpool @slavkap
20-
/plugins/storage/volume/ontap @rajiv1 @sandeeplocharla @piyush5 @suryag
20+
/plugins/storage/volume/ontap @rajiv-jain-netapp @sandeeplocharla @piyush5netapp @suryag1201
2121

2222
.pre-commit-config.yaml @jbampton
2323
/.github/linters/ @jbampton

engine/api/src/main/java/org/apache/cloudstack/engine/subsystem/api/storage/DataStoreProvider.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,11 @@ public interface DataStoreProvider {
2929
String SAMPLE_IMAGE = "Sample";
3030
String SMB = "NFS";
3131
String DEFAULT_PRIMARY = "DefaultPrimary";
32+
/**
33+
* Primary storage provider name for NetApp ONTAP ({@code OntapPrimaryDatastoreProvider#getName}).
34+
* Single canonical value; use this instead of duplicating the string across modules.
35+
*/
36+
String ONTAP_PLUGIN_NAME = "NetApp ONTAP";
3237

3338
enum DataStoreProviderType {
3439
PRIMARY, IMAGE, ImageCache, OBJECT

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

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@
5151
import com.cloud.vm.snapshot.VMSnapshotVO;
5252
import org.apache.cloudstack.backup.BackupOfferingVO;
5353
import org.apache.cloudstack.backup.dao.BackupOfferingDao;
54+
import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreProvider;
5455
import org.apache.cloudstack.engine.subsystem.api.storage.ObjectInDataStoreStateMachine;
5556
import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotInfo;
5657
import org.apache.cloudstack.engine.subsystem.api.storage.StrategyPriority;
@@ -77,8 +78,6 @@ public class KvmFileBasedStorageVmSnapshotStrategy extends StorageVMSnapshotStra
7778

7879
private static final List<Storage.StoragePoolType> supportedStoragePoolTypes = List.of(Storage.StoragePoolType.Filesystem, Storage.StoragePoolType.NetworkFilesystem, Storage.StoragePoolType.SharedMountPoint);
7980

80-
private static final String ONTAP_PROVIDER_NAME = "NetApp ONTAP";
81-
8281
@Inject
8382
protected SnapshotDataStoreDao snapshotDataStoreDao;
8483

@@ -327,13 +326,13 @@ public StrategyPriority canHandle(Long vmId, Long rootPoolId, boolean snapshotMe
327326
List<VolumeVO> volumes = volumeDao.findByInstance(vmId);
328327
for (VolumeVO volume : volumes) {
329328
StoragePoolVO storagePoolVO = storagePool.findById(volume.getPoolId());
330-
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()));
329+
if (storagePoolVO.isManaged() && DataStoreProvider.ONTAP_PLUGIN_NAME.equals(storagePoolVO.getStorageProviderName())) {
330+
logger.debug("{} as the VM has a volume on ONTAP managed storage pool [{}]. " +
331+
"ONTAP managed storage has its own dedicated VM snapshot strategy.", cantHandleLog, storagePoolVO.getName());
333332
return StrategyPriority.CANT_HANDLE;
334333
}
335334
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()));
335+
logger.debug("{} as the VM has a volume that is in a storage with unsupported type [{}].", cantHandleLog, storagePoolVO.getPoolType());
337336
return StrategyPriority.CANT_HANDLE;
338337
}
339338
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: 63 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,9 @@
1616
// under the License.
1717
package com.cloud.hypervisor.kvm.storage;
1818

19+
import java.io.File;
20+
import java.io.FileWriter;
21+
import java.nio.file.Path;
1922
import java.util.HashMap;
2023
import java.util.List;
2124
import java.util.Map;
@@ -97,19 +100,8 @@ public boolean connectPhysicalDisk(String volumeUuid, KVMStoragePool pool, Map<S
97100

98101
String result = iScsiAdmCmd.execute();
99102

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);
103+
if (!handleNodeCreateResult(result, volumeUuid)) {
104+
return false;
113105
}
114106

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

144136
result = iScsiAdmCmd.execute();
145137

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);
138+
if (!handleLoginResult(result, volumeUuid)) {
139+
return false;
140+
}
163141

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);
142+
// If the session already existed, a newly mapped LUN won't be visible until a rescan.
143+
if (result != null) {
144+
rescanIscsiSessions(iqn, host, port);
169145
}
170146

171147
// 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
183159
return true;
184160
}
185161

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;
162+
/**
163+
* Checks the result of an iscsiadm node-create command.
164+
* Returns true if the node was created or already exists, false on failure.
165+
*/
166+
boolean handleNodeCreateResult(String result, String volumeUuid) {
167+
if (result == null) {
168+
logger.debug("Successfully added iSCSI node for target {}", volumeUuid);
169+
return true;
170+
}
190171
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");
172+
if (msg.contains("already exists") || msg.contains("database exists") || msg.contains("exists")) {
173+
logger.debug("iSCSI node already exists for target {}, proceeding", volumeUuid);
174+
return true;
175+
}
176+
logger.debug("Failed to add iSCSI node for target {}: {}", volumeUuid, result);
177+
return false;
193178
}
194179

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

201212
private void waitForDiskToBecomeAvailable(String volumeUuid, KVMStoragePool pool) {
@@ -340,15 +351,15 @@ private String getComponent(String path, int index) {
340351
*/
341352
private boolean hasOtherActiveLuns(String host, int port, String iqn, String lun) {
342353
String prefix = "ip-" + host + ":" + port + "-iscsi-" + iqn + "-lun-";
343-
java.io.File byPathDir = new java.io.File("/dev/disk/by-path");
354+
File byPathDir = new File("/dev/disk/by-path");
344355
if (!byPathDir.exists() || !byPathDir.isDirectory()) {
345356
return false;
346357
}
347-
java.io.File[] entries = byPathDir.listFiles();
358+
File[] entries = byPathDir.listFiles();
348359
if (entries == null) {
349360
return false;
350361
}
351-
for (java.io.File entry : entries) {
362+
for (File entry : entries) {
352363
String name = entry.getName();
353364
// Skip partition entries (e.g. lun-0-part1, lun-0-part2) — these are not
354365
// 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
378389
*/
379390
private void removeStaleScsiDevice(String host, int port, String iqn, String lun) {
380391
String byPath = getByPath(host, port, "/" + iqn + "/" + lun);
381-
java.nio.file.Path byPathLink = java.nio.file.Paths.get(byPath);
382-
if (!java.nio.file.Files.exists(byPathLink)) {
392+
Path byPathLink = Paths.get(byPath);
393+
if (!Files.exists(byPathLink)) {
383394
logger.debug("by-path entry for LUN " + lun + " already gone, nothing to remove");
384395
return;
385396
}
386397
try {
387-
java.nio.file.Path realDevice = byPathLink.toRealPath();
398+
Path realDevice = byPathLink.toRealPath();
388399
String devName = realDevice.getFileName().toString();
389-
java.io.File deleteFile = new java.io.File("/sys/block/" + devName + "/device/delete");
400+
File deleteFile = new File("/sys/block/" + devName + "/device/delete");
390401
if (!deleteFile.exists()) {
391402
logger.warn("sysfs delete entry not found for device " + devName + " — cannot remove stale SCSI device");
392403
return;
393404
}
394-
try (java.io.FileWriter fw = new java.io.FileWriter(deleteFile)) {
405+
try (FileWriter fw = new FileWriter(deleteFile)) {
395406
fw.write("1");
396407
}
397408
logger.info("Removed stale SCSI device " + devName + " for LUN /" + iqn + "/" + lun + " via sysfs");

plugins/storage/volume/ontap/pom.xml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -137,7 +137,7 @@
137137
<dependency>
138138
<groupId>org.apache.cloudstack</groupId>
139139
<artifactId>cloud-engine-storage-snapshot</artifactId>
140-
<version>4.23.0.0-SNAPSHOT</version>
140+
<version>${project.version}</version>
141141
<scope>compile</scope>
142142
</dependency>
143143
</dependencies>

0 commit comments

Comments
 (0)