CSTACKEX-158: if ontap snapshot are already delete from ontap side, d…#82
CSTACKEX-158: if ontap snapshot are already delete from ontap side, d…#82rajiv-jain-netapp wants to merge 1 commit into
Conversation
…eletion of CS side of snapshot should not fail on not finding ontap snapshot.
There was a problem hiding this comment.
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
StorageStrategyandOntapPrimaryDatastoreDriver). - 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.
| * 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) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
This was added purely to improve readability. You can treat it as a helper method rather than an exception class specific to ONTAP.
| * Delete workflows treat a missing backend snapshot as idempotent success. | ||
| */ | ||
| public static boolean isOntapSnapshotNotFoundError(Throwable error) { | ||
| if (error == null) { |
There was a problem hiding this comment.
Agree, we can also use the same method for other ontap entities like igroup, export policy, volume etc
There was a problem hiding this comment.
We can reuse this method for any "not found" scenario, regardless of the object type. That's the reason I placed it in the utility class rather than tying it to a specific implementation.
| } | ||
| String message = error.getMessage(); | ||
| if (message != null) { | ||
| String lower = message.toLowerCase(); |
There was a problem hiding this comment.
it is better to check the status code as 404
There was a problem hiding this comment.
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.
| return false; | ||
| } | ||
| String message = error.getMessage(); | ||
| if (message != null) { |
There was a problem hiding this comment.
if message is null somehow, I think func run infinitely ?
There was a problem hiding this comment.
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.
…Deletion of CS side of snapshot should not fail on not finding ontap snapshot.
Description
This PR...
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
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?