CSTACKEX-158: if ontap snapshot are already delete from ontap side, d… by rajiv-jain-netapp · Pull Request #82 · NetApp/cloudstack

rajiv-jain-netapp · 2026-07-20T09:07:15Z

…Deletion of CS side of snapshot should not fail on not finding ontap snapshot.

Description

This PR...

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)
Build/CI
Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Test -1: Ran VM snapshot delete operation when the respective snapshot is not available at ONTAP, it passed.
Test -2: Ran VM snapshot delete operation when the respective snapshot is available at ONTAP; it passed
Test -3: Ran cloudstack volume snapshot delete workflow when the respective snapshot is not available at ONTAP, it passed.
Test -4: Ran cloudstack volume snapshot delete workflow when the respective snapshot is available at ONTAP, it passed.

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

…eletion of CS side of snapshot should not fail on not finding ontap snapshot.

Copilot

Pull request overview

This PR makes ONTAP snapshot deletion idempotent from CloudStack’s perspective: if the backend ONTAP snapshot is already gone, CloudStack-side deletion should not fail.

Changes:

Add a shared helper (OntapStorageUtils.isOntapSnapshotNotFoundError) to recognize “snapshot already missing” errors.
Update ONTAP snapshot delete workflows to treat those errors as success (both in StorageStrategy and OntapPrimaryDatastoreDriver).
Add/extend unit tests to cover the new behavior.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java	Introduces shared “snapshot not found” detection helper used by delete paths.
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java	Wraps FlexVol snapshot deletion with “already absent” handling.
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java	Replaces local matcher with shared helper for delete idempotency.
plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/utils/OntapStorageUtilsTest.java	Adds tests for the new helper (with recommended regression coverage).
plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java	Adds a test ensuring delete succeeds when ONTAP reports the snapshot is missing.

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

sandeeplocharla · 2026-07-22T05:39:09Z

+     * Returns true when the exception indicates the ONTAP snapshot was already removed.
+     * Delete workflows treat a missing backend snapshot as idempotent success.
+     */
+    public static boolean isOntapSnapshotNotFoundError(Throwable error) {


If the idea is to throw a polished message back to the user, wouldn't it be better to have a generic ONTAP Exception wrapper class and have similar implementations for all possible ONTAP error codes?

This was added purely to improve readability. You can treat it as a helper method rather than an exception class specific to ONTAP.

This could be a generic method not specific to snapshot, like instead of isOntapSnapshotNotFoundError(Throwable error) could be isNotFoundError(Throwable error). And, maybe this could be in a separate class and similar methods could be written for 401/403/500...?

suryag1201 · 2026-07-22T05:49:22Z

+        }
+        String message = error.getMessage();
+        if (message != null) {
+            String lower = message.toLowerCase();


it is better to check the status code as 404

To keep the implementation generic, I chose to rely on the exception message rather than a specific error code. We could certainly check for error codes as well, but that would make the logic more implementation-specific and reduce its reusability across different scenarios.

but for any entity which is not found, we will receive 404 error code,

All these checks are placed at exception handling level. Error code related checks can be handled at feign response level.

The snapshot delete operation is asynchronous and initially returns a job response. As a result, any "snapshot not found" condition would typically be surfaced during job polling rather than as an immediate response to the DELETE request. Therefore, we should not expect a direct 404 from the initial delete call. ONTAP uses its own error codes for these scenarios, which are reported through the asynchronous job status and results.

piyush5netapp · 2026-07-22T06:11:53Z

+            return false;
+        }
+        String message = error.getMessage();
+        if (message != null) {


if message is null somehow, I think func run infinitely ?

It shouldn't recurse indefinitely, as we already have a null check at the beginning of the method. My intent here is to ensure that a 404 (Not Found) condition is not missed simply because the exception is wrapped or thrown through multiple nested layers.

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java:258

isOntapObjectNotFoundError lowercases with the default locale and also treats any message containing generic "not found" as an ONTAP-missing-object signal. This can mask non-ONTAP failures (e.g., CloudRuntimeException("Snapshot not found …") thrown in this plugin) as idempotent success when used by delete flows. Consider scoping the match to ONTAP delete/job-failure messages (and still allowing HTTP 404), and use a locale-stable lowercasing.

        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 isOntapObjectNotFoundError(error.getCause());

+    @Test
+    public void isOntapSnapshotNotFoundError_matchesEntryDoesNotExist() {
+        CloudRuntimeException ex = new CloudRuntimeException("Job failed with error: entry doesn't exist");
+        assertTrue(OntapStorageUtils.isOntapObjectNotFoundError(ex));
+    }
+
+    @Test
+    public void isOntapSnapshotNotFoundError_rejectsUnrelatedErrors() {
+        assertFalse(OntapStorageUtils.isOntapObjectNotFoundError(


Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/utils/OntapStorageUtilsTest.java:86

Test method name refers to "Snapshot" not found, but the helper under test is isOntapObjectNotFoundError. Renaming the test improves clarity and keeps naming consistent with the production method.

    public void isOntapSnapshotNotFoundError_matchesEntryDoesNotExist() {

plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/utils/OntapStorageUtilsTest.java:92

Test method name refers to "Snapshot" not found, but the helper under test is isOntapObjectNotFoundError. Renaming the test improves clarity and keeps naming consistent with the production method.

    public void isOntapSnapshotNotFoundError_rejectsUnrelatedErrors() {

CSTACKEX-158: if ontap snapshot are already delete from ontap side, d…

56c552f

…eletion of CS side of snapshot should not fail on not finding ontap snapshot.

rajiv-jain-netapp requested review from piyush5netapp, sandeeplocharla and suryag1201 as code owners July 20, 2026 09:07

Copilot AI review requested due to automatic review settings July 20, 2026 09:07

Copilot started reviewing on behalf of rajiv-jain-netapp July 20, 2026 09:07 View session

Copilot AI reviewed Jul 20, 2026

View reviewed changes

Comment thread ...torage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java Outdated

Comment thread ...ge/volume/ontap/src/test/java/org/apache/cloudstack/storage/utils/OntapStorageUtilsTest.java

sandeeplocharla reviewed Jul 22, 2026

View reviewed changes

suryag1201 reviewed Jul 22, 2026

View reviewed changes

piyush5netapp reviewed Jul 22, 2026

View reviewed changes

CSTACKEX-158: renaming method to generic text

312c13f

Copilot AI review requested due to automatic review settings July 24, 2026 06:10

Copilot started reviewing on behalf of rajiv-jain-netapp July 24, 2026 06:10 View session

Copilot AI reviewed Jul 24, 2026

View reviewed changes

CSTACKEX-158: validating for 404 with error-code check

827c800

Copilot AI review requested due to automatic review settings July 27, 2026 14:55

Copilot started reviewing on behalf of rajiv-jain-netapp July 27, 2026 14:56 View session

Copilot AI reviewed Jul 27, 2026

View reviewed changes

Uh oh!

Conversation

rajiv-jain-netapp commented Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

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

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rajiv-jain-netapp commented Jul 20, 2026 •

edited

Loading