Feature/cstackex 198 by rajiv-jain-netapp · Pull Request #63 · NetApp/cloudstack

rajiv-jain-netapp · 2026-06-12T11:37:46Z

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?

Copilot

Pull request overview

This PR refactors the ONTAP primary storage plugin’s snapshot and VM snapshot “revert” flow to use clone operations (NAS file clone / SAN LUN clone with override) rather than the prior FlexVol snapshot restore / CLI restore-file approach, and updates tests and Feign models/clients accordingly.

Changes:

Switch snapshot/VM snapshot revert implementations to clone-based operations (cloneFile, cloneLun with is_override=true).
Introduce/adjust Feign request models and client methods to support file/LUN clone requests and required payload fields.
Update naming logic to derive ONTAP-safe clone names from CloudStack snapshot names and expand snapshot-name length constant.

Reviewed changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java	Changes volume snapshot creation/revert to clone-backed behavior; persists clone metadata to snapshot_details.
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategy.java	Changes VM snapshot creation/revert to clone-backed behavior; adds LUN UUID resolution helper; updates snapshot name building.
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedNASStrategy.java	Implements NFS revert via file clone override request.
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java	Implements iSCSI revert via LUN clone override request.
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java	Exposes `getSanFeignClient()` accessor for strategy consumers.
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/NASFeignClient.java	Adds `cloneFile` endpoint.
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/SANFeignClient.java	Adds `cloneLun` endpoint; removes LUN restore endpoint.
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/SnapshotFeignClient.java	Removes snapshot restore and file restore endpoints (and CLI restore-file).
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/FileCloneRequest.java	Replaces prior snapshot-file-restore request model with clone request model.
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/Lun.java	Adds `location` and `is_override` fields used for clone requests.
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java	Adds clone name normalization helper.
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageConstants.java	Adds clone detail keys; increases max snapshot name length constant.
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/CliSnapshotRestoreRequest.java	Removed (no longer used).
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/LunRestoreRequest.java	Removed (no longer used).
plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriverTest.java	Adds tests for clone-backed takeSnapshot/revertSnapshot behavior and metadata usage.
plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/UnifiedNASStrategyTest.java	Adds test to validate file-clone override payload for revert.
plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/UnifiedSANStrategyTest.java	Adds test to validate LUN-clone override payload for revert.
plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategyTest.java	Updates snapshot name-format test to match new naming behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

        snapshotDetail = new SnapshotDetailsVO(csSnapshotId,
-                OntapStorageConstants.ONTAP_SNAP_ID, ontapSnapshotUuid, false);
+                OntapStorageConstants.ONTAP_SNAP_ID, ontapCloneId, false);
        snapshotDetailsDao.persist(snapshotDetail);


                        String volumePath = resolveVolumePathOnOntap(volumeId, protocol, groupInfo.poolDetails);
+                        String cloneName = snapshotNameBase;
+                        String cloneUuid = cloneName;
+                        JobResponse jobResponse;


                        FlexVolSnapshotDetail detail = new FlexVolSnapshotDetail(
-                                flexVolUuid, snapshotUuid, snapshotNameBase, volumePath, groupInfo.poolId, protocol);
+                                flexVolUuid, cloneUuid, cloneName, volumePath, groupInfo.poolId, protocol);
                        createdSnapshots.add(detail);


+            if (flexVolUuid == null || snapshotName == null || ontapCloneName == null || volumePath == null || poolIdStr == null) {
                throw new CloudRuntimeException("Missing required snapshot details for snapshot " + snapshotId +
                        " (flexVolUuid=" + flexVolUuid + ", snapshotName=" + snapshotName +
-                        ", volumePath=" + volumePath + ", poolId=" + poolIdStr + ")");
+                        ", cloneName=" + ontapCloneName + ", volumePath=" + volumePath + ", poolId=" + poolIdStr + ")");
            }


+        if (candidate.length() > OntapStorageConstants.MAX_SNAPSHOT_NAME_LENGTH) {
+            candidate = candidate.substring(0, OntapStorageConstants.MAX_SNAPSHOT_NAME_LENGTH);
+        }


     * Builds a deterministic, ONTAP-safe snapshot name for a VM snapshot.
     * Format: {@code vmsnap_<vmSnapshotId>_<timestamp>}
     */
    String buildSnapshotName(VMSnapshot vmSnapshot) {
-        String name = "vmsnap_" + vmSnapshot.getId() + "_" + System.currentTimeMillis();
-        // ONTAP snapshot names: max 256 chars, must start with letter, only alphanumeric and underscores
-        if (name.length() > OntapStorageConstants.MAX_SNAPSHOT_NAME_LENGTH) {
-            name = name.substring(0, OntapStorageConstants.MAX_SNAPSHOT_NAME_LENGTH);
-        }
-        return name;
+        return OntapStorageUtils.getOntapCloneName(vmSnapshot.getName());


sandeeplocharla · 2026-06-12T12:16:26Z

    @Override
    public void takeSnapshot(SnapshotInfo snapshot, AsyncCompletionCallback<CreateCmdResult> callback) {
-        logger.info("OntapPrimaryDatastoreDriver.takeSnapshot: Creating FlexVolume snapshot for snapshot [{}]", snapshot.getId());
+        logger.info("OntapPrimaryDatastoreDriver.takeSnapshot: Creating clone-backed snapshot for snapshot [{}]", snapshot.getId());


Best not to mention 'clone-backed'

sandeeplocharla · 2026-06-12T12:18:28Z

+                Svm svm = new Svm();
+                svm.setName(poolDetails.get(OntapStorageConstants.SVM_NAME));
+                cloneRequest.setSvm(svm);
+                Lun.Location location = new Lun.Location();


Same, it would be better to offload this to UnifiedSANStrategy implementation

sandeeplocharla · 2026-06-12T12:22:03Z

+                volumeRef.setUuid(flexVolUuid);
+                volumeRef.setName(poolDetails.get(OntapStorageConstants.VOLUME_NAME));
+                fileCloneRequest.setVolume(volumeRef);
+                fileCloneRequest.setSourcePath(volumePath);


Let's create a common method in StorageStrategy and offload this to UnifiedNASStrategy

sandeeplocharla · 2026-06-12T12:23:29Z

-            JobResponse jobResponse = snapshotClient.createSnapshot(authHeader, flexVolUuid, snapshotRequest);
            if (jobResponse == null || jobResponse.getJob() == null) {
-                throw new CloudRuntimeException("Failed to initiate FlexVolume snapshot on FlexVol UUID [" + flexVolUuid + "]");
+                throw new CloudRuntimeException("Failed to initiate clone-backed snapshot for volume " + volumeVO.getId());


Best not to mention, 'clone-backed' anywhere. It should be our IP.

sandeeplocharla · 2026-06-12T12:25:10Z


-            // Set snapshot path for CloudStack (format: snapshotName for identification)
-            snapshotObjectTo.setPath(OntapStorageConstants.ONTAP_SNAP_ID + "=" + ontapSnapshotUuid);
+            snapshotObjectTo.setPath(OntapStorageConstants.ONTAP_CLONE_NAME + "=" + cloneName);


Let's retain everything everything related to 'snapshot' as it is, instead of calling out 'clone' everywhere?

sandeeplocharla · 2026-06-12T12:31:12Z

-        OntapResponse<FlexVolSnapshot> response = snapshotClient.getSnapshots(authHeader, flexVolUuid, queryParams);
-        if (response != null && response.getRecords() != null && !response.getRecords().isEmpty()) {
-            return response.getRecords().get(0).getUuid();
+    private String resolveLunUuidByName(StorageStrategy storageStrategy, String authHeader, String svmName, String lunName) {


Can't we use 'getCloudStackVolume' method from the strategy classes here?

sandeeplocharla · 2026-06-12T12:41:07Z

            String protocol = getSnapshotDetail(snapshotId, OntapStorageConstants.PROTOCOL);

-            if (flexVolUuid == null || snapshotName == null || volumePath == null || poolIdStr == null) {
+            if (flexVolUuid == null || snapshotName == null || ontapCloneName == null || volumePath == null || poolIdStr == null) {


There's a chance for Cloudstack DB updation failure at a later stage. This would make the snapshots unusable. We need to check if there's a garbage collector for such cloudstack entities and tie our ONTAP entities also to be cleaned up with it.
This is a generic issue with all the workflows.

suryag1201 · 2026-06-12T13:10:07Z

-public class SnapshotFileRestoreRequest {
+public class FileCloneRequest {
+    @JsonProperty("volume")
+    private VolumeRef volume;


we can use VolumeConcise class instead creating new

suryag1201 · 2026-06-12T13:16:22Z

@@ -454,32 +454,31 @@
    public JobResponse revertSnapshotForCloudStackVolume(String snapshotName, String flexVolUuid,
                                                          String snapshotUuid, String volumePath,
                                                          String lunUuid, String flexVolName) {


what is the use and where are we using these snapshotuuid and lunuuid, flexVolName?

suryag1201 · 2026-06-12T13:23:53Z

+     * Uses CloudStack UI snapshot name as the preferred ONTAP clone name.
+     * If needed, normalizes just enough to satisfy ONTAP naming limits.
+     */
+    public static String getOntapCloneName(String snapshotName) {


This snapshot name is for VM or CS Volume?
bcz for VM name is unique but for volume it is not

suryag1201 · 2026-06-12T13:39:58Z

    }

    /**
     * Reverts a file to a snapshot using the ONTAP CLI-based snapshot file restore API.


we are using CLI based APIs

suryag1201 · 2026-06-12T13:40:12Z

        return response;
    }
    /**
     * Reverts a LUN to a snapshot using the ONTAP CLI-based snapshot file restore API.


same here, no CLI

suryag1201 · 2026-06-12T13:46:41Z

            CreateVMSnapshotCommand ccmd = new CreateVMSnapshotCommand(
                    userVm.getInstanceName(), userVm.getUuid(), target, volumeTOs, guestOS.getDisplayName());

            logger.info("takeVMSnapshot: Creating ONTAP FlexVolume VM Snapshot for VM [{}] with quiesce={}", userVm.getInstanceName(), quiesceVm);


we have to modify this as per the changed approach

suryag1201 · 2026-06-12T13:53:25Z

                    String protocol = groupInfo.poolDetails.get(OntapStorageConstants.PROTOCOL);

-                    // Create one detail per CloudStack volume in this FlexVol group (for single-file restore during revert)
+                    // Create one clone per CloudStack volume and persist detail for protocol-specific revert.


this method is becoming very long, can we have sub methods created for clone request or for any other sub tasks

suryag1201 · 2026-06-12T13:54:39Z

-            // ── Step 2: Create FlexVolume-level snapshots ──
+            // ── Step 2: Create clone-backed VM snapshot entries ──
            try {
                String snapshotNameBase = buildSnapshotName(vmSnapshot);


How snapshot and revert work for the CS Volume which is not backend by ontap lun or file?

suryag1201 · 2026-06-12T14:09:47Z

@@ -639,7 +639,7 @@ public long getUsedIops(StoragePool storagePool) {
     */
    @Override
    public void takeSnapshot(SnapshotInfo snapshot, AsyncCompletionCallback<CreateCmdResult> callback) {


this approach change also require changes inAPI calls for delete snapshot workflow

suryag1201 · 2026-06-12T14:13:55Z

            CreateVMSnapshotCommand ccmd = new CreateVMSnapshotCommand(
                    userVm.getInstanceName(), userVm.getUuid(), target, volumeTOs, guestOS.getDisplayName());

            logger.info("takeVMSnapshot: Creating ONTAP FlexVolume VM Snapshot for VM [{}] with quiesce={}", userVm.getInstanceName(), quiesceVm);


this class and datastore driver class is using same clone request creation, cant we have a util method for the both?

CSTACKEX-198: Snapshot workflows backed by clone feature

47f5aae

rajiv-jain-netapp requested review from piyush5netapp, sandeeplocharla and suryag1201 as code owners June 12, 2026 11:37

rajiv-jain-netapp requested a review from Copilot June 12, 2026 11:56

Copilot started reviewing on behalf of rajiv-jain-netapp June 12, 2026 11:57 View session

Copilot AI reviewed Jun 12, 2026

View reviewed changes

sandeeplocharla reviewed Jun 12, 2026

View reviewed changes

CSTACKEX-198: Lun path needs to be in a pattern

1f449a4

suryag1201 requested changes Jun 12, 2026

View reviewed changes

suryag1201 reviewed Jun 12, 2026

View reviewed changes

suryag1201 requested changes Jun 12, 2026

View reviewed changes

Conversation

rajiv-jain-netapp commented Jun 12, 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!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rajiv-jain-netapp commented Jun 12, 2026 •

edited

Loading