Feature/cstackex 265: Netapp ONTAP iscsi provisioning using lun serial number instead of logical unit number

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,24 +74,111 @@ public void adaptorSatisfiesThePoolManagerDiscoveryContract() throws ReflectiveO
 
         StorageAdaptor adaptor = OntapIscsiStorageAdaptor.class.getDeclaredConstructor().newInstance();
         assertEquals(StoragePoolType.OntapiSCSI, adaptor.getStoragePoolType());
-        assertFalse("Must implement StorageAdaptor directly, not extend IscsiAdmStorageAdaptor",
-                IscsiAdmStorageAdaptor.class.isAssignableFrom(OntapIscsiStorageAdaptor.class));
         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 ontapAndGenericIscsiAdaptorsDoNotShareThePoolMap() {
-        String uuid = "shared-looking-uuid";
-        OntapIscsiStorageAdaptor ontap = new OntapIscsiStorageAdaptor();
-        IscsiAdmStorageAdaptor generic = new IscsiAdmStorageAdaptor();
+    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());
+    }
 
-        KVMStoragePool ontapPool = ontap.createStoragePool(uuid, "10.0.0.1", 3260, null, null,
+    /**
+     * 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);
 
-        assertSame(ontapPool, ontap.getStoragePool(uuid));
-        assertEquals(null, generic.getStoragePool(uuid));
-        assertTrue(ontap.deleteStoragePool(uuid));
-        assertEquals(null, ontap.getStoragePool(uuid));
+        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/scsi-3" + 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/scsi-3600a098078304d2d383f2f6b45734a51-part1"));
+        assertFalse("Only the scsi-3 form is handed out, never the wwn-0x alias",
+                adaptor.disconnectPhysicalDiskByPath("/dev/disk/by-id/wwn-0x600a098078304d2d383f2f6b45734a51"));
+        assertFalse("A bare kernel name carries no identity", adaptor.disconnectPhysicalDiskByPath("/dev/sdb"));
+        assertFalse(adaptor.disconnectPhysicalDiskByPath(null));
+    }
+
+    /**
+     * Other vendors publish the same scsi-3 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 disconnectByPathDoesNotClaimOtherVendorsScsiDevices() {
+        OntapIscsiStorageAdaptor adaptor = new OntapIscsiStorageAdaptor();
+
+        assertFalse("Pure FlashArray also publishes scsi-3 names",
+                adaptor.disconnectPhysicalDiskByPath("/dev/disk/by-id/scsi-3624a9370f1f4f5a4b0d3c2e1000113aa"));
+        assertFalse("HPE Primera also publishes scsi-3 names",
+                adaptor.disconnectPhysicalDiskByPath("/dev/disk/by-id/scsi-360002ac0000000000000012300020a1b"));
+        assertFalse("A local RAID volume also publishes scsi-3 names",
+                adaptor.disconnectPhysicalDiskByPath("/dev/disk/by-id/scsi-3600508b1001c4d2e8a0f6b1c2d3e4f50"));
+        assertFalse("NetApp E-Series is not managed by this adaptor",
+                adaptor.disconnectPhysicalDiskByPath("/dev/disk/by-id/scsi-3600a0b800029e3a4000012345f6a7b8c"));
+    }
+
+    /**
+     * 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/scsi-3600a098078304d2d383f2f6b45734a51"));
     }
 }

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

@@ -23,6 +23,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;
@@ -90,6 +91,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.LinkedHashMap;
 import java.util.List;
@@ -144,6 +146,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.
      */
@@ -1218,11 +1232,15 @@ private void grantAccessIscsi(Host host, VolumeVO volumeVO, Map<String, String>
 
         ensureAccessGroupForHost(sanStrategy, host, storagePool, svmName, accessGroupName);
 
-        // Create or retrieve existing LUN mapping
+        // Create or retrieve existing LUN mapping. The logical unit number it returns is specific to
+        // this host's igroup and so is only useful for diagnostics; the path is keyed on the WWID.
         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 = buildIscsiPath(storagePool, 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);
@@ -1258,7 +1276,12 @@ private void grantAccessTemplate(TemplateInfo templateInfo, Host host, DataStore
         try {
             String lunNumber = sanStrategy.ensureLunMapped(svmName, lunName, accessGroupName);
             lunMapped = true;
-            String iscsiPath = buildIscsiPath(storagePool, lunNumber);
+            // The template cache LUN has no volume_details row to cache the WWID in, and this runs
+            // once per template per pool, so it is fetched from the array each time.
+            String lunWwid = fetchLunWwid(sanStrategy, svmName, lunName);
+            logger.debug("grantAccessTemplate: template cache LUN [{}] mapped as logical unit number [{}], WWID [{}]",
+                    lunName, lunNumber, lunWwid);
+            String iscsiPath = buildIscsiPath(storagePool, lunWwid);
 
             templatePoolRef.setInstallPath(iscsiPath);
             vmTemplatePoolDao.update(templatePoolRef.getId(), templatePoolRef);
@@ -1314,10 +1337,75 @@ private void ensureAccessGroupForHost(UnifiedSANStrategy sanStrategy, Host host,
     }
 
     /**
-     * Builds the volume path the KVM agent expects for managed iSCSI: {@code /<targetIQN>/<lunNumber>}.
+     * Builds the volume path the KVM agent expects for managed iSCSI: {@code /<targetIQN>/<lunWwid>}.
+     *
+     * <p>The second component is the LUN's SCSI WWID, not its logical unit number. ONTAP presents
+     * every LUN of an SVM through one target IQN and assigns the logical unit number per igroup, so
+     * the same LUN answers at a different number on each host. A plain host-to-host live migration
+     * ships the source domain XML verbatim, so anything host-specific in the device path would make
+     * the destination open whichever LUN happened to occupy that number there. The WWID comes from
+     * the LUN's own inquiry data and is identical on every host, so the path stays valid.</p>
+     *
+     * <p>The hex WWID is used rather than the raw serial deliberately: ONTAP serial numbers contain
+     * characters including {@code /}, which would break the {@code /targetIQN/LUN} parsing the
+     * hypervisor resources apply to this path.</p>
+     */
+    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.
+     *
+     * <p>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.</p>
      */
-    private String buildIscsiPath(StoragePoolVO storagePool, String lunNumber) {
-        return OntapStorageConstants.SLASH + storagePool.getPath() + OntapStorageConstants.SLASH + lunNumber;
+    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);
+
+        // The serial is recorded as well as the WWID it encodes, because the serial is what 'lun show'
+        // reports on the array while the WWID is what 'ls /dev/disk/by-id' reports on the host. The
+        // path carries the WWID rather than the serial: ONTAP serials contain characters such as '/'
+        // and '?' that are unsafe in a value flowing through paths, globs and API responses.
+        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.
+     *
+     * <p>NetApp builds the NAA IEEE Registered Extended identifier as its OUI followed by the ASCII
+     * bytes of the serial number in hex. For serial {@code x0M-8?/kEsJT} this yields
+     * {@code 600a098078304d2d383f2f6b45734a54}, which the host shows as
+     * {@code /dev/disk/by-id/scsi-3600a098078304d2d383f2f6b45734a54} and vSphere shows as
+     * {@code naa.600a098078304d2d383f2f6b45734a54}.</p>
+     */
+    private String toNaaWwid(String serialNumber) {
+        StringBuilder wwid = new StringBuilder(OntapStorageConstants.NETAPP_NAA_OUI);
+        for (byte b : serialNumber.getBytes(StandardCharsets.US_ASCII)) {
+            wwid.append(String.format("%02x", b));
+        }
+        return wwid.toString();
     }
 
     private void refreshManagedStoreTarget(DataStore dataStore, String iscsiPath) {

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java

@@ -387,6 +387,46 @@ public CloudStackVolume getCloudStackVolume(Map<String, String> values) {
         }
     }
 
+    /**
+     * Fetches a LUN's serial number, the array-assigned identity its SCSI WWID is derived from.
+     *
+     * <p>Kept separate from {@link #getCloudStackVolume(Map)} rather than widening that method's
+     * query: ONTAP narrows a response to the requested {@code fields}, and existing callers of
+     * {@code getCloudStackVolume} rely on {@code name} and {@code uuid} being present.</p>
+     *
+     * @return the serial number, or null if the LUN no longer exists on the array
+     */
+    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<String, Object> queryParams = Map.of(
+                    OntapStorageConstants.SVM_DOT_NAME, svmName,
+                    OntapStorageConstants.NAME, lunName,
+                    OntapStorageConstants.FIELDS, OntapStorageConstants.SERIAL_NUMBER);
+            OntapResponse<Lun> 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());
+        }
+    }
+
     @Override
     public AccessGroup createAccessGroup(AccessGroup accessGroup) {
         logger.trace("createAccessGroup : Creating Igroup with access group request {} ", accessGroup);

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageConstants.java

@@ -63,7 +63,16 @@ public class OntapStorageConstants {
     // Query params
     public static final String NAME = "name";
     public static final String FIELDS = "fields";
-    /** ONTAP volume field: bytes consumed, {@code space.used}. */
+    public static final String SERIAL_NUMBER = "serial_number";
+
+    /**
+     * NetApp's IEEE OUI, which prefixes the NAA identifier of every ONTAP LUN.
+     *
+     * <p>A LUN's WWID is this followed by the hex of its 12-character serial number, giving the
+     * 32-hex-character value udev publishes as {@code /dev/disk/by-id/wwn-0x<wwid>} and (with the
+     * NAA designator type prepended by {@code scsi_id}) as {@code /dev/disk/by-id/scsi-3<wwid>}.</p>
+     */
+    public static final String NETAPP_NAA_OUI = "600a0980";
     public static final String SPACE_USED = "space.used";
     public static final String CLUSTER_NODE_ASUP_FIELDS = "model,is_all_flash_optimized,is_capacity_optimized,is_perf_optimized";
     public  static final String INITIATORS = "initiators";
@@ -110,6 +119,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 QOS_POLICY_UUID = "qosPolicyUuid";
     public static final String IS_AFF = "isAFF";
     public static final String MIN_IOPS_NOT_SUPPORTED_ON_FAS =