core: make VmBackupStop async using flag

Currently StopVmBackup command changed VM Backup phase in order to stop/finalize backup. This patch changes StopVmBackupCommand to set a flag on the backup entity that it is stopped (only for Hybrid backup), and then as HybridBackupCommand detects that the flag was raised and finalizes the Backup accordingly.
oVirt · May 10, 2022 · b6aa1bd · b6aa1bd
1 parent 9ac3383
commit b6aa1bd
Show file tree

Hide file tree

Showing 8 changed files with 97 additions and 33 deletions.
diff --git a/...dules/bll/src/main/java/org/ovirt/engine/core/bll/storage/backup/HybridBackupCommand.java b/...dules/bll/src/main/java/org/ovirt/engine/core/bll/storage/backup/HybridBackupCommand.java
@@ -135,7 +135,9 @@ protected void executeCommand() {
     @Override
     public boolean performNextOperation(int completedChildCount) {
         restoreCommandState();
-        if (getParameters().getVmBackup().getPhase() == VmBackupPhase.STARTING) {
+        VmBackupPhase phase = getParameters().getVmBackup().getPhase();
+
+        if (phase == VmBackupPhase.STARTING) {
             auditLog(AuditLogType.USER_CREATE_SNAPSHOT_FINISHED_SUCCESS);
             VmBackup vmBackup = getParameters().getVmBackup();
             Map<Guid, Guid> diskIdToSnapshot =
@@ -160,13 +162,11 @@ public boolean performNextOperation(int completedChildCount) {
             return true;
         }
 
-        if (getParameters().getVmBackup().getPhase() == VmBackupPhase.READY) {
-            return true;
-        }
-
-        if (getParameters().getVmBackup().getPhase() == VmBackupPhase.FINALIZING) {
-            removeAutoGeneratedSnapshot(createStepsContext(StepEnum.MERGE_SNAPSHOTS), getActionType(), getParameters());
-            updateVmBackupPhase(VmBackupPhase.REMOVING_SNAPSHOT);
+        if (phase == VmBackupPhase.READY) {
+            if (getParameters().getVmBackup().isStopped()) {
+                removeAutoGeneratedSnapshot(createStepsContext(StepEnum.MERGE_SNAPSHOTS), getActionType(), getParameters());
+                updateVmBackupPhase(VmBackupPhase.REMOVING_SNAPSHOT);
+            }
             return true;
         }
 
@@ -187,10 +187,7 @@ protected void endWithFailure() {
             return;
         }
 
-        if (snapshotDao.exists(getVmId(), getParameters().getAutoGeneratedSnapshotId())) {
-            // Try to remove the snapshot
-            removeAutoGeneratedSnapshot(cloneContextAndDetachFromParent(), null, null);
-        }
+        removeAutoGeneratedSnapshotIfExistsDetached();
 
         updateVmBackupPhase(VmBackupPhase.FAILED);
         setSucceeded(true);
@@ -249,6 +246,13 @@ protected List<DiskImage> getDisks() {
         return diskImageDao.getAllSnapshotsForVmSnapshot(getParameters().getAutoGeneratedSnapshotId());
     }
 
+    private void removeAutoGeneratedSnapshotIfExistsDetached() {
+        if (snapshotDao.exists(getVmId(), getParameters().getAutoGeneratedSnapshotId())) {
+            // Try to remove the snapshot
+            removeAutoGeneratedSnapshot(cloneContextAndDetachFromParent(), null, null);
+        }
+    }
+
     private void removeAutoGeneratedSnapshot(CommandContext commandContext, ActionType actionType, VmBackupParameters parameters) {
         RemoveSnapshotParameters removeSnapshotParameters =
                 new RemoveSnapshotParameters(getParameters().getAutoGeneratedSnapshotId(), getVmId());

diff --git a/...dules/bll/src/main/java/org/ovirt/engine/core/bll/storage/backup/StopVmBackupCommand.java b/...dules/bll/src/main/java/org/ovirt/engine/core/bll/storage/backup/StopVmBackupCommand.java
@@ -70,31 +70,41 @@ protected void executeCommand() {
             vmBackup.setDisks(vmBackupDao.getDisksByBackupId(vmBackup.getId()));
 
             VmBackupPhase phase = vmBackup.getPhase();
-            if (phase.isBackupFinalizing() || phase.isBackupFinished()) {
-                String errorMsg = String.format(
-                        "VM '%s' backup '%s' is already in '%s' phase, no need to stop the backup again",
-                        vmBackup.getVmId(), vmBackup.getId(), phase);
+            if (phase.isBackupFinalizing() || phase.isBackupFinished() || vmBackup.isStopped()) {
+                String errorMsg = phase.isBackupFinalizing() || phase.isBackupFinished()
+                        ? String.format(
+                            "VM '%s' backup '%s' is already in '%s' phase, no need to stop the backup again",
+                            vmBackup.getVmId(), vmBackup.getId(), phase)
+                        : String.format(
+                            "VM '%s' backup '%s' is already stopped, no need to stop the backup again",
+                            vmBackup.getVmId(), vmBackup.getId());
                 log.warn(errorMsg);
                 getReturnValue().setExecuteFailedMessages(new ArrayList<>(Collections.singleton(errorMsg)));
                 return;
             }
 
-            if (stopVmBackup()) {
-                if (phase == VmBackupPhase.READY) {
-                    updateVmBackupPhase(VmBackupPhase.FINALIZING);
-                    setSucceeded(true);
+            if (isHybridBackup()) {
+                updateVmBackupStopped();
+                setSucceeded(true);
+            } else {
+                if (stopVmBackup()) {
+                    if (phase == VmBackupPhase.READY) {
+                        // Finalize backup
+                        updateVmBackupPhase(VmBackupPhase.FINALIZING);
+                        setSucceeded(true);
+                    } else {
+                        // Premature backup finish
+                        String errorMsg = String.format(
+                                "Backup '%s' is in '%s' phase, premature backup stop was initiated.",
+                                vmBackup.getId(), phase);
+                        log.warn(errorMsg);
+                        getReturnValue().setExecuteFailedMessages(new ArrayList<>(Collections.singleton(errorMsg)));
+                        updateVmBackupPhase(VmBackupPhase.FINALIZING_FAILURE);
+                    }
                 } else {
-                    // Premature backup finish.
-                    String errorMsg = String.format(
-                            "Backup '%s' is in '%s' phase, premature backup stop was initiated.",
-                            vmBackup.getId(), phase);
-                    log.warn(errorMsg);
-                    getReturnValue().setExecuteFailedMessages(new ArrayList<>(Collections.singleton(errorMsg)));
+                    log.error("Failed to stop backup '{}' in '{}' phase", vmBackup.getId(), phase);
                     updateVmBackupPhase(VmBackupPhase.FINALIZING_FAILURE);
                 }
-            } else {
-                log.error("Failed to stop backup '{}' in '{}' phase", vmBackup.getId(), phase);
-                updateVmBackupPhase(VmBackupPhase.FINALIZING_FAILURE);
             }
         }
     }
@@ -130,13 +140,22 @@ private boolean isLiveBackup() {
         return vmBackup.getBackupType() == VmBackupType.Live;
     }
 
+    private boolean isHybridBackup() {
+        return vmBackup.getBackupType() == VmBackupType.Hybrid;
+    }
+
     private void updateVmBackupPhase(VmBackupPhase phase) {
         log.info("Change VM '{}' backup '{}' phase from '{}' to '{}'",
                 vmBackup.getVmId(), vmBackup.getId(), vmBackup.getPhase(), phase);
         vmBackup.setPhase(phase);
         vmBackupDao.update(vmBackup);
     }
 
+    private void updateVmBackupStopped() {
+        log.info("VM '{}' backup '{}' marked as stopped", vmBackup.getVmId(), vmBackup.getId());
+        vmBackupDao.setBackupStopped(vmBackup.getId());
+    }
+
     private EngineLock getEntityUpdateLock(Guid backupId) {
         Map<String, Pair<String, String>> lockMap = Collections.singletonMap(
                 backupId.toString(),
@@ -163,6 +182,10 @@ public List<PermissionSubject> getPermissionCheckSubjects() {
     public AuditLogType getAuditLogTypeValue() {
         addCustomValue("VmName", getVm().getName());
         addCustomValue("backupId", getParameters().getVmBackup().getId().toString());
-        return getSucceeded() ? AuditLogType.VM_BACKUP_FINALIZED : AuditLogType.VM_BACKUP_FAILED_TO_FINALIZE;
+        if (isHybridBackup()) {
+            return getSucceeded() ? AuditLogType.VM_BACKUP_STOPPED : AuditLogType.UNASSIGNED;
+        } else {
+            return getSucceeded() ? AuditLogType.VM_BACKUP_FINALIZED : AuditLogType.VM_BACKUP_FAILED_TO_FINALIZE;
+        }
     }
 }
diff --git a/.../modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmBackup.java b/.../modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmBackup.java
@@ -36,6 +36,8 @@ public class VmBackup implements Queryable, BusinessEntity<Guid> {
 
     private Guid snapshotId;
 
+    private boolean stopped;
+
     public Guid getId() {
         return id;
     }
@@ -136,6 +138,14 @@ public void setSnapshotId(Guid snapshotId) {
         this.snapshotId = snapshotId;
     }
 
+    public boolean isStopped() {
+        return stopped;
+    }
+
+    public void setStopped(boolean stopped) {
+        this.stopped = stopped;
+    }
+
     @Override
     public int hashCode() {
         return Objects.hash(
@@ -150,7 +160,8 @@ public int hashCode() {
                 modificationDate,
                 description,
                 backupType,
-                snapshotId
+                snapshotId,
+                stopped
         );
     }
 
@@ -174,7 +185,8 @@ public boolean equals(Object obj) {
                 && Objects.equals(modificationDate, other.modificationDate)
                 && Objects.equals(description, other.description)
                 && Objects.equals(backupType, other.backupType)
-                && Objects.equals(snapshotId, other.snapshotId);
+                && Objects.equals(snapshotId, other.snapshotId)
+                && Objects.equals(stopped, other.isStopped());
     }
 
     @Override

diff --git a/...les/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmBackupPhase.java b/...les/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmBackupPhase.java
@@ -31,7 +31,7 @@ public boolean isBackupFinished() {
     }
 
     public boolean isBackupFinalizing() {
-        return this == FINALIZING || this == FINALIZING_FAILURE;
+        return this == FINALIZING || this == FINALIZING_FAILURE || this == REMOVING_SNAPSHOT;
     }
 
     public static VmBackupPhase forName(String name) {

diff --git a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmBackupDao.java b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmBackupDao.java
@@ -74,4 +74,11 @@ public interface VmBackupDao extends GenericDao<VmBackup, Guid> {
      * @param failedBackups all failed backups having older end time than this date will be deleted.
      */
     void deleteCompletedBackups(Date succeededBackups, Date failedBackups);
+
+    /**
+     * Marks the backup as stopped
+     *
+     * @param backupId the VM backup ID
+     */
+    void setBackupStopped(Guid backupId);
 }
diff --git a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmBackupDaoImpl.java b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmBackupDaoImpl.java
@@ -66,6 +66,7 @@ protected RowMapper<VmBackup> createEntityRowMapper() {
         entity.setDescription(rs.getString("description"));
         entity.setBackupType(VmBackupType.forName(rs.getString("backup_type")));
         entity.setSnapshotId(getGuid(rs, "snapshot_id"));
+        entity.setStopped(rs.getBoolean("is_stopped"));
         return entity;
     };
 
@@ -141,4 +142,11 @@ public void deleteCompletedBackups(Date succeededBackups, Date failedBackups) {
                 .addValue("failed_end_time", failedBackups);
         getCallsHandler().executeModification("DeleteCompletedBackupsOlderThanDate", parameterSource);
     }
+
+    @Override
+    public void setBackupStopped(Guid backupId) {
+        getCallsHandler().executeModification("UpdateVmBackupStopped",
+                getCustomMapSqlParameterSource()
+                        .addValue("backup_id", backupId));
+    }
 }
diff --git a/packaging/dbscripts/upgrade/04_05_0270_add_backup_is_stopped.sql b/packaging/dbscripts/upgrade/04_05_0270_add_backup_is_stopped.sql
@@ -0,0 +1 @@
+SELECT fn_db_add_column('vm_backups', 'is_stopped', 'BOOL NOT NULL DEFAULT FALSE');
diff --git a/packaging/dbscripts/vm_backups_sp.sql b/packaging/dbscripts/vm_backups_sp.sql
@@ -117,6 +117,15 @@ BEGIN
 END;$PROCEDURE$
 LANGUAGE plpgsql;
 
+CREATE OR REPLACE FUNCTION UpdateVmBackupStopped (v_backup_id UUID)
+RETURNS VOID AS $PROCEDURE$
+BEGIN
+    UPDATE vm_backups
+    SET is_stopped = true
+    WHERE backup_id = v_backup_id;
+END;$PROCEDURE$
+LANGUAGE plpgsql;
+
 
 ----------------------------------------------------------------
 -- [vm_backup_disk_map] Table