Feature/cstackex 265: Netapp ONTAP iscsi provisioning using lun serial number instead of logical unit number by piyush5netapp · Pull Request #114 · NetApp/cloudstack

piyush5netapp · 2026-10-08T04:16:13Z

Description

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)
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

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

github-actions · 2026-10-08T05:17:44Z

🔴 Test Coverage Grade: `D` — Marginal

Metric	Value
Line coverage	24.74%
Branch coverage	18.86%

Grade Scale

Grade	Line Coverage	Meaning
🟢 A	≥ 80%	Excellent - this code sleeps well at night 😴
🟡 B	60-79%	Good - almost there, don't stop now 😉
🟠 C	40-59%	Acceptable - your code is wearing a seatbelt, but no airbags 😬
🔴 D	20-39%	Marginal - boldly shipping where no test has gone before 🖖
⛔ F	< 20%	Failing - tests? what tests? 🔥

Branch coverage is shown as a secondary signal. Grade is determined by line coverage.
View full Actions run

github-actions · 2026-10-08T11:25:54Z

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

github-actions · 2026-10-12T05:29:49Z

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

sandeeplocharla · 2026-10-12T15:48:17Z

                OntapStorageConstants.SVM_DOT_NAME, svmName,
                OntapStorageConstants.IGROUP_DOT_NAME, accessGroupName
        );
        String lunNumber = getLogicalAccess(getMap);


Do change this code according to fetch the WWID instead.

sandeeplocharla · 2026-10-12T15:50:25Z

+     *
+     * @return the serial number, or null if the LUN no longer exists on the array
+     */
+    public String getLunSerialNumber(String svmName, String lunName) {


This method might not be needed, as earlier LUN was fetched in ensureLunMapped, so the same implementation would be present there. Just change the query param accordingly.

sandeeplocharla · 2026-10-12T15:52:38Z

+    /**
+     * 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


Try to restrict the explanation to 1-2 lines highlighting the structure of the path

sandeeplocharla · 2026-10-12T15:53:52Z

    }

+    @Override
+    public boolean zoneWideVolumesAvailableWithoutClusterMotion(){


Maybe its better to raise a separate PR for this, as we would be cherry-picking the commits for upstream

sandeeplocharla · 2026-10-12T15:56:27Z

+ *
+ * So {@code OntapPrimaryDatastoreDriver} writes volume paths as {@code /<targetIQN>/<lunWwid>} and
+ * this adaptor reports devices as {@code /dev/disk/by-id/scsi-3<lunWwid>}. The WWID is derived from
+ * the LUN's own inquiry data (VPD page 0x83), so it is identical on every host and the domain XML


Most of this comment may not be necessary for future, so, consider pruning this to explain the adaptor in brief, instead.

sandeeplocharla · 2026-10-12T16:02:21Z

+     * udev's scsi_id builtin prepends the NAA designator type to the WWID, so a LUN whose WWID is
+     * {@code 600a0980...} is published as {@code scsi-3600a0980...}.
+     */
+    private static final String BY_ID_SCSI_PREFIX = "/dev/disk/by-id/scsi-3";


For future reference, add a comment here or in our spec that /dev/disk/by-id/scsi-3 is set here as ONTAP uses NAA and 600a098... is the NetApp OUI vendor prefix 00:a0:98

sandeeplocharla · 2026-10-12T16:15:34Z

+        return null;
+    }
+
+    private boolean createIscsiNode(String host, int port, String iqn, String volumePath) {


Add null checks for all the variables

sandeeplocharla · 2026-10-12T16:21:58Z

-            KVMStoragePool destPool, int timeout, byte[] passphrase) {
-        throw new UnsupportedOperationException("Creating a disk from a template is not yet supported for this configuration.");
+    private void waitForDeviceToDisappear(String devicePath) {
+        for (int attempt = 0; attempt < DEVICE_WAIT_TRIES && getDeviceSize(devicePath) > 0; attempt++) {


Is there any reason for 15s wait period for devices to respond everywhere in this file? Or was this pulled in from iSCSIAdmAdaptor?

suryag1201 · 2026-10-12T19:28:37Z

 import org.apache.commons.lang3.StringUtils;
-import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;


could u run this command
udevadm info -q symlink -n /dev/sdb
will this show both forms in that output (Id and path )?

suryag1201 · 2026-10-12T19:33:56Z


    @Override
-    public boolean connectPhysicalDisk(String volumeUuid, KVMStoragePool pool, Map<String, String> details, boolean isVMMigrate) {
+    public boolean connectPhysicalDisk(String volumePath, KVMStoragePool pool, Map<String, String> details, boolean isVMMigrate) {


please check this method: IscsiStorageCleanupMonitor, this used by-path param and if not exact match then iSCSI cleanup thread will log out live ONTAP sessions

suryag1201 · 2026-10-12T19:35:30Z


-        // ex. sudo iscsiadm -m node -T iqn.2012-03.com.test:volume1 -p 192.168.233.10:3260 -o new
-        Script iScsiAdmCmd = new Script(true, "iscsiadm", 0, logger);
+        if (!createIscsiNode(host, port, iqn, volumePath)) {


Perform this test also, when 2 VMs are running and if u stop 1 VM what happens?
does this logout the whole session ?

suryag1201 · 2026-10-12T19:35:50Z


 import java.io.File;
 import java.io.FileWriter;
+import java.io.IOException;


Update Testing details

suryag1201 · 2026-10-12T19:46:39Z


-        physicalDisk.setSize(deviceSize);
-        physicalDisk.setVirtualSize(deviceSize);
+        if (!logoutAndForgetTarget(host, port, iqn, lunWwid)) {


If the session cannot be identified, do not log out. Remove only this SCSI device, or fail the disconnect so it can be retried. Logout should run only after the session is known and no other LUNs remain on it.

Srivastava, Piyush added 4 commits September 21, 2026 20:36

feature/CSTACKEX-265: Pilot commit

7322d33

feature/CSTACKEX-265: testing code

8399fcf

feature/CSTACKEX-265: Updated netapp oui check for disconnect

850f7b2

feature/CSTACKEX-265: Pulled latest from main

2b9b817

piyush5netapp requested review from rajiv-jain-netapp, sandeeplocharla and suryag1201 as code owners October 8, 2026 04:16

github-actions Bot added the status:has-conflicts label Oct 8, 2026

Srivastava, Piyush added 2 commits October 12, 2026 01:11

feature/CSTACKEX-265: udpated code

c2bb377

feature/CSTACKEX-265: Pulled latest from main

8d220ae

github-actions Bot added status:has-conflicts and removed status:has-conflicts labels Oct 11, 2026

feature/CSTACKEX-265: Pulled latest from main

f4bf026

github-actions Bot removed the status:has-conflicts label Oct 12, 2026

sandeeplocharla reviewed Oct 12, 2026

View reviewed changes

suryag1201 reviewed Oct 12, 2026

View reviewed changes

Conversation

piyush5netapp commented Oct 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

Uh oh!

github-actions Bot commented Oct 8, 2026

🔴 Test Coverage Grade: D — Marginal

Grade Scale

Uh oh!

github-actions Bot commented Oct 8, 2026

Uh oh!

github-actions Bot commented Oct 12, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

piyush5netapp commented Oct 8, 2026 •

edited

Loading

🔴 Test Coverage Grade: `D` — Marginal