feature/CSTACKEX-46: Create, Delete iSCSI type Cloudstack volumes, Enter, Cancel Maintenance mode #27

sandeeplocharla · 2026-01-21T16:42:27Z

Description

This PR has the following changes:

Create iSCSI type cloudstack volume
Delete iSCSI type cloudstack volume
Enter and Cancel Maintenance when storagepool has CS volumes

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?

https://netapp.zoom.us/rec/share/r0rh_HhXD_lZNuppLeGGMk4Y4jg8-p9DXwIB_GD2e-uEslEH5geYzchrYcBDY7PJ.WjzaTxXoGt0B8nqe?startTime=1768919430000
Passcode: n=AE=.C9

Create ISCSI type Primary storage pool
Create Compute & Disk offering.
Create an instance with compute & disk offering to create both ROOT & DATA disks via ontap plugin.
Add additional CS Volume with ontap plugin and allocate it to the instance created.
Disable, Re-Enable Storage Pool (with CS Volumes)
Enter, Cancel Maintenance of Storage Pool (with CS Volumes)
Delete Instance + Expunge Volumes
Disable, Re-Enable Storage Pool (without CS Volumes)
Enter, Cancel Maintenance of Storage Pool (without CS Volumes)
Delete Storage Pool

https://netapp.zoom.us/rec/share/Ax74XcCVkQtXglaNwNb9rTcn_1k1TvxplvaLmXTdFxjz7DeQixmiqj-uwX0QXjz0.EwnlsurMGr3jLku3?startTime=1768920957000
Passcode: $%j3+K4B

Create an instance with compute & disk offering to create both ROOT & DATA disks via Default Primary & ontap plugins.
Add additional CS Volume with ontap plugin and allocate it to the instance created.
Disable, Re-Enable Storage Pool (with CS Volumes)
Enter, Cancel Maintenance of Storage Pool (with CS Volumes)
Delete Instance + Expunge Volumes
Disable, Re-Enable Storage Pool (without CS Volumes)
Enter, Cancel Maintenance of Storage Pool (without CS Volumes)
Delete Storage Pool

Observations:

Though we have increased retries to wait for discovery of luns, the additional volume attachment still times out, so, it needs a manual retry.
When placed into Maintenance mode, Instance is getting Stopped in the first case while it remains in Running state in the second case. We currently don't have any code attributing to this behaviour in our plugin code. Need to understand why cloudstack is behaving this way.

I've also tested force delete of storage pool when CS volumes were present, but, haven't covered it in these recordings.

…Storage pool workflows

…cel Maintenance mode

…base and renamed plugin name

…base in agent code

…ragePool create and delete

sandeeplocharla · 2026-01-27T09:45:23Z

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

+                if (volumeVO != null) {
+                    volumeVO.setPath(null);
+                    if (cloudStackVolume.getLun().getUuid() != null) {
+                        volumeVO.setFolder(cloudStackVolume.getLun().getUuid());


Add a comment on the need for this

sandeeplocharla · 2026-01-27T10:01:46Z

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

+        if (mapResp != null && mapResp.containsKey(Constants.LOGICAL_UNIT_NUMBER)) {
+            String lunNumber = mapResp.get(Constants.LOGICAL_UNIT_NUMBER);
+            s_logger.info("ensureLunMapped: Existing LunMap found for LUN [{}] in igroup [{}] with LUN number [{}]", lunName, accessGroupName, lunNumber);
+            return lunNumber;


Add a TODO comment to keep an eye on possibility of duplicate LUN for luns

sandeeplocharla · 2026-01-27T10:28:37Z

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

+                    VolumeVO volumeVO = volumeDao.findById(volInfo.getId());
+                    if (volumeVO != null) {
+                        String iscsiPath = Constants.SLASH + storagePool.getPath() + Constants.SLASH + lunNumber;
+                        volumeVO.set_iScsiName(iscsiPath);


Remove any redundant code

sandeeplocharla · 2026-01-27T10:46:41Z

...torage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java

+                return false;
+            }
+            // Set the aggregates which are according to the storage requirements
+            for (Aggregate aggr : aggrs) {


Check for rebase issue here

sandeeplocharla · 2026-01-27T10:56:47Z

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

+            Map<String, Object> queryParams = Map.of("allow_delete_while_mapped", "true");
+            try {
+                sanFeignClient.deleteLun(authHeader, cloudstackVolume.getLun().getUuid(), queryParams);
+            } catch (Exception ex) {


Check if feignException should be added

sandeeplocharla · 2026-01-27T11:05:07Z

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

+            String authHeader = Utility.generateAuthHeader(storage.getUsername(), storage.getPassword());
+            sanFeignClient.deleteLunMap(authHeader, lunUUID, igroupUUID);
+            s_logger.info("disableLogicalAccess: LunMap deleted successfully.");
+        } catch (Exception e) {


Use feignException

piyush5netapp · 2026-01-28T15:39:28Z

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

+        return null;
+    }

+    /**


I think its would be better to skip these comments on func as some of the information are related to our plugin internal design.

piyush5netapp · 2026-01-28T15:41:59Z

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

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


a better name would be more meaningful.

piyush5netapp · 2026-01-28T15:47:05Z

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

+                        }
+
+                        // Determine scope ID based on storage pool scope (cluster or zone level igroup)
+                        long scopeId = (storagePool.getScope() == ScopeType.CLUSTER)


@rajiv-jain-netapp Can we discuss on including scope id and scope both for igroup name. It would be better to have only storage pool name reference in igroup name . Having scope or any other entity creates a dependency of their lifecycle on igroup.

@piyush5netapp lets discuss in tomorrow scrum

rajiv-jain-netapp · 2026-01-29T13:45:15Z

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

+                return 0L;
+            }
+        } catch (Exception ignore) {
+            // If FS check fails for any reason, fall back to blockdev call


lets add a logger here

rajiv-jain-netapp · 2026-01-29T13:57:41Z

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

+                        }
+
+                        // Determine scope ID based on storage pool scope (cluster or zone level igroup)
+                        long scopeId = (storagePool.getScope() == ScopeType.CLUSTER)


@piyush5netapp lets discuss in tomorrow scrum

rajiv-jain-netapp · 2026-01-29T14:05:22Z

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

+                        volumeVO.setPath(iscsiPath);
+                        s_logger.info("createAsync: Volume [{}] iSCSI path set to {}", volumeVO.getId(), iscsiPath);
+
+                    } else if (ProtocolType.NFS3.name().equalsIgnoreCase(details.get(Constants.PROTOCOL))) {


I am not seeing anything significant for NFS block. I would recommend to not have if-else for protocol atleast for this situation, we should handle it via strategy concrete implementation.

rajiv-jain-netapp · 2026-01-29T14:08:31Z

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

I appreciated the method-level comments that reference the algorithms used in each method. However, since this is open-source code, I recommend moving these details to the design specification instead of keeping them in the code. You can add a separate section in the spec to capture this information.
In the code, we can retain comments only for complex conditions or specific use cases for future reference.

rajiv-jain-netapp · 2026-01-29T14:10:55Z

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

-                    // ManagedNFS qcow2 backing file deletion handled by KVM host/libvirt; nothing to do via ONTAP REST.
-                    s_logger.info("deleteAsync: ManagedNFS volume {} no-op ONTAP deletion", data.getId());
+                    // NFS file deletion is handled by the hypervisor; no ONTAP REST call needed
+                    s_logger.info("deleteAsync: ManagedNFS volume {} - file deletion handled by hypervisor", data.getId());


same comment here as well

rajiv-jain-netapp · 2026-01-29T14:20:15Z

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

+                            VirtualMachine.State.Destroyed,
+                            VirtualMachine.State.Expunging,
+                            VirtualMachine.State.Error).contains(vm.getState())) {
+                        s_logger.debug("revokeAccess: Volume [{}] is still attached to VM [{}] in state [{}], skipping revokeAccess",


I guess, this logger can be made error instead debug, to highlight in the logs

rajiv-jain-netapp · 2026-01-29T14:24:08Z

...ins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/Igroup.java


    @JsonProperty("protocol")
-    private ProtocolEnum protocol = ProtocolEnum.mixed;
+    private ProtocolEnum protocol = null;


this is good catch, since we have 1-1 mapping between igroup and storage-pool, we should not keep protocol type as mixed, lets keep as per the storage-pool protocol.

rajiv-jain-netapp · 2026-01-29T14:25:04Z

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/Svm.java


    @JsonInclude(JsonInclude.Include.NON_NULL)
    public static class Links { }
-


There is no code change in this file. Can you exclude this file?

rajiv-jain-netapp · 2026-01-29T14:32:08Z

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

-    CloudStackVolume getCloudStackVolume(CloudStackVolume cloudstackVolume) {
-        //TODO
-        return null;
+    public void copyCloudStackVolume(CloudStackVolume cloudstackVolume) {


this code is added for which workflow?

suryag1201 · 2026-01-29T17:16:17Z

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

        if (callback == null) {
            throw new InvalidParameterValueException("createAsync: callback should not be null");
        }
+


add dataObject null condition check, also add error logger

suryag1201 · 2026-01-29T17:18:40Z

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

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


it can also come for snapshot ?

suryag1201 · 2026-01-29T17:21:16Z

...ntap/src/main/java/org/apache/cloudstack/storage/provider/OntapPrimaryDatastoreProvider.java

    public String getName() {
        s_logger.trace("OntapPrimaryDatastoreProvider: getName: Called");
-        return "ONTAP Primary Datastore Provider";
+        return "ONTAP";


please add this string to some constant and use it here

Locharla, Sandeep and others added 23 commits January 21, 2026 21:53

CSTACKEX-46: Disable, Re-Enable, Delete Storage pool and Enter, Exit …

9c85c6b

…Storage pool workflows

CSTACKEX-50: Fixed some issues seen while testing

41e8324

CSTACKEX-46 Create Async, Attach Cluster/Zone and Grant/Revoke Access

7f47d93

CSTACKEX-46 Added Logging

d6e1d70

CSTACKEX-46 Resolve Copilot review comments

eff6d97

CSTACKEX-46 Resolve review comments

faf8dbf

CSTACKEX-46 Resolve review bot comments

8852bb4

CSTACKEX-46 Change method name

dc5f01a

CSTACKEX-46 Resolve check styling issues

14ec034

CSTACKEX-46 Testing: Resolve fetch Storage details issue

423aee2

CSTACKEX-46 Testing: Protocol Type fetch issue

905ae4a

CSTACKEX-46 Testing: Added debug log

3ee5ead

CSTACKEX-46 Testing: Added debug log

dc0e84e

CSTACKEX-46 Testing: Corrected the URL for Create Lun and Igroup

b2056ff

CSTACKEX-46 Resolve bot review comments

67bf958

CSTACKEX-46 Resolve review comments

8732f7f

CSTACKEX-46 Resolve review comments again

f1b7dbd

CSTACKEX-46 Change log level

e9ba8b3

CSTACKEX-46 added valiodation for lun name

f067b50

CSTACKEX-46 resolve check style issues

dac753f

CSTACKEX-46 Resolve import issues

716fcaf

CSTACKEX-46 Resolve copilot comments

e6ece89

CSTACKEX-46: Create, Delete iSCSI type Cloudstack volumes, Enter, Can…

d111436

…cel Maintenance mode

sandeeplocharla requested review from piyush5netapp, rajiv-jain-netapp and suryag1201 January 21, 2026 16:42

sandeeplocharla self-assigned this Jan 21, 2026

Locharla, Sandeep added 3 commits January 22, 2026 13:24

CSTACKEX-46: Fixed a check style issue which got introduced during re…

a5fa9d2

…base and renamed plugin name

CSTACKEX-46: Fixed a check style issue which got introduced during re…

3eea5c4

…base in agent code

CSTACKEX-46: Fixed a couple of issues observed while testing NFS3 sto…

63addcd

…ragePool create and delete

sandeeplocharla commented Jan 27, 2026

View reviewed changes

CSTACKEX-46: Refactored driver class

dfc1ee9

piyush5netapp reviewed Jan 28, 2026

View reviewed changes

rajiv-jain-netapp reviewed Jan 29, 2026

View reviewed changes

suryag1201 reviewed Jan 29, 2026

View reviewed changes


		@JsonInclude(JsonInclude.Include.NON_NULL)
		public static class Links { }

feature/CSTACKEX-46: Create, Delete iSCSI type Cloudstack volumes, Enter, Cancel Maintenance mode #27

Are you sure you want to change the base?

feature/CSTACKEX-46: Create, Delete iSCSI type Cloudstack volumes, Enter, Cancel Maintenance mode #27

Uh oh!

Conversation

sandeeplocharla commented Jan 21, 2026

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

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!

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!

suryag1201 Jan 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

5 participants

suryag1201 Jan 29, 2026 •

edited

Loading