From 4e53997ca2c72c0a7d8d2195708ca0a28135538b Mon Sep 17 00:00:00 2001
From: Wei Zhou <weizhou@apache.org>
Date: Tue, 31 Aug 2021 18:48:58 +0200
Subject: [PATCH] server: do not remove volume from DB if fail to expunge it
 from primary storage or secondary storage (#5373)

* server: do not remove volume from DB if fail to expunge it from primary storage or secondary storage

* server/VolumeApiServiceImpl.java: move to method

* update #5373
---
 .../CloudStackPrimaryDataStoreDriverImpl.java |  4 +--
 .../cloud/storage/VolumeApiServiceImpl.java   | 36 +++++++++++--------
 2 files changed, 24 insertions(+), 16 deletions(-)

diff --git a/plugins/storage/volume/default/src/main/java/org/apache/cloudstack/storage/datastore/driver/CloudStackPrimaryDataStoreDriverImpl.java b/plugins/storage/volume/default/src/main/java/org/apache/cloudstack/storage/datastore/driver/CloudStackPrimaryDataStoreDriverImpl.java
index cb977de5631..e43bc4621a3 100644
--- a/plugins/storage/volume/default/src/main/java/org/apache/cloudstack/storage/datastore/driver/CloudStackPrimaryDataStoreDriverImpl.java
+++ b/plugins/storage/volume/default/src/main/java/org/apache/cloudstack/storage/datastore/driver/CloudStackPrimaryDataStoreDriverImpl.java
@@ -138,7 +138,7 @@ public class CloudStackPrimaryDataStoreDriverImpl implements PrimaryDataStoreDri
         EndPoint ep = epSelector.select(volume);
         Answer answer = null;
         if (ep == null) {
-            String errMsg = "No remote endpoint to send DeleteCommand, check if host or ssvm is down?";
+            String errMsg = "No remote endpoint to send CreateObjectCommand, check if host or ssvm is down?";
             s_logger.error(errMsg);
             answer = new Answer(cmd, false, errMsg);
         } else {
@@ -280,7 +280,7 @@ public class CloudStackPrimaryDataStoreDriverImpl implements PrimaryDataStoreDri
                 EndPoint ep = epSelector.select(srcData, destData);
                 Answer answer = null;
                 if (ep == null) {
-                    String errMsg = "No remote endpoint to send command, check if host or ssvm is down?";
+                    String errMsg = "No remote endpoint to send CopyCommand, check if host or ssvm is down?";
                     s_logger.error(errMsg);
                     answer = new Answer(cmd, false, errMsg);
                 } else {
diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
index abb8265f20f..42026ef9283 100644
--- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
+++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
@@ -1407,12 +1407,7 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic
      * If the volume is not in the primary storage, we do nothing here.
      */
     protected void expungeVolumesInPrimaryStorageIfNeeded(VolumeVO volume) throws InterruptedException, ExecutionException {
-        VolumeInfo volOnPrimary = volFactory.getVolume(volume.getId(), DataStoreRole.Primary);
-        if (volOnPrimary != null) {
-            s_logger.info("Expunging volume " + volume.getId() + " from primary data store");
-            AsyncCallFuture<VolumeApiResult> future = volService.expungeVolumeAsync(volOnPrimary);
-            future.get();
-        }
+        expungeVolumesInPrimaryOrSecondary(volume, DataStoreRole.Primary);
     }
 
     /**
@@ -1420,16 +1415,29 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic
      * If it is, we will execute an asynchronous call to delete it there. Then, we decrement the {@link ResourceType#secondary_storage} for the account that owns the volume.
      */
     protected void expungeVolumesInSecondaryStorageIfNeeded(VolumeVO volume) throws InterruptedException, ExecutionException {
-        VolumeInfo volOnSecondary = volFactory.getVolume(volume.getId(), DataStoreRole.Image);
-        if (volOnSecondary != null) {
-            s_logger.info("Expunging volume " + volume.getId() + " from secondary data store");
-            AsyncCallFuture<VolumeApiResult> future2 = volService.expungeVolumeAsync(volOnSecondary);
-            future2.get();
-
-            _resourceLimitMgr.decrementResourceCount(volOnSecondary.getAccountId(), ResourceType.secondary_storage, volOnSecondary.getSize());
-        }
+        expungeVolumesInPrimaryOrSecondary(volume, DataStoreRole.Image);
     }
 
+    private void expungeVolumesInPrimaryOrSecondary(VolumeVO volume, DataStoreRole role) throws InterruptedException, ExecutionException {
+        VolumeInfo volOnStorage = volFactory.getVolume(volume.getId(), role);
+        if (volOnStorage != null) {
+            s_logger.info("Expunging volume " + volume.getId() + " from " + role + " data store");
+            AsyncCallFuture<VolumeApiResult> future = volService.expungeVolumeAsync(volOnStorage);
+            VolumeApiResult result = future.get();
+            if (result.isFailed()) {
+                String msg = "Failed to expunge the volume " + volume + " in " + role + " data store";
+                s_logger.warn(msg);
+                String details = "";
+                if (result.getResult() != null && !result.getResult().isEmpty()) {
+                    details = msg + " : " + result.getResult();
+                }
+                throw new CloudRuntimeException(details);
+            }
+            if (DataStoreRole.Image.equals(role)) {
+                _resourceLimitMgr.decrementResourceCount(volOnStorage.getAccountId(), ResourceType.secondary_storage, volOnStorage.getSize());
+            }
+        }
+    }
     /**
      * Clean volumes cache entries (if they exist).
      */