From 288f799edda1090d763e54364205e82675f87fcb Mon Sep 17 00:00:00 2001 From: Arik Hadas Date: Sun, 4 Dec 2022 22:13:46 +0200 Subject: [PATCH 1/2] core: fix execution of RemoveImage RemoveImage now attaches the created async task to itself rather than to its parent command and its EndProcedure type is changed to COMMAND_MANAGED. With these changes, first the execution of RemoveImage is completed and then its parent's callback detects it and completes the execution of the parent command. Signed-off-by: Arik Hadas --- .../bll/storage/disk/RemoveDiskCommand.java | 2 +- .../disk/image/RemoveImageCommand.java | 48 +++-------- .../image/RemoveImageCommandCallback.java | 82 ------------------- .../common/action/RemoveImageParameters.java | 10 +-- .../bundles/AuditLogMessages.properties | 2 +- 5 files changed, 14 insertions(+), 130 deletions(-) delete mode 100644 backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/disk/image/RemoveImageCommandCallback.java diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/disk/RemoveDiskCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/disk/RemoveDiskCommand.java index 33f5fbccaf8..16a41463b8f 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/disk/RemoveDiskCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/disk/RemoveDiskCommand.java @@ -516,7 +516,7 @@ private RemoveImageParameters buildRemoveImageParameters(DiskImage diskImage) { RemoveImageParameters result = new RemoveImageParameters(diskImage.getImageId()); result.setTransactionScopeOption(TransactionScopeOption.Suppress); result.setDiskImage(diskImage); - result.setParentCommand(ActionType.RemoveDisk); + result.setParentCommand(getActionType()); result.setEntityInfo(new EntityInfo(VdcObjectType.Disk, getParameters().getDiskId())); result.setParentParameters(getParameters()); result.setRemoveFromSnapshots(true); diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/disk/image/RemoveImageCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/disk/image/RemoveImageCommand.java index 964a2b1d05d..de2d0168bfb 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/disk/image/RemoveImageCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/disk/image/RemoveImageCommand.java @@ -9,8 +9,6 @@ import java.util.Set; import java.util.stream.Collectors; -import javax.enterprise.inject.Instance; -import javax.enterprise.inject.Typed; import javax.inject.Inject; import org.apache.commons.lang.StringUtils; @@ -18,7 +16,6 @@ import org.ovirt.engine.core.bll.NonTransactiveCommandAttribute; import org.ovirt.engine.core.bll.context.CommandContext; import org.ovirt.engine.core.bll.storage.domain.PostDeleteActionHandler; -import org.ovirt.engine.core.bll.tasks.interfaces.CommandCallback; import org.ovirt.engine.core.common.AuditLogType; import org.ovirt.engine.core.common.VdcObjectType; import org.ovirt.engine.core.common.action.ActionType; @@ -88,10 +85,6 @@ public class RemoveImageCommand extends BaseIma @Inject private VmDao vmDao; - @Inject - @Typed(RemoveImageCommandCallback.class) - private Instance callbackProvider; - public RemoveImageCommand(T parameters, CommandContext cmdContext) { super(parameters, cmdContext); @@ -136,7 +129,7 @@ protected void executeCommand() { VDSReturnValue vdsReturnValue = performDeleteImageVdsmOperation(); Guid vdsmTaskId = createTask(taskId, vdsReturnValue.getCreationInfo(), - getParameters().getParentCommand(), + ActionType.RemoveImage, VdcObjectType.Storage, getStorageDomainId()); getTaskIdList().add(vdsmTaskId); @@ -286,11 +279,21 @@ protected List prepareSnapshotConfigWithoutImage(Guid imageGroupToRemo @Override protected void endSuccessfully() { + performImageDbOperations(); setSucceeded(true); } @Override protected void endWithFailure() { + List snapshotIds = diskImageDao + .getAllSnapshotsForImageGroup(getImageGroupId()).stream() + .map(DiskImage::getImageId) + .collect(Collectors.toList()); + addCustomValue("DiskId", getImageGroupId().toString()); + addCustomValue("VolumeIds", StringUtils.join(snapshotIds, ',')); + auditLog(this, AuditLogType.DELETE_IMAGE_GROUP_TASK_FAILED); + + imageDao.updateStatusOfImagesByImageGroupId(getImageGroupId(), ImageStatus.ILLEGAL); setSucceeded(true); } @@ -337,33 +340,4 @@ protected VDSReturnValue performDeleteImageVdsmOperation() { getDiskImage().isWipeAfterDelete(), getStorageDomain().getDiscardAfterDelete(), getParameters().isForceDelete()))); } - - @Override - public CommandCallback getCallback() { - return callbackProvider.get(); - } - - public void onSucceeded() { - performImageDbOperations(); - } - - public void onFailed() { - if (getParameters().getAsyncTaskErrorMessage() != null) { - Guid diskId = getParameters().getDiskImage().getId(); - List snapshotIds = diskImageDao - .getAllSnapshotsForImageGroup(diskId).stream() - .map(DiskImage::getImageId) - .collect(Collectors.toList()); - addCustomValue("DiskId", diskId.toString()); - addCustomValue("VolumeIds", StringUtils.join(snapshotIds, ',')); - addCustomValue("ErrorMessage", getParameters().getAsyncTaskErrorMessage()); - auditLog(this, AuditLogType.DELETE_IMAGE_GROUP_TASK_FAILED); - } - imageDao.updateStatusOfImagesByImageGroupId(getImageGroupId(), ImageStatus.ILLEGAL); - } - - @Override - protected void setSucceeded(boolean value) { - super.setSucceeded(value); - } } diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/disk/image/RemoveImageCommandCallback.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/disk/image/RemoveImageCommandCallback.java deleted file mode 100644 index 3d32d37bafe..00000000000 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/disk/image/RemoveImageCommandCallback.java +++ /dev/null @@ -1,82 +0,0 @@ -package org.ovirt.engine.core.bll.storage.disk.image; - -import java.util.ArrayList; -import java.util.HashSet; -import java.util.List; -import java.util.Map; -import java.util.Set; - -import javax.enterprise.inject.Typed; -import javax.inject.Inject; - -import org.ovirt.engine.core.bll.tasks.CommandCoordinatorUtil; -import org.ovirt.engine.core.bll.tasks.interfaces.CommandCallback; -import org.ovirt.engine.core.common.action.RemoveImageParameters; -import org.ovirt.engine.core.common.businessentities.AsyncTaskStatus; -import org.ovirt.engine.core.compat.CommandStatus; -import org.ovirt.engine.core.compat.Guid; -import org.ovirt.engine.core.vdsbroker.vdsbroker.VDSErrorException; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - -@Typed(RemoveImageCommandCallback.class) -public class RemoveImageCommandCallback implements CommandCallback { - private static final Logger log = LoggerFactory.getLogger(RemoveImageCommandCallback.class); - - @Inject - private CommandCoordinatorUtil commandCoordinatorUtil; - - @Override - public void doPolling(Guid cmdId, List childCmdIds) { - RemoveImageCommand command = getRemoveImageCommand(cmdId); - Set taskIds = new HashSet<>(command.getReturnValue().getVdsmTaskIdList()); - Map idToTaskStatusMap = commandCoordinatorUtil.pollTasks(taskIds); - for (Map.Entry idToTaskStatus : idToTaskStatusMap.entrySet()) { - Guid id = idToTaskStatus.getKey(); - AsyncTaskStatus status = idToTaskStatus.getValue(); - if (status.getTaskIsRunning()) { - log.info("Waiting on remove image command to complete the task '{}'", id); - return; - } - } - - List failedTasks = new ArrayList<>(); - idToTaskStatusMap.forEach((id, status) -> { - if (!status.getTaskEndedSuccessfully()) { - failedTasks.add(status); - } - }); - if (failedTasks.isEmpty()) { - command.setSucceeded(true); - command.setCommandStatus(CommandStatus.SUCCEEDED); - command.persistCommand(command.getParameters().getParentCommand()); - log.info("Remove image command has completed successfully for disk '{}' with async task(s) '{}'.", - command.getParameters().getDiskImage().getId(), taskIds); - } else { - RuntimeException exception = failedTasks.get(0).getException(); - if (exception instanceof VDSErrorException) { - command.getParameters() - .setAsyncTaskErrorMessage(((VDSErrorException) exception).getVdsError().getMessage()); - } - command.setSucceeded(false); - command.setCommandStatus(CommandStatus.FAILED); - log.info("Remove image command has failed for disk '{}' with async task(s) '{}'.", - command.getParameters().getDiskImage().getId(), failedTasks); - } - command.persistCommand(command.getParameters().getParentCommand()); - } - - @Override - public void onSucceeded(Guid cmdId, List childCmdIds) { - getRemoveImageCommand(cmdId).onSucceeded(); - } - - @Override - public void onFailed(Guid cmdId, List childCmdIds) { - getRemoveImageCommand(cmdId).onFailed(); - } - - private RemoveImageCommand getRemoveImageCommand(Guid cmdId) { - return commandCoordinatorUtil.retrieveCommand(cmdId); - } -} diff --git a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/RemoveImageParameters.java b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/RemoveImageParameters.java index 70ad268fb4f..5b4ea7885db 100644 --- a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/RemoveImageParameters.java +++ b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/RemoveImageParameters.java @@ -13,13 +13,13 @@ public class RemoveImageParameters extends ImagesContainterParametersBase implem private ImageDbOperationScope dbOperationScope; private boolean removeFromSnapshots; private boolean shouldLockImage; - private String asyncTaskErrorMessage; public RemoveImageParameters(Guid imageId) { super(imageId, null); setForceDelete(false); dbOperationScope = ImageDbOperationScope.IMAGE; shouldLockImage= true; + setEndProcedure(EndProcedure.COMMAND_MANAGED); } public RemoveImageParameters() { @@ -56,12 +56,4 @@ public boolean isRemoveFromSnapshots() { public void setRemoveFromSnapshots(boolean removeFromSnapshots) { this.removeFromSnapshots = removeFromSnapshots; } - - public String getAsyncTaskErrorMessage() { - return asyncTaskErrorMessage; - } - - public void setAsyncTaskErrorMessage(String asyncTaskErrorMessage) { - this.asyncTaskErrorMessage = asyncTaskErrorMessage; - } } diff --git a/backend/manager/modules/dal/src/main/resources/bundles/AuditLogMessages.properties b/backend/manager/modules/dal/src/main/resources/bundles/AuditLogMessages.properties index 0f5bb43c2b0..166af6b5784 100644 --- a/backend/manager/modules/dal/src/main/resources/bundles/AuditLogMessages.properties +++ b/backend/manager/modules/dal/src/main/resources/bundles/AuditLogMessages.properties @@ -1360,4 +1360,4 @@ VNICS_OUT_OF_SYNC_ON_NETWORK_UPDATE=Update to network ${NetworkName} was not app VNICS_OUT_OF_SYNC_ON_PROFILE_UPDATE=Update to network interface profile ${ProfileName} was not applied to virtual network interfaces [${VnicNames}]. The actual configuration on the interfaces may differ from the displayed one. VM_BACKUP_SNAPSHOT_POSSIBLE_LEFTOVER=Possible failure while removing a snapshot that was generated automatically during backup of VM '${VmName}'. The snapshot may be removed manually VM_BACKUP_FAILED_UNMONITORED=Backup ${BackupId} for VM ${VmName} was marked as failed since it is no longer monitored, cleanup may be required. -DELETE_IMAGE_GROUP_TASK_FAILED=Image group deletion task for disk ${DiskId} with volume(s) [${VolumeIds}] failed. Manual volume removal might be required. Underlying error message: ${ErrorMessage} +DELETE_IMAGE_GROUP_TASK_FAILED=Image group deletion task for disk ${DiskId} with volume(s) [${VolumeIds}] failed. Manual volume removal might be required. From d08c4bc9bd571fbd8054aa96f49d56d6ff32eb80 Mon Sep 17 00:00:00 2001 From: Arik Hadas Date: Sun, 4 Dec 2022 22:42:47 +0200 Subject: [PATCH 2/2] core: fix async task-less execution of remove-image RemoveImage may lack async tasks when: 1. The image doesn't exist 2. There's a failure during delete image With these changes, the image is removed from the database (rather than staying in LOCKED state) and the command would finish successfully in the first case, and in the second case the command end with failure and the image switches to ILLEGAL state. Signed-off-by: Arik Hadas --- .../core/bll/storage/disk/image/RemoveImageCommand.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/disk/image/RemoveImageCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/disk/image/RemoveImageCommand.java index de2d0168bfb..a08219453cf 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/disk/image/RemoveImageCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/disk/image/RemoveImageCommand.java @@ -138,12 +138,14 @@ protected void executeCommand() { if (e.getErrorCode() == EngineError.ImageDoesNotExistInDomainError) { log.info("Disk '{}' doesn't exist on storage domain '{}', rolling forward", getDiskImage().getId(), getStorageDomainId()); + endSuccessfully(); } else if (e.getErrorCode() == EngineError.ImageDeleteError && isImageRemovedFromStorage()) { // VDSM renames the image before deleting it, so technically the image doesn't exist after renaming, // but the actual delete can still fail with ImageDeleteError. // In this case, Engine has to check whether image still exists on the storage or not. - log.info("Disk '{}' was deleted from storage domain '{}'", getDiskImage().getId(), - getStorageDomainId()); + log.info("Failed to delete Disk '{}' from storage domain '{}', changing to illegal", + getDiskImage().getId(), getStorageDomainId()); + endWithFailure(); } else { throw e; }