CloudStack Volume support with ONTAP storage by rajiv-jain-netapp · Pull Request #13053 · apache/cloudstack

rajiv-jain-netapp · 2026-04-22T02:58:39Z

Description

Co-authored-by: Sandeep Locharla sandeep.locharla@netapp.com
Co-authored-by: Piyush Srivastava piyush5@netapp.com
Co-authored-by: Surya Gupta suryag@netapp.com

This PR...

Supports cloudstack-volume usecases support for NFS3 and iSCSI protocols.
Support for cloudStack-volume create/delete/attach/detach/snapshot-create.
Instance create/delete/snapshot-create support
Snapshot-create would not support
1. memory snapshot
2. user input for quicing option during snapshot creation

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?

Testing Done:

VM create

Validate the respective CloudStack volume

Storage view for the cloudStack volumes

VM snapshot support

Storage view, post snapshot create

CloudStack UI list view for snapshot

CloudStack volume snapshot support

Storage view, post snapshot create

CloudStack UI list view for snapshot

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

… added EOF fixes

… added EOF fixes + correcting license header

Initial primary storage pool plugin skeleton

…POJOs" This reverts commit fe0f752.

…POJOs" This reverts commit 28faca1.

Feignconfiguration and volume feignClient along with desired POJOs with cstack 28

* CSTACKEX-29 Cluster, SVM and Aggr Feign Client * CSTACKEX-29 Change the endpoint method name in feign client * CSTACKEX-29 Make the alignment proper * CSTACKEX-29 Added License Info * CSTACKEX-29 Resolve Review Comments * CSTACKEX-29 Remove Component Annotation from datastoredriverclass * CSTACKEX-29 Resolve Style check issues * CSTACKEX-29 Resolve ALL Style issues * CSTACKEX-29 Resolve Precommits Issues * CSTACKEX-29 Added Method comments and change the ontap response class name --------- Co-authored-by: Gupta, Surya <Surya.Gupta@netapp.com>

* CSTACKEX-31 NAS and Job Feign Client and POJOs * CSTACKEX-31 Fixed Checks Issues * CSTACKEX-31 Resolve Review Comments * CSTACKEX-31 Resolve Review Comments * CSTACKEX-31 Resolve Review Comments * CSTACKEX-31 Added Aggr and size to volume model * CSTACKEX-31 Change the export policy endpoint path * CSTACKEX-31 Fixed check styles --------- Co-authored-by: Gupta, Surya <Surya.Gupta@netapp.com>

* CSTACKEX-30 SAN Feign Client * CSTACKEX-30 Fixed check style issues * CSTACKEX-30 Fixed review comments --------- Co-authored-by: Gupta, Surya <Surya.Gupta@netapp.com>

* CSTACKEX-7: ONTAP Primary storage pool --------- Co-authored-by: Locharla, Sandeep <Sandeep.Locharla@netapp.com>

CSTACKEX-34: Upgrade to framework classes design

* CSTACKEX-35 Create Async * CSTACKEX-35 Added Null and empty check * CSTACKEX-35 Resolved review comments * CSTACKEX-35 Removed Type Casting for logger --------- Co-authored-by: Gupta, Surya <Surya.Gupta@netapp.com>

* CSTACKEX-1: Feign changes and fixes for getting storage pool creation to work * CSTACKEX-01: Create Primary Storage pool changes with working code * CSTACKEX-01: Addressed all review comments and updated some code * CSTACKEX-01: Made some changes to fix some errors seen during testing * CSTACKEX-01: Addressed additional comments --------- Co-authored-by: Locharla, Sandeep <Sandeep.Locharla@netapp.com>

rajiv-jain-netapp · 2026-04-27T02:48:41Z

@DaanHoogland @winterhazel , How can we re-trigger the build?

winterhazel · 2026-04-27T02:57:34Z

@DaanHoogland @winterhazel , How can we re-trigger the build?

@rajiv-jain-netapp you can ping @blueorangutan and use the package command, like this:

@blueorangutan package

blueorangutan · 2026-04-27T02:58:04Z

@winterhazel a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2026-04-27T03:55:17Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17610

DaanHoogland · 2026-04-28T14:32:34Z

@blueorangutan test

blueorangutan · 2026-04-28T14:34:04Z

@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

blueorangutan · 2026-04-28T14:36:24Z

[SF] Trillian Build Failed (tid-15969)

blueorangutan · 2026-04-29T23:08:06Z

[SF] Trillian test result (tid-15974)
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 56045 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr13053-t15974-kvm-ol8.zip
Smoke tests completed. 151 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

DaanHoogland · 2026-04-30T09:03:57Z

@weizhouapache @winterhazel , at your discretion (ready for merge for my part)

winterhazel

Currently reviewing. This will take some time, so I am submitting the reviews partially.

winterhazel · 2026-05-01T15:39:14Z


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

+    private static final String ONTAP_PROVIDER_NAME = "NetApp ONTAP";


This constant is declared in 3 places. Perhaps we could create a file in a single common dependency (maybe cloud-api) to declare the plugin names?

@winterhazel I did not find any suitable place in cloud-api for keeping this constant, It would be helpful if you could point out some place to keep this constant.

org.apache.cloudstack.engine.subsystem.api.storage.DataStoreProvider should be the most appropriate place (this one is in cloud-engine-api, not cloud-api).

winterhazel · 2026-05-04T12:45:58Z

+        } catch (Exception e) {
+            logger.error("Unexpected exception while creating disk-only VM snapshot for VM [{}]. Deleting leftover deltas.", vmName, e);
+            cleanupLeftoverDeltas(volumeObjectTos, mapVolumeToSnapshotSizeAndNewVolumePath, storagePoolMgr);
+            return new CreateDiskOnlyVmSnapshotAnswer(cmd, false,
+                    String.format("Creation of disk-only VM snapshot for VM [%s] failed due to %s.", vmName, e.getMessage()), null);


Can this be unified with the catch block above?

@winterhazel, I added this logic separately after encountering failures in negative scenarios where the UI did not surface any error details. With this change, users are now notified of the failure reason when the exception falls outside the scope of the existing catch block. This is expected to improve overall user experience. Please let me know if you still feel this should be removed.

@rajiv-jain-netapp got it. Notifying operators of why there was a failure is the right thing to do. I was just thinking about converting the two catches of this try to a single catch, as they execute pretty much the same things. This would be done by replacing catch (LibvirtException | QemuImgException e) with catch (Exception e) and keeping return new CreateDiskOnlyVmSnapshotAnswer(... instead of return new Answer(....

winterhazel · 2026-05-04T13:42:14Z

+                logger.info("Other LUNs still active after removing /" + iqn + "/" + lun + " — session kept alive.");
+                return true;


As the disconnect operation failed, I wonder if it makes more sense to throw an exception here instead

winterhazel · 2026-05-05T18:39:18Z

        logger.info("Deleting ONTAP volume by name: " + volume.getName() + " and uuid: " + volume.getUuid());
        String authHeader = OntapStorageUtils.generateAuthHeader(storage.getUsername(), storage.getPassword());
        try {
+            // TODO: Implement lun and file deletion, if any, before deleting the volume


Is this going to be addressed in this PR, or separately?

winterhazel · 2026-05-05T20:10:18Z

+        CloudStackVolume cloudStackVolume = null;
+        FileInfo fileInfo = getFile(cloudStackVolumeMap.get(OntapStorageConstants.VOLUME_UUID),cloudStackVolumeMap.get(OntapStorageConstants.FILE_PATH));
+
+        if(fileInfo != null){


Suggested change

if(fileInfo != null){

if (fileInfo != null) {

winterhazel · 2026-05-05T20:13:29Z

@@ -180,11 +212,11 @@ public void disableLogicalAccess(Map<String, String> values) {

    @Override
    public Map<String, String> getLogicalAccess(Map<String, String> values) {


This method does not seem to be used outside the tests. Is this expected?

winterhazel · 2026-05-05T20:14:38Z

+        }
+    }
+
+


Methods from line 301-367 are not used. Is this expected?

winterhazel · 2026-05-07T19:33:20Z

+        } catch (CloudRuntimeException e) {
+            throw e;
+        } finally {
+            if (!result) {


This if can be inverted to reduce indentation

winterhazel · 2026-05-07T19:39:22Z

+                deleteFlexVolSnapshots(flexVolDetails);
+            }
+
+            // Also handle legacy STORAGE_SNAPSHOT details (backward compatibility)


Is this needed for the ONTAP plugin?

winterhazel · 2026-05-07T19:40:40Z

+                revertFlexVolSnapshots(flexVolDetails);
+            }
+
+            // Also handle legacy STORAGE_SNAPSHOT details (backward compatibility)


Is this needed for the ONTAP plugin?

piyush5netapp · 2026-05-08T13:09:32Z

@winterhazel I have pushed changes addressing most of the changes you suggested except for the comments "This if can be inverted to throw early and reduce the indentation" where we can discuss more. Please re-review.
Also, let me know if you want me to resolve conversation 1 by 1, I can do that .

winterhazel · 2026-05-08T13:15:38Z

@piyush5netapp thanks. I intend to have a look at the feedback and the new changes later today. I will resolve the open conversations that were already addressed as I read them.

winterhazel · 2026-05-14T01:30:04Z

@piyush5netapp @rajiv-jain-netapp I went through the reviews and resolved the ones that already seem addressed. Could you go through the comments that are still open and address them, or close with a comment if you judge it unnecessary (the indentation ones)?

- extracted the ONTAP plugin name to interface nane to use it from one place. - Corrected indentation gaps.

rajiv-jain-netapp · 2026-05-14T10:53:41Z

Thank you for your comments, @winterhazel.
I have updated the PR—please take a look at your convenience. I’ve addressed several of the pending items with additional commits today and have also provided responses to clarify some of the points raised.

Jain, Rajiv and others added 30 commits October 3, 2025 14:02

Initial primary storage pool plugin skeleton

4c8c8d9

Initial primary storage pool plugin skeleton - added license string

bf838b3

Initial primary storage pool plugin skeleton - added license string +…

b82fb40

… added EOF fixes

Initial primary storage pool plugin skeleton - added license string +…

1199d55

… added EOF fixes + correcting license header

Merge pull request #1 from NetApp/feature-CSTACKEX-24

ebc3f00

Initial primary storage pool plugin skeleton

Feignconfiguration and volume feignClient along with desired POJOs

28faca1

Feignconfiguration and volume feignClient along with desired POJOs

fe0f752

Revert "Feignconfiguration and volume feignClient along with desired …

35f1011

…POJOs" This reverts commit fe0f752.

Revert "Feignconfiguration and volume feignClient along with desired …

a03a2c4

…POJOs" This reverts commit 28faca1.

Feignconfiguration and volume feignClient along with desired POJOs

2e0efe8

Feignconfiguration and volume feignClient along with desired POJOs

a96eb9e

CSTACKEX-28: added copyright comment in the logger configuration

0f09c5f

CSTACKEX-28: added newline in the end of the file

2cc4b0c

CSTACKEX-28 - incorporated review comments

033c23d

Merge pull request #4 from NetApp/feature/CSTACKEX-28

2822946

Feignconfiguration and volume feignClient along with desired POJOs with cstack 28

CSTACKEX-30 SAN Feign Client (#8)

a492797

* CSTACKEX-30 SAN Feign Client * CSTACKEX-30 Fixed check style issues * CSTACKEX-30 Fixed review comments --------- Co-authored-by: Gupta, Surya <Surya.Gupta@netapp.com>

CSTACKEX-7: ONTAP Primary storage pool (#9)

25353c2

* CSTACKEX-7: ONTAP Primary storage pool --------- Co-authored-by: Locharla, Sandeep <Sandeep.Locharla@netapp.com>

CSTACKEX-34: Upgrade to framework classes design

973f5e2

CSTACKEX-34: incorporating the review comments

686a892

CSTACKEX-34: transient changes to the review comments

edfcdde

CSTACKEX-34: Unable to get checkstyle pass hence fixing this as well

73eb9f5

CSTACKEX-34: further review comments incorporations

465fffe

CSTACKEX-34: addressing checkstyle issues

3d6bd91

CSTACKEX-34: fix checksyle issues

5815ebd

Merge pull request #13 from NetApp/feature/CSTACKEX-34

618f957

CSTACKEX-34: Upgrade to framework classes design

CSTACKEX-35 Create Async (#14)

6c4b24e

* CSTACKEX-35 Create Async * CSTACKEX-35 Added Null and empty check * CSTACKEX-35 Resolved review comments * CSTACKEX-35 Removed Type Casting for logger --------- Co-authored-by: Gupta, Surya <Surya.Gupta@netapp.com>

Merge branch 'apache:main' into main

54ddfa9

Correction in code owners username

c86d377

boring-cyborg Bot added the component:integration label Apr 24, 2026

winterhazel added the status:needs-review label Apr 30, 2026

winterhazel reviewed May 1, 2026

View reviewed changes

winterhazel reviewed May 4, 2026

View reviewed changes

rajiv-jain-netapp force-pushed the sync/apache-main-apr-2026 branch from 10cb764 to c86d377 Compare May 5, 2026 11:46

winterhazel reviewed May 5, 2026

View reviewed changes

winterhazel reviewed May 7, 2026

View reviewed changes

Srivastava, Piyush and others added 4 commits May 8, 2026 14:35

sync: fixed review comments without test class changes

199c463

sync: review comments changes for test files

11c937d

sync:fix full qualified import

a0234e4

bugfix/CSTACKEX-169: Fix enum case sensitive issues (#53)

cc9a6fb

winterhazel mentioned this pull request May 11, 2026

Review comments from community #13145

Closed

14 tasks

rajiv-jain-netapp added 3 commits May 14, 2026 14:55

- Removed occurences of string.format usage

0f3cb2b

- extracted the ONTAP plugin name to interface nane to use it from one place. - Corrected indentation gaps.

CSTACKEX-183: added missed import

03bf16f

CSTACKEX-183: Incorporated subsequent comments

37760c3


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

		private static final String ONTAP_PROVIDER_NAME = "NetApp ONTAP";

		logger.info("Other LUNs still active after removing /" + iqn + "/" + lun + " — session kept alive.");
		return true;

		@@ -180,11 +212,11 @@ public void disableLogicalAccess(Map<String, String> values) {

		@Override
		public Map<String, String> getLogicalAccess(Map<String, String> values) {

Conversation

rajiv-jain-netapp commented Apr 22, 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!

rajiv-jain-netapp commented Apr 27, 2026

Uh oh!

winterhazel commented Apr 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blueorangutan commented Apr 27, 2026

Uh oh!

blueorangutan commented Apr 27, 2026

Uh oh!

DaanHoogland commented Apr 28, 2026

Uh oh!

blueorangutan commented Apr 28, 2026

Uh oh!

blueorangutan commented Apr 28, 2026

Uh oh!

blueorangutan commented Apr 29, 2026

Uh oh!

DaanHoogland commented Apr 30, 2026

Uh oh!

winterhazel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

rajiv-jain-netapp commented Apr 22, 2026 •

edited

Loading

winterhazel commented Apr 27, 2026 •

edited

Loading