[GitHub] [cloudstack] harikrishna-patnala commented on a diff in pull request #6527: [KVM] Fix for Revert volume snapshot

[GitBox <gi...@apache.org>]

harikrishna-patnala commented on code in PR #6527:
URL: https://github.com/apache/cloudstack/pull/6527#discussion_r914452217


##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRevertSnapshotCommandWrapper.java:
##########
@@ -178,20 +178,24 @@ protected void revertVolumeToSnapshot(SnapshotObjectTO snapshotOnPrimaryStorage,
      * and the snapshot path from the primary storage.
      */
     protected Pair<String, SnapshotObjectTO> getSnapshot(SnapshotObjectTO snapshotOnPrimaryStorage, SnapshotObjectTO snapshotOnSecondaryStorage,
-            KVMStoragePool kvmStoragePoolPrimary, KVMStoragePool kvmStoragePoolSecondary){
-        String snapshotPath = snapshotOnPrimaryStorage.getPath();
-
-        if (Files.exists(Paths.get(snapshotPath))) {
-            return new Pair<>(snapshotPath, snapshotOnPrimaryStorage);
+            KVMStoragePool kvmStoragePoolPrimary, KVMStoragePool kvmStoragePoolSecondary) {
+        String snapshotPath = null;

Review Comment:
   No need to set or use it. If the snapshot on primary is not there then we will get it from secondary storage.



##########
plugins/storage/volume/default/src/main/java/org/apache/cloudstack/storage/datastore/driver/CloudStackPrimaryDataStoreDriverImpl.java:
##########
@@ -379,11 +382,21 @@ public void takeSnapshot(SnapshotInfo snapshot, AsyncCompletionCallback<CreateCm
 
     @Override
     public void revertSnapshot(SnapshotInfo snapshot, SnapshotInfo snapshotOnPrimaryStore, AsyncCompletionCallback<CommandResult> callback) {
-        RevertSnapshotCommand cmd = new RevertSnapshotCommand((SnapshotObjectTO)snapshot.getTO(), (SnapshotObjectTO)snapshotOnPrimaryStore.getTO());
+        SnapshotObjectTO dataOnPrimaryStorage = null;
+        if (snapshotOnPrimaryStore != null) {

Review Comment:
   It can be null. We are handling the same case where snapshotOnPrimary is null.
   Following is the complete flow 
   1. Look for snapshotOnPrimary entry exists
   2. If exists, use that entry to find the appropriate driver to execute revertSnapshotCommand
   3. If not found, use volume information to find the appropriate driver to execute revertSnapshotCommand
   4. While executing the revertSnapshotCommand, if snapshotOnPrimary exists, use it to revert the volume
   5. If snapshotOnPrimary does not exists, use the snapshot from secondary storage to revert the volume



##########
engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/SnapshotServiceImpl.java:
##########
@@ -428,11 +431,15 @@ public boolean deleteSnapshot(SnapshotInfo snapInfo) {
 
     @Override
     public boolean revertSnapshot(SnapshotInfo snapshot) {
+        PrimaryDataStore store = null;
         SnapshotInfo snapshotOnPrimaryStore = _snapshotFactory.getSnapshot(snapshot.getId(), DataStoreRole.Primary);
         if (snapshotOnPrimaryStore == null) {
-            throw new CloudRuntimeException("Cannot find an entry for snapshot " + snapshot.getId() + " on primary storage pools");
+            s_logger.warn("Cannot find an entry for snapshot " + snapshot.getId() + " on primary storage pools, searching with volume's primary storage pool");
+            VolumeInfo volumeInfo = volFactory.getVolume(snapshot.getVolumeId(), DataStoreRole.Primary);
+            store = (PrimaryDataStore)volumeInfo.getDataStore();

Review Comment:
   We are not looking for snapshotOnPrimaryStore again. We are just trying to get the appropriate datastore driver to run the command. We are using volume's info here if snapshot on primary os not found. So need to check again.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org