From 3efaa7a18cbc71fb751d9db9d1ee288d7e154d6a Mon Sep 17 00:00:00 2001 From: "Jain, Rajiv" Date: Tue, 23 Jun 2026 19:06:06 +0530 Subject: [PATCH 1/8] CSTACKEX:200 - added temp CG logic for VM snapshots if VM span across multiple volumes at storage --- .../driver/OntapPrimaryDatastoreDriver.java | 19 +- .../feign/client/SnapshotFeignClient.java | 92 +++++ .../storage/utils/OntapStorageConstants.java | 3 + .../storage/utils/OntapStorageUtils.java | 42 ++ .../vmsnapshot/OntapVMSnapshotStrategy.java | 369 ++++++++++++++---- .../OntapVMSnapshotStrategyTest.java | 269 +++++++++++-- 6 files changed, 671 insertions(+), 123 deletions(-) diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java index 04996b74d2a5..ac9b7e818747 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java @@ -67,7 +67,6 @@ import org.apache.cloudstack.storage.service.model.ProtocolType; import org.apache.cloudstack.storage.to.SnapshotObjectTO; import org.apache.cloudstack.storage.utils.OntapStorageUtils; -import org.apache.commons.lang3.StringUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.Nullable; @@ -670,8 +669,8 @@ public void takeSnapshot(SnapshotInfo snapshot, AsyncCompletionCallback-} + * Builds an ONTAP-safe snapshot name from the CloudStack UI name with uniqueness suffix. */ - private String buildSnapshotName(String volumeName, String snapshotUuid) { - String name = volumeName + "-" + snapshotUuid; - int maxLength = OntapStorageConstants.MAX_SNAPSHOT_NAME_LENGTH; - int trimRequired = name.length() - maxLength; - - if (trimRequired > 0) { - name = StringUtils.left(volumeName, volumeName.length() - trimRequired) + "-" + snapshotUuid; - } - return name; + private String buildSnapshotName(String cloudStackSnapshotName, long snapshotId) { + return OntapStorageUtils.buildOntapSnapshotName(cloudStackSnapshotName, "cs" + snapshotId); } /** diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/SnapshotFeignClient.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/SnapshotFeignClient.java index 2f0e050d6f55..d9566b422e38 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/SnapshotFeignClient.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/SnapshotFeignClient.java @@ -181,4 +181,96 @@ JobResponse restoreFileFromSnapshot(@Param("authHeader") String authHeader, @Headers({"Authorization: {authHeader}", "Content-Type: application/json"}) JobResponse restoreFileFromSnapshotCli(@Param("authHeader") String authHeader, CliSnapshotRestoreRequest request); + + /** + * Creates a consistency group. + * + *

ONTAP REST: {@code POST /api/application/consistency-groups}

+ * + * @param authHeader Basic auth header + * @param request consistency group create request body + * @return JobResponse containing the async job reference + */ + @RequestLine("POST /api/application/consistency-groups") + @Headers({"Authorization: {authHeader}", "Content-Type: application/json"}) + JobResponse createConsistencyGroup(@Param("authHeader") String authHeader, + Map request); + + /** + * Lists consistency groups. + * + *

ONTAP REST: {@code GET /api/application/consistency-groups}

+ * + * @param authHeader Basic auth header + * @param queryParams Optional query parameters + * @return Paginated consistency group records + */ + @RequestLine("GET /api/application/consistency-groups") + @Headers({"Authorization: {authHeader}"}) + OntapResponse> getConsistencyGroups(@Param("authHeader") String authHeader, + @QueryMap Map queryParams); + + /** + * Creates (starts) a consistency group snapshot. + * + *

ONTAP REST: {@code POST /api/application/consistency-groups/{cgUuid}/snapshots}

+ * + * @param authHeader Basic auth header + * @param cgUuid consistency group UUID + * @param request snapshot start request body + * @return JobResponse containing the async job reference + */ + @RequestLine("POST /api/application/consistency-groups/{cgUuid}/snapshots") + @Headers({"Authorization: {authHeader}", "Content-Type: application/json"}) + JobResponse createConsistencyGroupSnapshot(@Param("authHeader") String authHeader, + @Param("cgUuid") String cgUuid, + Map request); + + /** + * Lists snapshots for a consistency group. + * + *

ONTAP REST: {@code GET /api/application/consistency-groups/{cgUuid}/snapshots}

+ * + * @param authHeader Basic auth header + * @param cgUuid consistency group UUID + * @param queryParams Optional query parameters + * @return Paginated consistency group snapshot records + */ + @RequestLine("GET /api/application/consistency-groups/{cgUuid}/snapshots") + @Headers({"Authorization: {authHeader}"}) + OntapResponse> getConsistencyGroupSnapshots(@Param("authHeader") String authHeader, + @Param("cgUuid") String cgUuid, + @QueryMap Map queryParams); + + /** + * Commits a started consistency group snapshot. + * + *

ONTAP REST: {@code PATCH /api/application/consistency-groups/{cgUuid}/snapshots/{snapshotUuid}}

+ * + * @param authHeader Basic auth header + * @param cgUuid consistency group UUID + * @param snapshotUuid consistency group snapshot UUID + * @param request commit request body + * @return JobResponse containing the async job reference + */ + @RequestLine("PATCH /api/application/consistency-groups/{cgUuid}/snapshots/{snapshotUuid}") + @Headers({"Authorization: {authHeader}", "Content-Type: application/json"}) + JobResponse commitConsistencyGroupSnapshot(@Param("authHeader") String authHeader, + @Param("cgUuid") String cgUuid, + @Param("snapshotUuid") String snapshotUuid, + Map request); + + /** + * Deletes a consistency group. + * + *

ONTAP REST: {@code DELETE /api/application/consistency-groups/{cgUuid}}

+ * + * @param authHeader Basic auth header + * @param cgUuid consistency group UUID + * @return JobResponse containing the async job reference + */ + @RequestLine("DELETE /api/application/consistency-groups/{cgUuid}") + @Headers({"Authorization: {authHeader}"}) + JobResponse deleteConsistencyGroup(@Param("authHeader") String authHeader, + @Param("cgUuid") String cgUuid); } diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageConstants.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageConstants.java index d0ea1783aa1d..39ffb60633e7 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageConstants.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageConstants.java @@ -105,6 +105,9 @@ public class OntapStorageConstants { public static final String ONTAP_SNAP_SIZE = "ontap_snap_size"; public static final String FILE_PATH = "file_path"; public static final int MAX_SNAPSHOT_NAME_LENGTH = 64; + public static final String ONTAP_TEMP_CG_PREFIX = "cs-temp-cg-"; + public static final int ONTAP_CG_JOB_MAX_RETRIES = 60; + public static final int ONTAP_CG_JOB_POLL_INTERVAL_MS = 2000; /** vm_snapshot_details key for ONTAP FlexVolume-level VM snapshots. */ public static final String ONTAP_FLEXVOL_SNAPSHOT = "ontapFlexVolSnapshot"; diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java index 596372edcf16..b05100a4886c 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java @@ -154,4 +154,46 @@ public static String getLunName(String volName, String lunName) { return OntapStorageConstants.VOLUME_PATH_PREFIX + volName + OntapStorageConstants.SLASH + lunName; } + /** + * Builds an ONTAP-safe name token from user-provided snapshot text. + */ + public static String getOntapCloneName(String cloudStackSnapshotName) { + if (cloudStackSnapshotName == null || cloudStackSnapshotName.trim().isEmpty()) { + throw new InvalidParameterValueException("Snapshot name cannot be null or blank"); + } + String normalized = cloudStackSnapshotName.replaceAll("[^a-zA-Z0-9_]", "_"); + if (normalized.isEmpty()) { + normalized = "snapshot"; + } + if (!Character.isLetter(normalized.charAt(0))) { + normalized = "s_" + normalized; + } + if (normalized.length() > OntapStorageConstants.MAX_SNAPSHOT_NAME_LENGTH) { + normalized = normalized.substring(0, OntapStorageConstants.MAX_SNAPSHOT_NAME_LENGTH); + } + return normalized; + } + + /** + * Builds an ONTAP-safe snapshot name that preserves the CloudStack UI snapshot name + * and appends a uniqueness suffix. + */ + public static String buildOntapSnapshotName(String cloudStackSnapshotName, String uniquenessSuffix) { + String normalizedBase = (cloudStackSnapshotName == null || cloudStackSnapshotName.trim().isEmpty()) + ? "snapshot" + : getOntapCloneName(cloudStackSnapshotName); + String suffix = (uniquenessSuffix == null || uniquenessSuffix.isEmpty()) + ? "" + : "_" + uniquenessSuffix.replaceAll("[^a-zA-Z0-9_]", "_"); + int maxLength = OntapStorageConstants.MAX_SNAPSHOT_NAME_LENGTH; + int maxBaseLength = maxLength - suffix.length(); + if (maxBaseLength <= 0) { + return normalizedBase.substring(0, maxLength); + } + if (normalizedBase.length() > maxBaseLength) { + normalizedBase = normalizedBase.substring(0, maxBaseLength); + } + return normalizedBase + suffix; + } + } diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategy.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategy.java index a71df4c2e349..d07d2aebe604 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategy.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategy.java @@ -20,8 +20,10 @@ import java.util.ArrayList; import java.util.HashMap; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.concurrent.TimeUnit; import javax.inject.Inject; @@ -72,25 +74,22 @@ import org.apache.cloudstack.storage.utils.OntapStorageConstants; /** - * VM Snapshot strategy for NetApp ONTAP managed storage using FlexVolume-level snapshots. + * VM Snapshot strategy for NetApp ONTAP managed storage using temporary consistency-group orchestration. * *

This strategy handles VM-level (instance) snapshots for VMs whose volumes - * reside on ONTAP managed primary storage. Instead of creating per-file clones - * (the old approach), it takes ONTAP FlexVolume-level snapshots via the - * ONTAP REST API ({@code POST /api/storage/volumes/{uuid}/snapshots}).

- * - *

Key Advantage:

- *

When multiple CloudStack disks (ROOT + DATA) reside on the same ONTAP - * FlexVolume, a single FlexVolume snapshot atomically captures all of them. - * This is both faster and more storage-efficient than per-file clones.

+ * reside on ONTAP managed primary storage. When VM volumes span multiple FlexVols, + * snapshot creation is coordinated through a temporary ONTAP consistency group (CG) + * and two-phase snapshot flow (start + commit). When all volumes share a single FlexVol, + * a direct FlexVol snapshot is used instead.

* *

Flow:

*
    *
  1. Group all VM volumes by their parent FlexVolume UUID
    2. *
  2. Freeze the VM via QEMU guest agent ({@code fsfreeze}) — if quiesce requested
    3. - *
  3. For each unique FlexVolume, create one ONTAP snapshot
    4. + *
  4. If VM spans multiple FlexVolumes: create temporary CG, start + commit CG snapshot (two-phase)
    5. + *
  5. If VM spans a single FlexVolume: create one FlexVol snapshot directly (no CG overhead)
    6. *
  6. Thaw the VM
    7. - *
  7. Record FlexVolume → snapshot UUID mappings in {@code vm_snapshot_details}
    8. + *
  8. Resolve FlexVolume → snapshot UUID mappings and persist in {@code vm_snapshot_details}
    9. *
* *

Metadata in vm_snapshot_details:

@@ -251,12 +250,14 @@ boolean allVolumesOnOntapManagedStorage(long vmId) { /** * Takes a VM-level snapshot by freezing the VM, creating ONTAP FlexVolume-level - * snapshots (one per unique FlexVolume), and then thawing the VM. + * snapshot(s), and then thawing the VM. * *

Volumes are grouped by their parent FlexVolume UUID (from storage pool details). - * For each unique FlexVolume, exactly one ONTAP snapshot is created via - * {@code POST /api/storage/volumes/{uuid}/snapshots}. This means if a VM has - * ROOT and DATA disks on the same FlexVolume, only one snapshot is created.

+ * When the VM spans more than one unique FlexVolume, a temporary ONTAP + * consistency group is used with two-phase snapshot semantics (start + commit) so + * all FlexVols are captured at the same point in time. When all VM volumes reside + * on a single FlexVolume, a direct per-FlexVol snapshot is taken instead — + * CG orchestration is unnecessary in that case.

* *

Memory Snapshots Not Supported: This strategy only supports disk-only * (crash-consistent) snapshots. Memory snapshots (snapshotmemory=true) are rejected @@ -286,7 +287,7 @@ public VMSnapshot takeVMSnapshot(VMSnapshot vmSnapshot) { FreezeThawVMAnswer thawAnswer = null; long startFreeze = 0; - // Track which FlexVolume snapshots were created (for rollback) + // Track which FlexVolume snapshots were created (for rollback and detail persistence) List createdSnapshots = new ArrayList<>(); boolean result = false; @@ -338,7 +339,8 @@ public VMSnapshot takeVMSnapshot(VMSnapshot vmSnapshot) { 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); + logger.info("takeVMSnapshot: Creating ONTAP VM snapshot for VM [{}] with quiesce={}", + userVm.getInstanceName(), quiesceVm); // Prepare volume info list and calculate sizes for (VolumeObjectTO volumeObjectTO : volumeTOs) { @@ -375,56 +377,20 @@ public VMSnapshot takeVMSnapshot(VMSnapshot vmSnapshot) { userVm.getInstanceName(), quiesceVm, vmIsRunning); } - // ── Step 2: Create FlexVolume-level snapshots ── + // ── Step 2: Create FlexVolume-level snapshot(s) ── try { String snapshotNameBase = buildSnapshotName(vmSnapshot); - for (Map.Entry entry : flexVolGroups.entrySet()) { - String flexVolUuid = entry.getKey(); - FlexVolGroupInfo groupInfo = entry.getValue(); - long startSnapshot = System.nanoTime(); - - // Build storage strategy from pool details to get the feign client - StorageStrategy storageStrategy = OntapStorageUtils.getStrategyByStoragePoolDetails(groupInfo.poolDetails); - SnapshotFeignClient snapshotClient = storageStrategy.getSnapshotFeignClient(); - String authHeader = storageStrategy.getAuthHeader(); - - // Use the same snapshot name for all FlexVolumes in this VM snapshot - // (each FlexVolume gets its own independent snapshot with this name) - FlexVolSnapshot snapshotRequest = new FlexVolSnapshot(snapshotNameBase, - "CloudStack VM snapshot " + vmSnapshot.getName() + " for VM " + userVm.getInstanceName()); - - logger.info("takeVMSnapshot: Creating ONTAP FlexVolume snapshot [{}] on FlexVol UUID [{}] covering {} volume(s)", - snapshotNameBase, flexVolUuid, groupInfo.volumeIds.size()); - - 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 + "]"); - } - - // Poll for job completion - Boolean jobSucceeded = storageStrategy.jobPollForSuccess(jobResponse.getJob().getUuid(), 30, 2000); - if (!jobSucceeded) { - throw new CloudRuntimeException("FlexVolume snapshot job failed on FlexVol UUID [" + flexVolUuid + "]"); - } - - // Retrieve the created snapshot UUID by name - String snapshotUuid = resolveSnapshotUuid(snapshotClient, authHeader, flexVolUuid, snapshotNameBase); - - String protocol = groupInfo.poolDetails.get(OntapStorageConstants.PROTOCOL); - - // Create one detail per CloudStack volume in this FlexVol group (for single-file restore during revert) - for (Long volumeId : groupInfo.volumeIds) { - String volumePath = resolveVolumePathOnOntap(volumeId, protocol, groupInfo.poolDetails); - FlexVolSnapshotDetail detail = new FlexVolSnapshotDetail( - flexVolUuid, snapshotUuid, snapshotNameBase, volumePath, groupInfo.poolId, protocol); - createdSnapshots.add(detail); - } - - logger.info("takeVMSnapshot: ONTAP FlexVolume snapshot [{}] (uuid={}) on FlexVol [{}] completed in {} ms. Covers volumes: {}", - snapshotNameBase, snapshotUuid, flexVolUuid, - TimeUnit.MILLISECONDS.convert(System.nanoTime() - startSnapshot, TimeUnit.NANOSECONDS), - groupInfo.volumeIds); + // CG orchestration is only required when VM disks span multiple FlexVols. + // A single FlexVol already provides atomic capture for all volumes on that FlexVol. + if (flexVolGroups.size() > 1) { + logger.info("takeVMSnapshot: VM [{}] spans {} FlexVol(s); using temporary CG two-phase snapshot flow", + userVm.getInstanceName(), flexVolGroups.size()); + createVmSnapshotsViaTemporaryCg(vmSnapshot, userVm, flexVolGroups, snapshotNameBase, createdSnapshots); + } else { + logger.info("takeVMSnapshot: VM [{}] spans a single FlexVol; using direct FlexVol snapshot flow", + userVm.getInstanceName()); + createVmSnapshotsViaSingleFlexVol(vmSnapshot, userVm, flexVolGroups, snapshotNameBase, createdSnapshots); } } finally { // ── Step 3: Thaw the VM (only if it was frozen, always even on error) ── @@ -456,7 +422,7 @@ public VMSnapshot takeVMSnapshot(VMSnapshot vmSnapshot) { answer.setVolumeTOs(volumeTOs); processAnswer(vmSnapshotVO, userVm, answer, null); - logger.info("takeVMSnapshot: ONTAP FlexVolume VM Snapshot [{}] created successfully for VM [{}] ({} FlexVol snapshot(s))", + logger.info("takeVMSnapshot: ONTAP VM Snapshot [{}] created successfully for VM [{}] ({} detail row(s))", vmSnapshot.getName(), userVm.getInstanceName(), createdSnapshots.size()); long newChainSize = 0; @@ -668,16 +634,140 @@ Map groupVolumesByFlexVol(List volumeT } /** - * Builds a deterministic, ONTAP-safe snapshot name for a VM snapshot. - * Format: {@code vmsnap__} + * Creates VM snapshot artifacts via direct FlexVol snapshot API. + * + *

Used when all VM volumes map to a single FlexVol. In that case a CG is not + * needed because one FlexVol snapshot already captures every disk atomically.

*/ - 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); + void createVmSnapshotsViaSingleFlexVol(VMSnapshot vmSnapshot, UserVm userVm, + Map flexVolGroups, + String snapshotNameBase, + List createdSnapshots) { + for (Map.Entry entry : flexVolGroups.entrySet()) { + String flexVolUuid = entry.getKey(); + FlexVolGroupInfo groupInfo = entry.getValue(); + long startSnapshot = System.nanoTime(); + + StorageStrategy storageStrategy = resolveStorageStrategy(groupInfo.poolDetails); + SnapshotFeignClient snapshotClient = storageStrategy.getSnapshotFeignClient(); + String authHeader = storageStrategy.getAuthHeader(); + + FlexVolSnapshot snapshotRequest = new FlexVolSnapshot(snapshotNameBase, + "CloudStack VM snapshot " + vmSnapshot.getName() + " for VM " + userVm.getInstanceName()); + + logger.info("takeVMSnapshot: [FlexVol] Creating snapshot [{}] on FlexVol UUID [{}] covering {} volume(s)", + snapshotNameBase, flexVolUuid, groupInfo.volumeIds.size()); + + 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 + "]"); + } + + Boolean jobSucceeded = storageStrategy.jobPollForSuccess(jobResponse.getJob().getUuid(), 30, 2000); + if (!jobSucceeded) { + throw new CloudRuntimeException("FlexVolume snapshot job failed on FlexVol UUID [" + flexVolUuid + "]"); + } + + String snapshotUuid = resolveSnapshotUuid(snapshotClient, authHeader, flexVolUuid, snapshotNameBase); + String protocol = groupInfo.poolDetails.get(OntapStorageConstants.PROTOCOL); + + for (Long volumeId : groupInfo.volumeIds) { + String volumePath = resolveVolumePathOnOntap(volumeId, protocol, groupInfo.poolDetails); + createdSnapshots.add(new FlexVolSnapshotDetail( + flexVolUuid, snapshotUuid, snapshotNameBase, volumePath, groupInfo.poolId, protocol)); + } + + logger.info("takeVMSnapshot: [FlexVol] Snapshot [{}] (uuid={}) on FlexVol [{}] completed in {} ms. Covers volumes: {}", + snapshotNameBase, snapshotUuid, flexVolUuid, + TimeUnit.MILLISECONDS.convert(System.nanoTime() - startSnapshot, TimeUnit.NANOSECONDS), + groupInfo.volumeIds); } - return name; + } + + /** + * Creates VM snapshot artifacts via temporary consistency-group two-phase flow. + * + *

Used when VM volumes span multiple FlexVols and require a consistent + * point-in-time capture across all participating FlexVolumes.

+ */ + void createVmSnapshotsViaTemporaryCg(VMSnapshot vmSnapshot, UserVm userVm, + Map flexVolGroups, + String snapshotNameBase, + List createdSnapshots) { + String tempCgName = buildTemporaryConsistencyGroupName(vmSnapshot); + String tempCgUuid = null; + String cgSnapshotUuid = null; + long cgFlowStart = System.nanoTime(); + + // All volumes in a VM snapshot belong to ONTAP-managed pools and share the same ONTAP credentials. + // Use any one FlexVol group to obtain strategy/client objects for this operation. + FlexVolGroupInfo referenceGroup = flexVolGroups.values().iterator().next(); + StorageStrategy storageStrategy = resolveStorageStrategy(referenceGroup.poolDetails); + SnapshotFeignClient snapshotClient = storageStrategy.getSnapshotFeignClient(); + String authHeader = storageStrategy.getAuthHeader(); + + try { + logger.info("takeVMSnapshot: [CG:Create] Creating temporary consistency group [{}] for VM [{}] over {} FlexVol(s)", + tempCgName, userVm.getInstanceName(), flexVolGroups.size()); + tempCgUuid = createTemporaryConsistencyGroup(snapshotClient, storageStrategy, authHeader, tempCgName, flexVolGroups.keySet()); + + logger.info("takeVMSnapshot: [CG:Start] Starting phase-1 snapshot [{}] for temporary consistency group [{}]", + snapshotNameBase, tempCgUuid); + startConsistencyGroupSnapshot(snapshotClient, storageStrategy, authHeader, tempCgUuid, snapshotNameBase); + + cgSnapshotUuid = resolveConsistencyGroupSnapshotUuid(snapshotClient, authHeader, tempCgUuid, snapshotNameBase); + + logger.info("takeVMSnapshot: [CG:Commit] Committing phase-2 snapshot [{}] (uuid={}) for temporary consistency group [{}]", + snapshotNameBase, cgSnapshotUuid, tempCgUuid); + commitConsistencyGroupSnapshot(snapshotClient, storageStrategy, authHeader, tempCgUuid, cgSnapshotUuid); + + // Resolve per-FlexVol snapshot UUIDs and build one detail entry per CloudStack volume. + for (Map.Entry entry : flexVolGroups.entrySet()) { + String flexVolUuid = entry.getKey(); + FlexVolGroupInfo groupInfo = entry.getValue(); + String snapshotUuid = resolveSnapshotUuid(snapshotClient, authHeader, flexVolUuid, snapshotNameBase); + String protocol = groupInfo.poolDetails.get(OntapStorageConstants.PROTOCOL); + + for (Long volumeId : groupInfo.volumeIds) { + String volumePath = resolveVolumePathOnOntap(volumeId, protocol, groupInfo.poolDetails); + createdSnapshots.add(new FlexVolSnapshotDetail( + flexVolUuid, snapshotUuid, snapshotNameBase, volumePath, groupInfo.poolId, protocol)); + } + + logger.debug("takeVMSnapshot: [CG:Resolve] Snapshot [{}] resolved to FlexVol snapshot uuid [{}] for FlexVol [{}], volumes={}", + snapshotNameBase, snapshotUuid, flexVolUuid, groupInfo.volumeIds); + } + } finally { + // CG is only a transaction boundary; remove it after commit/failure and keep snapshots intact. + if (tempCgUuid != null) { + try { + logger.info("takeVMSnapshot: [CG:Cleanup] Deleting temporary consistency group [{}]", tempCgUuid); + deleteTemporaryConsistencyGroup(snapshotClient, storageStrategy, authHeader, tempCgUuid); + } catch (Exception cleanupEx) { + logger.warn("takeVMSnapshot: Failed to delete temporary consistency group [{}]: {}", + tempCgUuid, cleanupEx.getMessage()); + } + } + } + + logger.info("takeVMSnapshot: Temporary consistency-group two-phase flow completed for VM [{}] in {} ms. CG snapshot uuid={}, detail rows={}", + userVm.getInstanceName(), + TimeUnit.MILLISECONDS.convert(System.nanoTime() - cgFlowStart, TimeUnit.NANOSECONDS), + cgSnapshotUuid, createdSnapshots.size()); + } + + /** + * Builds an ONTAP-safe snapshot name from the CloudStack VM snapshot UI name. + */ + String buildSnapshotName(VMSnapshot vmSnapshot) { + return OntapStorageUtils.buildOntapSnapshotName(vmSnapshot.getName(), "vm" + vmSnapshot.getId()); + } + + /** + * Wrapper for static utility to simplify unit testing and keep call sites explicit. + */ + StorageStrategy resolveStorageStrategy(Map poolDetails) { + return OntapStorageUtils.getStrategyByStoragePoolDetails(poolDetails); } /** @@ -695,6 +785,133 @@ String resolveSnapshotUuid(SnapshotFeignClient client, String authHeader, return response.getRecords().get(0).getUuid(); } + /** + * Builds a deterministic temporary CG name for the VM snapshot transaction. + */ + String buildTemporaryConsistencyGroupName(VMSnapshot vmSnapshot) { + return OntapStorageUtils.buildOntapSnapshotName( + OntapStorageConstants.ONTAP_TEMP_CG_PREFIX + vmSnapshot.getId(), + "cg" + vmSnapshot.getId()); + } + + /** + * Creates a temporary consistency group for the involved FlexVol UUIDs and returns its UUID. + */ + String createTemporaryConsistencyGroup(SnapshotFeignClient client, StorageStrategy storageStrategy, + String authHeader, String cgName, Set flexVolUuids) { + List> volumeRefs = new ArrayList<>(); + for (String flexVolUuid : flexVolUuids) { + Map volumeRef = new LinkedHashMap<>(); + volumeRef.put("uuid", flexVolUuid); + volumeRefs.add(volumeRef); + } + + Map payload = new LinkedHashMap<>(); + payload.put("name", cgName); + payload.put("volumes", volumeRefs); + + JobResponse response = client.createConsistencyGroup(authHeader, payload); + pollJobIfPresent(storageStrategy, response, "create temporary consistency group " + cgName); + + String cgUuid = resolveConsistencyGroupUuidByName(client, authHeader, cgName); + if (cgUuid == null || cgUuid.isEmpty()) { + throw new CloudRuntimeException("Unable to resolve temporary consistency group UUID for [" + cgName + "]"); + } + return cgUuid; + } + + /** + * Starts phase-1 of the two-phase CG snapshot. + */ + void startConsistencyGroupSnapshot(SnapshotFeignClient client, StorageStrategy storageStrategy, + String authHeader, String cgUuid, String snapshotName) { + Map payload = new LinkedHashMap<>(); + payload.put("name", snapshotName); + payload.put("action", "start"); + JobResponse response = client.createConsistencyGroupSnapshot(authHeader, cgUuid, payload); + pollJobIfPresent(storageStrategy, response, "start CG snapshot " + snapshotName + " for " + cgUuid); + } + + /** + * Commits phase-2 of the started CG snapshot. + */ + void commitConsistencyGroupSnapshot(SnapshotFeignClient client, StorageStrategy storageStrategy, + String authHeader, String cgUuid, String snapshotUuid) { + Map payload = new LinkedHashMap<>(); + payload.put("action", "commit"); + JobResponse response = client.commitConsistencyGroupSnapshot(authHeader, cgUuid, snapshotUuid, payload); + pollJobIfPresent(storageStrategy, response, "commit CG snapshot " + snapshotUuid + " for " + cgUuid); + } + + /** + * Deletes the temporary consistency group used as a transaction boundary. + */ + void deleteTemporaryConsistencyGroup(SnapshotFeignClient client, StorageStrategy storageStrategy, + String authHeader, String cgUuid) { + JobResponse response = client.deleteConsistencyGroup(authHeader, cgUuid); + pollJobIfPresent(storageStrategy, response, "delete temporary consistency group " + cgUuid); + } + + /** + * Resolves consistency group UUID by name. + */ + String resolveConsistencyGroupUuidByName(SnapshotFeignClient client, String authHeader, String cgName) { + Map query = new HashMap<>(); + query.put("name", cgName); + query.put("fields", "uuid,name"); + OntapResponse> response = client.getConsistencyGroups(authHeader, query); + if (response == null || response.getRecords() == null || response.getRecords().isEmpty()) { + return null; + } + return getStringField(response.getRecords().get(0), "uuid"); + } + + /** + * Resolves consistency group snapshot UUID by name. + */ + String resolveConsistencyGroupSnapshotUuid(SnapshotFeignClient client, String authHeader, + String cgUuid, String snapshotName) { + Map query = new HashMap<>(); + query.put("name", snapshotName); + query.put("fields", "uuid,name"); + OntapResponse> response = client.getConsistencyGroupSnapshots(authHeader, cgUuid, query); + if (response == null || response.getRecords() == null || response.getRecords().isEmpty()) { + throw new CloudRuntimeException("Unable to resolve consistency group snapshot UUID for snapshot [" + + snapshotName + "] in CG [" + cgUuid + "]"); + } + String snapshotUuid = getStringField(response.getRecords().get(0), "uuid"); + if (snapshotUuid == null || snapshotUuid.isEmpty()) { + throw new CloudRuntimeException("Invalid consistency group snapshot UUID for snapshot [" + + snapshotName + "] in CG [" + cgUuid + "]"); + } + return snapshotUuid; + } + + /** + * Polls ONTAP job only when the endpoint returns a job reference. + */ + void pollJobIfPresent(StorageStrategy storageStrategy, JobResponse response, String operationName) { + if (response == null || response.getJob() == null || response.getJob().getUuid() == null) { + logger.debug("pollJobIfPresent: No async job returned for operation [{}], continuing without polling", operationName); + return; + } + Boolean success = storageStrategy.jobPollForSuccess( + response.getJob().getUuid(), + OntapStorageConstants.ONTAP_CG_JOB_MAX_RETRIES, + OntapStorageConstants.ONTAP_CG_JOB_POLL_INTERVAL_MS); + if (!Boolean.TRUE.equals(success)) { + throw new CloudRuntimeException("ONTAP operation failed: " + operationName); + } + } + + private String getStringField(Map record, String key) { + if (record == null) { + return null; + } + Object value = record.get(key); + return value != null ? value.toString() : null; + } + /** * Resolves the ONTAP-side path of a CloudStack volume within its FlexVolume. * @@ -735,7 +952,7 @@ String resolveVolumePathOnOntap(Long volumeId, String protocol, Map poolDetails = storagePoolDetailsDao.listDetailsKeyPairs(detail.poolId); - StorageStrategy storageStrategy = OntapStorageUtils.getStrategyByStoragePoolDetails(poolDetails); + StorageStrategy storageStrategy = resolveStorageStrategy(poolDetails); SnapshotFeignClient client = storageStrategy.getSnapshotFeignClient(); String authHeader = storageStrategy.getAuthHeader(); @@ -769,7 +986,7 @@ void deleteFlexVolSnapshots(List flexVolDetails) { // Only delete the ONTAP snapshot once per FlexVol+Snapshot pair if (!deletedSnapshots.containsKey(dedupeKey)) { Map poolDetails = storagePoolDetailsDao.listDetailsKeyPairs(detail.poolId); - StorageStrategy storageStrategy = OntapStorageUtils.getStrategyByStoragePoolDetails(poolDetails); + StorageStrategy storageStrategy = resolveStorageStrategy(poolDetails); SnapshotFeignClient client = storageStrategy.getSnapshotFeignClient(); String authHeader = storageStrategy.getAuthHeader(); @@ -818,7 +1035,7 @@ void revertFlexVolSnapshots(List flexVolDetails) { } Map poolDetails = storagePoolDetailsDao.listDetailsKeyPairs(detail.poolId); - StorageStrategy storageStrategy = OntapStorageUtils.getStrategyByStoragePoolDetails(poolDetails); + StorageStrategy storageStrategy = resolveStorageStrategy(poolDetails); SnapshotFeignClient snapshotClient = storageStrategy.getSnapshotFeignClient(); String authHeader = storageStrategy.getAuthHeader(); diff --git a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategyTest.java b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategyTest.java index b069ab7246a0..9d7989d65e7a 100644 --- a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategyTest.java +++ b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategyTest.java @@ -21,12 +21,17 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.atLeastOnce; import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -44,6 +49,12 @@ import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao; import org.apache.cloudstack.storage.datastore.db.StoragePoolDetailsDao; import org.apache.cloudstack.storage.datastore.db.StoragePoolVO; +import org.apache.cloudstack.storage.feign.client.SnapshotFeignClient; +import org.apache.cloudstack.storage.feign.model.FlexVolSnapshot; +import org.apache.cloudstack.storage.feign.model.Job; +import org.apache.cloudstack.storage.feign.model.response.JobResponse; +import org.apache.cloudstack.storage.feign.model.response.OntapResponse; +import org.apache.cloudstack.storage.service.StorageStrategy; import org.apache.cloudstack.storage.to.VolumeObjectTO; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; @@ -127,6 +138,10 @@ class OntapVMSnapshotStrategyTest { private VolumeDataFactory volumeDataFactory; @Mock private VolumeDetailsDao volumeDetailsDao; + @Mock + private StorageStrategy storageStrategy; + @Mock + private SnapshotFeignClient snapshotFeignClient; @Spy @InjectMocks @@ -226,14 +241,18 @@ void testCanHandle_AllocatedDiskType_VmxenHypervisor_ReturnsCantHandle() { } @Test - void testCanHandle_AllocatedDiskType_VmNotRunning_ReturnsCantHandle() { + void testCanHandle_AllocatedDiskType_VmStopped_ReturnsHighest() { UserVmVO userVm = createMockUserVm(Hypervisor.HypervisorType.KVM, VirtualMachine.State.Stopped); when(userVmDao.findById(VM_ID)).thenReturn(userVm); VMSnapshotVO vmSnapshot = createMockVmSnapshot(VMSnapshot.State.Allocated, VMSnapshot.Type.Disk); + VolumeVO vol = createMockVolume(VOLUME_ID_1, POOL_ID_1); + when(volumeDao.findByInstance(VM_ID)).thenReturn(Collections.singletonList(vol)); + StoragePoolVO pool = createOntapManagedPool(POOL_ID_1); + when(storagePool.findById(POOL_ID_1)).thenReturn(pool); StrategyPriority result = strategy.canHandle(vmSnapshot); - assertEquals(StrategyPriority.CANT_HANDLE, result); + assertEquals(StrategyPriority.HIGHEST, result); } @Test @@ -593,10 +612,11 @@ void testFlexVolSnapshotDetail_Parse5Parts_ThrowsException() { void testBuildSnapshotName_Format() { VMSnapshotVO vmSnapshot = mock(VMSnapshotVO.class); when(vmSnapshot.getId()).thenReturn(SNAPSHOT_ID); + when(vmSnapshot.getName()).thenReturn("UI VM Snapshot"); String name = strategy.buildSnapshotName(vmSnapshot); - assertEquals(true, name.startsWith("vmsnap_200_")); + assertEquals(true, name.startsWith("UI_VM_Snapshot_vm200")); assertEquals(true, name.length() <= OntapStorageConstants.MAX_SNAPSHOT_NAME_LENGTH); } @@ -732,6 +752,86 @@ void testTakeVMSnapshot_OperationTimeout_ThrowsCloudRuntimeException() throws Ex assertEquals(true, ex.getMessage().contains("timed out")); } + @Test + void testTakeVMSnapshot_SingleFlexVolSuccess_UsesDirectSnapshotNotCg() throws Exception { + VMSnapshotVO vmSnapshot = createTakeSnapshotVmSnapshot(); + setupTakeSnapshotCommon(vmSnapshot); + setupSingleVolumeForTakeSnapshot(); + + String snapshotName = strategy.buildSnapshotName(vmSnapshot); + setupSingleFlexVolFlowMocks(snapshotName); + + FreezeThawVMAnswer freezeAnswer = mock(FreezeThawVMAnswer.class); + when(freezeAnswer.getResult()).thenReturn(true); + FreezeThawVMAnswer thawAnswer = mock(FreezeThawVMAnswer.class); + when(thawAnswer.getResult()).thenReturn(true); + when(agentMgr.send(eq(HOST_ID), any(FreezeThawVMCommand.class))) + .thenReturn(freezeAnswer) + .thenReturn(thawAnswer); + + strategy.takeVMSnapshot(vmSnapshot); + + verify(snapshotFeignClient, times(1)).createSnapshot(any(), eq("flexvol-uuid-1"), any()); + verify(snapshotFeignClient, never()).createConsistencyGroup(any(), any()); + verify(snapshotFeignClient, never()).createConsistencyGroupSnapshot(any(), any(), any()); + verify(snapshotFeignClient, never()).commitConsistencyGroupSnapshot(any(), any(), any(), any()); + verify(snapshotFeignClient, never()).deleteConsistencyGroup(any(), any()); + verify(vmSnapshotDetailsDao, atLeastOnce()).persist(any(VMSnapshotDetailsVO.class)); + } + + @Test + void testTakeVMSnapshot_TemporaryCgTwoPhaseSuccess_PersistsDetailsAndCleansUpCg() throws Exception { + VMSnapshotVO vmSnapshot = createTakeSnapshotVmSnapshot(); + setupTakeSnapshotCommon(vmSnapshot); + setupMultiFlexVolForTakeSnapshot(); + + String snapshotName = strategy.buildSnapshotName(vmSnapshot); + setupTemporaryCgFlowMocks(snapshotName); + + FreezeThawVMAnswer freezeAnswer = mock(FreezeThawVMAnswer.class); + when(freezeAnswer.getResult()).thenReturn(true); + FreezeThawVMAnswer thawAnswer = mock(FreezeThawVMAnswer.class); + when(thawAnswer.getResult()).thenReturn(true); + when(agentMgr.send(eq(HOST_ID), any(FreezeThawVMCommand.class))) + .thenReturn(freezeAnswer) + .thenReturn(thawAnswer); + + strategy.takeVMSnapshot(vmSnapshot); + + verify(snapshotFeignClient, times(1)).createConsistencyGroup(any(), any()); + verify(snapshotFeignClient, times(1)).createConsistencyGroupSnapshot(any(), eq("cg-uuid-1"), any()); + verify(snapshotFeignClient, times(1)).commitConsistencyGroupSnapshot(any(), eq("cg-uuid-1"), eq("cg-snap-uuid-1"), any()); + verify(snapshotFeignClient, times(1)).deleteConsistencyGroup(any(), eq("cg-uuid-1")); + verify(vmSnapshotDetailsDao, atLeastOnce()).persist(any(VMSnapshotDetailsVO.class)); + } + + @Test + void testTakeVMSnapshot_TemporaryCgStartFails_TransitionsToOperationFailed() throws Exception { + VMSnapshotVO vmSnapshot = createTakeSnapshotVmSnapshot(); + setupTakeSnapshotCommon(vmSnapshot); + setupMultiFlexVolForTakeSnapshot(); + + String snapshotName = strategy.buildSnapshotName(vmSnapshot); + setupTemporaryCgFlowMocks(snapshotName); + when(snapshotFeignClient.createConsistencyGroupSnapshot(any(), eq("cg-uuid-1"), any())) + .thenThrow(new CloudRuntimeException("start phase failed")); + + FreezeThawVMAnswer freezeAnswer = mock(FreezeThawVMAnswer.class); + when(freezeAnswer.getResult()).thenReturn(true); + FreezeThawVMAnswer thawAnswer = mock(FreezeThawVMAnswer.class); + when(thawAnswer.getResult()).thenReturn(true); + when(agentMgr.send(eq(HOST_ID), any(FreezeThawVMCommand.class))) + .thenReturn(freezeAnswer) + .thenReturn(thawAnswer); + when(vmSnapshotDetailsDao.listDetails(SNAPSHOT_ID)).thenReturn(Collections.emptyList()); + doReturn(true).when(vmSnapshotHelper).vmSnapshotStateTransitTo(any(), eq(VMSnapshot.Event.OperationFailed)); + + assertThrows(CloudRuntimeException.class, () -> strategy.takeVMSnapshot(vmSnapshot)); + + verify(snapshotFeignClient, times(1)).deleteConsistencyGroup(any(), eq("cg-uuid-1")); + verify(vmSnapshotHelper, atLeastOnce()).vmSnapshotStateTransitTo(any(), eq(VMSnapshot.Event.OperationFailed)); + } + // ══════════════════════════════════════════════════════════════════════════ // Tests: Quiesce Behavior // ══════════════════════════════════════════════════════════════════════════ @@ -746,20 +846,9 @@ void testTakeVMSnapshot_QuiesceFalse_SkipsFreezeThaw() throws Exception { setupTakeSnapshotCommon(vmSnapshot); setupSingleVolumeForTakeSnapshot(); + setupSingleFlexVolFlowMocks(strategy.buildSnapshotName(vmSnapshot)); - // The FlexVolume snapshot flow will try to call Utility.getStrategyByStoragePoolDetails - // which is a static method that makes real connections. We expect this to fail in unit tests. - // The important thing is that freeze/thaw was NOT called before the failure. - when(vmSnapshotDetailsDao.listDetails(SNAPSHOT_ID)).thenReturn(Collections.emptyList()); - doReturn(true).when(vmSnapshotHelper).vmSnapshotStateTransitTo(any(), eq(VMSnapshot.Event.OperationFailed)); - - // Since Utility.getStrategyByStoragePoolDetails is static and creates real Feign clients, - // this will fail. We just verify that freeze was never called. - try { - strategy.takeVMSnapshot(vmSnapshot); - } catch (Exception e) { - // Expected — static utility can't be mocked in unit test - } + strategy.takeVMSnapshot(vmSnapshot); // No freeze/thaw commands should be sent when quiesce is false verify(agentMgr, never()).send(eq(HOST_ID), any(FreezeThawVMCommand.class)); @@ -790,16 +879,9 @@ void testTakeVMSnapshot_WithParentSnapshot_SetsParentId() throws Exception { when(agentMgr.send(eq(HOST_ID), any(FreezeThawVMCommand.class))) .thenReturn(freezeAnswer) .thenReturn(thawAnswer); + setupSingleFlexVolFlowMocks(strategy.buildSnapshotName(vmSnapshot)); - when(vmSnapshotDetailsDao.listDetails(SNAPSHOT_ID)).thenReturn(Collections.emptyList()); - doReturn(true).when(vmSnapshotHelper).vmSnapshotStateTransitTo(any(), eq(VMSnapshot.Event.OperationFailed)); - - // FlexVol snapshot flow will fail on static method, but parent should already be set - try { - strategy.takeVMSnapshot(vmSnapshot); - } catch (Exception e) { - // Expected - } + strategy.takeVMSnapshot(vmSnapshot); // Verify parent was set on the VM snapshot before the FlexVol snapshot attempt verify(vmSnapshot).setParent(199L); @@ -820,15 +902,9 @@ void testTakeVMSnapshot_WithNoParentSnapshot_SetsParentNull() throws Exception { when(agentMgr.send(eq(HOST_ID), any(FreezeThawVMCommand.class))) .thenReturn(freezeAnswer) .thenReturn(thawAnswer); + setupSingleFlexVolFlowMocks(strategy.buildSnapshotName(vmSnapshot)); - when(vmSnapshotDetailsDao.listDetails(SNAPSHOT_ID)).thenReturn(Collections.emptyList()); - doReturn(true).when(vmSnapshotHelper).vmSnapshotStateTransitTo(any(), eq(VMSnapshot.Event.OperationFailed)); - - try { - strategy.takeVMSnapshot(vmSnapshot); - } catch (Exception e) { - // Expected - } + strategy.takeVMSnapshot(vmSnapshot); verify(vmSnapshot).setParent(null); } @@ -866,6 +942,9 @@ private UserVmVO setupTakeSnapshotCommon(VMSnapshotVO vmSnapshot) throws Excepti when(vmSnapshotDao.findCurrentSnapshotByVmId(VM_ID)).thenReturn(null); doReturn(true).when(vmSnapshotHelper).vmSnapshotStateTransitTo(vmSnapshot, VMSnapshot.Event.CreateRequested); + doNothing().when(strategy).processAnswer(any(), any(), any(), any()); + doNothing().when(strategy).publishUsageEvent(any(), any(), any(), any()); + doNothing().when(strategy).publishUsageEvent(any(), any(), any(), anyLong(), anyLong()); return userVm; } @@ -880,6 +959,7 @@ private void setupSingleVolumeForTakeSnapshot() { VolumeVO volumeVO = mock(VolumeVO.class); when(volumeVO.getId()).thenReturn(VOLUME_ID_1); when(volumeVO.getPoolId()).thenReturn(POOL_ID_1); + when(volumeVO.getPath()).thenReturn("volume-301.qcow2"); when(volumeVO.getVmSnapshotChainSize()).thenReturn(null); when(volumeDao.findById(VOLUME_ID_1)).thenReturn(volumeVO); @@ -899,4 +979,127 @@ private void setupSingleVolumeForTakeSnapshot() { when(volumeInfo.getName()).thenReturn("vol-1"); when(volumeDataFactory.getVolume(VOLUME_ID_1)).thenReturn(volumeInfo); } + + private void setupMultiFlexVolForTakeSnapshot() { + VolumeObjectTO volumeTO1 = mock(VolumeObjectTO.class); + when(volumeTO1.getId()).thenReturn(VOLUME_ID_1); + when(volumeTO1.getSize()).thenReturn(10737418240L); + VolumeObjectTO volumeTO2 = mock(VolumeObjectTO.class); + when(volumeTO2.getId()).thenReturn(VOLUME_ID_2); + when(volumeTO2.getSize()).thenReturn(10737418240L); + List volumeTOs = Arrays.asList(volumeTO1, volumeTO2); + when(vmSnapshotHelper.getVolumeTOList(VM_ID)).thenReturn(volumeTOs); + + VolumeVO volumeVO1 = mock(VolumeVO.class); + when(volumeVO1.getId()).thenReturn(VOLUME_ID_1); + when(volumeVO1.getPoolId()).thenReturn(POOL_ID_1); + when(volumeVO1.getPath()).thenReturn("volume-301.qcow2"); + when(volumeVO1.getVmSnapshotChainSize()).thenReturn(null); + when(volumeDao.findById(VOLUME_ID_1)).thenReturn(volumeVO1); + + VolumeVO volumeVO2 = mock(VolumeVO.class); + when(volumeVO2.getId()).thenReturn(VOLUME_ID_2); + when(volumeVO2.getPoolId()).thenReturn(POOL_ID_2); + when(volumeVO2.getPath()).thenReturn("volume-302.qcow2"); + when(volumeVO2.getVmSnapshotChainSize()).thenReturn(null); + when(volumeDao.findById(VOLUME_ID_2)).thenReturn(volumeVO2); + + Map poolDetails1 = new HashMap<>(); + poolDetails1.put(OntapStorageConstants.VOLUME_UUID, "flexvol-uuid-1"); + poolDetails1.put(OntapStorageConstants.USERNAME, "admin"); + poolDetails1.put(OntapStorageConstants.PASSWORD, "pass"); + poolDetails1.put(OntapStorageConstants.STORAGE_IP, "10.0.0.1"); + poolDetails1.put(OntapStorageConstants.SVM_NAME, "svm1"); + poolDetails1.put(OntapStorageConstants.SIZE, "107374182400"); + poolDetails1.put(OntapStorageConstants.PROTOCOL, "NFS3"); + when(storagePoolDetailsDao.listDetailsKeyPairs(POOL_ID_1)).thenReturn(poolDetails1); + + Map poolDetails2 = new HashMap<>(); + poolDetails2.put(OntapStorageConstants.VOLUME_UUID, "flexvol-uuid-2"); + poolDetails2.put(OntapStorageConstants.USERNAME, "admin"); + poolDetails2.put(OntapStorageConstants.PASSWORD, "pass"); + poolDetails2.put(OntapStorageConstants.STORAGE_IP, "10.0.0.1"); + poolDetails2.put(OntapStorageConstants.SVM_NAME, "svm1"); + poolDetails2.put(OntapStorageConstants.SIZE, "107374182400"); + poolDetails2.put(OntapStorageConstants.PROTOCOL, "NFS3"); + when(storagePoolDetailsDao.listDetailsKeyPairs(POOL_ID_2)).thenReturn(poolDetails2); + + VolumeInfo volumeInfo1 = mock(VolumeInfo.class); + when(volumeInfo1.getId()).thenReturn(VOLUME_ID_1); + when(volumeDataFactory.getVolume(VOLUME_ID_1)).thenReturn(volumeInfo1); + VolumeInfo volumeInfo2 = mock(VolumeInfo.class); + when(volumeInfo2.getId()).thenReturn(VOLUME_ID_2); + when(volumeDataFactory.getVolume(VOLUME_ID_2)).thenReturn(volumeInfo2); + } + + private JobResponse createJobResponse(String uuid) { + Job job = new Job(); + job.setUuid(uuid); + JobResponse response = new JobResponse(); + response.setJob(job); + return response; + } + + private void setupSingleFlexVolFlowMocks(String snapshotName) { + doReturn(storageStrategy).when(strategy).resolveStorageStrategy(any()); + when(storageStrategy.getSnapshotFeignClient()).thenReturn(snapshotFeignClient); + when(storageStrategy.getAuthHeader()).thenReturn("Basic dGVzdDp0ZXN0"); + when(storageStrategy.jobPollForSuccess(any(), anyInt(), anyInt())).thenReturn(true); + + when(snapshotFeignClient.createSnapshot(any(), eq("flexvol-uuid-1"), any())) + .thenReturn(createJobResponse("job-fv-snap")); + + OntapResponse flexVolSnapshots = new OntapResponse<>(); + FlexVolSnapshot flexVolSnapshot = new FlexVolSnapshot(); + flexVolSnapshot.setUuid("fv-snap-uuid-1"); + flexVolSnapshot.setName(snapshotName); + flexVolSnapshots.setRecords(Collections.singletonList(flexVolSnapshot)); + when(snapshotFeignClient.getSnapshots(any(), eq("flexvol-uuid-1"), any())) + .thenReturn(flexVolSnapshots); + } + + private void setupTemporaryCgFlowMocks(String snapshotName) { + doReturn(storageStrategy).when(strategy).resolveStorageStrategy(any()); + when(storageStrategy.getSnapshotFeignClient()).thenReturn(snapshotFeignClient); + when(storageStrategy.getAuthHeader()).thenReturn("Basic dGVzdDp0ZXN0"); + when(storageStrategy.jobPollForSuccess(any(), anyInt(), anyInt())).thenReturn(true); + + when(snapshotFeignClient.createConsistencyGroup(any(), any())).thenReturn(createJobResponse("job-cg-create")); + OntapResponse> cgResponse = new OntapResponse<>(); + Map cgRecord = new HashMap<>(); + cgRecord.put("uuid", "cg-uuid-1"); + cgResponse.setRecords(Collections.singletonList(cgRecord)); + when(snapshotFeignClient.getConsistencyGroups(any(), any())).thenReturn(cgResponse); + + when(snapshotFeignClient.createConsistencyGroupSnapshot(any(), eq("cg-uuid-1"), any())) + .thenReturn(createJobResponse("job-cg-start")); + OntapResponse> cgSnapshotResponse = new OntapResponse<>(); + Map cgSnapshotRecord = new HashMap<>(); + cgSnapshotRecord.put("uuid", "cg-snap-uuid-1"); + cgSnapshotRecord.put("name", snapshotName); + cgSnapshotResponse.setRecords(Collections.singletonList(cgSnapshotRecord)); + when(snapshotFeignClient.getConsistencyGroupSnapshots(any(), eq("cg-uuid-1"), any())) + .thenReturn(cgSnapshotResponse); + when(snapshotFeignClient.commitConsistencyGroupSnapshot(any(), eq("cg-uuid-1"), eq("cg-snap-uuid-1"), any())) + .thenReturn(createJobResponse("job-cg-commit")); + + when(snapshotFeignClient.deleteConsistencyGroup(any(), eq("cg-uuid-1"))) + .thenReturn(createJobResponse("job-cg-delete")); + + OntapResponse flexVolSnapshots = new OntapResponse<>(); + FlexVolSnapshot flexVolSnapshot = new FlexVolSnapshot(); + flexVolSnapshot.setUuid("fv-snap-uuid-1"); + flexVolSnapshot.setName(snapshotName); + flexVolSnapshots.setRecords(Collections.singletonList(flexVolSnapshot)); + when(snapshotFeignClient.getSnapshots(any(), eq("flexvol-uuid-1"), any())) + .thenReturn(flexVolSnapshots); + + OntapResponse flexVolSnapshots2 = new OntapResponse<>(); + FlexVolSnapshot flexVolSnapshot2 = new FlexVolSnapshot(); + flexVolSnapshot2.setUuid("fv-snap-uuid-2"); + flexVolSnapshot2.setName(snapshotName); + flexVolSnapshots2.setRecords(Collections.singletonList(flexVolSnapshot2)); + when(snapshotFeignClient.getSnapshots(any(), eq("flexvol-uuid-2"), any())) + .thenReturn(flexVolSnapshots2); + } } From 4ec7c181a2d023df7440509211962cdebe812df5 Mon Sep 17 00:00:00 2001 From: "Jain, Rajiv" Date: Sat, 27 Jun 2026 12:58:30 +0530 Subject: [PATCH 2/8] CSTACKEX-200: added consistency for the VM level snapshots if CSV are at multiple flexvolumes --- .../driver/OntapPrimaryDatastoreDriver.java | 25 ++------- .../storage/service/StorageStrategy.java | 55 ++++++++++++++++--- .../storage/utils/OntapStorageConstants.java | 2 + .../vmsnapshot/OntapVMSnapshotStrategy.java | 53 +++--------------- .../OntapPrimaryDatastoreDriverTest.java | 1 + .../storage/service/StorageStrategyTest.java | 43 +++++++++++++++ 6 files changed, 107 insertions(+), 72 deletions(-) diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java index ac9b7e818747..5a1bbc9ee41c 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java @@ -97,6 +97,7 @@ public Map getCapabilities() { Map mapCapabilities = new HashMap<>(); mapCapabilities.put(DataStoreCapabilities.STORAGE_SYSTEM_SNAPSHOT.toString(), Boolean.TRUE.toString()); mapCapabilities.put(DataStoreCapabilities.CAN_CREATE_VOLUME_FROM_SNAPSHOT.toString(), Boolean.TRUE.toString()); + mapCapabilities.put(DataStoreCapabilities.CAN_REVERT_VOLUME_TO_SNAPSHOT.toString(), Boolean.TRUE.toString()); return mapCapabilities; } @@ -683,6 +684,7 @@ public void takeSnapshot(SnapshotInfo snapshot, AsyncCompletionCallback * - *

Protocol-specific handling (delegated to strategy classes):

- *
    - *
  • NFS (UnifiedNASStrategy): Uses the single-file restore API: - * {@code POST /api/storage/volumes/{volume_uuid}/snapshots/{snapshot_uuid}/files/{file_path}/restore} - * Restores the QCOW2 file from the FlexVolume snapshot to its original location.
    • - *
  • iSCSI (UnifiedSANStrategy): Uses the LUN restore API: - * {@code POST /api/storage/luns/{lun.uuid}/restore} - * Restores the LUN data from the snapshot to the specified destination path.
    • - *
+ *

Both NFS and iSCSI delegate to CLI-based SFSR: + * {@code POST /api/private/cli/volume/snapshot/restore-file}

*/ @Override public void revertSnapshot(SnapshotInfo snapshotOnImageStore, SnapshotInfo snapshotOnPrimaryStore, @@ -846,17 +841,7 @@ public void revertSnapshot(SnapshotInfo snapshotOnImageStore, SnapshotInfo snaps JobResponse jobResponse = storageStrategy.revertSnapshotForCloudStackVolume( snapshotName, flexVolUuid, ontapSnapshotUuid, volumePath, lunUuid, flexVolName); - if (jobResponse == null || jobResponse.getJob() == null) { - throw new CloudRuntimeException("Failed to initiate restore from snapshot [" + - snapshotName + "]"); - } - - // Poll for job completion (use longer timeout for large LUNs/files) - Boolean jobSucceeded = storageStrategy.jobPollForSuccess(jobResponse.getJob().getUuid(), 60, 2000); - if (!jobSucceeded) { - throw new CloudRuntimeException("Restore job failed for snapshot [" + - snapshotName + "]"); - } + storageStrategy.executeCliSfsrRestore(jobResponse, "revert snapshot [" + snapshotName + "]"); logger.info("revertSnapshot: Successfully restored {} [{}] from snapshot [{}]", ProtocolType.ISCSI.name().equalsIgnoreCase(protocol) ? "LUN" : "file", diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java index bd808a26d6f8..f6b9e57bc0ac 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java @@ -527,15 +527,13 @@ public String getNetworkInterface() { abstract public CloudStackVolume getCloudStackVolume(Map cloudStackVolumeMap); /** - * Reverts a CloudStack volume to a snapshot using protocol-specific ONTAP APIs. + * Reverts a CloudStack volume to a snapshot using ONTAP CLI-based Single File Snap Restore (SFSR). * - *

This method encapsulates the snapshot revert behavior based on protocol:

- *
    - *
  • iSCSI/FC: Uses {@code POST /api/storage/luns/{lun.uuid}/restore} - * to restore LUN data from the FlexVolume snapshot.
    • - *
  • NFS: Uses {@code POST /api/storage/volumes/{vol.uuid}/snapshots/{snap.uuid}/files/{path}/restore} - * to restore a single file from the FlexVolume snapshot.
    • - *
+ *

Both NFS and iSCSI use the CLI passthrough API: + * {@code POST /api/private/cli/volume/snapshot/restore-file}

+ * + *

Callers should invoke {@link #executeCliSfsrRestore(JobResponse, String)} after this + * method returns to poll the async job when present, or treat a missing job as synchronous success.

* * @param snapshotName The ONTAP FlexVolume snapshot name * @param flexVolUuid The FlexVolume UUID containing the snapshot @@ -681,4 +679,45 @@ public Boolean jobPollForSuccess(String jobUUID, int maxRetries, int sleepTimeIn } return true; } + + /** + * Polls an ONTAP async job when the API response includes a job reference. + * + *

When no job is returned (common for CLI passthrough SFSR on synchronous completion), + * the operation is treated as successful after HTTP 2xx.

+ * + * @param response ONTAP job response (may be null or without a job) + * @param operationName label for logging and error messages + */ + public void pollJobIfPresent(JobResponse response, String operationName) { + pollJobIfPresent(response, operationName, + OntapStorageConstants.ONTAP_CG_JOB_MAX_RETRIES, + OntapStorageConstants.ONTAP_CG_JOB_POLL_INTERVAL_MS); + } + + /** + * Polls an ONTAP async job when present, using caller-supplied retry settings. + */ + public void pollJobIfPresent(JobResponse response, String operationName, + int maxRetries, int pollIntervalMs) { + if (response == null || response.getJob() == null || response.getJob().getUuid() == null) { + logger.debug("pollJobIfPresent: No async job returned for operation [{}], continuing without polling", + operationName); + return; + } + Boolean success = jobPollForSuccess(response.getJob().getUuid(), maxRetries, pollIntervalMs); + if (!Boolean.TRUE.equals(success)) { + throw new CloudRuntimeException("ONTAP operation failed: " + operationName); + } + } + + /** + * Completes CLI-based SFSR ({@code restore-file}) orchestration: poll job when returned, + * otherwise accept synchronous success. + */ + public void executeCliSfsrRestore(JobResponse response, String operationName) { + pollJobIfPresent(response, operationName, + OntapStorageConstants.ONTAP_SFSR_JOB_MAX_RETRIES, + OntapStorageConstants.ONTAP_SFSR_JOB_POLL_INTERVAL_MS); + } } diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageConstants.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageConstants.java index 39ffb60633e7..a5243f35ac05 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageConstants.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageConstants.java @@ -108,6 +108,8 @@ public class OntapStorageConstants { public static final String ONTAP_TEMP_CG_PREFIX = "cs-temp-cg-"; public static final int ONTAP_CG_JOB_MAX_RETRIES = 60; public static final int ONTAP_CG_JOB_POLL_INTERVAL_MS = 2000; + public static final int ONTAP_SFSR_JOB_MAX_RETRIES = 60; + public static final int ONTAP_SFSR_JOB_POLL_INTERVAL_MS = 2000; /** vm_snapshot_details key for ONTAP FlexVolume-level VM snapshots. */ public static final String ONTAP_FLEXVOL_SNAPSHOT = "ontapFlexVolSnapshot"; diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategy.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategy.java index d07d2aebe604..aae4f289e100 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategy.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategy.java @@ -35,7 +35,6 @@ import org.apache.cloudstack.storage.datastore.db.StoragePoolDetailsDao; import org.apache.cloudstack.storage.datastore.db.StoragePoolVO; import org.apache.cloudstack.storage.feign.client.SnapshotFeignClient; -import org.apache.cloudstack.storage.feign.model.CliSnapshotRestoreRequest; import org.apache.cloudstack.storage.feign.model.FlexVolSnapshot; import org.apache.cloudstack.storage.feign.model.response.JobResponse; import org.apache.cloudstack.storage.feign.model.response.OntapResponse; @@ -811,7 +810,7 @@ String createTemporaryConsistencyGroup(SnapshotFeignClient client, StorageStrate payload.put("volumes", volumeRefs); JobResponse response = client.createConsistencyGroup(authHeader, payload); - pollJobIfPresent(storageStrategy, response, "create temporary consistency group " + cgName); + storageStrategy.pollJobIfPresent(response, "create temporary consistency group " + cgName); String cgUuid = resolveConsistencyGroupUuidByName(client, authHeader, cgName); if (cgUuid == null || cgUuid.isEmpty()) { @@ -829,7 +828,7 @@ void startConsistencyGroupSnapshot(SnapshotFeignClient client, StorageStrategy s payload.put("name", snapshotName); payload.put("action", "start"); JobResponse response = client.createConsistencyGroupSnapshot(authHeader, cgUuid, payload); - pollJobIfPresent(storageStrategy, response, "start CG snapshot " + snapshotName + " for " + cgUuid); + storageStrategy.pollJobIfPresent(response, "start CG snapshot " + snapshotName + " for " + cgUuid); } /** @@ -840,7 +839,7 @@ void commitConsistencyGroupSnapshot(SnapshotFeignClient client, StorageStrategy Map payload = new LinkedHashMap<>(); payload.put("action", "commit"); JobResponse response = client.commitConsistencyGroupSnapshot(authHeader, cgUuid, snapshotUuid, payload); - pollJobIfPresent(storageStrategy, response, "commit CG snapshot " + snapshotUuid + " for " + cgUuid); + storageStrategy.pollJobIfPresent(response, "commit CG snapshot " + snapshotUuid + " for " + cgUuid); } /** @@ -849,7 +848,7 @@ void commitConsistencyGroupSnapshot(SnapshotFeignClient client, StorageStrategy void deleteTemporaryConsistencyGroup(SnapshotFeignClient client, StorageStrategy storageStrategy, String authHeader, String cgUuid) { JobResponse response = client.deleteConsistencyGroup(authHeader, cgUuid); - pollJobIfPresent(storageStrategy, response, "delete temporary consistency group " + cgUuid); + storageStrategy.pollJobIfPresent(response, "delete temporary consistency group " + cgUuid); } /** @@ -887,23 +886,6 @@ String resolveConsistencyGroupSnapshotUuid(SnapshotFeignClient client, String au return snapshotUuid; } - /** - * Polls ONTAP job only when the endpoint returns a job reference. - */ - void pollJobIfPresent(StorageStrategy storageStrategy, JobResponse response, String operationName) { - if (response == null || response.getJob() == null || response.getJob().getUuid() == null) { - logger.debug("pollJobIfPresent: No async job returned for operation [{}], continuing without polling", operationName); - return; - } - Boolean success = storageStrategy.jobPollForSuccess( - response.getJob().getUuid(), - OntapStorageConstants.ONTAP_CG_JOB_MAX_RETRIES, - OntapStorageConstants.ONTAP_CG_JOB_POLL_INTERVAL_MS); - if (!Boolean.TRUE.equals(success)) { - throw new CloudRuntimeException("ONTAP operation failed: " + operationName); - } - } - private String getStringField(Map record, String key) { if (record == null) { return null; @@ -1036,40 +1018,23 @@ void revertFlexVolSnapshots(List flexVolDetails) { Map poolDetails = storagePoolDetailsDao.listDetailsKeyPairs(detail.poolId); StorageStrategy storageStrategy = resolveStorageStrategy(poolDetails); - SnapshotFeignClient snapshotClient = storageStrategy.getSnapshotFeignClient(); - String authHeader = storageStrategy.getAuthHeader(); - // Get SVM name and FlexVolume name from pool details - String svmName = poolDetails.get(OntapStorageConstants.SVM_NAME); String flexVolName = poolDetails.get(OntapStorageConstants.VOLUME_NAME); - - if (svmName == null || svmName.isEmpty()) { - throw new CloudRuntimeException("SVM name not found in pool details for pool [" + detail.poolId + "]"); - } if (flexVolName == null || flexVolName.isEmpty()) { throw new CloudRuntimeException("FlexVolume name not found in pool details for pool [" + detail.poolId + "]"); } - // The path must start with "/" for the ONTAP CLI API String ontapFilePath = detail.volumePath.startsWith("/") ? detail.volumePath : "/" + detail.volumePath; logger.info("revertFlexVolSnapshots: Restoring volume [{}] from FlexVol snapshot [{}] on FlexVol [{}] (protocol={})", ontapFilePath, detail.snapshotName, flexVolName, detail.protocol); - // Use CLI-based restore API: POST /api/private/cli/volume/snapshot/restore-file - CliSnapshotRestoreRequest restoreRequest = new CliSnapshotRestoreRequest( - svmName, flexVolName, detail.snapshotName, ontapFilePath); - - JobResponse jobResponse = snapshotClient.restoreFileFromSnapshotCli(authHeader, restoreRequest); + JobResponse jobResponse = storageStrategy.revertSnapshotForCloudStackVolume( + detail.snapshotName, detail.flexVolUuid, detail.snapshotUuid, + detail.volumePath, null, flexVolName); - if (jobResponse != null && jobResponse.getJob() != null) { - Boolean success = storageStrategy.jobPollForSuccess(jobResponse.getJob().getUuid(), 60, 2000); - if (!success) { - throw new CloudRuntimeException("Snapshot file restore failed for volume path [" + - ontapFilePath + "] from snapshot [" + detail.snapshotName + - "] on FlexVol [" + flexVolName + "]"); - } - } + storageStrategy.executeCliSfsrRestore(jobResponse, + "VM snapshot file restore for path [" + ontapFilePath + "] from snapshot [" + detail.snapshotName + "]"); logger.info("revertFlexVolSnapshots: Successfully restored volume [{}] from snapshot [{}] on FlexVol [{}]", ontapFilePath, detail.snapshotName, flexVolName); diff --git a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriverTest.java b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriverTest.java index b535217fd235..e176bd3993b3 100644 --- a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriverTest.java +++ b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriverTest.java @@ -134,6 +134,7 @@ void testGetCapabilities() { // so StorageSystemSnapshotStrategy handles snapshot backup to secondary storage assertEquals(Boolean.TRUE.toString(), capabilities.get("STORAGE_SYSTEM_SNAPSHOT")); assertEquals(Boolean.TRUE.toString(), capabilities.get("CAN_CREATE_VOLUME_FROM_SNAPSHOT")); + assertEquals(Boolean.TRUE.toString(), capabilities.get("CAN_REVERT_VOLUME_TO_SNAPSHOT")); } @Test diff --git a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java index 86ef1d7c79b6..ca952fef0887 100644 --- a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java +++ b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java @@ -823,4 +823,47 @@ private void setupSuccessfulJobCreation() { when(volumeFeignClient.getVolume(anyString(), anyMap())) .thenReturn(volumeResponse); } + + // ========== pollJobIfPresent / executeCliSfsrRestore Tests ========== + + @Test + void testPollJobIfPresent_NoJob_DoesNotPoll() { + storageStrategy.pollJobIfPresent(null, "test operation"); + storageStrategy.pollJobIfPresent(new JobResponse(), "test operation"); + verify(jobFeignClient, times(0)).getJobByUUID(anyString(), anyString()); + } + + @Test + void testPollJobIfPresent_WithJob_PollsUntilSuccess() { + Job job = new Job(); + job.setUuid("sfsr-job-1"); + JobResponse response = new JobResponse(); + response.setJob(job); + + Job completedJob = new Job(); + completedJob.setUuid("sfsr-job-1"); + completedJob.setState(OntapStorageConstants.JOB_SUCCESS); + when(jobFeignClient.getJobByUUID(anyString(), eq("sfsr-job-1"))).thenReturn(completedJob); + + storageStrategy.executeCliSfsrRestore(response, "CLI SFSR restore"); + + verify(jobFeignClient, atLeastOnce()).getJobByUUID(anyString(), eq("sfsr-job-1")); + } + + @Test + void testPollJobIfPresent_JobFailure_ThrowsCloudRuntimeException() { + Job job = new Job(); + job.setUuid("sfsr-job-fail"); + JobResponse response = new JobResponse(); + response.setJob(job); + + Job failedJob = new Job(); + failedJob.setUuid("sfsr-job-fail"); + failedJob.setState(OntapStorageConstants.JOB_FAILURE); + failedJob.setMessage("restore failed"); + when(jobFeignClient.getJobByUUID(anyString(), eq("sfsr-job-fail"))).thenReturn(failedJob); + + assertThrows(CloudRuntimeException.class, + () -> storageStrategy.executeCliSfsrRestore(response, "CLI SFSR restore")); + } } From 4b482df935d7479042426488839fdc6d85945ca7 Mon Sep 17 00:00:00 2001 From: "Jain, Rajiv" Date: Sun, 28 Jun 2026 10:32:27 +0530 Subject: [PATCH 3/8] CSTACKEX-200: ONTAP will have snapshot always on primary, adding respective checks by which delete snapshot would be handled from primary only --- .../driver/OntapPrimaryDatastoreDriver.java | 140 ++++++++++++------ .../storage/service/StorageStrategy.java | 43 ++++++ .../storage/utils/OntapStorageConstants.java | 2 + .../vmsnapshot/OntapVMSnapshotStrategy.java | 11 +- .../storage/service/StorageStrategyTest.java | 39 ++++- .../storage/snapshot/SnapshotManagerImpl.java | 48 +++++- 6 files changed, 214 insertions(+), 69 deletions(-) diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java index 5a1bbc9ee41c..b3ced8dc9900 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java @@ -32,6 +32,8 @@ import com.cloud.storage.VolumeDetailVO; import com.cloud.storage.VolumeVO; import com.cloud.storage.ScopeType; +import com.cloud.storage.SnapshotVO; +import com.cloud.storage.dao.SnapshotDao; import com.cloud.storage.dao.SnapshotDetailsDao; import com.cloud.storage.dao.SnapshotDetailsVO; import com.cloud.storage.dao.VolumeDao; @@ -90,6 +92,7 @@ public class OntapPrimaryDatastoreDriver implements PrimaryDataStoreDriver { @Inject private VolumeDao volumeDao; @Inject private VolumeDetailsDao volumeDetailsDao; @Inject private SnapshotDetailsDao snapshotDetailsDao; + @Inject private SnapshotDao snapshotDao; @Override public Map getCapabilities() { @@ -210,8 +213,14 @@ private CloudStackVolume createCloudStackVolume(StoragePoolVO storagePool, Volum /** * Deletes a volume or snapshot from the ONTAP storage system. * - *

For volumes, deletes the backend storage object (LUN for iSCSI, no-op for NFS). - * For snapshots, deletes the FlexVolume snapshot from ONTAP that was created by takeSnapshot.

+ *

For volumes, deletes the backend storage object (LUN for iSCSI, file for NFS) via + * {@link StorageStrategy#deleteCloudStackVolume}.

+ * + *

For volume snapshots, this driver is invoked by the standard CloudStack delete chain + * ({@code StorageSystemSnapshotStrategy} → {@code SnapshotServiceImpl.deleteSnapshot} → + * {@code deleteAsync}). It reads ONTAP metadata from {@code snapshot_details} and delegates + * the actual FlexVol snapshot delete to {@link StorageStrategy} (NFS or iSCSI implementation). + * ONTAP REST/delete-job logic must not live here — keep it in the storage-strategy layer.

*/ @Override public void deleteAsync(DataStore store, DataObject data, AsyncCompletionCallback callback) { @@ -237,8 +246,9 @@ public void deleteAsync(DataStore store, DataObject data, AsyncCompletionCallbac commandResult.setResult(null); commandResult.setSuccess(true); } else if (data.getType() == DataObjectType.SNAPSHOT) { - // Delete the ONTAP FlexVolume snapshot that was created by takeSnapshot - deleteOntapSnapshot((SnapshotInfo) data, commandResult); + logger.info("deleteAsync: volume-snapshot delete for CloudStack snapshot [{}] on primary pool [{}] — " + + "delegating ONTAP FlexVol cleanup to StorageStrategy", data.getId(), store.getId()); + deleteCloudStackVolumeSnapshot((SnapshotInfo) data, commandResult); } else { throw new CloudRuntimeException("Unsupported data object type: " + data.getType()); } @@ -252,79 +262,107 @@ public void deleteAsync(DataStore store, DataObject data, AsyncCompletionCallbac } /** - * Deletes an ONTAP FlexVolume snapshot. + * Orchestrates CloudStack volume-snapshot delete on ONTAP. * - *

Retrieves the snapshot details stored during takeSnapshot and calls the ONTAP - * REST API to delete the FlexVolume snapshot.

+ *

This method is intentionally thin: it resolves identifiers persisted during + * {@link #takeSnapshot} into {@code snapshot_details} and delegates to the protocol + * {@link StorageStrategy} selected from pool details (NFS → {@code UnifiedNASStrategy}, + * iSCSI → {@code UnifiedSANStrategy}). Both protocols share the same FlexVol-level + * snapshot delete REST API.

* - * @param snapshotInfo The CloudStack snapshot to delete - * @param commandResult Result object to populate with success/failure + *

Required {@code snapshot_details} keys (see {@link OntapStorageConstants}):

+ *
    + *
  • {@code base_ontap_fv_id} — FlexVol UUID
    • + *
  • {@code ontap_snap_id} — ONTAP snapshot UUID
    • + *
  • {@code ontap_snap_name} — snapshot name (logging)
    • + *
  • {@code primary_pool_id} — pool used to obtain credentials/protocol strategy
    • + *
*/ - private void deleteOntapSnapshot(SnapshotInfo snapshotInfo, CommandResult commandResult) { + private void deleteCloudStackVolumeSnapshot(SnapshotInfo snapshotInfo, CommandResult commandResult) { long snapshotId = snapshotInfo.getId(); - logger.info("deleteOntapSnapshot: Deleting ONTAP FlexVolume snapshot for CloudStack snapshot [{}]", snapshotId); + logger.info("deleteCloudStackVolumeSnapshot: starting ONTAP delete for CloudStack volume snapshot [{}]", snapshotId); try { - // Retrieve snapshot details stored during takeSnapshot String flexVolUuid = getSnapshotDetail(snapshotId, OntapStorageConstants.BASE_ONTAP_FV_ID); String ontapSnapshotUuid = getSnapshotDetail(snapshotId, OntapStorageConstants.ONTAP_SNAP_ID); String snapshotName = getSnapshotDetail(snapshotId, OntapStorageConstants.ONTAP_SNAP_NAME); String poolIdStr = getSnapshotDetail(snapshotId, OntapStorageConstants.PRIMARY_POOL_ID); if (flexVolUuid == null || ontapSnapshotUuid == null) { - logger.warn("deleteOntapSnapshot: Missing ONTAP snapshot details for snapshot [{}]. " + - "flexVolUuid={}, ontapSnapshotUuid={}. Snapshot may have been created by a different method or already deleted.", + logger.warn("deleteCloudStackVolumeSnapshot: missing ONTAP identity for snapshot [{}] " + + "(flexVolUuid={}, ontapSnapshotUuid={}). Cannot call ONTAP delete; " + + "treating as no-op — verify snapshot_details were written during takeSnapshot", snapshotId, flexVolUuid, ontapSnapshotUuid); - // Consider this a success since there's nothing to delete on ONTAP commandResult.setSuccess(true); commandResult.setResult(null); return; } - long poolId = Long.parseLong(poolIdStr); + long poolId = resolveSnapshotPoolId(poolIdStr, snapshotId); Map poolDetails = storagePoolDetailsDao.listDetailsKeyPairs(poolId); - + String protocol = poolDetails.get(OntapStorageConstants.PROTOCOL); StorageStrategy storageStrategy = OntapStorageUtils.getStrategyByStoragePoolDetails(poolDetails); - SnapshotFeignClient snapshotClient = storageStrategy.getSnapshotFeignClient(); - String authHeader = storageStrategy.getAuthHeader(); - logger.info("deleteOntapSnapshot: Deleting ONTAP snapshot [{}] (uuid={}) from FlexVol [{}]", - snapshotName, ontapSnapshotUuid, flexVolUuid); + logger.info("deleteCloudStackVolumeSnapshot: snapshot [{}] — protocol [{}], pool [{}], " + + "flexVol [{}], ontapSnapshot [{}] (name [{}])", + snapshotId, protocol, poolId, flexVolUuid, ontapSnapshotUuid, snapshotName); - // Call ONTAP REST API to delete the snapshot - JobResponse jobResponse = snapshotClient.deleteSnapshot(authHeader, flexVolUuid, ontapSnapshotUuid); - - if (jobResponse != null && jobResponse.getJob() != null) { - // Poll for job completion - Boolean jobSucceeded = storageStrategy.jobPollForSuccess(jobResponse.getJob().getUuid(), 30, 2000); - if (!jobSucceeded) { - throw new CloudRuntimeException("Delete job failed for snapshot [" + - snapshotName + "] on FlexVol [" + flexVolUuid + "]"); - } - } - - logger.info("deleteOntapSnapshot: Successfully deleted ONTAP snapshot [{}] (uuid={}) for CloudStack snapshot [{}]", - snapshotName, ontapSnapshotUuid, snapshotId); + storageStrategy.deleteFlexVolSnapshotForCloudStackVolume(flexVolUuid, ontapSnapshotUuid, snapshotName); + logger.info("deleteCloudStackVolumeSnapshot: completed ONTAP delete for CloudStack volume snapshot [{}]", snapshotId); commandResult.setSuccess(true); commandResult.setResult(null); - } catch (Exception e) { - // Check if the error indicates snapshot doesn't exist (already deleted) - String errorMsg = e.getMessage(); - if (errorMsg != null && (errorMsg.contains("404") || errorMsg.contains("not found") || - errorMsg.contains("does not exist"))) { - logger.warn("deleteOntapSnapshot: ONTAP snapshot for CloudStack snapshot [{}] not found, " + - "may have been already deleted. Treating as success.", snapshotId); + if (isSnapshotNotFoundError(e)) { + logger.warn("deleteCloudStackVolumeSnapshot: ONTAP snapshot for CloudStack snapshot [{}] " + + "already absent (idempotent success): {}", snapshotId, e.getMessage()); commandResult.setSuccess(true); commandResult.setResult(null); - } else { - logger.error("deleteOntapSnapshot: Failed to delete ONTAP snapshot for CloudStack snapshot [{}]: {}", - snapshotId, e.getMessage(), e); - commandResult.setSuccess(false); - commandResult.setResult(e.getMessage()); + return; + } + logger.error("deleteCloudStackVolumeSnapshot: ONTAP delete failed for CloudStack snapshot [{}]: {}", + snapshotId, e.getMessage(), e); + commandResult.setSuccess(false); + commandResult.setResult(e.getMessage()); + } + } + + /** + * Returns true when the exception indicates the ONTAP snapshot was already removed. + * Delete is idempotent: a missing backend snapshot is treated as success. + */ + private boolean isSnapshotNotFoundError(Throwable error) { + if (error == null) { + return false; + } + String message = error.getMessage(); + if (message != null) { + String lower = message.toLowerCase(); + if (lower.contains("404") || lower.contains("not found") || lower.contains("does not exist") + || lower.contains("entry doesn't exist")) { + return true; } } + return isSnapshotNotFoundError(error.getCause()); + } + + private long resolveSnapshotPoolId(String poolIdStr, long snapshotId) { + if (poolIdStr != null && !poolIdStr.isEmpty()) { + return Long.parseLong(poolIdStr); + } + SnapshotVO snapshotVO = snapshotDao.findById(snapshotId); + if (snapshotVO == null) { + throw new CloudRuntimeException("Cannot resolve storage pool for snapshot [" + snapshotId + "]"); + } + VolumeVO volumeVO = volumeDao.findByIdIncludingRemoved(snapshotVO.getVolumeId()); + if (volumeVO == null) { + throw new CloudRuntimeException("Cannot resolve storage pool for snapshot [" + snapshotId + "]"); + } + Long poolId = volumeVO.getPoolId() != null ? volumeVO.getPoolId() : volumeVO.getLastPoolId(); + if (poolId == null || poolId <= 0) { + throw new CloudRuntimeException("Cannot resolve storage pool for snapshot [" + snapshotId + "]"); + } + return poolId; } @Override @@ -714,7 +752,8 @@ public void takeSnapshot(SnapshotInfo snapshot, AsyncCompletionCallback flexVolDetails) { if (!deletedSnapshots.containsKey(dedupeKey)) { Map poolDetails = storagePoolDetailsDao.listDetailsKeyPairs(detail.poolId); StorageStrategy storageStrategy = resolveStorageStrategy(poolDetails); - SnapshotFeignClient client = storageStrategy.getSnapshotFeignClient(); - String authHeader = storageStrategy.getAuthHeader(); logger.info("deleteFlexVolSnapshots: Deleting ONTAP FlexVol snapshot [{}] (uuid={}) on FlexVol [{}]", detail.snapshotName, detail.snapshotUuid, detail.flexVolUuid); - JobResponse jobResponse = client.deleteSnapshot(authHeader, detail.flexVolUuid, detail.snapshotUuid); - if (jobResponse != null && jobResponse.getJob() != null) { - storageStrategy.jobPollForSuccess(jobResponse.getJob().getUuid(), 30, 2000); - } + storageStrategy.deleteFlexVolSnapshotForCloudStackVolume( + detail.flexVolUuid, detail.snapshotUuid, detail.snapshotName); deletedSnapshots.put(dedupeKey, Boolean.TRUE); - logger.info("deleteFlexVolSnapshots: Deleted ONTAP FlexVol snapshot [{}] on FlexVol [{}]", detail.snapshotName, detail.flexVolUuid); + logger.info("deleteFlexVolSnapshots: Deleted ONTAP FlexVol snapshot [{}] on FlexVol [{}]", + detail.snapshotName, detail.flexVolUuid); } // Always remove the DB detail row diff --git a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java index ca952fef0887..1639853a24f1 100644 --- a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java +++ b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java @@ -24,6 +24,7 @@ import org.apache.cloudstack.storage.feign.client.JobFeignClient; import org.apache.cloudstack.storage.feign.client.NetworkFeignClient; import org.apache.cloudstack.storage.feign.client.SANFeignClient; +import org.apache.cloudstack.storage.feign.client.SnapshotFeignClient; import org.apache.cloudstack.storage.feign.client.SvmFeignClient; import org.apache.cloudstack.storage.feign.client.VolumeFeignClient; import org.apache.cloudstack.storage.feign.model.Aggregate; @@ -88,6 +89,9 @@ public class StorageStrategyTest { @Mock private SANFeignClient sanFeignClient; + @Mock + private SnapshotFeignClient snapshotFeignClient; + private TestableStorageStrategy storageStrategy; // Concrete implementation for testing abstract class @@ -98,7 +102,8 @@ public TestableStorageStrategy(OntapStorage ontapStorage, SvmFeignClient svmFeignClient, JobFeignClient jobFeignClient, NetworkFeignClient networkFeignClient, - SANFeignClient sanFeignClient) { + SANFeignClient sanFeignClient, + SnapshotFeignClient snapshotFeignClient) { super(ontapStorage); // Use reflection to replace the private Feign client fields with mocked ones injectMockedClient("aggregateFeignClient", aggregateFeignClient); @@ -107,6 +112,7 @@ public TestableStorageStrategy(OntapStorage ontapStorage, injectMockedClient("jobFeignClient", jobFeignClient); injectMockedClient("networkFeignClient", networkFeignClient); injectMockedClient("sanFeignClient", sanFeignClient); + injectMockedClient("snapshotFeignClient", snapshotFeignClient); } private void injectMockedClient(String fieldName, Object mockedClient) { @@ -192,7 +198,7 @@ void setUp() { // For testing, we'll need to mock the FeignClientFactory behavior storageStrategy = new TestableStorageStrategy(ontapStorage, aggregateFeignClient, volumeFeignClient, svmFeignClient, - jobFeignClient, networkFeignClient, sanFeignClient); + jobFeignClient, networkFeignClient, sanFeignClient, snapshotFeignClient); } // ========== connect() Tests ========== @@ -291,7 +297,7 @@ public void testConnect_iscsiNotEnabled() { "svm1", 5000000000L, ProtocolType.ISCSI); storageStrategy = new TestableStorageStrategy(iscsiStorage, aggregateFeignClient, volumeFeignClient, svmFeignClient, - jobFeignClient, networkFeignClient, sanFeignClient); + jobFeignClient, networkFeignClient, sanFeignClient, snapshotFeignClient); Svm svm = new Svm(); svm.setName("svm1"); @@ -608,7 +614,7 @@ public void testGetStoragePath_iscsi() { "svm1", null, ProtocolType.ISCSI); storageStrategy = new TestableStorageStrategy(iscsiStorage, aggregateFeignClient, volumeFeignClient, svmFeignClient, - jobFeignClient, networkFeignClient, sanFeignClient); + jobFeignClient, networkFeignClient, sanFeignClient, snapshotFeignClient); IscsiService.IscsiServiceTarget target = new IscsiService.IscsiServiceTarget(); target.setName("iqn.1992-08.com.netapp:sn.123456:vs.1"); @@ -638,7 +644,7 @@ public void testGetStoragePath_iscsi_noService() { "svm1", null, ProtocolType.ISCSI); storageStrategy = new TestableStorageStrategy(iscsiStorage, aggregateFeignClient, volumeFeignClient, svmFeignClient, - jobFeignClient, networkFeignClient, sanFeignClient); + jobFeignClient, networkFeignClient, sanFeignClient, snapshotFeignClient); OntapResponse emptyResponse = new OntapResponse<>(); emptyResponse.setRecords(new ArrayList<>()); @@ -659,7 +665,7 @@ public void testGetStoragePath_iscsi_noTargetIqn() { "svm1", null, ProtocolType.ISCSI); storageStrategy = new TestableStorageStrategy(iscsiStorage, aggregateFeignClient, volumeFeignClient, svmFeignClient, - jobFeignClient, networkFeignClient, sanFeignClient); + jobFeignClient, networkFeignClient, sanFeignClient, snapshotFeignClient); IscsiService iscsiService = new IscsiService(); iscsiService.setTarget(null); @@ -709,7 +715,7 @@ public void testGetNetworkInterface_iscsi() { "svm1", null, ProtocolType.ISCSI); storageStrategy = new TestableStorageStrategy(iscsiStorage, aggregateFeignClient, volumeFeignClient, svmFeignClient, - jobFeignClient, networkFeignClient, sanFeignClient); + jobFeignClient, networkFeignClient, sanFeignClient, snapshotFeignClient); IpInterface.IpInfo ipInfo = new IpInterface.IpInfo(); ipInfo.setAddress("192.168.1.51"); @@ -866,4 +872,23 @@ void testPollJobIfPresent_JobFailure_ThrowsCloudRuntimeException() { assertThrows(CloudRuntimeException.class, () -> storageStrategy.executeCliSfsrRestore(response, "CLI SFSR restore")); } + + @Test + void testDeleteFlexVolSnapshotForCloudStackVolume_PollsJobAndSucceeds() { + Job job = new Job(); + job.setUuid("delete-job-1"); + JobResponse response = new JobResponse(); + response.setJob(job); + when(snapshotFeignClient.deleteSnapshot(anyString(), eq("fv-uuid-1"), eq("snap-uuid-1"))) + .thenReturn(response); + + Job completedJob = new Job(); + completedJob.setUuid("delete-job-1"); + completedJob.setState(OntapStorageConstants.JOB_SUCCESS); + when(jobFeignClient.getJobByUUID(anyString(), eq("delete-job-1"))).thenReturn(completedJob); + + storageStrategy.deleteFlexVolSnapshotForCloudStackVolume("fv-uuid-1", "snap-uuid-1", "snap-name-1"); + + verify(snapshotFeignClient).deleteSnapshot(anyString(), eq("fv-uuid-1"), eq("snap-uuid-1")); + } } diff --git a/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java b/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java index ce01048eaa56..dd27aa1f15a5 100755 --- a/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java +++ b/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java @@ -1633,7 +1633,15 @@ public SnapshotInfo takeSnapshot(VolumeInfo volume) throws ResourceAllocationExc boolean isKvmAndFileBasedStorage = isHypervisorKvmAndFileBasedStorage(volume, storagePool); boolean backupSnapToSecondary = isBackupSnapshotToSecondaryForZone(volume.getDataCenterId()); - if (isKvmAndFileBasedStorage && backupSnapToSecondary) { + updateSnapshotPayload(volume.getPoolId(), payload, isKvmAndFileBasedStorage, clusterId); + + // Managed PRIMARY snapshots (ONTAP volume snapshots, etc.) remain on primary/array storage. + // They must not use secondary archive bookkeeping (postSnapshotDirectlyToSecondary) or a physical + // secondary copy — delete is handled via StorageSystemSnapshotStrategy → driver deleteAsync. + boolean archiveSnapshotToSecondary = backupSnapToSecondary + && !isManagedPrimaryLocationSnapshot(storagePool, payload); + + if (isKvmAndFileBasedStorage && archiveSnapshotToSecondary) { DataStore imageStore = snapshotSrv.findSnapshotImageStore(snapshot); if (imageStore == null) { throw new CloudRuntimeException(String.format("Could not find any secondary storage to allocate snapshot [%s].", snapshot)); @@ -1641,8 +1649,6 @@ public SnapshotInfo takeSnapshot(VolumeInfo volume) throws ResourceAllocationExc snapshot.setImageStore(imageStore); } - updateSnapshotPayload(volume.getPoolId(), payload, isKvmAndFileBasedStorage, clusterId); - snapshot.addPayload(payload); try { SnapshotStrategy snapshotStrategy = _storageStrategyFactory.getSnapshotStrategy(snapshot, SnapshotOperation.TAKE); @@ -1656,14 +1662,22 @@ public SnapshotInfo takeSnapshot(VolumeInfo volume) throws ResourceAllocationExc SnapshotInfo snapshotOnPrimary = snapshotStrategy.takeSnapshot(snapshot); - if (backupSnapToSecondary) { + if (archiveSnapshotToSecondary) { if (!isKvmAndFileBasedStorage) { backupSnapshotToSecondary(payload.getAsyncBackup(), snapshotStrategy, snapshotOnPrimary, payload.getZoneIds(), payload.getStoragePoolIds()); } else { postSnapshotDirectlyToSecondary(snapshot, snapshotOnPrimary, snapshotId); } } else { - logger.debug("Skipping backup of snapshot [{}] to secondary due to configuration [{}].", snapshotOnPrimary.getUuid(), SnapshotInfo.BackupSnapshotAfterTakingSnapshot.key()); + if (backupSnapToSecondary && isManagedPrimaryLocationSnapshot(storagePool, payload)) { + logger.info("takeSnapshot: snapshot [{}] on managed primary pool [{}] with locationType=PRIMARY — " + + "keeping snapshot on primary/array storage only; not archiving to secondary " + + "(backup.snapshot.after.take is ignored for this snapshot class)", + snapshotId, storagePool.getId()); + } else { + logger.debug("Skipping backup of snapshot [{}] to secondary due to configuration [{}].", + snapshotOnPrimary.getUuid(), SnapshotInfo.BackupSnapshotAfterTakingSnapshot.key()); + } if (CollectionUtils.isNotEmpty(payload.getStoragePoolIds()) && payload.getAsyncBackup()) { snapshotStrategy = _storageStrategyFactory.getSnapshotStrategy(snapshot, SnapshotOperation.COPY); @@ -1678,7 +1692,7 @@ public SnapshotInfo takeSnapshot(VolumeInfo volume) throws ResourceAllocationExc postCreateSnapshot(volume.getId(), snapshotId, payload.getSnapshotPolicyId(), clusterId); snapshotZoneDao.addSnapshotToZone(snapshotId, snapshot.getDataCenterId()); - DataStoreRole dataStoreRole = backupSnapToSecondary ? snapshotHelper.getDataStoreRole(snapshot) : DataStoreRole.Primary; + DataStoreRole dataStoreRole = archiveSnapshotToSecondary ? snapshotHelper.getDataStoreRole(snapshot) : DataStoreRole.Primary; List snapshotStoreRefs = _snapshotStoreDao.listReadyBySnapshot(snapshotId, dataStoreRole); if (CollectionUtils.isEmpty(snapshotStoreRefs)) { @@ -1693,7 +1707,7 @@ public SnapshotInfo takeSnapshot(VolumeInfo volume) throws ResourceAllocationExc _resourceLimitMgr.decrementResourceCount(snapshotOwner.getId(), storeResourceType, volume.getSize() - snapshotStoreRef.getPhysicalSize()); if (!payload.getAsyncBackup()) { - if (backupSnapToSecondary) { + if (archiveSnapshotToSecondary) { copyNewSnapshotToZones(snapshotId, snapshot.getDataCenterId(), payload.getZoneIds()); } if (CollectionUtils.isNotEmpty(payload.getStoragePoolIds())) { @@ -1723,6 +1737,12 @@ public SnapshotInfo takeSnapshot(VolumeInfo volume) throws ResourceAllocationExc return snapshot; } + /** + * KVM file-based fast-path: records snapshot on image store without copying bytes, then drops the + * primary {@code snapshot_data_store} row. Used only for non-managed primary storage when + * {@code backup.snapshot.after.take} is enabled. Managed PRIMARY snapshots (e.g. ONTAP) never + * call this method — see {@link #isManagedPrimaryLocationSnapshot}. + */ private void postSnapshotDirectlyToSecondary(SnapshotInfo snapshot, SnapshotInfo snapshotOnPrimary, Long snapshotId) { logger.debug("{} was directly copied to secondary storage because the hypervisor is KVM, the primary storage is file-based and the [{}] configuration" + " is set to true.", snapshot.getSnapshotVO().toString(), SnapshotInfo.BackupSnapshotAfterTakingSnapshot); @@ -1739,6 +1759,20 @@ private void postSnapshotDirectlyToSecondary(SnapshotInfo snapshot, SnapshotInfo snapshotDetailsDao.removeDetail(snapshotOnPrimary.getId(), AsyncJob.Constants.MS_ID); } + /** + * Returns true when a volume snapshot is explicitly kept on managed primary/array storage. + * + *

For managed pools, {@link #updateSnapshotPayload} defaults {@code locationType} to + * {@link Snapshot.LocationType#PRIMARY}. ONTAP volume snapshots therefore stay on the FlexVol + * on primary — they are not moved or mirrored to secondary storage. The primary + * {@code snapshot_data_store} row must remain so volume-snapshot DELETE uses + * {@code StorageSystemSnapshotStrategy} and the primary datastore driver.

+ */ + private boolean isManagedPrimaryLocationSnapshot(StoragePool storagePool, CreateSnapshotPayload payload) { + return storagePool != null && storagePool.isManaged() + && Snapshot.LocationType.PRIMARY.equals(payload.getLocationType()); + } + @Override public boolean isHypervisorKvmAndFileBasedStorage(VolumeInfo volumeInfo, StoragePool storagePool) { Set fileBasedStores = Set.of(Storage.StoragePoolType.SharedMountPoint, Storage.StoragePoolType.NetworkFilesystem, Storage.StoragePoolType.Filesystem); From 2724dbb2bd8709a0d92dfbafb78c6424c5ccc949 Mon Sep 17 00:00:00 2001 From: "Jain, Rajiv" Date: Sun, 28 Jun 2026 11:21:15 +0530 Subject: [PATCH 4/8] CSTACKEX-200: skip aggr space validations for existing volume operations --- .../storage/service/StorageStrategy.java | 84 ++++++++++++------- .../storage/utils/OntapStorageUtils.java | 11 ++- .../storage/service/StorageStrategyTest.java | 24 ++++++ 3 files changed, 89 insertions(+), 30 deletions(-) diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java index 35a58f646499..63e3deb03e99 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java @@ -97,10 +97,26 @@ public StorageStrategy(OntapStorage ontapStorage) { this.snapshotFeignClient = feignClientFactory.createClient(SnapshotFeignClient.class, baseURL); } - // Connect method to validate ONTAP cluster, credentials, protocol, and SVM + /** + * Validates ONTAP cluster reachability, credentials, SVM state, protocol, and aggregate capacity + * for new FlexVol creation (primary pool provisioning). + */ public boolean connect() { + return connect(true); + } + + /** + * Validates ONTAP cluster reachability and SVM/protocol settings. + * + *

Aggregate free-space checks apply only when {@code validateAggregatesForVolumeCreation} is + * {@code true} (pool provisioning). Snapshot, delete, revert, and grant/revoke paths must use + * {@code false} — they operate on an existing FlexVol and must not compare aggregate space to + * the full pool capacity stored in pool details.

+ */ + public boolean connect(boolean validateAggregatesForVolumeCreation) { logger.info("Attempting to connect to ONTAP cluster at " + storage.getStorageIP() + " and validate SVM " + - storage.getSvmName() + ", protocol " + storage.getProtocol()); + storage.getSvmName() + ", protocol " + storage.getProtocol() + + (validateAggregatesForVolumeCreation ? " (with aggregate validation)" : " (operations only)")); String authHeader = OntapStorageUtils.generateAuthHeader(storage.getUsername(), storage.getPassword()); String svmName = storage.getSvmName(); try { @@ -130,36 +146,16 @@ public boolean connect() { logger.error("ISCSI protocol is not enabled on SVM " + svmName); throw new CloudRuntimeException("ISCSI protocol is not enabled on SVM " + svmName); } - List aggrs = svm.getAggregates(); - if (aggrs == null || aggrs.isEmpty()) { - logger.error("No aggregates are assigned to SVM " + svmName); - throw new CloudRuntimeException("No aggregates are assigned to SVM " + svmName); - } - for (Aggregate aggr : aggrs) { - logger.debug("Found aggregate: " + aggr.getName() + " with UUID: " + aggr.getUuid()); - Aggregate aggrResp = aggregateFeignClient.getAggregateByUUID(authHeader, aggr.getUuid()); - if (aggrResp == null) { - logger.warn("Aggregate details response is null for aggregate " + aggr.getName() + ". Skipping."); - break; - } - if (!Objects.equals(aggrResp.getState(), Aggregate.StateEnum.ONLINE)) { - logger.warn("Aggregate " + aggr.getName() + " is not in online state. Skipping this aggregate."); - continue; - } else if (aggrResp.getSpace() == null || aggrResp.getAvailableBlockStorageSpace() == null || - aggrResp.getAvailableBlockStorageSpace() <= storage.getSize().doubleValue()) { - logger.warn("Aggregate " + aggr.getName() + " does not have sufficient available space. Skipping this aggregate."); - continue; - } - logger.info("Selected aggregate: " + aggr.getName() + " for volume operations."); - this.aggregates = List.of(aggr); - break; - } - if (this.aggregates == null || this.aggregates.isEmpty()) { - logger.error("No suitable aggregates found on SVM " + svmName + " for volume creation."); - throw new CloudRuntimeException("No suitable aggregates found on SVM " + svmName + " for volume creation."); + + if (validateAggregatesForVolumeCreation) { + validateAndSelectAggregatesForVolumeCreation(authHeader, svmName, svm.getAggregates()); + } else { + logger.debug("Skipping aggregate capacity validation — not required for existing-volume operations"); } logger.info("Successfully connected to ONTAP cluster and validated ONTAP details provided"); + } catch (CloudRuntimeException e) { + throw e; } catch (Exception e) { logger.error("Failed to connect to ONTAP cluster: " + e.getMessage(), e); throw new CloudRuntimeException("Failed to connect to ONTAP cluster: " + e.getMessage(), e); @@ -167,6 +163,36 @@ public boolean connect() { return true; } + private void validateAndSelectAggregatesForVolumeCreation(String authHeader, String svmName, List aggrs) { + if (aggrs == null || aggrs.isEmpty()) { + logger.error("No aggregates are assigned to SVM " + svmName); + throw new CloudRuntimeException("No aggregates are assigned to SVM " + svmName); + } + for (Aggregate aggr : aggrs) { + logger.debug("Found aggregate: " + aggr.getName() + " with UUID: " + aggr.getUuid()); + Aggregate aggrResp = aggregateFeignClient.getAggregateByUUID(authHeader, aggr.getUuid()); + if (aggrResp == null) { + logger.warn("Aggregate details response is null for aggregate " + aggr.getName() + ". Skipping."); + break; + } + if (!Objects.equals(aggrResp.getState(), Aggregate.StateEnum.ONLINE)) { + logger.warn("Aggregate " + aggr.getName() + " is not in online state. Skipping this aggregate."); + continue; + } else if (aggrResp.getSpace() == null || aggrResp.getAvailableBlockStorageSpace() == null || + aggrResp.getAvailableBlockStorageSpace() <= storage.getSize().doubleValue()) { + logger.warn("Aggregate " + aggr.getName() + " does not have sufficient available space. Skipping this aggregate."); + continue; + } + logger.info("Selected aggregate: " + aggr.getName() + " for volume operations."); + this.aggregates = List.of(aggr); + break; + } + if (this.aggregates == null || this.aggregates.isEmpty()) { + logger.error("No suitable aggregates found on SVM " + svmName + " for volume creation."); + throw new CloudRuntimeException("No suitable aggregates found on SVM " + svmName + " for volume creation."); + } + } + // Common methods like create/delete etc., should be here /** diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java index b05100a4886c..066c282e7b82 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java @@ -119,7 +119,16 @@ public static String getOSTypeFromHypervisor(String hypervisorType) { } } + /** + * Returns a connected {@link StorageStrategy} for operations on an existing pool (snapshots, + * delete, revert, grant/revoke). Does not require aggregate free space for the full pool size. + */ public static StorageStrategy getStrategyByStoragePoolDetails(Map details) { + return getStrategyByStoragePoolDetails(details, false); + } + + public static StorageStrategy getStrategyByStoragePoolDetails(Map details, + boolean validateAggregatesForVolumeCreation) { if (details == null || details.isEmpty()) { logger.error("getStrategyByStoragePoolDetails: Storage pool details are null or empty"); throw new CloudRuntimeException("Storage pool details are null or empty"); @@ -129,7 +138,7 @@ public static StorageStrategy getStrategyByStoragePoolDetails(Map svmResponse = new OntapResponse<>(); + svmResponse.setRecords(List.of(svm)); + + when(svmFeignClient.getSvmResponse(anyMap(), anyString())).thenReturn(svmResponse); + + boolean result = storageStrategy.connect(false); + + assertTrue(result); + verify(aggregateFeignClient, never()).getAggregateByUUID(anyString(), anyString()); + } + @Test public void testConnect_svmNotFound() { // Setup From 4d1377cb5826f4ff327ada2baa4d775f1f243ff5 Mon Sep 17 00:00:00 2001 From: "Jain, Rajiv" Date: Sun, 28 Jun 2026 12:05:32 +0530 Subject: [PATCH 5/8] CSTACKEX-200: CG create must have SVM name and respective validations --- .../vmsnapshot/OntapVMSnapshotStrategy.java | 42 ++++++++++++++++--- .../OntapVMSnapshotStrategyTest.java | 38 +++++++++++++++++ 2 files changed, 75 insertions(+), 5 deletions(-) diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategy.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategy.java index 8bbea431b726..013adc4e774a 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategy.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategy.java @@ -708,7 +708,8 @@ void createVmSnapshotsViaTemporaryCg(VMSnapshot vmSnapshot, UserVm userVm, try { logger.info("takeVMSnapshot: [CG:Create] Creating temporary consistency group [{}] for VM [{}] over {} FlexVol(s)", tempCgName, userVm.getInstanceName(), flexVolGroups.size()); - tempCgUuid = createTemporaryConsistencyGroup(snapshotClient, storageStrategy, authHeader, tempCgName, flexVolGroups.keySet()); + tempCgUuid = createTemporaryConsistencyGroup(snapshotClient, storageStrategy, authHeader, tempCgName, + resolveConsistencyGroupSvmName(flexVolGroups), flexVolGroups.keySet()); logger.info("takeVMSnapshot: [CG:Start] Starting phase-1 snapshot [{}] for temporary consistency group [{}]", snapshotNameBase, tempCgUuid); @@ -793,11 +794,36 @@ String buildTemporaryConsistencyGroupName(VMSnapshot vmSnapshot) { "cg" + vmSnapshot.getId()); } + /** + * ONTAP consistency groups are scoped to a single SVM. All participating FlexVols must belong to it. + */ + String resolveConsistencyGroupSvmName(Map flexVolGroups) { + String svmName = null; + for (FlexVolGroupInfo group : flexVolGroups.values()) { + String candidate = group.poolDetails.get(OntapStorageConstants.SVM_NAME); + if (candidate == null || candidate.trim().isEmpty()) { + throw new CloudRuntimeException("SVM name not found in pool details for pool [" + group.poolId + "]"); + } + if (svmName == null) { + svmName = candidate; + } else if (!svmName.equals(candidate)) { + throw new CloudRuntimeException("ONTAP consistency groups require all VM volumes on the same SVM; found [" + + svmName + "] and [" + candidate + "]"); + } + } + return svmName; + } + /** * Creates a temporary consistency group for the involved FlexVol UUIDs and returns its UUID. */ String createTemporaryConsistencyGroup(SnapshotFeignClient client, StorageStrategy storageStrategy, - String authHeader, String cgName, Set flexVolUuids) { + String authHeader, String cgName, String svmName, + Set flexVolUuids) { + if (svmName == null || svmName.trim().isEmpty()) { + throw new CloudRuntimeException("SVM name is required to create ONTAP consistency group [" + cgName + "]"); + } + List> volumeRefs = new ArrayList<>(); for (String flexVolUuid : flexVolUuids) { Map volumeRef = new LinkedHashMap<>(); @@ -805,14 +831,18 @@ String createTemporaryConsistencyGroup(SnapshotFeignClient client, StorageStrate volumeRefs.add(volumeRef); } + Map svmRef = new LinkedHashMap<>(); + svmRef.put("name", svmName); + Map payload = new LinkedHashMap<>(); payload.put("name", cgName); + payload.put("svm", svmRef); payload.put("volumes", volumeRefs); JobResponse response = client.createConsistencyGroup(authHeader, payload); storageStrategy.pollJobIfPresent(response, "create temporary consistency group " + cgName); - String cgUuid = resolveConsistencyGroupUuidByName(client, authHeader, cgName); + String cgUuid = resolveConsistencyGroupUuidByName(client, authHeader, cgName, svmName); if (cgUuid == null || cgUuid.isEmpty()) { throw new CloudRuntimeException("Unable to resolve temporary consistency group UUID for [" + cgName + "]"); } @@ -852,11 +882,13 @@ void deleteTemporaryConsistencyGroup(SnapshotFeignClient client, StorageStrategy } /** - * Resolves consistency group UUID by name. + * Resolves consistency group UUID by name within the given SVM. */ - String resolveConsistencyGroupUuidByName(SnapshotFeignClient client, String authHeader, String cgName) { + String resolveConsistencyGroupUuidByName(SnapshotFeignClient client, String authHeader, + String cgName, String svmName) { Map query = new HashMap<>(); query.put("name", cgName); + query.put("svm.name", svmName); query.put("fields", "uuid,name"); OntapResponse> response = client.getConsistencyGroups(authHeader, query); if (response == null || response.getRecords() == null || response.getRecords().isEmpty()) { diff --git a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategyTest.java b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategyTest.java index 9d7989d65e7a..c9dd210cdb39 100644 --- a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategyTest.java +++ b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategyTest.java @@ -551,6 +551,44 @@ void testGroupVolumesByFlexVol_VolumeNotFound_ThrowsException() { () -> strategy.groupVolumesByFlexVol(Collections.singletonList(volumeTO1))); } + @Test + void testCreateTemporaryConsistencyGroup_includesSvmName() { + SnapshotFeignClient client = mock(SnapshotFeignClient.class); + StorageStrategy storageStrategy = mock(StorageStrategy.class); + when(client.createConsistencyGroup(any(), any())).thenReturn(createJobResponse("job-cg-create")); + OntapResponse> cgResponse = new OntapResponse<>(); + Map cgRecord = new HashMap<>(); + cgRecord.put("uuid", "cg-uuid-1"); + cgResponse.setRecords(Collections.singletonList(cgRecord)); + when(client.getConsistencyGroups(any(), any())).thenReturn(cgResponse); + + String cgUuid = strategy.createTemporaryConsistencyGroup(client, storageStrategy, "auth", + "cg-name", "vs0", java.util.Set.of("flexvol-uuid-1", "flexvol-uuid-2")); + + assertEquals("cg-uuid-1", cgUuid); + org.mockito.ArgumentCaptor> payloadCaptor = org.mockito.ArgumentCaptor.forClass(Map.class); + verify(client).createConsistencyGroup(eq("auth"), payloadCaptor.capture()); + Map payload = payloadCaptor.getValue(); + assertEquals("cg-name", payload.get("name")); + @SuppressWarnings("unchecked") + Map svm = (Map) payload.get("svm"); + assertEquals("vs0", svm.get("name")); + } + + @Test + void testResolveConsistencyGroupSvmName_rejectsDifferentSvms() { + Map groups = new HashMap<>(); + Map poolDetails1 = new HashMap<>(); + poolDetails1.put(OntapStorageConstants.SVM_NAME, "vs0"); + groups.put("flexvol-uuid-1", new OntapVMSnapshotStrategy.FlexVolGroupInfo(poolDetails1, POOL_ID_1)); + + Map poolDetails2 = new HashMap<>(); + poolDetails2.put(OntapStorageConstants.SVM_NAME, "vs1"); + groups.put("flexvol-uuid-2", new OntapVMSnapshotStrategy.FlexVolGroupInfo(poolDetails2, POOL_ID_2)); + + assertThrows(CloudRuntimeException.class, () -> strategy.resolveConsistencyGroupSvmName(groups)); + } + // ══════════════════════════════════════════════════════════════════════════ // Tests: FlexVolSnapshotDetail parse/toString // ══════════════════════════════════════════════════════════════════════════ From f72d5cf1fddbc9e3bcd191c5c3a9570591f5debc Mon Sep 17 00:00:00 2001 From: "Jain, Rajiv" Date: Sun, 28 Jun 2026 12:51:34 +0530 Subject: [PATCH 6/8] CSTACKEX-200: add provisioning options --- .../cloudstack/storage/utils/OntapStorageConstants.java | 2 ++ .../storage/vmsnapshot/OntapVMSnapshotStrategy.java | 4 ++++ .../storage/vmsnapshot/OntapVMSnapshotStrategyTest.java | 6 ++++++ 3 files changed, 12 insertions(+) diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageConstants.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageConstants.java index bbd81f6ccc59..fa12f3750759 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageConstants.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageConstants.java @@ -106,6 +106,8 @@ public class OntapStorageConstants { public static final String FILE_PATH = "file_path"; public static final int MAX_SNAPSHOT_NAME_LENGTH = 64; public static final String ONTAP_TEMP_CG_PREFIX = "cs-temp-cg-"; + /** ONTAP CG API: action required when referencing existing FlexVols in a consistency group. */ + public static final String CG_VOLUME_PROVISIONING_ACTION_ADD = "add"; public static final int ONTAP_CG_JOB_MAX_RETRIES = 60; public static final int ONTAP_CG_JOB_POLL_INTERVAL_MS = 2000; public static final int ONTAP_SFSR_JOB_MAX_RETRIES = 60; diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategy.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategy.java index 013adc4e774a..60f47be17bff 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategy.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategy.java @@ -826,8 +826,12 @@ String createTemporaryConsistencyGroup(SnapshotFeignClient client, StorageStrate List> volumeRefs = new ArrayList<>(); for (String flexVolUuid : flexVolUuids) { + Map provisioningOptions = new LinkedHashMap<>(); + provisioningOptions.put("action", OntapStorageConstants.CG_VOLUME_PROVISIONING_ACTION_ADD); + Map volumeRef = new LinkedHashMap<>(); volumeRef.put("uuid", flexVolUuid); + volumeRef.put("provisioning_options", provisioningOptions); volumeRefs.add(volumeRef); } diff --git a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategyTest.java b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategyTest.java index c9dd210cdb39..9b689e20b936 100644 --- a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategyTest.java +++ b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategyTest.java @@ -573,6 +573,12 @@ void testCreateTemporaryConsistencyGroup_includesSvmName() { @SuppressWarnings("unchecked") Map svm = (Map) payload.get("svm"); assertEquals("vs0", svm.get("name")); + @SuppressWarnings("unchecked") + List> volumes = (List>) payload.get("volumes"); + assertEquals(2, volumes.size()); + @SuppressWarnings("unchecked") + Map provisioningOptions = (Map) volumes.get(0).get("provisioning_options"); + assertEquals(OntapStorageConstants.CG_VOLUME_PROVISIONING_ACTION_ADD, provisioningOptions.get("action")); } @Test From 5d9639e2f6933c2ad3bec6655a382ef7f640f891 Mon Sep 17 00:00:00 2001 From: "Jain, Rajiv" Date: Sun, 28 Jun 2026 15:02:04 +0530 Subject: [PATCH 7/8] CSTACKEX-200 race conditions needs to be handled while taking snapshot workflows --- .../OntapPrimaryDatastoreLifecycle.java | 3 + .../storage/service/StorageStrategy.java | 44 +++- .../storage/utils/OntapStorageConstants.java | 3 + .../storage/utils/OntapStorageUtils.java | 29 +++ .../vmsnapshot/OntapVMSnapshotStrategy.java | 221 ++++++++++++++---- .../OntapVMSnapshotStrategyTest.java | 63 ++++- 6 files changed, 313 insertions(+), 50 deletions(-) diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java index d7b89de25461..0bfccede6529 100755 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java @@ -142,6 +142,9 @@ public DataStore initialize(Map dsInfos) { } logger.info("Using Data LIF for storage access: " + dataLIF); details.put(OntapStorageConstants.DATA_LIF, dataLIF); + if (storageStrategy.getResolvedSvmUuid() != null && !storageStrategy.getResolvedSvmUuid().isEmpty()) { + details.put(OntapStorageConstants.SVM_UUID, storageStrategy.getResolvedSvmUuid()); + } logger.info("Creating ONTAP volume '" + storagePoolName + "' with size: " + capacityBytes + " bytes (" + (capacityBytes / (1024 * 1024 * 1024)) + " GB)"); try { diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java index 63e3deb03e99..bdbfec874c2a 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java @@ -78,6 +78,7 @@ public abstract class StorageStrategy { * Presents aggregate object for the unified storage, not eligible for disaggregated */ private List aggregates; + private String resolvedSvmUuid; private static final Logger logger = LogManager.getLogger(StorageStrategy.class); @@ -142,10 +143,11 @@ public boolean connect(boolean validateAggregatesForVolumeCreation) { if (Objects.equals(storage.getProtocol(), ProtocolType.NFS3) && !svm.getNfsEnabled()) { logger.error("NFS protocol is not enabled on SVM " + svmName); throw new CloudRuntimeException("NFS protocol is not enabled on SVM " + svmName); - } else if (Objects.equals(storage.getProtocol(), ProtocolType.ISCSI) && !svm.getIscsiEnabled()) { + } else if (Objects.equals(storage.getProtocol(), ProtocolType.ISCSI) && !svm.getIscsiEnabled()) { logger.error("ISCSI protocol is not enabled on SVM " + svmName); throw new CloudRuntimeException("ISCSI protocol is not enabled on SVM " + svmName); } + this.resolvedSvmUuid = svm.getUuid(); if (validateAggregatesForVolumeCreation) { validateAndSelectAggregatesForVolumeCreation(authHeader, svmName, svm.getAggregates()); @@ -163,6 +165,13 @@ public boolean connect(boolean validateAggregatesForVolumeCreation) { return true; } + /** + * ONTAP SVM UUID resolved during the last successful {@link #connect(boolean)} call. + */ + public String getResolvedSvmUuid() { + return resolvedSvmUuid; + } + private void validateAndSelectAggregatesForVolumeCreation(String authHeader, String svmName, List aggrs) { if (aggrs == null || aggrs.isEmpty()) { logger.error("No aggregates are assigned to SVM " + svmName); @@ -670,7 +679,13 @@ public String getAuthHeader() { * @return true if the job completed successfully */ public Boolean jobPollForSuccess(String jobUUID, int maxRetries, int sleepTimeInMilliSecs) { - //Create URI for GET Job API + return jobPollUntilSuccess(jobUUID, maxRetries, sleepTimeInMilliSecs) != null; + } + + /** + * Polls an ONTAP async job until it succeeds and returns the completed job record. + */ + public Job jobPollUntilSuccess(String jobUUID, int maxRetries, int sleepTimeInMilliSecs) { int jobRetryCount = 0; Job jobResp = null; try { @@ -696,14 +711,15 @@ public Boolean jobPollForSuccess(String jobUUID, int maxRetries, int sleepTimeIn Thread.sleep(sleepTimeInMilliSecs); } if (jobResp == null || !jobResp.getState().equals(OntapStorageConstants.JOB_SUCCESS)) { - return false; + return null; } + return jobResp; } catch (FeignException.FeignClientException e) { throw new CloudRuntimeException("Failed to fetch job status: " + e.getMessage()); } catch (InterruptedException e) { - throw new RuntimeException(e); + Thread.currentThread().interrupt(); + throw new CloudRuntimeException("Interrupted while polling ONTAP job " + jobUUID, e); } - return true; } /** @@ -737,6 +753,24 @@ public void pollJobIfPresent(JobResponse response, String operationName, } } + /** + * Polls an ONTAP async job when present and returns the completed job (for extracting created resource UUIDs). + */ + public Job pollJobIfPresentAndGetCompletedJob(JobResponse response, String operationName) { + return pollJobIfPresentAndGetCompletedJob(response, operationName, + OntapStorageConstants.ONTAP_CG_JOB_MAX_RETRIES, + OntapStorageConstants.ONTAP_CG_JOB_POLL_INTERVAL_MS); + } + + public Job pollJobIfPresentAndGetCompletedJob(JobResponse response, String operationName, + int maxRetries, int pollIntervalMs) { + if (response == null || response.getJob() == null || response.getJob().getUuid() == null) { + logger.debug("pollJobIfPresentAndGetCompletedJob: No async job for operation [{}]", operationName); + return null; + } + return jobPollUntilSuccess(response.getJob().getUuid(), maxRetries, pollIntervalMs); + } + /** * Completes CLI-based SFSR ({@code restore-file}) orchestration: poll job when returned, * otherwise accept synchronous success. diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageConstants.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageConstants.java index fa12f3750759..92697518d208 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageConstants.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageConstants.java @@ -36,6 +36,7 @@ public class OntapStorageConstants { public static final String SIZE = "size"; public static final String PROTOCOL = "protocol"; public static final String SVM_NAME = "svmName"; + public static final String SVM_UUID = "svmUUID"; public static final String USERNAME = "username"; public static final String PASSWORD = "password"; public static final String DATA_LIF = "dataLIF"; @@ -110,6 +111,8 @@ public class OntapStorageConstants { public static final String CG_VOLUME_PROVISIONING_ACTION_ADD = "add"; public static final int ONTAP_CG_JOB_MAX_RETRIES = 60; public static final int ONTAP_CG_JOB_POLL_INTERVAL_MS = 2000; + public static final int ONTAP_CG_SNAPSHOT_RESOLVE_MAX_RETRIES = 30; + public static final int ONTAP_CG_SNAPSHOT_RESOLVE_POLL_INTERVAL_MS = 1000; public static final int ONTAP_SFSR_JOB_MAX_RETRIES = 60; public static final int ONTAP_SFSR_JOB_POLL_INTERVAL_MS = 2000; public static final int ONTAP_SNAPSHOT_DELETE_JOB_MAX_RETRIES = 30; diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java index 066c282e7b82..790a325a5b51 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java @@ -205,4 +205,33 @@ public static String buildOntapSnapshotName(String cloudStackSnapshotName, Strin return normalizedBase + suffix; } + /** + * Extracts a resource UUID from an ONTAP job description path. + * + *

Example: {@code POST /api/application/consistency-groups/{cg}/snapshots/{uuid}} + * with {@code pathSegment} {@code "/snapshots/"} returns the snapshot UUID.

+ */ + public static String extractUuidFromOntapJobDescription(String description, String pathSegment) { + if (description == null || pathSegment == null || pathSegment.isEmpty()) { + return null; + } + int idx = description.indexOf(pathSegment); + if (idx < 0) { + return null; + } + String remainder = description.substring(idx + pathSegment.length()).trim(); + if (remainder.isEmpty()) { + return null; + } + int queryIdx = remainder.indexOf('?'); + if (queryIdx >= 0) { + remainder = remainder.substring(0, queryIdx); + } + int slashIdx = remainder.indexOf('/'); + if (slashIdx >= 0) { + remainder = remainder.substring(0, slashIdx); + } + return remainder.isEmpty() ? null : remainder; + } + } diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategy.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategy.java index 60f47be17bff..2f1fa7f883d1 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategy.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategy.java @@ -36,6 +36,7 @@ import org.apache.cloudstack.storage.datastore.db.StoragePoolVO; import org.apache.cloudstack.storage.feign.client.SnapshotFeignClient; import org.apache.cloudstack.storage.feign.model.FlexVolSnapshot; +import org.apache.cloudstack.storage.feign.model.Job; import org.apache.cloudstack.storage.feign.model.response.JobResponse; import org.apache.cloudstack.storage.feign.model.response.OntapResponse; import org.apache.cloudstack.storage.service.StorageStrategy; @@ -709,13 +710,12 @@ void createVmSnapshotsViaTemporaryCg(VMSnapshot vmSnapshot, UserVm userVm, logger.info("takeVMSnapshot: [CG:Create] Creating temporary consistency group [{}] for VM [{}] over {} FlexVol(s)", tempCgName, userVm.getInstanceName(), flexVolGroups.size()); tempCgUuid = createTemporaryConsistencyGroup(snapshotClient, storageStrategy, authHeader, tempCgName, - resolveConsistencyGroupSvmName(flexVolGroups), flexVolGroups.keySet()); + resolveConsistencyGroupScope(flexVolGroups), flexVolGroups.keySet()); logger.info("takeVMSnapshot: [CG:Start] Starting phase-1 snapshot [{}] for temporary consistency group [{}]", snapshotNameBase, tempCgUuid); - startConsistencyGroupSnapshot(snapshotClient, storageStrategy, authHeader, tempCgUuid, snapshotNameBase); - - cgSnapshotUuid = resolveConsistencyGroupSnapshotUuid(snapshotClient, authHeader, tempCgUuid, snapshotNameBase); + cgSnapshotUuid = resolveStartedConsistencyGroupSnapshotUuid(snapshotClient, storageStrategy, + authHeader, tempCgUuid, snapshotNameBase); logger.info("takeVMSnapshot: [CG:Commit] Committing phase-2 snapshot [{}] (uuid={}) for temporary consistency group [{}]", snapshotNameBase, cgSnapshotUuid, tempCgUuid); @@ -795,33 +795,50 @@ String buildTemporaryConsistencyGroupName(VMSnapshot vmSnapshot) { } /** - * ONTAP consistency groups are scoped to a single SVM. All participating FlexVols must belong to it. + * Validates and returns the ONTAP scope for a temporary consistency group. + * + *

CG membership requires all FlexVols on the same ONTAP management endpoint and SVM. + * SVM name alone is not sufficient — different clusters may reuse names such as {@code vs0}. + * Identity uses {@code storageIP} plus {@code svmUUID} when persisted, otherwise + * {@code storageIP} plus {@code svmName} for legacy pools.

*/ - String resolveConsistencyGroupSvmName(Map flexVolGroups) { - String svmName = null; + ConsistencyGroupScope resolveConsistencyGroupScope(Map flexVolGroups) { + ConsistencyGroupScope scope = null; for (FlexVolGroupInfo group : flexVolGroups.values()) { - String candidate = group.poolDetails.get(OntapStorageConstants.SVM_NAME); - if (candidate == null || candidate.trim().isEmpty()) { - throw new CloudRuntimeException("SVM name not found in pool details for pool [" + group.poolId + "]"); - } - if (svmName == null) { - svmName = candidate; - } else if (!svmName.equals(candidate)) { - throw new CloudRuntimeException("ONTAP consistency groups require all VM volumes on the same SVM; found [" - + svmName + "] and [" + candidate + "]"); + ConsistencyGroupScope candidate = consistencyGroupScopeFromPoolDetails(group.poolDetails, group.poolId); + if (scope == null) { + scope = candidate; + } else if (!scope.matches(candidate)) { + throw new CloudRuntimeException("ONTAP consistency groups require all VM volumes on the same " + + "ONTAP cluster and SVM. Found [" + scope + "] and [" + candidate + "]"); } } - return svmName; + return scope; + } + + ConsistencyGroupScope consistencyGroupScopeFromPoolDetails(Map poolDetails, long poolId) { + String storageIp = poolDetails.get(OntapStorageConstants.STORAGE_IP); + if (storageIp == null || storageIp.trim().isEmpty()) { + throw new CloudRuntimeException("ONTAP storage management IP not found in pool details for pool [" + + poolId + "]"); + } + String svmName = poolDetails.get(OntapStorageConstants.SVM_NAME); + if (svmName == null || svmName.trim().isEmpty()) { + throw new CloudRuntimeException("SVM name not found in pool details for pool [" + poolId + "]"); + } + String svmUuid = poolDetails.get(OntapStorageConstants.SVM_UUID); + return new ConsistencyGroupScope(storageIp.trim(), svmName.trim(), + svmUuid != null && !svmUuid.trim().isEmpty() ? svmUuid.trim() : null); } /** * Creates a temporary consistency group for the involved FlexVol UUIDs and returns its UUID. */ String createTemporaryConsistencyGroup(SnapshotFeignClient client, StorageStrategy storageStrategy, - String authHeader, String cgName, String svmName, + String authHeader, String cgName, ConsistencyGroupScope cgScope, Set flexVolUuids) { - if (svmName == null || svmName.trim().isEmpty()) { - throw new CloudRuntimeException("SVM name is required to create ONTAP consistency group [" + cgName + "]"); + if (cgScope == null) { + throw new CloudRuntimeException("ONTAP consistency group scope is required to create CG [" + cgName + "]"); } List> volumeRefs = new ArrayList<>(); @@ -835,8 +852,7 @@ String createTemporaryConsistencyGroup(SnapshotFeignClient client, StorageStrate volumeRefs.add(volumeRef); } - Map svmRef = new LinkedHashMap<>(); - svmRef.put("name", svmName); + Map svmRef = cgScope.toOntapSvmReference(); Map payload = new LinkedHashMap<>(); payload.put("name", cgName); @@ -846,7 +862,7 @@ String createTemporaryConsistencyGroup(SnapshotFeignClient client, StorageStrate JobResponse response = client.createConsistencyGroup(authHeader, payload); storageStrategy.pollJobIfPresent(response, "create temporary consistency group " + cgName); - String cgUuid = resolveConsistencyGroupUuidByName(client, authHeader, cgName, svmName); + String cgUuid = resolveConsistencyGroupUuidByName(client, authHeader, cgName, cgScope); if (cgUuid == null || cgUuid.isEmpty()) { throw new CloudRuntimeException("Unable to resolve temporary consistency group UUID for [" + cgName + "]"); } @@ -854,15 +870,26 @@ String createTemporaryConsistencyGroup(SnapshotFeignClient client, StorageStrate } /** - * Starts phase-1 of the two-phase CG snapshot. + * Starts phase-1 of the two-phase CG snapshot and returns the CG snapshot UUID when ONTAP exposes it in the job record. */ - void startConsistencyGroupSnapshot(SnapshotFeignClient client, StorageStrategy storageStrategy, + String startConsistencyGroupSnapshot(SnapshotFeignClient client, StorageStrategy storageStrategy, String authHeader, String cgUuid, String snapshotName) { Map payload = new LinkedHashMap<>(); payload.put("name", snapshotName); payload.put("action", "start"); JobResponse response = client.createConsistencyGroupSnapshot(authHeader, cgUuid, payload); - storageStrategy.pollJobIfPresent(response, "start CG snapshot " + snapshotName + " for " + cgUuid); + Job completedJob = storageStrategy.pollJobIfPresentAndGetCompletedJob(response, + "start CG snapshot " + snapshotName + " for " + cgUuid); + if (completedJob == null) { + return null; + } + String snapshotUuid = OntapStorageUtils.extractUuidFromOntapJobDescription( + completedJob.getDescription(), "/snapshots/"); + if (snapshotUuid != null) { + logger.info("takeVMSnapshot: [CG:Start] Resolved CG snapshot UUID [{}] from ONTAP job for snapshot [{}]", + snapshotUuid, snapshotName); + } + return snapshotUuid; } /** @@ -886,13 +913,13 @@ void deleteTemporaryConsistencyGroup(SnapshotFeignClient client, StorageStrategy } /** - * Resolves consistency group UUID by name within the given SVM. + * Resolves consistency group UUID by name within the given ONTAP cluster/SVM scope. */ String resolveConsistencyGroupUuidByName(SnapshotFeignClient client, String authHeader, - String cgName, String svmName) { + String cgName, ConsistencyGroupScope cgScope) { Map query = new HashMap<>(); query.put("name", cgName); - query.put("svm.name", svmName); + cgScope.applySvmQueryFilter(query); query.put("fields", "uuid,name"); OntapResponse> response = client.getConsistencyGroups(authHeader, query); if (response == null || response.getRecords() == null || response.getRecords().isEmpty()) { @@ -902,24 +929,86 @@ String resolveConsistencyGroupUuidByName(SnapshotFeignClient client, String auth } /** - * Resolves consistency group snapshot UUID by name. + * Resolves the started CG snapshot UUID after phase-1, using the job record when available and polling GET otherwise. */ - String resolveConsistencyGroupSnapshotUuid(SnapshotFeignClient client, String authHeader, - String cgUuid, String snapshotName) { + String resolveStartedConsistencyGroupSnapshotUuid(SnapshotFeignClient client, StorageStrategy storageStrategy, + String authHeader, String cgUuid, String snapshotName) { + String snapshotUuidFromJob = startConsistencyGroupSnapshot(client, storageStrategy, authHeader, cgUuid, snapshotName); + if (snapshotUuidFromJob != null && !snapshotUuidFromJob.isEmpty()) { + return snapshotUuidFromJob; + } + return resolveConsistencyGroupSnapshotUuid(client, storageStrategy, authHeader, cgUuid, snapshotName); + } + + /** + * Resolves consistency group snapshot UUID by name with retries (ONTAP list can lag behind job success). + */ + String resolveConsistencyGroupSnapshotUuid(SnapshotFeignClient client, StorageStrategy storageStrategy, + String authHeader, String cgUuid, String snapshotName) { + int maxRetries = OntapStorageConstants.ONTAP_CG_SNAPSHOT_RESOLVE_MAX_RETRIES; + int pollIntervalMs = OntapStorageConstants.ONTAP_CG_SNAPSHOT_RESOLVE_POLL_INTERVAL_MS; + + for (int attempt = 1; attempt <= maxRetries; attempt++) { + String snapshotUuid = lookupConsistencyGroupSnapshotUuid(client, authHeader, cgUuid, snapshotName); + if (snapshotUuid != null) { + if (attempt > 1) { + logger.info("takeVMSnapshot: [CG:Resolve] CG snapshot [{}] resolved on attempt {}/{}", + snapshotName, attempt, maxRetries); + } + return snapshotUuid; + } + if (attempt < maxRetries) { + logger.debug("takeVMSnapshot: [CG:Resolve] CG snapshot [{}] not yet visible in CG [{}], retry {}/{}", + snapshotName, cgUuid, attempt, maxRetries); + try { + Thread.sleep(pollIntervalMs); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new CloudRuntimeException("Interrupted while resolving CG snapshot [" + snapshotName + "]", e); + } + } + } + + throw new CloudRuntimeException("Unable to resolve consistency group snapshot UUID for snapshot [" + + snapshotName + "] in CG [" + cgUuid + "] after " + maxRetries + " attempts"); + } + + /** + * Single GET attempt: match by name, then fall back to listing all CG snapshots in this group (And it would one + * always since workflow is keep deleting the CG). + */ + String lookupConsistencyGroupSnapshotUuid(SnapshotFeignClient client, String authHeader, + String cgUuid, String snapshotName) { Map query = new HashMap<>(); query.put("name", snapshotName); - query.put("fields", "uuid,name"); + query.put("fields", "uuid,name,state"); OntapResponse> response = client.getConsistencyGroupSnapshots(authHeader, cgUuid, query); + String snapshotUuid = findConsistencyGroupSnapshotUuidInRecords(response, snapshotName); + if (snapshotUuid != null) { + return snapshotUuid; + } + + Map listAllQuery = new HashMap<>(); + listAllQuery.put("fields", "uuid,name,state"); + OntapResponse> allSnapshots = client.getConsistencyGroupSnapshots(authHeader, cgUuid, listAllQuery); + return findConsistencyGroupSnapshotUuidInRecords(allSnapshots, snapshotName); + } + + private String findConsistencyGroupSnapshotUuidInRecords(OntapResponse> response, + String snapshotName) { if (response == null || response.getRecords() == null || response.getRecords().isEmpty()) { - throw new CloudRuntimeException("Unable to resolve consistency group snapshot UUID for snapshot [" + - snapshotName + "] in CG [" + cgUuid + "]"); + return null; } - String snapshotUuid = getStringField(response.getRecords().get(0), "uuid"); - if (snapshotUuid == null || snapshotUuid.isEmpty()) { - throw new CloudRuntimeException("Invalid consistency group snapshot UUID for snapshot [" + - snapshotName + "] in CG [" + cgUuid + "]"); + for (Map record : response.getRecords()) { + String name = getStringField(record, "name"); + if (snapshotName.equals(name)) { + String uuid = getStringField(record, "uuid"); + if (uuid != null && !uuid.isEmpty()) { + return uuid; + } + } } - return snapshotUuid; + return null; } private String getStringField(Map record, String key) { @@ -1092,6 +1181,58 @@ static class FlexVolGroupInfo { } } + /** + * Identifies the ONTAP cluster management endpoint and SVM for CG operations. + */ + static class ConsistencyGroupScope { + final String storageIp; + final String svmName; + final String svmUuid; + + ConsistencyGroupScope(String storageIp, String svmName, String svmUuid) { + this.storageIp = storageIp; + this.svmName = svmName; + this.svmUuid = svmUuid; + } + + boolean matches(ConsistencyGroupScope other) { + return other != null && identityKey().equals(other.identityKey()); + } + + String identityKey() { + if (svmUuid != null && !svmUuid.isEmpty()) { + return storageIp + "|" + svmUuid; + } + return storageIp + "|" + svmName; + } + + Map toOntapSvmReference() { + Map svmRef = new LinkedHashMap<>(); + if (svmUuid != null && !svmUuid.isEmpty()) { + svmRef.put("uuid", svmUuid); + } else { + svmRef.put("name", svmName); + } + return svmRef; + } + + void applySvmQueryFilter(Map query) { + if (svmUuid != null && !svmUuid.isEmpty()) { + query.put("svm.uuid", svmUuid); + } else { + query.put("svm.name", svmName); + } + } + + @Override + public String toString() { + if (svmUuid != null && !svmUuid.isEmpty()) { + return "storageIP=" + storageIp + ", svmUUID=" + svmUuid + ", svmName=" + svmName; + } + return "storageIP=" + storageIp + ", svmName=" + svmName; + } + } + /** * Holds the metadata for a single volume's FlexVolume snapshot entry (used during create and for * serialization/deserialization to/from vm_snapshot_details). diff --git a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategyTest.java b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategyTest.java index 9b689e20b936..8494099dfc10 100644 --- a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategyTest.java +++ b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategyTest.java @@ -563,7 +563,8 @@ void testCreateTemporaryConsistencyGroup_includesSvmName() { when(client.getConsistencyGroups(any(), any())).thenReturn(cgResponse); String cgUuid = strategy.createTemporaryConsistencyGroup(client, storageStrategy, "auth", - "cg-name", "vs0", java.util.Set.of("flexvol-uuid-1", "flexvol-uuid-2")); + "cg-name", new OntapVMSnapshotStrategy.ConsistencyGroupScope("10.0.0.1", "vs0", "svm-uuid-1"), + java.util.Set.of("flexvol-uuid-1", "flexvol-uuid-2")); assertEquals("cg-uuid-1", cgUuid); org.mockito.ArgumentCaptor> payloadCaptor = org.mockito.ArgumentCaptor.forClass(Map.class); @@ -572,7 +573,7 @@ void testCreateTemporaryConsistencyGroup_includesSvmName() { assertEquals("cg-name", payload.get("name")); @SuppressWarnings("unchecked") Map svm = (Map) payload.get("svm"); - assertEquals("vs0", svm.get("name")); + assertEquals("svm-uuid-1", svm.get("uuid")); @SuppressWarnings("unchecked") List> volumes = (List>) payload.get("volumes"); assertEquals(2, volumes.size()); @@ -582,17 +583,57 @@ void testCreateTemporaryConsistencyGroup_includesSvmName() { } @Test - void testResolveConsistencyGroupSvmName_rejectsDifferentSvms() { + void testResolveConsistencyGroupScope_rejectsDifferentStorageIpWithSameSvmName() { Map groups = new HashMap<>(); Map poolDetails1 = new HashMap<>(); + poolDetails1.put(OntapStorageConstants.STORAGE_IP, "10.1.1.1"); poolDetails1.put(OntapStorageConstants.SVM_NAME, "vs0"); groups.put("flexvol-uuid-1", new OntapVMSnapshotStrategy.FlexVolGroupInfo(poolDetails1, POOL_ID_1)); Map poolDetails2 = new HashMap<>(); - poolDetails2.put(OntapStorageConstants.SVM_NAME, "vs1"); + poolDetails2.put(OntapStorageConstants.STORAGE_IP, "10.2.2.2"); + poolDetails2.put(OntapStorageConstants.SVM_NAME, "vs0"); groups.put("flexvol-uuid-2", new OntapVMSnapshotStrategy.FlexVolGroupInfo(poolDetails2, POOL_ID_2)); - assertThrows(CloudRuntimeException.class, () -> strategy.resolveConsistencyGroupSvmName(groups)); + assertThrows(CloudRuntimeException.class, () -> strategy.resolveConsistencyGroupScope(groups)); + } + + @Test + void testResolveConsistencyGroupScope_acceptsSameClusterAndSvmUuid() { + Map groups = new HashMap<>(); + Map poolDetails1 = new HashMap<>(); + poolDetails1.put(OntapStorageConstants.STORAGE_IP, "10.1.1.1"); + poolDetails1.put(OntapStorageConstants.SVM_NAME, "vs0"); + poolDetails1.put(OntapStorageConstants.SVM_UUID, "svm-uuid-shared"); + groups.put("flexvol-uuid-1", new OntapVMSnapshotStrategy.FlexVolGroupInfo(poolDetails1, POOL_ID_1)); + + Map poolDetails2 = new HashMap<>(); + poolDetails2.put(OntapStorageConstants.STORAGE_IP, "10.1.1.1"); + poolDetails2.put(OntapStorageConstants.SVM_NAME, "vs0"); + poolDetails2.put(OntapStorageConstants.SVM_UUID, "svm-uuid-shared"); + groups.put("flexvol-uuid-2", new OntapVMSnapshotStrategy.FlexVolGroupInfo(poolDetails2, POOL_ID_2)); + + OntapVMSnapshotStrategy.ConsistencyGroupScope scope = strategy.resolveConsistencyGroupScope(groups); + assertEquals("svm-uuid-shared", scope.svmUuid); + assertEquals("10.1.1.1", scope.storageIp); + } + + @Test + void testResolveConsistencyGroupScope_rejectsDifferentSvmUuidOnSameCluster() { + Map groups = new HashMap<>(); + Map poolDetails1 = new HashMap<>(); + poolDetails1.put(OntapStorageConstants.STORAGE_IP, "10.1.1.1"); + poolDetails1.put(OntapStorageConstants.SVM_NAME, "vs0"); + poolDetails1.put(OntapStorageConstants.SVM_UUID, "svm-uuid-1"); + groups.put("flexvol-uuid-1", new OntapVMSnapshotStrategy.FlexVolGroupInfo(poolDetails1, POOL_ID_1)); + + Map poolDetails2 = new HashMap<>(); + poolDetails2.put(OntapStorageConstants.STORAGE_IP, "10.1.1.1"); + poolDetails2.put(OntapStorageConstants.SVM_NAME, "vs0"); + poolDetails2.put(OntapStorageConstants.SVM_UUID, "svm-uuid-2"); + groups.put("flexvol-uuid-2", new OntapVMSnapshotStrategy.FlexVolGroupInfo(poolDetails2, POOL_ID_2)); + + assertThrows(CloudRuntimeException.class, () -> strategy.resolveConsistencyGroupScope(groups)); } // ══════════════════════════════════════════════════════════════════════════ @@ -1054,6 +1095,7 @@ private void setupMultiFlexVolForTakeSnapshot() { poolDetails1.put(OntapStorageConstants.PASSWORD, "pass"); poolDetails1.put(OntapStorageConstants.STORAGE_IP, "10.0.0.1"); poolDetails1.put(OntapStorageConstants.SVM_NAME, "svm1"); + poolDetails1.put(OntapStorageConstants.SVM_UUID, "svm-uuid-shared"); poolDetails1.put(OntapStorageConstants.SIZE, "107374182400"); poolDetails1.put(OntapStorageConstants.PROTOCOL, "NFS3"); when(storagePoolDetailsDao.listDetailsKeyPairs(POOL_ID_1)).thenReturn(poolDetails1); @@ -1064,6 +1106,7 @@ private void setupMultiFlexVolForTakeSnapshot() { poolDetails2.put(OntapStorageConstants.PASSWORD, "pass"); poolDetails2.put(OntapStorageConstants.STORAGE_IP, "10.0.0.1"); poolDetails2.put(OntapStorageConstants.SVM_NAME, "svm1"); + poolDetails2.put(OntapStorageConstants.SVM_UUID, "svm-uuid-shared"); poolDetails2.put(OntapStorageConstants.SIZE, "107374182400"); poolDetails2.put(OntapStorageConstants.PROTOCOL, "NFS3"); when(storagePoolDetailsDao.listDetailsKeyPairs(POOL_ID_2)).thenReturn(poolDetails2); @@ -1107,6 +1150,16 @@ private void setupTemporaryCgFlowMocks(String snapshotName) { when(storageStrategy.getSnapshotFeignClient()).thenReturn(snapshotFeignClient); when(storageStrategy.getAuthHeader()).thenReturn("Basic dGVzdDp0ZXN0"); when(storageStrategy.jobPollForSuccess(any(), anyInt(), anyInt())).thenReturn(true); + when(storageStrategy.pollJobIfPresentAndGetCompletedJob(any(), any())).thenAnswer(invocation -> { + Job completedJob = new Job(); + completedJob.setState(OntapStorageConstants.JOB_SUCCESS); + String operationName = invocation.getArgument(1); + if (operationName != null && operationName.startsWith("start CG snapshot")) { + completedJob.setDescription( + "POST /api/application/consistency-groups/cg-uuid-1/snapshots/cg-snap-uuid-1"); + } + return completedJob; + }); when(snapshotFeignClient.createConsistencyGroup(any(), any())).thenReturn(createJobResponse("job-cg-create")); OntapResponse> cgResponse = new OntapResponse<>(); From 26f20d7c9198e11943a0be1246ce32ffaa4986a6 Mon Sep 17 00:00:00 2001 From: "Jain, Rajiv" Date: Sun, 28 Jun 2026 20:34:31 +0530 Subject: [PATCH 8/8] CSTACKEX-200: state is not a valid field for CG --- .../storage/vmsnapshot/OntapVMSnapshotStrategy.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategy.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategy.java index 2f1fa7f883d1..4a4cea2db710 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategy.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategy.java @@ -981,7 +981,7 @@ String lookupConsistencyGroupSnapshotUuid(SnapshotFeignClient client, String aut String cgUuid, String snapshotName) { Map query = new HashMap<>(); query.put("name", snapshotName); - query.put("fields", "uuid,name,state"); + query.put("fields", "uuid,name"); OntapResponse> response = client.getConsistencyGroupSnapshots(authHeader, cgUuid, query); String snapshotUuid = findConsistencyGroupSnapshotUuidInRecords(response, snapshotName); if (snapshotUuid != null) { @@ -989,7 +989,7 @@ String lookupConsistencyGroupSnapshotUuid(SnapshotFeignClient client, String aut } Map listAllQuery = new HashMap<>(); - listAllQuery.put("fields", "uuid,name,state"); + listAllQuery.put("fields", "uuid,name"); OntapResponse> allSnapshots = client.getConsistencyGroupSnapshots(authHeader, cgUuid, listAllQuery); return findConsistencyGroupSnapshotUuidInRecords(allSnapshots, snapshotName); }