updates per review

Author: nathanejohnson

engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/SnapshotInfo.java

@@ -17,6 +17,7 @@
 package org.apache.cloudstack.engine.subsystem.api.storage;
 
 import com.cloud.storage.Snapshot;
+import com.cloud.utils.exception.CloudRuntimeException;
 
 public interface SnapshotInfo extends DataObject, Snapshot {
     SnapshotInfo getParent();
@@ -43,5 +44,5 @@ public interface SnapshotInfo extends DataObject, Snapshot {
 
     long getPhysicalSize();
 
-    boolean markBackedUp();
+    void markBackedUp() throws CloudRuntimeException;
 }

engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/SnapshotObject.java

@@ -150,14 +150,14 @@ public long getPhysicalSize() {
     }
 
     @Override
-    public boolean markBackedUp() {
+    public void markBackedUp() throws CloudRuntimeException{
         try {
             processEvent(Event.OperationNotPerformed);
         } catch (NoTransitionException ex) {
             s_logger.error("no transition error: ", ex);
-            return false;
+            throw new CloudRuntimeException("Error marking snapshot backed up: " +
+                    this.snapshot.getId() + " " + ex.getMessage());
         }
-        return true;
     }
 
     @Override

server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java

@@ -1123,25 +1123,16 @@ public SnapshotInfo takeSnapshot(VolumeInfo volume) throws ResourceAllocationExc
             }
 
             SnapshotInfo snapshotOnPrimary = snapshotStrategy.takeSnapshot(snapshot);
-            boolean backupFlag = BackupSnapshotAfterTakingSnapshot.value() == null ||
+            boolean backupSnapToSecondary = BackupSnapshotAfterTakingSnapshot.value() == null ||
                     BackupSnapshotAfterTakingSnapshot.value();
 
-            if (backupFlag) {
-                if (payload.getAsyncBackup()) {
-                    backupSnapshotExecutor.schedule(new BackupSnapshotTask(snapshotOnPrimary, snapshotBackupRetries - 1, snapshotStrategy), 0, TimeUnit.SECONDS);
-                } else {
-                    SnapshotInfo backupedSnapshot = snapshotStrategy.backupSnapshot(snapshotOnPrimary);
-                    if (backupedSnapshot != null) {
-                        snapshotStrategy.postSnapshotCreation(snapshotOnPrimary);
-                    }
-                }
+            if (backupSnapToSecondary) {
+                backupSnapshotToSecondary(payload.getAsyncBackup(), snapshotStrategy, snapshotOnPrimary);
             } else {
                 if(s_logger.isDebugEnabled()) {
-                    s_logger.debug("skipping backup of snapshot to secondary due to configuration");
-                }
-                if (!snapshotOnPrimary.markBackedUp()) {
-                    throw new CloudRuntimeException("Can't mark snapshot as backed up!");
+                    s_logger.debug("skipping backup of snapshot " + snapshotId + " to secondary due to configuration");
                 }
+                snapshotOnPrimary.markBackedUp();
             }
 
             try {
@@ -1183,6 +1174,17 @@ public SnapshotInfo takeSnapshot(VolumeInfo volume) throws ResourceAllocationExc
         return snapshot;
     }
 
+    protected void backupSnapshotToSecondary(boolean asyncBackup, SnapshotStrategy snapshotStrategy, SnapshotInfo snapshotOnPrimary) {
+        if (asyncBackup) {
+            backupSnapshotExecutor.schedule(new BackupSnapshotTask(snapshotOnPrimary, snapshotBackupRetries - 1, snapshotStrategy), 0, TimeUnit.SECONDS);
+        } else {
+            SnapshotInfo backupedSnapshot = snapshotStrategy.backupSnapshot(snapshotOnPrimary);
+            if (backupedSnapshot != null) {
+                snapshotStrategy.postSnapshotCreation(snapshotOnPrimary);
+            }
+        }
+    }
+
     protected class BackupSnapshotTask extends ManagedContextRunnable {
         SnapshotInfo snapshot;
         int attempts;