Maor Lipchuk has uploaded a new change for review. Change subject: core: Remove Cinder disk should remove all volume types. ......................................................................
core: Remove Cinder disk should remove all volume types. Remove of Ciner disk command should remove all relations from Cinder provider such as volumes or snapshots. It should also remove the Cinder disk from the VM snapshot configuration. Change-Id: I176b0b01d43fc6b9eab051dc93e6bb0a11fb934b Bug-Url: https://bugzilla.redhat.com/1185826 Signed-off-by: Maor Lipchuk <mlipc...@redhat.com> --- M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveDiskCommand.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/RemoveCinderDiskCommand.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/RemoveCinderDiskCommandCallback.java M backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/AuditLogType.java M backend/manager/modules/dal/src/main/resources/bundles/AuditLogMessages.properties 5 files changed, 199 insertions(+), 71 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/58/42058/1 diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveDiskCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveDiskCommand.java index c36aac9..781953a 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveDiskCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveDiskCommand.java @@ -25,6 +25,7 @@ import org.ovirt.engine.core.common.VdcObjectType; import org.ovirt.engine.core.common.action.LockProperties; import org.ovirt.engine.core.common.action.LockProperties.Scope; +import org.ovirt.engine.core.common.action.RemoveCinderDiskParameters; import org.ovirt.engine.core.common.action.RemoveDiskParameters; import org.ovirt.engine.core.common.action.RemoveImageParameters; import org.ovirt.engine.core.common.action.VdcActionType; @@ -39,9 +40,10 @@ import org.ovirt.engine.core.common.businessentities.VmDeviceId; import org.ovirt.engine.core.common.businessentities.VmTemplate; import org.ovirt.engine.core.common.businessentities.VmTemplateStatus; +import org.ovirt.engine.core.common.businessentities.storage.CinderDisk; import org.ovirt.engine.core.common.businessentities.storage.Disk; -import org.ovirt.engine.core.common.businessentities.storage.DiskStorageType; import org.ovirt.engine.core.common.businessentities.storage.DiskImage; +import org.ovirt.engine.core.common.businessentities.storage.DiskStorageType; import org.ovirt.engine.core.common.businessentities.storage.ImageDbOperationScope; import org.ovirt.engine.core.common.businessentities.storage.ImageStatus; import org.ovirt.engine.core.common.businessentities.storage.LunDisk; @@ -306,9 +308,14 @@ removeLunDisk(); break; case CINDER: + RemoveCinderDiskParameters params = new RemoveCinderDiskParameters(getParameters().getDiskId()); + if ( ((CinderDisk)getDisk()).getImageStatus() == ImageStatus.ILLEGAL) { + params.setFaultTolerant(true); + } + Future<VdcReturnValueBase> future = CommandCoordinatorUtil.executeAsyncCommand( VdcActionType.RemoveCinderDisk, - new RemoveDiskParameters(getParameters().getDiskId()), + params, cloneContextAndDetachFromParent(), new SubjectEntity(VdcObjectType.Storage, getParameters().getStorageDomainId())); try { diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/RemoveCinderDiskCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/RemoveCinderDiskCommand.java index 6bef3e6..7aaeefe 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/RemoveCinderDiskCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/RemoveCinderDiskCommand.java @@ -1,32 +1,44 @@ package org.ovirt.engine.core.bll.storage; +import java.util.Collections; +import java.util.Map; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.Future; + import org.ovirt.engine.core.bll.ImagesHandler; import org.ovirt.engine.core.bll.InternalCommandAttribute; -import org.ovirt.engine.core.bll.RemoveDiskCommand; +import org.ovirt.engine.core.bll.RemoveImageCommand; import org.ovirt.engine.core.bll.context.CommandContext; +import org.ovirt.engine.core.bll.tasks.CommandCoordinatorUtil; import org.ovirt.engine.core.bll.tasks.interfaces.CommandCallback; -import org.ovirt.engine.core.common.action.RemoveDiskParameters; +import org.ovirt.engine.core.common.AuditLogType; +import org.ovirt.engine.core.common.action.RemoveCinderDiskParameters; +import org.ovirt.engine.core.common.action.VdcActionType; +import org.ovirt.engine.core.common.action.VdcReturnValueBase; +import org.ovirt.engine.core.common.businessentities.Snapshot; +import org.ovirt.engine.core.common.businessentities.SubjectEntity; +import org.ovirt.engine.core.common.businessentities.VM; import org.ovirt.engine.core.common.businessentities.VmDeviceId; import org.ovirt.engine.core.common.businessentities.storage.CinderDisk; import org.ovirt.engine.core.common.businessentities.storage.ImageStatus; +import org.ovirt.engine.core.common.businessentities.storage.VolumeClassification; import org.ovirt.engine.core.common.utils.Pair; -import org.ovirt.engine.core.compat.CommandStatus; import org.ovirt.engine.core.compat.Guid; import org.ovirt.engine.core.compat.TransactionScopeOption; +import org.ovirt.engine.core.dal.dbbroker.auditloghandling.AuditLogDirector; import org.ovirt.engine.core.dao.ImageDao; +import org.ovirt.engine.core.dao.ImageStorageDomainMapDao; import org.ovirt.engine.core.utils.transaction.TransactionMethod; import org.ovirt.engine.core.utils.transaction.TransactionSupport; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import java.util.Collections; -import java.util.Map; - @InternalCommandAttribute -public class RemoveCinderDiskCommand<T extends RemoveDiskParameters> extends RemoveDiskCommand<T> { +public class RemoveCinderDiskCommand<T extends RemoveCinderDiskParameters> extends RemoveImageCommand<T> { private static final Logger log = LoggerFactory.getLogger(RemoveCinderDiskCommand.class); private CinderBroker cinderBroker; + private CinderDisk cinderDisk; private Guid storageDomainId; public RemoveCinderDiskCommand(T parameters) { @@ -39,38 +51,137 @@ @Override public void executeCommand() { - CinderDisk disk = (CinderDisk) getDisk(); - if (disk.getImageStatus() == ImageStatus.ILLEGAL) { - // Remove disk from DB - setCommandStatus(CommandStatus.SUCCEEDED); - } else { - // Remove disk from Cinder - ImagesHandler.updateImageStatus(disk.getId(), ImageStatus.LOCKED); - getCinderBroker().deleteDisk(disk); + CinderDisk disk = getDisk(); + lockDiskIfNecessary(); + CinderDisk lastCinderVolume = (CinderDisk) ImagesHandler.getSnapshotLeaf(disk.getId()); + if (getParameters().isLockVm()) { + VM vm = getVmForNonShareableDiskImage(disk); + + // if the disk is not part of a vm (floating), there are no snapshots to update + // so no lock is required. + if (getParameters().isRemoveFromSnapshots() && vm != null) { + lockVmSnapshotsWithWait(vm); + } } + removeCinderVolume(lastCinderVolume); + getParameters().setRemovedVolume(lastCinderVolume); persistCommand(getParameters().getParentCommand(), true); getReturnValue().setActionReturnValue(disk.getId()); setSucceeded(true); } - protected void removeDiskFromDb() { - final CinderDisk cinderDisk = (CinderDisk) getDisk(); - TransactionSupport.executeInScope(TransactionScopeOption.Required, + private void lockDiskIfNecessary() { + if (getDisk().getImageStatus() != ImageStatus.LOCKED) { + ImagesHandler.updateImageStatus(getDisk().getId(), ImageStatus.LOCKED); + } + } + + protected CinderDisk getDisk() { + if (cinderDisk == null) { + cinderDisk = (CinderDisk) getDiskDao().get(getParameters().getDiskId()); + } + return cinderDisk; + } + + private VolumeClassification removeCinderVolume(CinderDisk volume) { + VolumeClassification cinderVolumeType = volume.getVolumeClassification(); + if (cinderVolumeType == VolumeClassification.Volume) { + getCinderBroker().deleteDisk(volume); + } else if (cinderVolumeType == VolumeClassification.Snapshot) { + getCinderBroker().deleteSnapshot(volume.getImageId()); + } else { + log.error("Error, could not determine Cinder entity {} with id {} from Cinder provider.", + volume.getDiskAlias(), + volume.getImageId()); + } + return cinderVolumeType; + } + + protected void removeDiskFromDb(final CinderDisk lastCinderVolume) { + final Snapshot updated = getSnapshot(lastCinderVolume); + TransactionSupport.executeInScope(TransactionScopeOption.RequiresNew, new TransactionMethod<Object>() { @Override public Object runInTransaction() { - getDiskImageDynamicDAO().remove(cinderDisk.getImageId()); - getImageDao().remove(cinderDisk.getImageId()); - // todo: remove snapshot - getBaseDiskDao().remove(cinderDisk.getId()); - getVmDeviceDAO().remove(new VmDeviceId(cinderDisk.getId(), null)); + // If the image being removed has the same id as the disk id, we should remove the disk. + if (lastCinderVolume.getImageId().equals(getDisk().getImageId())) { + getDbFacade().getVmDeviceDao().remove(new VmDeviceId(lastCinderVolume.getId(), null)); + getBaseDiskDao().remove(lastCinderVolume.getId()); + } + getImageStorageDomainMapDao().remove(lastCinderVolume.getImageId()); + getImageDao().remove(lastCinderVolume.getImageId()); + getDiskImageDynamicDAO().remove(lastCinderVolume.getImageId()); + if (updated != null) { + getSnapshotDao().update(updated); + } return null; } }); } + private Snapshot getSnapshot(CinderDisk lastCinderVolume) { + Guid vmSnapshotId = lastCinderVolume.getVmSnapshotId(); + Snapshot updated = null; + if (vmSnapshotId != null && !Guid.Empty.equals(vmSnapshotId)) { + Snapshot snapshot = getSnapshotDao().get(vmSnapshotId); + updated = ImagesHandler.prepareSnapshotConfigWithoutImageSingleImage(snapshot, + lastCinderVolume.getImageId()); + } + return updated; + } + protected void endSuccessfully() { - setSucceeded(true); + endRemoveCinderDisk(getParameters().getRemovedVolume()); + } + + @Override + protected void endWithFailure() { + CinderDisk cinderVolume = getParameters().getRemovedVolume(); + log.error("Could not volume id {} from Cinder which is related to disk {}", + cinderVolume.getDiskAlias(), + cinderVolume.getImageId()); + if (getParameters().isFaultTolerant()) { + endRemoveCinderDisk(cinderVolume); + } else { + auditLogFailureWithImageDeletion(cinderVolume.getImageId()); + ImagesHandler.updateImageStatus(getDisk().getId(), ImageStatus.ILLEGAL); + } + } + + private void endRemoveCinderDisk(CinderDisk leafCinderVolume) { + removeDiskFromDb(leafCinderVolume); + if (!leafCinderVolume.getImageId().equals(getDisk().getImageId())) { + RemoveCinderDiskParameters removeParams = new RemoveCinderDiskParameters(getParameters().getDiskId()); + removeParams.setFaultTolerant(getParameters().isFaultTolerant()); + removeParams.setRemovedVolume(getParameters().getRemovedVolume()); + removeParams.setShouldBeLogged(getParameters().getShouldBeLogged()); + removeParams.setLockVm(false); + Future<VdcReturnValueBase> future = + CommandCoordinatorUtil.executeAsyncCommand(VdcActionType.RemoveCinderDisk, + removeParams, + cloneContext(), + new SubjectEntity[0]); + try { + setReturnValue(future.get()); + setSucceeded(getReturnValue().getSucceeded()); + } catch (InterruptedException | ExecutionException e) { + log.error("Error removing Cinder disk '{}': {}", + getDiskImage().getDiskAlias(), + e.getMessage()); + log.debug("Exception", e); + ImagesHandler.updateImageStatus(getDisk().getId(), ImageStatus.ILLEGAL); + auditLogFailureWithImageDeletion(leafCinderVolume.getParentId()); + } + } else { + if (getParameters().getShouldBeLogged()) { + new AuditLogDirector().log(this, AuditLogType.USER_FINISHED_REMOVE_DISK); + } + } + } + + private void auditLogFailureWithImageDeletion(Guid failedVolumeId) { + addCustomValue("imageId", failedVolumeId.toString()); + new AuditLogDirector().log(this, AuditLogType.USER_FINISHED_FAILED_REMOVE_CINDER_DISK); } protected ImageDao getImageDao() { @@ -97,22 +208,23 @@ return Collections.emptyMap(); } - protected CinderBroker getCinderBroker() { - if (cinderBroker == null) { - cinderBroker = new CinderBroker(getStorageDomainId(), getReturnValue().getExecuteFailedMessages()); - } - return cinderBroker; - } - @Override public Guid getStorageDomainId() { if (storageDomainId == null) { - storageDomainId = getCinderDisk().getStorageIds().get(0); + storageDomainId = ((CinderDisk) getDiskDao().get(getDisk().getId())).getStorageIds().get(0); + storageDomainId = getDisk().getStorageIds().get(0); } return storageDomainId; } - private CinderDisk getCinderDisk() { - return (CinderDisk) getDisk(); + protected ImageStorageDomainMapDao getImageStorageDomainMapDao() { + return getDbFacade().getImageStorageDomainMapDao(); + } + + protected String getDiskAlias() { + if (getDisk() != null) { + return getDisk().getDiskAlias(); + } + return ""; } } diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/RemoveCinderDiskCommandCallback.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/RemoveCinderDiskCommandCallback.java index e797414..dd16bee 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/RemoveCinderDiskCommandCallback.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/RemoveCinderDiskCommandCallback.java @@ -1,50 +1,64 @@ package org.ovirt.engine.core.bll.storage; -import org.ovirt.engine.core.bll.ImagesHandler; -import org.ovirt.engine.core.bll.tasks.CommandCoordinatorUtil; -import org.ovirt.engine.core.common.AuditLogType; -import org.ovirt.engine.core.common.action.RemoveDiskParameters; -import org.ovirt.engine.core.common.businessentities.storage.CinderDisk; -import org.ovirt.engine.core.common.businessentities.storage.DiskImage; -import org.ovirt.engine.core.common.businessentities.storage.ImageStatus; -import org.ovirt.engine.core.compat.CommandStatus; -import org.ovirt.engine.core.compat.Guid; -import org.ovirt.engine.core.dal.dbbroker.auditloghandling.AuditLogDirector; - import java.util.List; -public class RemoveCinderDiskCommandCallback extends AbstractCinderDiskCommandCallback<RemoveCinderDiskCommand<RemoveDiskParameters>> { +import org.ovirt.engine.core.bll.tasks.CommandCoordinatorUtil; +import org.ovirt.engine.core.common.action.RemoveCinderDiskParameters; +import org.ovirt.engine.core.common.businessentities.storage.CinderDisk; +import org.ovirt.engine.core.common.businessentities.storage.ImageStatus; +import org.ovirt.engine.core.common.businessentities.storage.VolumeClassification; +import org.ovirt.engine.core.compat.CommandStatus; +import org.ovirt.engine.core.compat.Guid; + +public class RemoveCinderDiskCommandCallback extends AbstractCinderDiskCommandCallback<RemoveCinderDiskCommand<RemoveCinderDiskParameters>> { @Override public void doPolling(Guid cmdId, List<Guid> childCmdIds) { super.doPolling(cmdId, childCmdIds); - CinderBroker cinderBroker = getCinderBroker(); - if (!cinderBroker.isDiskExist(getDiskId())) { + CinderDisk removedVolume = getCommand().getParameters().getRemovedVolume(); + if (!checkIfVolumeExists(removedVolume)) { // Disk has been deleted successfully getCommand().setCommandStatus(CommandStatus.SUCCEEDED); return; } - ImageStatus imageStatus = cinderBroker.getDiskStatus(getDiskId()); - DiskImage disk = getDisk(); + ImageStatus imageStatus = checkImageStatus(removedVolume); if (imageStatus != null && imageStatus != disk.getImageStatus()) { switch (imageStatus) { - case ILLEGAL: - getCommand().getParameters().setShouldBeLogged(true); - getCommand().setCommandStatus(CommandStatus.FAILED); - break; + case ILLEGAL: + getCommand().setCommandStatus(CommandStatus.FAILED); + break; } + } + } + + private ImageStatus checkImageStatus(CinderDisk removedVolume) { + if (removedVolume.getVolumeClassification() == VolumeClassification.Volume) { + return getCinderBroker().getDiskStatus(removedVolume.getImageId()); + } else if (removedVolume.getVolumeClassification() == VolumeClassification.Snapshot) { + return getCinderBroker().getSnapshotStatus(removedVolume.getImageId()); + } else { + log.error("No valid cinder volume type enum has been initialized in the Cinder disk business entity."); + return ImageStatus.ILLEGAL; + } + } + + private boolean checkIfVolumeExists(CinderDisk removedVolume) { + if (removedVolume.getVolumeClassification() == VolumeClassification.Volume) { + return getCinderBroker().isDiskExist(removedVolume.getImageId()); + } else if (removedVolume.getVolumeClassification() == VolumeClassification.Snapshot) { + return getCinderBroker().isSnapshotExist(removedVolume.getImageId()); + } else { + log.error("No valid cinder volume type enum has been initialized in the Cinder disk business entity."); + return true; } } @Override public void onFailed(Guid cmdId, List<Guid> childCmdIds) { super.onFailed(cmdId, childCmdIds); - log.error("Failed deleting disk from Cinder. ID: {}", getDiskId()); - if (getCommand().getParameters().getShouldBeLogged()) { - new AuditLogDirector().log(getCommand(), AuditLogType.USER_FINISHED_FAILED_REMOVE_DISK); - } - ImagesHandler.updateImageStatus(getDiskId(), ImageStatus.ILLEGAL); + getCommand().getParameters().setTaskGroupSuccess(false); + log.error("Failed deleting volume/snapshot from Cinder. ID: {}", getDiskId()); getCommand().endAction(); CommandCoordinatorUtil.removeAllCommandsInHierarchy(cmdId); } @@ -52,26 +66,19 @@ @Override public void onSucceeded(Guid cmdId, List<Guid> childCmdIds) { super.onSucceeded(cmdId, childCmdIds); - log.info("Disk has been successfully deleted from Cinder. ID: {}", getDiskId()); - if (getCommand().getParameters().getShouldBeLogged()) { - new AuditLogDirector().log(getCommand(), AuditLogType.USER_FINISHED_REMOVE_DISK); - } - getCommand().removeDiskFromDb(); + log.info("Volume/Snapshot has been successfully deleted from Cinder. ID: {}", getDiskId()); getCommand().endAction(); CommandCoordinatorUtil.removeAllCommandsInHierarchy(cmdId); } @Override protected Guid getDiskId() { - return getCommand().getParameters().getDiskId(); + return getCommand().getParameters().getRemovedVolume().getImageId(); } @Override protected CinderDisk getDisk() { - if (disk == null) { - disk = (CinderDisk) getCommand().getDiskDao().get(getDiskId()); - } - return disk; + return getCommand().getParameters().getRemovedVolume(); } @Override diff --git a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/AuditLogType.java b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/AuditLogType.java index a5a1758..bc36061 100644 --- a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/AuditLogType.java +++ b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/AuditLogType.java @@ -1208,6 +1208,7 @@ CINDER_PROVIDER_ERROR(10750, AuditLogSeverity.ERROR), CINDER_DISK_CONNECTION_FAILURE(10751, AuditLogSeverity.ERROR), CINDER_DISK_CONNECTION_VOLUME_DRIVER_UNSUPPORTED(10752, AuditLogSeverity.ERROR), + USER_FINISHED_FAILED_REMOVE_CINDER_DISK(10753, AuditLogSeverity.ERROR), // Host Devices VM_ADD_HOST_DEVICES(10800), 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 f96c406..49354dd 100644 --- a/backend/manager/modules/dal/src/main/resources/bundles/AuditLogMessages.properties +++ b/backend/manager/modules/dal/src/main/resources/bundles/AuditLogMessages.properties @@ -995,6 +995,7 @@ CINDER_PROVIDER_ERROR=An error occurred on Cinder provider: '${CinderException}' CINDER_DISK_CONNECTION_FAILURE=Failed to retrieve connection information for Cinder Disk '${DiskAlias}'. CINDER_DISK_CONNECTION_VOLUME_DRIVER_UNSUPPORTED=Unsupported volume driver for Cinder Disk '${DiskAlias}'. +USER_FINISHED_FAILED_REMOVE_CINDER_DISK=Failed to remove disk ${DiskAlias} from storage domain ${StorageDomainName}. The following entity id could not be deleted from the Cinder provider '${imageId}'. (User: ${UserName}). VM_ADD_HOST_DEVICES=Host devices ${NamesAdded} were attached to Vm ${VmName} by User ${UserName}. VM_REMOVE_HOST_DEVICES=Host devices ${NamesRemoved} were detached from Vm ${VmName} by User ${UserName}. HOST_AVAILABLE_UPDATES_FAILED=Failed to check for available updates on host '${VdsName}' with message '${Message}'. -- To view, visit https://gerrit.ovirt.org/42058 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I176b0b01d43fc6b9eab051dc93e6bb0a11fb934b Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Maor Lipchuk <mlipc...@redhat.com> _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
