From 5661946a128b659919d268314a867dc3653e4a07 Mon Sep 17 00:00:00 2001 From: piyush5netapp <91685498+piyush5netapp@users.noreply.github.com> Date: Mon, 7 Sep 2026 14:12:11 +0530 Subject: [PATCH 1/5] new pool type conflicts resolution --- .../main/java/com/cloud/storage/Storage.java | 3 +- .../java/com/cloud/storage/StorageTest.java | 2 + .../StorageSystemDataMotionStrategy.java | 4 +- .../kvm/storage/KVMStorageProcessor.java | 6 +- .../kvm/storage/OntapIscsiStorageAdaptor.java | 38 +++++++++ .../storage/OntapIscsiStorageAdaptorTest.java | 78 +++++++++++++++++++ .../driver/OntapPrimaryDatastoreDriver.java | 35 +++++++++ .../OntapPrimaryDatastoreLifecycle.java | 2 +- .../OntapPrimaryDatastoreDriverTest.java | 52 ++++++++++++- .../OntapPrimaryDatastoreLifecycleTest.java | 48 ++++++++++++ .../main/java/com/cloud/api/ApiDBUtils.java | 5 +- 11 files changed, 264 insertions(+), 9 deletions(-) create mode 100644 plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/OntapIscsiStorageAdaptor.java create mode 100644 plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/OntapIscsiStorageAdaptorTest.java diff --git a/api/src/main/java/com/cloud/storage/Storage.java b/api/src/main/java/com/cloud/storage/Storage.java index 275f6d25268d..5eea0f35d647 100644 --- a/api/src/main/java/com/cloud/storage/Storage.java +++ b/api/src/main/java/com/cloud/storage/Storage.java @@ -185,7 +185,8 @@ public static enum StoragePoolType { Linstor(true, true, EncryptionSupport.Storage), DatastoreCluster(true, true, EncryptionSupport.Unsupported), // for VMware, to abstract pool of clusters StorPool(true, true, EncryptionSupport.Hypervisor), - FiberChannel(true, true, EncryptionSupport.Unsupported); // Fiber Channel Pool for KVM hypervisors is used to find the volume by WWN value (/dev/disk/by-id/wwn-) + FiberChannel(true, true, EncryptionSupport.Unsupported), // Fiber Channel Pool for KVM hypervisors is used to find the volume by WWN value (/dev/disk/by-id/wwn-) + OntapiSCSI(true, true, EncryptionSupport.Unsupported); // NetApp ONTAP iSCSI private final boolean shared; private final boolean overProvisioning; diff --git a/api/src/test/java/com/cloud/storage/StorageTest.java b/api/src/test/java/com/cloud/storage/StorageTest.java index 2bcc28e2b4b6..b9edda569dab 100644 --- a/api/src/test/java/com/cloud/storage/StorageTest.java +++ b/api/src/test/java/com/cloud/storage/StorageTest.java @@ -49,6 +49,7 @@ public void isSharedStoragePool() { Assert.assertTrue(StoragePoolType.ManagedNFS.isShared()); Assert.assertTrue(StoragePoolType.DatastoreCluster.isShared()); Assert.assertTrue(StoragePoolType.Linstor.isShared()); + Assert.assertTrue(StoragePoolType.OntapiSCSI.isShared()); } @Test @@ -73,6 +74,7 @@ public void supportsOverProvisioningTestAllStoragePoolTypes() { Assert.assertFalse(StoragePoolType.ManagedNFS.supportsOverProvisioning()); Assert.assertTrue(StoragePoolType.DatastoreCluster.supportsOverProvisioning()); Assert.assertTrue(StoragePoolType.Linstor.supportsOverProvisioning()); + Assert.assertTrue(StoragePoolType.OntapiSCSI.supportsOverProvisioning()); } @Test diff --git a/engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java b/engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java index 7674f1ce25a1..76bbc9cac908 100644 --- a/engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java +++ b/engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java @@ -616,8 +616,8 @@ private void handleVolumeMigrationFromManagedStorageToNonManagedStorage(VolumeIn private void verifyFormatWithPoolType(ImageFormat imageFormat, StoragePoolType poolType) { if (imageFormat != ImageFormat.VHD && imageFormat != ImageFormat.OVA && imageFormat != ImageFormat.QCOW2 && !(imageFormat == ImageFormat.RAW && (StoragePoolType.PowerFlex == poolType || - StoragePoolType.FiberChannel == poolType))) { - throw new CloudRuntimeException(String.format("Only the following image types are currently supported: %s, %s, %s, %s (for PowerFlex and FiberChannel)", + StoragePoolType.FiberChannel == poolType || StoragePoolType.OntapiSCSI == poolType))) { + throw new CloudRuntimeException(String.format("Only the following image types are currently supported: %s, %s, %s, %s (for PowerFlex, FiberChannel and OntapiSCSI)", ImageFormat.VHD.toString(), ImageFormat.OVA.toString(), ImageFormat.QCOW2.toString(), ImageFormat.RAW.toString())); } } diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java index 0cb32da318ea..630d02d3c480 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java @@ -421,7 +421,8 @@ public Answer copyTemplateToPrimaryStorage(final CopyCommand cmd) { StoragePoolType.PowerFlex, StoragePoolType.Linstor, StoragePoolType.FiberChannel, - StoragePoolType.CLVM).contains(primaryPool.getType())) { + StoragePoolType.CLVM, + StoragePoolType.OntapiSCSI).contains(primaryPool.getType())) { newTemplate.setFormat(ImageFormat.RAW); } else { newTemplate.setFormat(ImageFormat.QCOW2); @@ -3513,7 +3514,8 @@ private Storage.ImageFormat getFormat(StoragePoolType poolType) { StoragePoolType.PowerFlex, StoragePoolType.Linstor, StoragePoolType.FiberChannel, - StoragePoolType.CLVM).contains(poolType)) { + StoragePoolType.CLVM, + StoragePoolType.OntapiSCSI).contains(poolType)) { return ImageFormat.RAW; } else { return ImageFormat.QCOW2; diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/OntapIscsiStorageAdaptor.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/OntapIscsiStorageAdaptor.java new file mode 100644 index 000000000000..73e8cb4026c7 --- /dev/null +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/OntapIscsiStorageAdaptor.java @@ -0,0 +1,38 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +package com.cloud.hypervisor.kvm.storage; + +import com.cloud.storage.Storage.StoragePoolType; + +/** + * Serves {@link StoragePoolType#OntapiSCSI} pools, which are ONTAP FlexVols exposed over iSCSI with one + * LUN per CloudStack volume. The host-side handling is identical to a generic iSCSI target + * + * The class exists so that ONTAP-specific host behaviour can diverge here without altering the storage + * path of the other vendors that register as {@link StoragePoolType#Iscsi} which all share the superclass. + * + * This must stay in the {@code com.cloud.hypervisor.kvm.storage} package: {@link KVMStoragePoolManager} + * discovers adaptors by a Reflections scan of that package alone, and an unregistered type silently + * falls back to {@link LibvirtStorageAdaptor} rather than failing at startup. + */ +public class OntapIscsiStorageAdaptor extends IscsiAdmStorageAdaptor { + + @Override + public StoragePoolType getStoragePoolType() { + return StoragePoolType.OntapiSCSI; + } +} diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/OntapIscsiStorageAdaptorTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/OntapIscsiStorageAdaptorTest.java new file mode 100644 index 000000000000..c89094081d35 --- /dev/null +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/OntapIscsiStorageAdaptorTest.java @@ -0,0 +1,78 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +package com.cloud.hypervisor.kvm.storage; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertTrue; + +import java.lang.reflect.Modifier; +import java.util.Set; + +import org.apache.cloudstack.utils.qemu.QemuImg.PhysicalDiskFormat; +import org.junit.Test; +import org.reflections.Reflections; + +import com.cloud.storage.Storage.StoragePoolType; + +public class OntapIscsiStorageAdaptorTest { + + @Test + public void getStoragePoolTypeReturnsOntapIscsi() { + assertEquals(StoragePoolType.OntapiSCSI, new OntapIscsiStorageAdaptor().getStoragePoolType()); + } + + @Test + public void createdPoolCarriesOntapIscsiTypeAndRawFormat() { + OntapIscsiStorageAdaptor adaptor = new OntapIscsiStorageAdaptor(); + + KVMStoragePool pool = adaptor.createStoragePool("ontap-iscsi-pool-uuid", "10.0.0.1", 3260, null, null, + StoragePoolType.OntapiSCSI, null, true); + + assertEquals(StoragePoolType.OntapiSCSI, pool.getType()); + // Attach builds a block-based disk off the physical disk format rather than the pool type, + // which is why splitting OntapiSCSI out of Iscsi leaves the generated domain XML unchanged. + assertEquals(PhysicalDiskFormat.RAW, pool.getDefaultFormat()); + assertSame(pool, adaptor.getStoragePool("ontap-iscsi-pool-uuid")); + } + + /** + * KVMStoragePoolManager discovers adaptors by a Reflections scan of its own package, instantiating + * each concrete implementation through a no-arg constructor and keying it on getStoragePoolType(). + * A type with no adaptor silently falls back to LibvirtStorageAdaptor instead of failing at + * startup, so this reproduces the discovery preconditions rather than waiting for the symptom. + * The manager itself is not constructed here because doing so also instantiates + * MultipathSCSIAdapterBase, which requires agent scripts resolvable from the working directory. + */ + @Test + public void adaptorSatisfiesThePoolManagerDiscoveryContract() throws ReflectiveOperationException { + String scannedPackage = KVMStoragePoolManager.class.getPackage().getName(); + Set> discovered = + new Reflections(scannedPackage).getSubTypesOf(StorageAdaptor.class); + + assertTrue("OntapIscsiStorageAdaptor must live in " + scannedPackage + " to be discovered", + discovered.contains(OntapIscsiStorageAdaptor.class)); + assertFalse("An abstract adaptor is skipped by the scan", + Modifier.isAbstract(OntapIscsiStorageAdaptor.class.getModifiers())); + + StorageAdaptor adaptor = OntapIscsiStorageAdaptor.class.getDeclaredConstructor().newInstance(); + assertEquals(StoragePoolType.OntapiSCSI, adaptor.getStoragePoolType()); + assertEquals("The superclass must keep serving the other iSCSI vendors", + StoragePoolType.Iscsi, new IscsiAdmStorageAdaptor().getStoragePoolType()); + } +} 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 ece29f7cd0ac..f90d37cf7746 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 @@ -18,6 +18,7 @@ */ package org.apache.cloudstack.storage.driver; +import com.cloud.hypervisor.Hypervisor; import org.apache.cloudstack.storage.utils.OntapStorageConstants; import com.cloud.agent.api.Answer; import com.cloud.agent.api.to.DataObjectType; @@ -74,6 +75,7 @@ import javax.inject.Inject; import java.util.ArrayList; +import java.util.Arrays; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -144,6 +146,7 @@ public void createAsync(DataStore dataStore, DataObject dataObject, AsyncComplet } Map details = storagePoolDetailsDao.listDetailsKeyPairs(dataStore.getId()); + validateProtocol(details, dataStore); if (dataObject.getType() == DataObjectType.VOLUME) { VolumeInfo volInfo = (VolumeInfo) dataObject; @@ -156,6 +159,8 @@ public void createAsync(DataStore dataStore, DataObject dataObject, AsyncComplet volumeVO.setPoolType(storagePool.getPoolType()); volumeVO.setPoolId(storagePool.getId()); + volumeVO.setFormat(getImageFormat(storagePool)); + logger.info("createAsync: Volume format set to [{}] for pool type [{}]", volumeVO.getFormat(), storagePool.getPoolType()); if (ProtocolType.ISCSI.name().equalsIgnoreCase(details.get(OntapStorageConstants.PROTOCOL))) { String lunName = created != null && created.getLun() != null ? created.getLun().getName() : null; @@ -989,6 +994,36 @@ private String buildSnapshotName(String volumeName, String snapshotUuid) { return name; } + /** + * Only ISCSI and NFS3 pools can be provisioned; any other protocol would fall through + * createAsync without producing a result, leaving the caller with a null callback value. + */ + private void validateProtocol(Map details, DataStore dataStore) { + String protocol = details == null ? null : details.get(OntapStorageConstants.PROTOCOL); + boolean supported = protocol != null && Arrays.stream(ProtocolType.values()) + .anyMatch(type -> type.name().equalsIgnoreCase(protocol)); + if (!supported) { + throw new CloudRuntimeException("Unsupported protocol [" + protocol + "] on storage pool [" + + dataStore.getName() + "]; supported protocols are " + Arrays.toString(ProtocolType.values())); + } + } + + private Storage.ImageFormat getImageFormat(StoragePoolVO storagePool) { + Hypervisor.HypervisorType hypervisorType = storagePool.getHypervisor(); + if (!Hypervisor.HypervisorType.KVM.equals(hypervisorType)) { + throw new CloudRuntimeException("Unsupported hypervisor [" + hypervisorType + "] for ONTAP image format resolution"); + } + Storage.StoragePoolType spType = storagePool.getPoolType(); + switch (spType) { + case OntapiSCSI: + return Storage.ImageFormat.RAW; + case NetworkFilesystem: + return Storage.ImageFormat.QCOW2; + default: + throw new CloudRuntimeException("Unsupported pool type [" + spType + "] for ONTAP image format resolution"); + } + } + /** * Persists snapshot metadata in snapshot_details table. * 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 fec594ea0ea6..887d11548f68 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 @@ -174,7 +174,7 @@ public DataStore initialize(Map dsInfos) { logger.info("Setting NFS path for storage pool: " + path + ", port: " + port + " with mount option: vers=3"); break; case ISCSI: - parameters.setType(Storage.StoragePoolType.Iscsi); + parameters.setType(Storage.StoragePoolType.OntapiSCSI); path = storageStrategy.getStoragePath(); port = OntapStorageConstants.ISCSI_PORT; logger.info("Setting iSCSI path for storage pool: " + path + ", port: " + port); diff --git a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriverTest.java b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriverTest.java index 3c139e23cb88..1c3d2777d0dd 100644 --- a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriverTest.java +++ b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriverTest.java @@ -21,6 +21,7 @@ import com.cloud.exception.InvalidParameterValueException; import com.cloud.host.Host; import com.cloud.host.HostVO; +import com.cloud.hypervisor.Hypervisor; import com.cloud.storage.ScopeType; import com.cloud.storage.Storage; import com.cloud.storage.VolumeVO; @@ -165,7 +166,8 @@ void testCreateAsync_VolumeWithISCSI_Success() { when(storagePoolDao.findById(1L)).thenReturn(storagePool); when(storagePool.getId()).thenReturn(1L); - when(storagePool.getPoolType()).thenReturn(Storage.StoragePoolType.NetworkFilesystem); + when(storagePool.getPoolType()).thenReturn(Storage.StoragePoolType.OntapiSCSI); + when(storagePool.getHypervisor()).thenReturn(Hypervisor.HypervisorType.KVM); when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(storagePoolDetails); when(volumeDao.findById(100L)).thenReturn(volumeVO); @@ -247,6 +249,54 @@ void testCreateAsync_VolumeWithNFS_Success() { } } + @Test + void testCreateAsync_UnsupportedHypervisor_FailsWithError() { + storagePoolDetails.put(OntapStorageConstants.PROTOCOL, ProtocolType.ISCSI.name()); + + when(dataStore.getId()).thenReturn(1L); + when(dataStore.getName()).thenReturn("ontap-pool"); + when(volumeInfo.getType()).thenReturn(VOLUME); + when(volumeInfo.getId()).thenReturn(100L); + when(volumeInfo.getName()).thenReturn("test-volume"); + + when(storagePoolDao.findById(1L)).thenReturn(storagePool); + when(storagePool.getId()).thenReturn(1L); + when(storagePool.getHypervisor()).thenReturn(Hypervisor.HypervisorType.VMware); + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(storagePoolDetails); + when(volumeDao.findById(100L)).thenReturn(volumeVO); + + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class)) { + utilityMock.when(() -> OntapStorageUtils.getStrategyByStoragePoolDetails(any())).thenReturn(sanStrategy); + + driver.createAsync(dataStore, volumeInfo, createCallback); + + ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(CreateCmdResult.class); + verify(createCallback).complete(resultCaptor.capture()); + assertFalse(resultCaptor.getValue().isSuccess()); + assertTrue(resultCaptor.getValue().getResult().contains("Unsupported hypervisor [VMware]")); + } + } + + @Test + void testCreateAsync_KvmUnsupportedProtocol_FailsWithError() { + storagePoolDetails.put(OntapStorageConstants.PROTOCOL, "FC"); + + when(dataStore.getId()).thenReturn(1L); + when(dataStore.getName()).thenReturn("ontap-pool"); + when(volumeInfo.getType()).thenReturn(VOLUME); + when(volumeInfo.getName()).thenReturn("test-volume"); + + when(storagePoolDao.findById(1L)).thenReturn(storagePool); + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(storagePoolDetails); + + driver.createAsync(dataStore, volumeInfo, createCallback); + + ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(CreateCmdResult.class); + verify(createCallback).complete(resultCaptor.capture()); + assertFalse(resultCaptor.getValue().isSuccess()); + assertTrue(resultCaptor.getValue().getResult().contains("Unsupported protocol [FC]")); + } + @Test void testDeleteAsync_NullStore_ThrowsException() { ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(CommandResult.class); diff --git a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycleTest.java b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycleTest.java index 751b864ecfcc..861c28d7476f 100644 --- a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycleTest.java +++ b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycleTest.java @@ -23,6 +23,7 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentCaptor; import org.mockito.InjectMocks; import org.mockito.Mock; import org.mockito.MockedStatic; @@ -36,10 +37,12 @@ import com.cloud.dc.ClusterVO; import com.cloud.host.HostVO; import com.cloud.resource.ResourceManager; +import com.cloud.storage.Storage; import com.cloud.storage.StorageManager; import org.apache.cloudstack.engine.subsystem.api.storage.ClusterScope; import org.apache.cloudstack.engine.subsystem.api.storage.DataStore; import org.apache.cloudstack.engine.subsystem.api.storage.PrimaryDataStoreInfo; +import org.apache.cloudstack.engine.subsystem.api.storage.PrimaryDataStoreParameters; import org.apache.cloudstack.engine.subsystem.api.storage.ZoneScope; import org.apache.cloudstack.storage.datastore.db.StoragePoolDetailsDao; import org.apache.cloudstack.storage.service.model.AccessGroup; @@ -54,6 +57,7 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.times; import static org.mockito.Mockito.withSettings; +import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.assertFalse; @@ -183,6 +187,50 @@ public void testInitialize_positive() { } } + private Map buildDsInfosForProtocol(String protocol) { + HashMap detailsMap = new HashMap(); + detailsMap.put(OntapStorageConstants.USERNAME, "testUser"); + detailsMap.put(OntapStorageConstants.PASSWORD, "testPassword"); + detailsMap.put(OntapStorageConstants.STORAGE_IP, "10.10.10.10"); + detailsMap.put(OntapStorageConstants.SVM_NAME, "vs0"); + detailsMap.put(OntapStorageConstants.PROTOCOL, protocol); + + Map dsInfos = new HashMap<>(); + dsInfos.put("zoneId", 1L); + dsInfos.put("podId", 1L); + dsInfos.put("clusterId", 1L); + dsInfos.put("name", "testStoragePool"); + dsInfos.put("providerName", "testProvider"); + dsInfos.put("capacityBytes", 1073741824L); + dsInfos.put("managed", true); + dsInfos.put("tags", "testTag"); + dsInfos.put("isTagARule", false); + dsInfos.put("details", detailsMap); + return dsInfos; + } + + private Storage.StoragePoolType initializeAndCapturePoolType(String protocol) { + try (MockedStatic storageProviderFactory = Mockito.mockStatic(StorageProviderFactory.class)) { + storageProviderFactory.when(() -> StorageProviderFactory.getStrategy(any())).thenReturn(storageStrategy); + ontapPrimaryDatastoreLifecycle.initialize(buildDsInfosForProtocol(protocol)); + } + ArgumentCaptor captor = ArgumentCaptor.forClass(PrimaryDataStoreParameters.class); + verify(_dataStoreHelper).createPrimaryDataStore(captor.capture()); + return captor.getValue().getType(); + } + + @Test + public void testInitialize_iscsiPoolUsesOntapIscsiType() { + when(storageStrategy.getStoragePath()).thenReturn("iqn.1992-08.com.netapp:sn.abc123"); + + assertEquals(Storage.StoragePoolType.OntapiSCSI, initializeAndCapturePoolType("ISCSI")); + } + + @Test + public void testInitialize_nfsPoolKeepsNetworkFilesystemType() { + assertEquals(Storage.StoragePoolType.NetworkFilesystem, initializeAndCapturePoolType("NFS3")); + } + @Test public void testInitialize_null_Arg() { Exception ex = assertThrows(CloudRuntimeException.class,() -> diff --git a/server/src/main/java/com/cloud/api/ApiDBUtils.java b/server/src/main/java/com/cloud/api/ApiDBUtils.java index 934600eb2b61..00ff20b85258 100644 --- a/server/src/main/java/com/cloud/api/ApiDBUtils.java +++ b/server/src/main/java/com/cloud/api/ApiDBUtils.java @@ -1338,7 +1338,7 @@ public static HypervisorType getHypervisorTypeFromFormat(long dcId, ImageFormat type = HypervisorType.Hyperv; } } if (format == ImageFormat.RAW) { - // Currently, KVM only supports RBD, PowerFlex, and FiberChannel images of type RAW. + // Currently, KVM only supports RBD, PowerFlex, FiberChannel and OntapiSCSI images of type RAW. // This results in a weird collision with OVM volumes which // can only be raw, thus making KVM RBD volumes show up as OVM // rather than RBD. This block of code can (hopefully) by checking to @@ -1355,7 +1355,8 @@ public static HypervisorType getHypervisorTypeFromFormat(long dcId, ImageFormat StoragePoolType.PowerFlex, StoragePoolType.CLVM, StoragePoolType.Linstor, - StoragePoolType.FiberChannel).contains(pool.getPoolType())) { + StoragePoolType.FiberChannel, + StoragePoolType.OntapiSCSI).contains(pool.getPoolType())) { // This case will note the presence of non-qcow2 primary stores, suggesting KVM without NFS. Otherwse, // If this check is not passed, the hypervisor type will remain OVM. type = HypervisorType.KVM; From 58d70a24bb3da06fcabf415b1a54a72e08a4176e Mon Sep 17 00:00:00 2001 From: "Srivastava, Piyush" Date: Thu, 17 Sep 2026 16:27:39 +0530 Subject: [PATCH 2/5] UT failure fix --- .../storage/driver/OntapPrimaryDatastoreDriverTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriverTest.java b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriverTest.java index 1c3d2777d0dd..0cfcd2e403c8 100644 --- a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriverTest.java +++ b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriverTest.java @@ -221,6 +221,7 @@ void testCreateAsync_VolumeWithNFS_Success() { when(storagePoolDao.findById(1L)).thenReturn(storagePool); when(storagePool.getId()).thenReturn(1L); when(storagePool.getPoolType()).thenReturn(Storage.StoragePoolType.NetworkFilesystem); + when(storagePool.getHypervisor()).thenReturn(Hypervisor.HypervisorType.KVM); when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(storagePoolDetails); when(volumeDao.findById(100L)).thenReturn(volumeVO); when(volumeVO.getId()).thenReturn(100L); From dbcd92282da16441d51a6e6e355944d23b4704fc Mon Sep 17 00:00:00 2001 From: Sathvika Date: Thu, 8 Oct 2026 18:22:29 +0530 Subject: [PATCH 3/5] =?UTF-8?q?CSTACKEX-245:=20enabling=20VM=20snapshot=20?= =?UTF-8?q?for=20ISCSI=20stopped=20vm's=20in=20UI=20using=E2=80=A6=20(#115?= =?UTF-8?q?)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### Description enabling VM snapshot for ISCSI stopped vm's in UI using ontapiSCSI pool type This PR... ### Types of changes - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] New feature (non-breaking change which adds functionality) - [X] Bug fix (non-breaking change which fixes an issue) - [ ] Enhancement (improves an existing feature and functionality) - [ ] Cleanup (Code refactoring and cleanup, that may add test cases) - [ ] Build/CI - [ ] Test (unit or integration test code) ### Feature/Enhancement Scale or Bug Severity #### Feature/Enhancement Scale - [ ] Major - [ ] Minor #### Bug Severity - [ ] BLOCKER - [ ] Critical - [ ] Major - [X] Minor - [ ] Trivial ### Screenshots (if appropriate): ### How Has This Been Tested? 1) created a ISCSI vm and stopped IT 2) Took a snapshot in stopped state 3) powered On the VM then created a file on it and write in to it. 4) Powered of the vm and reverted to snapshot that was taken when it was in stopped state 5) after revert verified that file and contents are gone before reverting: Screenshot 2026-10-08 at 12 03 39 PM after reverting: Screenshot 2026-10-08 at 12 02 09 PM #### How did you try to break this feature and the system with this change? --- ui/src/config/section/compute.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ui/src/config/section/compute.js b/ui/src/config/section/compute.js index 830997835800..3d4f3fc57e1d 100644 --- a/ui/src/config/section/compute.js +++ b/ui/src/config/section/compute.js @@ -216,7 +216,7 @@ export default { show: (record) => { return record.hypervisor !== 'External' && (((['Running'].includes(record.state) && record.hypervisor !== 'LXC') || (['Stopped'].includes(record.state) && (!['KVM', 'LXC'].includes(record.hypervisor) || - (record.hypervisor === 'KVM' && ['PowerFlex', 'Filesystem', 'NetworkFilesystem', 'SharedMountPoint'].includes(record.pooltype))))) && record.vmtype !== 'sharedfsvm') + (record.hypervisor === 'KVM' && ['PowerFlex', 'Filesystem', 'NetworkFilesystem', 'SharedMountPoint', 'OntapiSCSI'].includes(record.pooltype))))) && record.vmtype !== 'sharedfsvm') }, disabled: (record) => { return record.hostcontrolstate === 'Offline' && record.hypervisor === 'KVM' }, mapping: { From ee919afc821a89b13e0c9210f5de2279f5325e21 Mon Sep 17 00:00:00 2001 From: "Srivastava, Piyush" Date: Wed, 14 Oct 2026 22:59:40 +0530 Subject: [PATCH 4/5] ontap-new-iscsi-pool-type: added migration as well as adapter code for serial number and by-id instead of by-path --- .../kvm/storage/OntapIscsiStorageAdaptor.java | 727 +++++++++++++++++- .../storage/OntapIscsiStorageAdaptorTest.java | 108 ++- .../driver/OntapPrimaryDatastoreDriver.java | 79 +- .../storage/service/UnifiedSANStrategy.java | 31 + .../storage/utils/OntapStorageConstants.java | 19 + .../OntapPrimaryDatastoreDriverTest.java | 11 + 6 files changed, 966 insertions(+), 9 deletions(-) diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/OntapIscsiStorageAdaptor.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/OntapIscsiStorageAdaptor.java index 73e8cb4026c7..4e07a5023ce0 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/OntapIscsiStorageAdaptor.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/OntapIscsiStorageAdaptor.java @@ -16,23 +16,738 @@ // under the License. package com.cloud.hypervisor.kvm.storage; +import java.io.File; +import java.io.FileWriter; +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.regex.Pattern; + +import org.apache.cloudstack.utils.qemu.QemuImg; +import org.apache.cloudstack.utils.qemu.QemuImg.PhysicalDiskFormat; +import org.apache.cloudstack.utils.qemu.QemuImgException; +import org.apache.cloudstack.utils.qemu.QemuImgFile; +import org.apache.commons.lang3.StringUtils; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.libvirt.LibvirtException; + +import com.cloud.agent.api.to.DiskTO; +import com.cloud.storage.Storage; +import com.cloud.storage.Storage.ProvisioningType; import com.cloud.storage.Storage.StoragePoolType; +import com.cloud.utils.exception.CloudRuntimeException; +import com.cloud.utils.script.OutputInterpreter; +import com.cloud.utils.script.Script; /** - * Serves {@link StoragePoolType#OntapiSCSI} pools, which are ONTAP FlexVols exposed over iSCSI with one - * LUN per CloudStack volume. The host-side handling is identical to a generic iSCSI target - * - * The class exists so that ONTAP-specific host behaviour can diverge here without altering the storage - * path of the other vendors that register as {@link StoragePoolType#Iscsi} which all share the superclass. + * Serves {@link StoragePoolType#OntapiSCSI} pools: * * This must stay in the {@code com.cloud.hypervisor.kvm.storage} package: {@link KVMStoragePoolManager} * discovers adaptors by a Reflections scan of that package alone, and an unregistered type silently * falls back to {@link LibvirtStorageAdaptor} rather than failing at startup. */ -public class OntapIscsiStorageAdaptor extends IscsiAdmStorageAdaptor { +public class OntapIscsiStorageAdaptor implements StorageAdaptor { + + protected Logger logger = LogManager.getLogger(getClass()); + private static final Map MAP_STORAGE_UUID_TO_STORAGE_POOL = new HashMap<>(); + private static final String BY_ID_WWN_PREFIX = "/dev/disk/by-id/wwn-0x"; + + /** + * A LUN WWID is NetApp's NAA prefix (type 6, OUI 00a098, vendor nibble 0) followed by the hex of + * a 12-character serial: 32 hex digits. + * Anchoring on the prefix is what stops this adaptor claiming another vendor's WWN device + * when KVMStoragePoolManager.disconnectPhysicalDiskByPath scans every adaptor. + */ + private static final String NETAPP_NAA_PREFIX = "600a0980"; + private static final Pattern LUN_WWID = Pattern.compile(NETAPP_NAA_PREFIX + "[0-9a-f]{24}"); + + private static final String SYS_BLOCK = "/sys/block"; + private static final String SYS_ISCSI_SESSION = "/sys/class/iscsi_session"; + private static final String SESSION_DIR_PREFIX = "session"; + + /** iscsiadm's ISCSI_ERR_NO_OBJS_FOUND: returned by "-m session" when no session is established. */ + private static final int ISCSI_ERR_NO_OBJS_FOUND = 21; + + /** iscsiadm's ISCSI_ERR_SESS_EXISTS: returned by "--login" when already logged in (e.g. Ubuntu). */ + private static final int ISCSI_SESSION_EXISTS_CODE = 15; + + private static final int DEVICE_WAIT_TRIES = 15; + private static final int DEVICE_WAIT_INTERVAL_MS = 1000; + private static final int DEFAULT_ISCSI_PORT = 3260; @Override public StoragePoolType getStoragePoolType() { return StoragePoolType.OntapiSCSI; } + + @Override + public KVMStoragePool createStoragePool(String uuid, String host, int port, String path, String userInfo, + StoragePoolType storagePoolType, Map details, boolean isPrimaryStorage) { + IscsiAdmStoragePool storagePool = new IscsiAdmStoragePool(uuid, host, port, storagePoolType, this); + + MAP_STORAGE_UUID_TO_STORAGE_POOL.put(uuid, storagePool); + + return storagePool; + } + + @Override + public KVMStoragePool getStoragePool(String uuid) { + return MAP_STORAGE_UUID_TO_STORAGE_POOL.get(uuid); + } + + @Override + public KVMStoragePool getStoragePool(String uuid, boolean refreshInfo) { + return MAP_STORAGE_UUID_TO_STORAGE_POOL.get(uuid); + } + + @Override + public boolean deleteStoragePool(String uuid) { + return MAP_STORAGE_UUID_TO_STORAGE_POOL.remove(uuid) != null; + } + + @Override + public boolean deleteStoragePool(KVMStoragePool pool) { + return deleteStoragePool(pool.getUuid()); + } + + @Override + public KVMPhysicalDisk getPhysicalDisk(String volumeUuid, KVMStoragePool pool) { + String devicePath = getDeviceById(getLunWwid(volumeUuid)); + + KVMPhysicalDisk physicalDisk = new KVMPhysicalDisk(devicePath, volumeUuid, pool); + physicalDisk.setFormat(PhysicalDiskFormat.RAW); + + long deviceSize = getDeviceSize(devicePath); + + physicalDisk.setSize(deviceSize); + physicalDisk.setVirtualSize(deviceSize); + + return physicalDisk; + } + + @Override + public boolean connectPhysicalDisk(String volumePath, KVMStoragePool pool, Map details, boolean isVMMigrate) { + final String host = pool.getSourceHost(); + final int port = pool.getSourcePort(); + final String iqn = getTargetIqn(volumePath); + + if (!createIscsiNode(host, port, iqn, volumePath)) { + return false; + } + + if (!applyChapCredentials(host, port, iqn, volumePath, details)) { + return false; + } + + if (!loginOrRescanExistingSession(iqn, host, port, volumePath)) { + return false; + } + + // Logging in can return before the kernel has added the device, so the disk is not usable + // the moment iscsiadm succeeds. Waiting on a non-zero size also guards against reporting + // success when the device never appears, which would otherwise let a caller create a plain + // file where the LUN device was expected. + if (!waitForDeviceToAppear(volumePath, pool)) { + logger.warn("iSCSI device for LUN {} on target {} at {}:{} did not become available", + getLunWwid(volumePath), iqn, host, port); + return false; + } + + return true; + } + + @Override + public boolean disconnectPhysicalDisk(String volumePath, KVMStoragePool pool) { + return disconnectLun(pool.getSourceHost(), pool.getSourcePort(), getTargetIqn(volumePath), getLunWwid(volumePath)); + } + + @Override + public boolean disconnectPhysicalDisk(Map volumeToDisconnect) { + String host = volumeToDisconnect.get(DiskTO.STORAGE_HOST); + String port = volumeToDisconnect.get(DiskTO.STORAGE_PORT); + String path = volumeToDisconnect.get(DiskTO.IQN); + + if (host == null || port == null || path == null) { + return false; + } + + return disconnectLun(host, Integer.parseInt(port), getTargetIqn(path), getLunWwid(path)); + } + + /** + * Claims the by-id devices this adaptor hands out. + * + * The target IQN and portal are not recoverable from a by-id name, so they are read back from + * the device's iSCSI session in sysfs. + * + * Returning false for anything else is required by the {@link StorageAdaptor} contract: + * {@code KVMStoragePoolManager.disconnectPhysicalDiskByPath} scans every adaptor and stops at + * the first one that claims the path. + */ + @Override + public boolean disconnectPhysicalDiskByPath(String localPath) { + if (!isOntapDevicePath(localPath)) { + return false; + } + + String kernelDevice = resolveKernelDevice(localPath); + if (kernelDevice == null) { + logger.info("Device {} is already gone, nothing to disconnect", localPath); + return true; + } + + Integer sessionId = findSessionId(kernelDevice); + String iqn = sessionId == null ? null : readSessionAttribute(sessionId, "targetname"); + if (iqn == null) { + // A real ONTAP LUN always sits under an iSCSI session. If this one does not, it is not + // ours, so leave it alone and let the manager try the next adaptor. + logger.debug("Device {} ({}) is not on a readable iSCSI session, not claiming it", + localPath, kernelDevice); + return false; + } + + return disconnectLun(null, 0, iqn, localPath.substring(BY_ID_WWN_PREFIX.length())); + } + + @Override + public KVMPhysicalDisk copyPhysicalDisk(KVMPhysicalDisk disk, String name, KVMStoragePool destPool, int timeout) { + return copyPhysicalDisk(disk, name, destPool, timeout, null, null, null); + } + + @Override + public KVMPhysicalDisk copyPhysicalDisk(KVMPhysicalDisk srcDisk, String destVolumeUuid, KVMStoragePool destPool, + int timeout, byte[] srcPassphrase, byte[] destPassphrase, ProvisioningType provisioningType) { + KVMStoragePool srcPool = srcDisk.getPool(); + QemuImgFile srcFile = srcPool.getType() == StoragePoolType.RBD + ? new QemuImgFile(KVMPhysicalDisk.RBDStringBuilder(srcPool, srcDisk.getPath()), srcDisk.getFormat()) + : new QemuImgFile(srcDisk.getPath(), srcDisk.getFormat()); + + KVMPhysicalDisk destDisk = destPool.getPhysicalDisk(destVolumeUuid); + QemuImgFile destFile = new QemuImgFile(destDisk.getPath(), destDisk.getFormat()); + + try { + new QemuImg(timeout).convert(srcFile, destFile); + + // A small template can still be sitting in the page cache when convert returns. The LUN + // is disconnected right after a copy, so without an explicit flush that data would never + // reach the array and the copy would be reported successful while the LUN stayed empty. + flushToDevice(destDisk.getPath()); + } catch (QemuImgException | LibvirtException ex) { + String msg = "Failed to copy data from " + srcDisk.getPath() + " to " + destDisk.getPath() + + ". The error was the following: " + ex.getMessage(); + logger.error(msg); + throw new CloudRuntimeException(msg); + } + + return destPool.getPhysicalDisk(destVolumeUuid); + } + + @Override + public boolean refresh(KVMStoragePool pool) { + return true; + } + + @Override + public KVMPhysicalDisk createPhysicalDisk(String volumeUuid, KVMStoragePool pool, PhysicalDiskFormat format, + ProvisioningType provisioningType, long size, byte[] passphrase) { + throw new UnsupportedOperationException("Creating a physical disk is not supported; ONTAP LUNs are provisioned by the management server."); + } + + @Override + public boolean deletePhysicalDisk(String volumeUuid, KVMStoragePool pool, Storage.ImageFormat format) { + throw new UnsupportedOperationException("Deleting a physical disk is not supported; ONTAP LUNs are removed by the management server."); + } + + @Override + public List listPhysicalDisks(String storagePoolUuid, KVMStoragePool pool) { + throw new UnsupportedOperationException("Listing disks is not supported for this configuration."); + } + + @Override + public KVMPhysicalDisk createDiskFromTemplate(KVMPhysicalDisk template, String name, PhysicalDiskFormat format, + ProvisioningType provisioningType, long size, KVMStoragePool destPool, + int timeout, byte[] passphrase) { + throw new UnsupportedOperationException("Creating a disk from a template is not supported for this configuration."); + } + + @Override + public KVMPhysicalDisk createTemplateFromDisk(KVMPhysicalDisk disk, String name, PhysicalDiskFormat format, long size, KVMStoragePool destPool) { + throw new UnsupportedOperationException("Creating a template from a disk is not supported for this configuration."); + } + + @Override + public boolean createFolder(String uuid, String path) { + return createFolder(uuid, path, null); + } + + @Override + public boolean createFolder(String uuid, String path, String localPath) { + throw new UnsupportedOperationException("A folder cannot be created in this configuration."); + } + + @Override + public KVMPhysicalDisk createDiskFromTemplateBacking(KVMPhysicalDisk template, String name, PhysicalDiskFormat format, + long size, KVMStoragePool destPool, int timeout, byte[] passphrase) { + return null; + } + + @Override + public KVMPhysicalDisk createTemplateFromDirectDownloadFile(String templateFilePath, String destTemplatePath, + KVMStoragePool destPool, Storage.ImageFormat format, int timeout) { + return null; + } + + private boolean createIscsiNode(String host, int port, String iqn, String volumePath) { + String result = runIscsiadmNodeCommand(host, port, iqn, "-o", "new"); + + if (result == null) { + logger.debug("Added iSCSI node for target {}", iqn); + return true; + } + if (result.toLowerCase().contains("exists")) { + logger.debug("iSCSI node already exists for target {}, proceeding", iqn); + return true; + } + logger.warn("Failed to add iSCSI node for {}: {}", volumePath, result); + return false; + } + + private boolean applyChapCredentials(String host, int port, String iqn, String volumePath, Map details) { + if (details == null) { + return true; + } + + String username = details.get(DiskTO.CHAP_INITIATOR_USERNAME); + String secret = details.get(DiskTO.CHAP_INITIATOR_SECRET); + + if (!StringUtils.isNoneBlank(username, secret)) { + return true; + } + + return updateNodeSetting(host, port, iqn, "node.session.auth.authmethod", "CHAP", volumePath) + && updateNodeSetting(host, port, iqn, "node.session.auth.username", username, volumePath) + && updateNodeSetting(host, port, iqn, "node.session.auth.password", secret, volumePath); + } + + private boolean updateNodeSetting(String host, int port, String iqn, String name, String value, String volumePath) { + String result = runIscsiadmNodeCommand(host, port, iqn, "--op", "update", "-n", name, "-v", value); + if (result != null) { + // The value is not logged: for the CHAP settings it is a credential. + logger.warn("Failed to set {} on iSCSI target {} for {}: {}", name, iqn, volumePath, result); + return false; + } + return true; + } + + /** + * Logs in, rescanning instead when the session was already established. + * + * Login is idempotent but its exit status is not portable: re-login exits 0 on some + * distributions and ISCSI_ERR_SESS_EXISTS on others, where it also produces an error message + * that would otherwise read as a failure. The session is therefore checked beforehand and a + * pre-existing session treated as success. + */ + private boolean loginOrRescanExistingSession(String iqn, String host, int port, String volumePath) { + boolean sessionAlreadyActive = isIscsiSessionActive(iqn, host); + + Script login = new Script(true, "iscsiadm", 0, logger); + login.add("-m", "node"); + login.add("-T", iqn); + login.add("-p", host + ":" + port); + login.add("--login"); + + String result = login.execute(); + boolean sessionPreExisted = login.getExitValue() == ISCSI_SESSION_EXISTS_CODE || sessionAlreadyActive; + + if (sessionPreExisted) { + logger.debug("iSCSI session for target {} at {}:{} pre-existed, rescanning", iqn, host, port); + rescanIscsiSession(host, port, iqn); + return true; + } + if (result == null) { + logger.debug("Logged in to iSCSI target {} for {}", iqn, volumePath); + return true; + } + logger.warn("Failed to log in to iSCSI target {} for {}: {}", iqn, volumePath, result); + return false; + } + + /** + * Reports whether a session to this target and portal already exists. + * + * ISCSI_ERR_NO_OBJS_FOUND simply means no session exists, which is a normal outcome here; any + * other non-zero exit is treated as "not confirmed active" so that login is still attempted. + */ + private boolean isIscsiSessionActive(String iqn, String host) { + Script sessionCmd = new Script(true, "iscsiadm", 0, logger); + sessionCmd.add("-m", "session"); + + OutputInterpreter.AllLinesParser parser = new OutputInterpreter.AllLinesParser(); + sessionCmd.executeIgnoreExitValue(parser, ISCSI_ERR_NO_OBJS_FOUND); + + int exitValue = sessionCmd.getExitValue(); + if (exitValue != 0 && exitValue != ISCSI_ERR_NO_OBJS_FOUND) { + logger.warn("Unable to determine iSCSI session state for target {} at {}: 'iscsiadm -m session' exited with {}", + iqn, host, exitValue); + return false; + } + + String sessions = parser.getLines(); + if (StringUtils.isBlank(sessions)) { + return false; + } + for (String line : sessions.split("\n")) { + if (line.contains(iqn) && line.contains(host)) { + return true; + } + } + return false; + } + + private void rescanIscsiSession(String host, int port, String iqn) { + String result = runIscsiadmNodeCommand(host, port, iqn, "--rescan"); + if (result != null) { + logger.warn("iSCSI session rescan of target {} returned: {}", iqn, result); + } + } + + /** + * Releases one LUN, tearing the session down only once it is the last one on the target. + * + * ONTAP presents all of an SVM's LUNs through a single IQN, so {@code iscsiadm --logout} would + * drop every LUN on the session rather than just this one. While siblings remain, the LUN is + * released by deleting its SCSI device through sysfs instead; the kernel does not remove + * devices from a live session on its own when the array unmaps a LUN. + * + * @param host the portal address, or null to read it back from the device's session + */ + private boolean disconnectLun(String host, int port, String iqn, String lunWwid) { + String devicePath = getDeviceById(lunWwid); + String kernelDevice = resolveKernelDevice(devicePath); + + if (kernelDevice == null) { + logger.info("Device {} for LUN {} on target {} is already gone; nothing to disconnect", devicePath, lunWwid, iqn); + return true; + } + + Integer sessionId = findSessionId(kernelDevice); + if (sessionId == null) { + logger.warn("Unable to resolve the iSCSI session id for device {} (LUN {}, target {}); " + + "Removing the SCSI device only ", kernelDevice, lunWwid, iqn); + removeScsiDevice(kernelDevice); + return true; + } + if (hasOtherLunsInSession(sessionId, kernelDevice)) { + logger.info("Skipping iSCSI logout for LUN {} on target {}: other LUNs on the same session are still " + + "active. Removing device {} only.", lunWwid, iqn, kernelDevice); + removeScsiDevice(kernelDevice); + return true; + } + + if (host == null) { + host = readConnectionAttribute(sessionId, "persistent_address"); + port = parsePort(readConnectionAttribute(sessionId, "persistent_port")); + } + if (host == null) { + logger.warn("Unable to determine the portal of target {} even with sessionId={}; removing device {} without logging out", + iqn, sessionId, kernelDevice); + removeScsiDevice(kernelDevice); + return true; + } + + if (!logoutAndForgetTarget(host, port, iqn, lunWwid)) { + return false; + } + + waitForDeviceToDisappear(devicePath); + return true; + } + + private boolean logoutAndForgetTarget(String host, int port, String iqn, String lunWwid) { + String logoutResult = runIscsiadmNodeCommand(host, port, iqn, "--logout"); + if (logoutResult != null) { + logger.warn("Failed to log out of iSCSI target {} while releasing LUN {}: {}", iqn, lunWwid, logoutResult); + return false; + } + logger.debug("Logged out of iSCSI target {} while releasing LUN {}", iqn, lunWwid); + + String deleteResult = runIscsiadmNodeCommand(host, port, iqn, "-o", "delete"); + if (deleteResult != null) { + logger.warn("Failed to delete the iSCSI node record for target {}: {}", iqn, deleteResult); + return false; + } + logger.debug("Deleted the iSCSI node record for target {}", iqn); + + return true; + } + + /** @return null on success, or the command output describing the failure */ + private String runIscsiadmNodeCommand(String host, int port, String iqn, String... operation) { + Script command = new Script(true, "iscsiadm", 0, logger); + command.add("-m", "node"); + command.add("-T", iqn); + command.add("-p", host + ":" + port); + for (String argument : operation) { + command.add(argument); + } + return command.execute(); + } + + // --------------------------------------------------------------------------------------------- + // Device and sysfs handling + // --------------------------------------------------------------------------------------------- + + /** + * Removes a single SCSI device from the kernel, the standard alternative to tearing down the + * whole session. The kernel removes the device's udev aliases once it processes the delete. + */ + private void removeScsiDevice(String kernelDevice) { + File deleteFile = new File(SYS_BLOCK + "/" + kernelDevice + "/device/delete"); + if (!deleteFile.exists()) { + logger.warn("No sysfs delete entry for device {}; cannot remove it", kernelDevice); + return; + } + try (FileWriter writer = new FileWriter(deleteFile)) { + writer.write("1"); + logger.info("Removed SCSI device {} via sysfs", kernelDevice); + } catch (IOException ex) { + logger.warn("Failed to remove SCSI device {}: {}", kernelDevice, ex.getMessage()); + } + } + + /** + * Reports whether the session carries any LUN other than {@code ownDevice}. + * + * Every SCSI disk reachable through an iSCSI session has that session in its sysfs device path, + * so comparing session ids across {@code /sys/block} identifies the siblings without needing to + * know any logical unit numbers. + */ + private boolean hasOtherLunsInSession(int sessionId, String ownDevice) { + File[] blockDevices = new File(SYS_BLOCK).listFiles(); + if (blockDevices == null) { + return false; + } + for (File blockDevice : blockDevices) { + String name = blockDevice.getName(); + if (name.equals(ownDevice)) { + continue; + } + Integer otherSessionId = findSessionId(name); + if (otherSessionId != null && otherSessionId == sessionId) { + logger.debug("Device {} is another LUN on iSCSI session {}", name, sessionId); + return true; + } + } + return false; + } + + /** + * Extracts the iSCSI session id from a block device's sysfs path, which looks like + * {@code /sys/devices/platform/host33/session13/target33:0:0/33:0:0:1/block/sdc}. + * + * @return the session id, or null if the device is not backed by iSCSI + */ + private Integer findSessionId(String kernelDevice) { + try { + Path deviceLink = Paths.get(SYS_BLOCK, kernelDevice, "device"); + if (!Files.exists(deviceLink)) { + return null; + } + for (Path element : deviceLink.toRealPath()) { + String name = element.toString(); + if (name.startsWith(SESSION_DIR_PREFIX)) { + return Integer.parseInt(name.substring(SESSION_DIR_PREFIX.length())); + } + } + } catch (IOException | NumberFormatException ex) { + logger.debug("Unable to determine the iSCSI session of device {}: {}", kernelDevice, ex.getMessage()); + } + return null; + } + + private String readSessionAttribute(int sessionId, String attribute) { + return readSysfsValue(Paths.get(SYS_ISCSI_SESSION, SESSION_DIR_PREFIX + sessionId, attribute)); + } + + /** + * Reads a connection attribute of a session. A connection's own index is not guaranteed to + * match its session's, so the connection directory is discovered under the session rather than + * assumed to be {@code connection:0}. + */ + private String readConnectionAttribute(int sessionId, String attribute) { + Path sessionDevice = Paths.get(SYS_ISCSI_SESSION, SESSION_DIR_PREFIX + sessionId, "device"); + File[] children = sessionDevice.toFile().listFiles(); + if (children == null) { + return null; + } + for (File child : children) { + String name = child.getName(); + if (!name.startsWith("connection")) { + continue; + } + String value = readSysfsValue(sessionDevice.resolve(name).resolve("iscsi_connection").resolve(name).resolve(attribute)); + if (value != null) { + return value; + } + } + return null; + } + + private String readSysfsValue(Path path) { + try { + if (!Files.exists(path)) { + return null; + } + String value = new String(Files.readAllBytes(path)).trim(); + return value.isEmpty() ? null : value; + } catch (IOException ex) { + logger.debug("Unable to read {}: {}", path, ex.getMessage()); + return null; + } + } + + private int parsePort(String port) { + try { + return port != null ? Integer.parseInt(port) : DEFAULT_ISCSI_PORT; + } catch (NumberFormatException ex) { + return DEFAULT_ISCSI_PORT; + } + } + + private String resolveKernelDevice(String devicePath) { + try { + Path link = Paths.get(devicePath); + if (!Files.exists(link)) { + return null; + } + return link.toRealPath().getFileName().toString(); + } catch (IOException ex) { + logger.debug("Unable to resolve {}: {}", devicePath, ex.getMessage()); + return null; + } + } + + private boolean waitForDeviceToAppear(String volumePath, KVMStoragePool pool) { + for (int attempt = 0; attempt < DEVICE_WAIT_TRIES; attempt++) { + if (getPhysicalDisk(volumePath, pool).getSize() > 0) { + return true; + } + if (!sleepBetweenAttempts()) { + return false; + } + } + return getPhysicalDisk(volumePath, pool).getSize() > 0; + } + + private void waitForDeviceToDisappear(String devicePath) { + for (int attempt = 0; attempt < DEVICE_WAIT_TRIES && getDeviceSize(devicePath) > 0; attempt++) { + if (!sleepBetweenAttempts()) { + return; + } + } + } + + private boolean sleepBetweenAttempts() { + try { + Thread.sleep(DEVICE_WAIT_INTERVAL_MS); + return true; + } catch (InterruptedException ex) { + Thread.currentThread().interrupt(); + return false; + } + } + + /** + * @return the device size in bytes, or 0 if the device is absent or not a block device + */ + private long getDeviceSize(String devicePath) { + Path path = Paths.get(devicePath); + + if (!Files.exists(path)) { + logger.debug("Device does not exist yet: {}", devicePath); + return 0L; + } + if (Files.isRegularFile(path)) { + // A plain file here means something wrote to the device name before the LUN appeared; + // treating it as a disk would silently back a volume with local storage. + logger.warn("Found a regular file at {} where a block device was expected; it must be removed manually", devicePath); + return 0L; + } + + Script command = new Script(true, "blockdev", 0, logger); + command.add("--getsize64", devicePath); + + OutputInterpreter.OneLineParser parser = new OutputInterpreter.OneLineParser(); + String result = command.execute(parser); + + if (result != null) { + logger.warn("Unable to get the size of device {}: {}", devicePath, result); + return 0L; + } + + try { + return Long.parseLong(parser.getLine().trim()); + } catch (NumberFormatException | NullPointerException ex) { + logger.warn("Unable to parse the size of device {}", devicePath); + return 0L; + } + } + + private void flushToDevice(String devicePath) { + Script flush = new Script(true, "blockdev", 0, logger); + flush.add("--flushbufs", devicePath); + String flushResult = flush.execute(); + if (flushResult != null) { + logger.warn("blockdev --flushbufs on {} returned: {}", devicePath, flushResult); + } + new Script(true, "sync", 0, logger).execute(); + logger.debug("Flushed buffers to {}", devicePath); + } + + // --------------------------------------------------------------------------------------------- + // Volume path handling + // --------------------------------------------------------------------------------------------- + + private boolean isOntapDevicePath(String localPath) { + return localPath != null + && localPath.startsWith(BY_ID_WWN_PREFIX) + && LUN_WWID.matcher(localPath.substring(BY_ID_WWN_PREFIX.length())).matches(); + } + + private String getDeviceById(String lunWwid) { + return BY_ID_WWN_PREFIX + lunWwid; + } + + private String getTargetIqn(String volumePath) { + return getPathComponent(volumePath, 1); + } + + private String getLunWwid(String volumePath) { + return getPathComponent(volumePath, 2); + } + + /** + * Splits a {@code //} volume path. + * + * The WWID occupies the component a logical unit number would hold for other iSCSI vendors, so + * the path keeps the two-component shape every hypervisor resource expects. That is also why + * the path carries the WWID rather than the ONTAP serial it encodes: serials contain '/'. + */ + private String getPathComponent(String volumePath, int index) { + String[] components = volumePath == null ? new String[0] : volumePath.split("/"); + + if (components.length != 3) { + String message = "Wrong format for ONTAP iSCSI path: " + volumePath + + ". It should be formatted as '/targetIQN/lunWwid'."; + logger.warn(message); + throw new CloudRuntimeException(message); + } + + return components[index].trim(); + } } diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/OntapIscsiStorageAdaptorTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/OntapIscsiStorageAdaptorTest.java index c89094081d35..dc3ced853f5c 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/OntapIscsiStorageAdaptorTest.java +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/OntapIscsiStorageAdaptorTest.java @@ -19,6 +19,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; import java.lang.reflect.Modifier; @@ -29,6 +30,7 @@ import org.reflections.Reflections; import com.cloud.storage.Storage.StoragePoolType; +import com.cloud.utils.exception.CloudRuntimeException; public class OntapIscsiStorageAdaptorTest { @@ -72,7 +74,111 @@ public void adaptorSatisfiesThePoolManagerDiscoveryContract() throws ReflectiveO StorageAdaptor adaptor = OntapIscsiStorageAdaptor.class.getDeclaredConstructor().newInstance(); assertEquals(StoragePoolType.OntapiSCSI, adaptor.getStoragePoolType()); - assertEquals("The superclass must keep serving the other iSCSI vendors", + assertEquals("IscsiAdmStorageAdaptor must keep serving the other iSCSI vendors", StoragePoolType.Iscsi, new IscsiAdmStorageAdaptor().getStoragePoolType()); } + + /** + * The two adaptors are deliberately unrelated: device naming runs through nearly every method + * that does real work, so inheriting IscsiAdmStorageAdaptor coupled them through behaviour + * neither could change safely. Registration only requires implementing StorageAdaptor, and + * KVMStoragePoolManager keys adaptors on getStoragePoolType(), so nothing depends on a shared + * superclass. Asserting it keeps a later "reuse" refactor from quietly restoring by-path + * naming through an un-overridden inherited method. + */ + @Test + public void adaptorDoesNotInheritTheByPathBasedIscsiAdaptor() { + assertTrue("The adaptor must implement StorageAdaptor directly", + StorageAdaptor.class.isAssignableFrom(OntapIscsiStorageAdaptor.class)); + assertFalse("The adaptor must not extend the by-path based iSCSI adaptor", + IscsiAdmStorageAdaptor.class.isAssignableFrom(OntapIscsiStorageAdaptor.class)); + assertEquals("It should sit directly on StorageAdaptor, with no intermediate base class", + Object.class, OntapIscsiStorageAdaptor.class.getSuperclass()); + } + + /** + * The WWID occupies the component the superclass reads a logical unit number from, which is what + * keeps the path at the two components every hypervisor resource's '/targetIQN/LUN' parsing + * demands. ONTAP serial numbers contain '/' (byte 0x2f), so carrying the raw serial here instead + * of its hex WWID would split into four components and throw. + */ + @Test + public void physicalDiskIsNamedByLunWwidRatherThanLogicalUnitNumber() { + OntapIscsiStorageAdaptor adaptor = new OntapIscsiStorageAdaptor(); + KVMStoragePool pool = adaptor.createStoragePool("ontap-iscsi-pool-uuid", "10.196.37.157", 3260, null, null, + StoragePoolType.OntapiSCSI, null, true); + + String targetIqn = "iqn.1992-08.com.netapp:sn.45048fd9b65111f1b106005056bd83cf:vs.3"; + String lunWwid = "600a098078304d2d383f2f6b45734a54"; + + KVMPhysicalDisk disk = adaptor.getPhysicalDisk("/" + targetIqn + "/" + lunWwid, pool); + + assertEquals("/dev/disk/by-id/wwn-0x" + lunWwid, disk.getPath()); + assertFalse("The reported path must carry nothing host-specific", disk.getPath().contains("lun-")); + assertFalse("The reported path must not be derived from by-path", disk.getPath().contains("by-path")); + assertEquals(PhysicalDiskFormat.RAW, disk.getFormat()); + } + + @Test + public void malformedVolumePathIsRejected() { + OntapIscsiStorageAdaptor adaptor = new OntapIscsiStorageAdaptor(); + KVMStoragePool pool = adaptor.createStoragePool("ontap-iscsi-pool-uuid", "10.196.37.157", 3260, null, null, + StoragePoolType.OntapiSCSI, null, true); + + // A raw ONTAP serial such as 'x0M-8?/kEsJT' would land here, splitting into four components. + assertThrows(CloudRuntimeException.class, + () -> adaptor.getPhysicalDisk("/iqn.1992-08.com.netapp:sn.abc:vs.3/x0M-8?/kEsJT", pool)); + assertThrows(CloudRuntimeException.class, + () -> adaptor.getPhysicalDisk("/iqn.1992-08.com.netapp:sn.abc:vs.3", pool)); + } + + /** + * KVMStoragePoolManager.disconnectPhysicalDiskByPath walks every registered adaptor and stops at + * the first one returning true, so an adaptor that over-claims tears down another vendor's + * session. By-path devices still belong to the superclass, which serves the other iSCSI vendors. + */ + @Test + public void disconnectByPathOnlyClaimsTheDevicesThisAdaptorHandsOut() { + OntapIscsiStorageAdaptor adaptor = new OntapIscsiStorageAdaptor(); + + assertFalse("A by-path device is the superclass's to disconnect", adaptor.disconnectPhysicalDiskByPath( + "/dev/disk/by-path/ip-10.0.0.1:3260-iscsi-iqn.1992-08.com.netapp:sn.abc:vs.3-lun-0")); + assertFalse("PowerFlex publishes its own by-id names", + adaptor.disconnectPhysicalDiskByPath("/dev/disk/by-id/emc-vol-1235dc1s0-4a2f6b45")); + assertFalse("A partition is not the LUN itself", + adaptor.disconnectPhysicalDiskByPath("/dev/disk/by-id/wwn-0x600a098078304d2d383f2f6b45734a51-part1")); + assertFalse("Only the wwn-0x form is handed out, never the scsi-3 alias", + adaptor.disconnectPhysicalDiskByPath("/dev/disk/by-id/scsi-3600a098078304d2d383f2f6b45734a51")); + assertFalse("A bare kernel name carries no identity", adaptor.disconnectPhysicalDiskByPath("/dev/sdb")); + assertFalse(adaptor.disconnectPhysicalDiskByPath(null)); + } + + /** + * Other vendors publish the same wwn-0x plus 32 hex shape. Only NetApp's NAA prefix is ours, so a + * different array's LUN or a local RAID volume must fall through to the next adaptor. + */ + @Test + public void disconnectByPathDoesNotClaimOtherVendorsWwnDevices() { + OntapIscsiStorageAdaptor adaptor = new OntapIscsiStorageAdaptor(); + + assertFalse("Pure FlashArray also publishes wwn-0x names", + adaptor.disconnectPhysicalDiskByPath("/dev/disk/by-id/wwn-0x624a9370f1f4f5a4b0d3c2e1000113aa")); + assertFalse("HPE Primera also publishes wwn-0x names", + adaptor.disconnectPhysicalDiskByPath("/dev/disk/by-id/wwn-0x60002ac0000000000000012300020a1b")); + assertFalse("A local RAID volume also publishes wwn-0x names", + adaptor.disconnectPhysicalDiskByPath("/dev/disk/by-id/wwn-0x600508b1001c4d2e8a0f6b1c2d3e4f50")); + assertFalse("NetApp E-Series is not managed by this adaptor", + adaptor.disconnectPhysicalDiskByPath("/dev/disk/by-id/wwn-0x600a0b800029e3a4000012345f6a7b8c")); + } + + /** + * A claimed device that has already gone reports success: there is no session left to tear down, + * and returning false would send the manager on to adaptors that would mishandle the path. + */ + @Test + public void disconnectByPathSucceedsWhenTheClaimedDeviceIsAlreadyGone() { + OntapIscsiStorageAdaptor adaptor = new OntapIscsiStorageAdaptor(); + + assertTrue(adaptor.disconnectPhysicalDiskByPath("/dev/disk/by-id/wwn-0x600a098078304d2d383f2f6b45734a51")); + } } 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 f90d37cf7746..25a6c8f750c8 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 @@ -24,6 +24,7 @@ import com.cloud.agent.api.to.DataObjectType; import com.cloud.agent.api.to.DataStoreTO; import com.cloud.agent.api.to.DataTO; +import com.cloud.agent.api.to.DiskTO; import com.cloud.exception.InvalidParameterValueException; import com.cloud.host.Host; import com.cloud.host.HostVO; @@ -76,6 +77,7 @@ import javax.inject.Inject; import java.util.ArrayList; import java.util.Arrays; +import java.nio.charset.StandardCharsets; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -117,6 +119,18 @@ public boolean volumesRequireGrantAccessWhenUsed() { return true; } + @Override + public boolean zoneWideVolumesAvailableWithoutClusterMotion(){ + logger.trace("zoneWideVolumesAvailableWithoutClusterMotion invoked"); + return true; + } + + @Override + public boolean requiresAccessForMigration(DataObject dataObject) { + logger.trace("requiresAccessForMigration invoked"); + return true; + } + /** * Creates a volume on the ONTAP storage system. */ @@ -447,9 +461,12 @@ private void grantAccessIscsi(Host host, VolumeVO volumeVO, Map 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); + String lunWwid = resolveVolumeLunWwid(sanStrategy, svmName, cloudStackVolumeName, volumeVO.getId()); + logger.debug("grantAccessIscsi: LUN [{}] mapped to igroup [{}] as logical unit number [{}], WWID [{}]", + cloudStackVolumeName, accessGroupName, lunNumber, lunWwid); - // Update volume path if changed (e.g., after migration or re-mapping) - String iscsiPath = OntapStorageConstants.SLASH + storagePool.getPath() + OntapStorageConstants.SLASH + lunNumber; + // Update volume path if changed (e.g. the WWID was not yet known for a pre-existing volume) + String iscsiPath = buildIscsiPath(storagePool, lunWwid); if (volumeVO.getPath() == null || !volumeVO.getPath().equals(iscsiPath)) { volumeVO.set_iScsiName(iscsiPath); volumeVO.setPath(iscsiPath); @@ -1077,4 +1094,62 @@ private void updateSnapshotDetails(long csSnapshotId, long csVolumeId, String fl } } + /** + * Builds the volume path the KVM agent expects for managed iSCSI: {@code //}. + */ + private String buildIscsiPath(StoragePoolVO storagePool, String lunWwid) { + return OntapStorageConstants.SLASH + storagePool.getPath() + OntapStorageConstants.SLASH + lunWwid; + } + + /** + * Resolves a volume's LUN WWID, caching it as a volume detail after the first lookup. + * + *

Resolved at grant time rather than at create time so that volumes created before this + * became the path format pick the WWID up on their next start.

+ */ + private String resolveVolumeLunWwid(UnifiedSANStrategy sanStrategy, String svmName, String lunName, long volumeId) { + VolumeDetailVO cached = volumeDetailsDao.findDetail(volumeId, DiskTO.SCSI_NAA_DEVICE_ID); + if (cached != null && cached.getValue() != null && !cached.getValue().isEmpty()) { + return cached.getValue(); + } + + String serialNumber = fetchLunSerialNumber(sanStrategy, svmName, lunName); + String wwid = toNaaWwid(serialNumber); + + volumeDetailsDao.addDetail(volumeId, OntapStorageConstants.LUN_DOT_SERIAL_NUMBER, serialNumber, false); + volumeDetailsDao.addDetail(volumeId, DiskTO.SCSI_NAA_DEVICE_ID, wwid, false); + return wwid; + } + + private String fetchLunWwid(UnifiedSANStrategy sanStrategy, String svmName, String lunName) { + return toNaaWwid(fetchLunSerialNumber(sanStrategy, svmName, lunName)); + } + + private String fetchLunSerialNumber(UnifiedSANStrategy sanStrategy, String svmName, String lunName) { + String serialNumber = sanStrategy.getLunSerialNumber(svmName, lunName); + if (serialNumber == null || serialNumber.isEmpty()) { + throw new CloudRuntimeException("ONTAP did not report a serial number for LUN " + lunName + + " on SVM " + svmName + "; cannot build a host-independent device path"); + } + return serialNumber; + } + + /** + * Derives a LUN's SCSI WWID from its ONTAP serial number. + * + *

NetApp builds the NAA IEEE Registered Extended identifier as the format designator ({@code 6}), + * its OUI ({@code 00a098}), a 4-bit pad nibble ({@code 0}), and then the ASCII bytes of the 12-char + * serial number in hex. For serial {@code x0M-8?/kEsJT} this yields + * {@code 600a098078304d2d383f2f6b45734a54}, which the host exposes as + * {@code /dev/disk/by-id/wwn-0x600a098078304d2d383f2f6b45734a54} and vSphere shows as + * {@code naa.600a098078304d2d383f2f6b45734a54}.

+ */ + private String toNaaWwid(String serialNumber) { + StringBuilder wwid = new StringBuilder(OntapStorageConstants.NETAPP_NAA_PREFIX); + for (byte b : serialNumber.getBytes(StandardCharsets.US_ASCII)) { + wwid.append(String.format("%02x", b)); + } + return wwid.toString(); + } + } 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 2b0e65f9f7ea..31d4523c6290 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 @@ -591,4 +591,35 @@ public JobResponse revertSnapshotForCloudStackVolume(String snapshotName, String return getSnapshotFeignClient().restoreFileFromSnapshotCli(authHeader, restoreRequest); } + + public String getLunSerialNumber(String svmName, String lunName) { + logger.trace("getLunSerialNumber: fetching serial number of LUN [{}] on SVM [{}]", lunName, svmName); + if (svmName == null || svmName.isEmpty() || lunName == null || lunName.isEmpty()) { + throw new CloudRuntimeException("Failed to get LUN serial number, invalid svm or LUN name"); + } + try { + String authHeader = OntapStorageUtils.generateAuthHeader(storage.getUsername(), storage.getPassword()); + Map queryParams = Map.of( + OntapStorageConstants.SVM_DOT_NAME, svmName, + OntapStorageConstants.NAME, lunName, + OntapStorageConstants.FIELDS, OntapStorageConstants.SERIAL_NUMBER); + OntapResponse lunResponse = sanFeignClient.getLunResponse(authHeader, queryParams); + if (lunResponse == null || lunResponse.getRecords() == null || lunResponse.getRecords().isEmpty()) { + logger.warn("getLunSerialNumber: LUN [{}] on SVM [{}] not found", lunName, svmName); + return null; + } + return lunResponse.getRecords().get(0).getSerialNumber(); + } catch (FeignException e) { + if (e.status() == 404) { + logger.warn("getLunSerialNumber: LUN [{}] on SVM [{}] not found (status 404)", lunName, svmName); + return null; + } + logger.error("FeignException while fetching serial number of LUN [{}], Status: {}, Exception: {}", + lunName, e.status(), e.getMessage()); + throw new CloudRuntimeException("Failed to fetch LUN serial number: " + e.getMessage()); + } catch (Exception e) { + logger.error("Exception while fetching serial number of LUN [{}]: {}", lunName, e.getMessage()); + throw new CloudRuntimeException("Failed to fetch LUN serial number: " + e.getMessage()); + } + } } diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageConstants.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageConstants.java index e5224237e526..b0cf867e7626 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageConstants.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageConstants.java @@ -91,6 +91,7 @@ public class OntapStorageConstants { public static final String LUN_DOT_NAME = "lun.name"; public static final String IQN = "iqn"; public static final String LUN_DOT_UUID = "lun.uuid"; + public static final String LUN_DOT_SERIAL_NUMBER = "lun.serial_number"; 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"; @@ -109,4 +110,22 @@ public class OntapStorageConstants { /** vm_snapshot_details key for ONTAP FlexVolume-level VM snapshots. */ public static final String ONTAP_FLEXVOL_SNAPSHOT = "ontapFlexVolSnapshot"; + + public static final String SERIAL_NUMBER = "serial_number"; + + /** + * NAA format designator. + * {@code 6} = "IEEE Registered Extended" (128-bit WWID). + */ + public static final String NAA_IEEE_REG_EXT = "6"; + /** + * NetApp's IEEE-registered OUI. Lookup: https://standards-oui.ieee.org/ → "NetApp, Inc." + */ + public static final String NETAPP_OUI = "00a098"; + /** + * the 4-bit pad nibble + *

The full WWID is {@code NETAPP_NAA_PREFIX} + {@code serial_number_hex} (12 ASCII + * bytes of the LUN serial, hex-encoded), yielding 32 hex chars / 128 bits.

+ */ + public static final String NETAPP_NAA_PREFIX = NAA_IEEE_REG_EXT + NETAPP_OUI + "0"; } diff --git a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriverTest.java b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriverTest.java index 0cfcd2e403c8..e72a966161cb 100644 --- a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriverTest.java +++ b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriverTest.java @@ -78,6 +78,15 @@ @ExtendWith(MockitoExtension.class) class OntapPrimaryDatastoreDriverTest { + /** + * A real serial number taken from an ONTAP array, and the WWID NetApp derives from it as its + * OUI plus the serial's ASCII bytes in hex. The serial deliberately contains a '/', which is + * why volume paths carry the WWID: the raw serial would break the '/targetIQN/LUN' parsing that + * every hypervisor resource applies to the path. + */ + private static final String LUN_SERIAL_NUMBER = "x0M-8?/kEsJT"; + private static final String LUN_WWID = "600a098078304d2d383f2f6b45734a54"; + @Mock private StoragePoolDetailsDao storagePoolDetailsDao; @@ -418,6 +427,7 @@ void testGrantAccess_ClusterScope_Success() { when(sanStrategy.getAccessGroup(any())).thenReturn(existingAccessGroup); when(sanStrategy.ensureLunMapped(anyString(), anyString(), anyString())).thenReturn("0"); + when(sanStrategy.getLunSerialNumber(anyString(), anyString())).thenReturn(LUN_SERIAL_NUMBER); // Execute boolean result = driver.grantAccess(volumeInfo, host, dataStore); @@ -470,6 +480,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"); + when(sanStrategy.getLunSerialNumber(anyString(), anyString())).thenReturn(LUN_SERIAL_NUMBER); // Execute boolean result = driver.grantAccess(volumeInfo, hostVO, dataStore); From dbff79759cd0e558ff8387d776e841916dd9133e Mon Sep 17 00:00:00 2001 From: "Srivastava, Piyush" Date: Thu, 15 Oct 2026 21:09:50 +0530 Subject: [PATCH 5/5] removing the changes in iscsiadmadapter which were introduced for ontap as new adapter for ontap exist now --- .../kvm/storage/IscsiAdmStorageAdaptor.java | 204 ++---------------- 1 file changed, 21 insertions(+), 183 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 495ae45a2896..311154801b98 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 @@ -16,14 +16,9 @@ // under the License. package com.cloud.hypervisor.kvm.storage; -import java.io.File; -import java.io.FileWriter; -import java.nio.file.Path; import java.util.HashMap; import java.util.List; import java.util.Map; -import java.nio.file.Files; -import java.nio.file.Paths; import org.apache.cloudstack.utils.qemu.QemuImg; import org.apache.cloudstack.utils.qemu.QemuImg.PhysicalDiskFormat; @@ -100,8 +95,14 @@ public boolean connectPhysicalDisk(String volumeUuid, KVMStoragePool pool, Map/device/delete — the standard Linux kernel SCSI - * API for removing a single device without tearing down the entire iSCSI session. - * Once the kernel processes the delete, it also removes the by-path symlink. - * - * This is used instead of iscsiadm --logout when other LUNs on the same IQN are still - * active (ONTAP single-IQN-per-SVM model), since logout would tear down ALL LUNs. - */ - private void removeStaleScsiDevice(String host, int port, String iqn, String lun) { - String byPath = getByPath(host, port, "/" + iqn + "/" + lun); - Path byPathLink = Paths.get(byPath); - if (!Files.exists(byPathLink)) { - logger.debug("by-path entry for LUN " + lun + " already gone, nothing to remove"); - return; - } - try { - Path realDevice = byPathLink.toRealPath(); - String devName = realDevice.getFileName().toString(); - File deleteFile = new File("/sys/block/" + devName + "/device/delete"); - if (!deleteFile.exists()) { - logger.warn("sysfs delete entry not found for device " + devName + " — cannot remove stale SCSI device"); - return; - } - try (FileWriter fw = new FileWriter(deleteFile)) { - fw.write("1"); - } - logger.info("Removed stale SCSI device " + devName + " for LUN /" + iqn + "/" + lun + " via sysfs"); - } catch (Exception e) { - logger.warn("Failed to remove stale SCSI device for LUN /" + iqn + "/" + lun + ": " + e.getMessage()); - } - } - private boolean disconnectPhysicalDisk(String host, int port, String iqn, String lun) { - // Check if other LUNs on the same IQN target are still in use. - // ONTAP (and similar) uses a single IQN per SVM with multiple LUNs. - // Doing iscsiadm --logout tears down the ENTIRE target session, - // which would destroy access to ALL LUNs — not just the one being disconnected. - if (hasOtherActiveLuns(host, port, iqn, lun)) { - logger.info("Skipping iSCSI logout for /" + iqn + "/" + lun + - " — other LUNs on the same target are still active. Removing stale SCSI device for this LUN only."); - removeStaleScsiDevice(host, port, iqn, lun); - // After removing this LUN's device, re-check: if no other LUNs remain active, - // If it is the last one then must logout to clean up the iSCSI session entirely. - if (hasOtherActiveLuns(host, port, iqn, lun)) { - logger.info("Other LUNs still active after removing /" + iqn + "/" + lun + " — session kept alive."); - return true; - } - logger.info("No more active LUNs on target after removing /" + iqn + "/" + lun + " — proceeding with iSCSI logout."); - } - - // No other LUNs active on this target — safe to logout and delete the node record. + // use iscsiadm to log out of the iSCSI target and un-discover it // ex. sudo iscsiadm -m node -T iqn.2012-03.com.test:volume1 -p 192.168.233.10:3260 --logout Script iScsiAdmCmd = new Script(true, "iscsiadm", 0, logger); @@ -536,8 +387,8 @@ public List listPhysicalDisks(String storagePoolUuid, KVMStorag @Override public KVMPhysicalDisk createDiskFromTemplate(KVMPhysicalDisk template, String name, PhysicalDiskFormat format, - ProvisioningType provisioningType, long size, - KVMStoragePool destPool, int timeout, byte[] passphrase) { + ProvisioningType provisioningType, long size, + KVMStoragePool destPool, int timeout, byte[] passphrase) { throw new UnsupportedOperationException("Creating a disk from a template is not yet supported for this configuration."); } @@ -571,19 +422,6 @@ public KVMPhysicalDisk copyPhysicalDisk(KVMPhysicalDisk srcDisk, String destVolu try { QemuImg q = new QemuImg(timeout); q.convert(srcFile, destFile); - // Below fix is required when vendor depends on host based copy rather than storage CAN_CREATE_VOLUME_FROM_VOLUME capability - // When host based template copy is triggered , small size template sits in RAM(depending on host memory and RAM) and copy is marked successful and by the time flush to storage is triggered - // disconnectPhysicalDisk would disconnect the lun , hence template staying in RAM is not copied to storage lun. Below does flushing of data to storage and marking - // copy as successful once flush is complete. - Script flushCmd = new Script(true, "blockdev", 0, logger); - flushCmd.add("--flushbufs", destDisk.getPath()); - String flushResult = flushCmd.execute(); - if (flushResult != null) { - logger.warn("iSCSI copyPhysicalDisk: blockdev --flushbufs returned: {}", flushResult); - } - Script syncCmd = new Script(true, "sync", 0, logger); - syncCmd.execute(); - logger.info("iSCSI copyPhysicalDisk: flush/sync completed "); } catch (QemuImgException | LibvirtException ex) { String msg = "Failed to copy data from " + srcDisk.getPath() + " to " + destDisk.getPath() + ". The error was the following: " + ex.getMessage();