Liron Ar has uploaded a new change for review. Change subject: core: remove vm - remove end method code ......................................................................
core: remove vm - remove end method code When removing vm the operation is done as synchronized operation, task is created for each disk and then the vm is removed from db. In order to deal with engine crash, in the end method a roll forward is performed in case that the vm still exists (the vm is being deleted and it's disks are being marked as illegal). This logic is unneeded and can cause to a bug, in case of deleted vm and imported vm with the same id, this can cause that the imported vm would be deleted from the db as the deletion tasks would end - therefore this logic is being removed. this patch contains the following changes: *RemoveVm command - 1. the first step is to remove the vm, lock all the disks - in case of failure they will be in ILLEGAL status. 2. added a dao method to update multiple image statuses at once to save db calls - other flows will move to use it as well. 3. added a boolean member indicating whether RemoveImage should lock the image. 4. RemoveAllVmImages shouldn't fail in case of failure to create a task for deletion, the engine should continue and try to delete the other images- this change also prevents a storage leak in removing vm and templates from export domains. 5. RemoveAllVmImages returns a list of disks it failed to remove, instead of querying the db again. Change-Id: I7a3e497e52471f7255e6c17b59a2e3bf362b8783 Signed-off-by: Liron Aravot <[email protected]> --- M backend/manager/dbscripts/images_sp.sql M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImagesHandler.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveAllVmImagesCommand.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveImageCommand.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmCommand.java M backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/RemoveImageParameters.java M backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/ImageDao.java M backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/ImageDaoDbFacadeImpl.java M backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/FixturesTool.java M backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/ImageDaoTest.java 10 files changed, 111 insertions(+), 72 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/69/14569/1 diff --git a/backend/manager/dbscripts/images_sp.sql b/backend/manager/dbscripts/images_sp.sql index 65f829e..1b1965e 100644 --- a/backend/manager/dbscripts/images_sp.sql +++ b/backend/manager/dbscripts/images_sp.sql @@ -73,6 +73,21 @@ +Create or replace FUNCTION UpdateImagesStatus( + v_images_ids VARCHAR(5000), + v_status INTEGER) +RETURNS VOID +AS $procedure$ +BEGIN + UPDATE images + SET imageStatus = v_status + WHERE image_guid IN (SELECT * FROM fnSplitterUuid(v_images_ids)); +END; $procedure$ +LANGUAGE plpgsql; + + + + Create or replace FUNCTION UpdateImageVmSnapshotId( v_image_id UUID, v_vm_snapshot_id UUID) diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImagesHandler.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImagesHandler.java index 02b34c4..4c03b05 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImagesHandler.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImagesHandler.java @@ -9,6 +9,8 @@ import java.util.Map; import java.util.Set; +import org.apache.commons.collections.CollectionUtils; +import org.apache.commons.collections.Transformer; import org.apache.commons.lang.StringUtils; import org.ovirt.engine.core.bll.storage.StorageHelperDirector; import org.ovirt.engine.core.bll.utils.VmDeviceUtils; @@ -555,6 +557,18 @@ DbFacade.getInstance().getBaseDiskDao().remove(diskId); } + public static void updateDiskImagesStatus(Collection<DiskImage> diskImages, ImageStatus imageStatus) { + @SuppressWarnings("unchecked") + Collection<Guid> imageIds = CollectionUtils.collect(diskImages, new Transformer() { + + @Override + public Object transform(Object input) { + return ((DiskImage)input).getImage().getId(); + } + }); + DbFacade.getInstance().getImageDao().updateImagesStatus(imageIds, imageStatus); + } + public static void updateImageStatus(Guid imageId, ImageStatus imageStatus) { DbFacade.getInstance().getImageDao().updateStatus(imageId, imageStatus); } diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveAllVmImagesCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveAllVmImagesCommand.java index 2ca3606..2289bb4 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveAllVmImagesCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveAllVmImagesCommand.java @@ -1,6 +1,8 @@ package org.ovirt.engine.core.bll; +import java.util.Collection; import java.util.HashSet; +import java.util.LinkedList; import java.util.List; import java.util.Set; @@ -45,7 +47,7 @@ } } - boolean noImagesRemovedYet = true; + Collection<DiskImage> failedToBeRemoved = new LinkedList<>(); for (final DiskImage image : images) { if (mImagesToBeRemoved.contains(image.getImageId())) { VdcReturnValueBase vdcReturnValue = @@ -56,12 +58,7 @@ if (vdcReturnValue.getSucceeded()) { getReturnValue().getInternalTaskIdList().addAll(vdcReturnValue.getInternalTaskIdList()); } else { - if (noImagesRemovedYet) { - setSucceeded(false); - getReturnValue().setFault(vdcReturnValue.getFault()); - return; - } - + failedToBeRemoved.add(image); log.errorFormat("Can't remove image id: {0} for VM id: {1} due to: {2}. Image will be set at illegal state with no snapshot id.", image.getImageId(), getParameters().getVmId(), @@ -78,10 +75,10 @@ } }); } - noImagesRemovedYet = false; } } + setActionReturnValue(failedToBeRemoved); setSucceeded(true); } @@ -92,6 +89,7 @@ result.setDiskImage(image); result.setEntityId(getParameters().getEntityId()); result.setForceDelete(getParameters().getForceDelete()); + result.setShouldLockImage(false); return result; } diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveImageCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveImageCommand.java index d306488..221cce8 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveImageCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveImageCommand.java @@ -299,9 +299,7 @@ @Override protected VDSReturnValue performImageVdsmOperation() { - boolean isShouldBeLocked = getParameters().getParentCommand() != VdcActionType.RemoveVmFromImportExport - && getParameters().getParentCommand() != VdcActionType.RemoveVmTemplateFromImportExport; - if (isShouldBeLocked) { + if (getParameters().isShouldLockImage()) { // the image status should be set to ILLEGAL, so that in case compensation runs the image status will // be revert to be ILLEGAL, as we can't tell whether the task started on vdsm side or not. getDiskImage().setImageStatus(ImageStatus.ILLEGAL); diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmCommand.java index 99b6ff8..5a307e5 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmCommand.java @@ -76,20 +76,32 @@ VM vm = getVm(); hasImages = vm.getDiskList().size() > 0; - if (getParameters().isRemoveDisks() && hasImages) { - if (!removeVmImages(null)) { - return false; - } - processUnremovedDisks(false); - } + final List<DiskImage> diskImages = ImagesHandler.filterImageDisks(getVm().getDiskList(), + true, + false); TransactionSupport.executeInNewTransaction(new TransactionMethod<Void>() { @Override public Void runInTransaction() { removeVmFromDb(); + if (getParameters().isRemoveDisks()) { + for (DiskImage image : diskImages) { + getCompensationContext().snapshotEntityStatus(image.getImage(), ImageStatus.ILLEGAL); + } + ImagesHandler.updateDiskImagesStatus(diskImages, ImageStatus.LOCKED); + getCompensationContext().stateChanged(); + } return null; } }); + + if (getParameters().isRemoveDisks() && hasImages) { + Collection<DiskImage> unremovedDisks = (Collection<DiskImage>)removeVmImages(diskImages).getActionReturnValue(); + if (!unremovedDisks.isEmpty()) { + processUnremovedDisks(unremovedDisks); + setSucceeded(false); + } + } return true; } @@ -211,7 +223,7 @@ return true; } - protected boolean removeVmImages(List<DiskImage> images) { + protected VdcReturnValueBase removeVmImages(List<DiskImage> images) { RemoveAllVmImagesParameters tempVar = new RemoveAllVmImagesParameters(getVmId(), images); tempVar.setParentCommand(getActionType()); tempVar.setEntityId(getParameters().getEntityId()); @@ -225,21 +237,13 @@ getReturnValue().getTaskIdList().addAll(vdcRetValue.getInternalTaskIdList()); } - return vdcRetValue.getSucceeded(); + return vdcRetValue; } @Override public AuditLogType getAuditLogTypeValue() { - switch (getActionState()) { - case EXECUTE: - return getSucceeded() ? (disksLeftInVm.isEmpty() ? AuditLogType.USER_REMOVE_VM_FINISHED : AuditLogType.USER_REMOVE_VM_FINISHED_WITH_ILLEGAL_DISKS) - : AuditLogType.USER_FAILED_REMOVE_VM; - case END_FAILURE: - case END_SUCCESS: - default: - return disksLeftInVm.isEmpty() ? AuditLogType.UNASSIGNED - : AuditLogType.USER_REMOVE_VM_FINISHED_WITH_ILLEGAL_DISKS; - } + return getSucceeded() ? (disksLeftInVm.isEmpty() ? AuditLogType.USER_REMOVE_VM_FINISHED : AuditLogType.USER_REMOVE_VM_FINISHED_WITH_ILLEGAL_DISKS) + : AuditLogType.USER_FAILED_REMOVE_VM; } @Override @@ -270,50 +274,16 @@ @Override protected void endVmCommand() { - try { - if (acquireLock()) { - // Ensures the lock on the VM guid can be acquired. This prevents a race - // between executeVmCommand (for example, of a first multiple VMs removal that includes VM A, - // and a second multiple VMs removal that include the same VM). - setVm(DbFacade.getInstance().getVmDao().get(getVmId())); - if (getVm() != null) { - processUnremovedDisks(true); + setCommandShouldBeLogged(false); - // Remove VM from DB. - removeVmFromDb(); - } - } - setSucceeded(true); - } finally { - freeLock(); - } + setSucceeded(true); } - /** - * Update disks for VM after disks were removed. - */ - private void processUnremovedDisks(boolean shouldUpdateDiskStatus) { - VmHandler.updateDisksFromDb(getVm()); - - // Get all disk images for VM (VM should not have any image disk associated with it). - List<DiskImage> diskImages = ImagesHandler.filterImageDisks(getVm().getDiskMap().values(), - true, - false); - - // If the VM still has disk images related to it, change their status to Illegal. - if (!diskImages.isEmpty()) { - for (DiskImage diskImage : diskImages) { - if (shouldUpdateDiskStatus && diskImage.getImageStatus() != ImageStatus.ILLEGAL) { - log.errorFormat("Disk {0} which is part of VM {1} was not at ILLEGAL state.", - diskImage.getDiskAlias(), - getVm().getName()); - ImagesHandler.updateImageStatus(diskImage.getImage().getId(), ImageStatus.ILLEGAL); - } - - disksLeftInVm.add(diskImage.getDiskAlias()); - } - addCustomValue("DisksNames", StringUtils.join(disksLeftInVm, ",")); + private void processUnremovedDisks(Collection<DiskImage> diskImages) { + for (DiskImage diskImage : diskImages) { + disksLeftInVm.add(diskImage.getDiskAlias()); } + addCustomValue("DisksNames", StringUtils.join(disksLeftInVm, ",")); } @Override 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 90504e5..f2b48b4 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 @@ -9,11 +9,13 @@ private DiskImage diskImage; private boolean removeFromDB; private boolean removeFromSnapshots; + private boolean shouldLockImage; public RemoveImageParameters(Guid imageId) { super(imageId, null); setForceDelete(false); removeFromDB = true; + shouldLockImage= true; } public RemoveImageParameters() { @@ -31,6 +33,14 @@ this.removeFromDB = removeFromDB; } + public boolean isShouldLockImage() { + return shouldLockImage; + } + + public void setShouldLockImage(boolean shouldLockImage) { + this.shouldLockImage = shouldLockImage; + } + public boolean getRemoveFromDB() { return removeFromDB; } diff --git a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/ImageDao.java b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/ImageDao.java index 17bda44..a507958 100644 --- a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/ImageDao.java +++ b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/ImageDao.java @@ -1,5 +1,7 @@ package org.ovirt.engine.core.dao; +import java.util.Collection; + import org.ovirt.engine.core.common.businessentities.Image; import org.ovirt.engine.core.common.businessentities.ImageStatus; import org.ovirt.engine.core.compat.Guid; @@ -12,4 +14,6 @@ void updateQuotaForImageAndSnapshots(Guid imageGroupId, NGuid quotaId); public void updateImageVmSnapshotId(Guid id, Guid vmSnapshotId); + + public void updateImagesStatus(Collection<Guid> ids, ImageStatus status); } diff --git a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/ImageDaoDbFacadeImpl.java b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/ImageDaoDbFacadeImpl.java index 31965af..4840151 100644 --- a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/ImageDaoDbFacadeImpl.java +++ b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/ImageDaoDbFacadeImpl.java @@ -2,7 +2,9 @@ import java.sql.ResultSet; import java.sql.SQLException; +import java.util.Collection; +import org.apache.commons.lang.StringUtils; import org.ovirt.engine.core.common.businessentities.Image; import org.ovirt.engine.core.common.businessentities.ImageStatus; import org.ovirt.engine.core.common.businessentities.VolumeFormat; @@ -28,6 +30,14 @@ } @Override + public void updateImagesStatus(Collection<Guid> ids, ImageStatus status) { + MapSqlParameterSource parameterSource = getCustomMapSqlParameterSource() + .addValue("images_ids", StringUtils.join(ids, ",")) + .addValue("status", status); + getCallsHandler().executeModification("UpdateImagesStatus", parameterSource); + } + + @Override public void updateImageVmSnapshotId(Guid id, Guid vmSnapshotId) { MapSqlParameterSource parameterSource = getCustomMapSqlParameterSource() .addValue("image_id", id) diff --git a/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/FixturesTool.java b/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/FixturesTool.java index 59a2c25..e0af509 100644 --- a/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/FixturesTool.java +++ b/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/FixturesTool.java @@ -219,6 +219,11 @@ protected static final Guid IMAGE_ID = new Guid("42058975-3d5e-484a-80c1-01c31207f578"); /** + * Predefined image for testing. + */ + protected static final Guid IMAGE_ID2 = new Guid("52058975-3d5e-484a-80c1-01c31207f578"); + + /** * Predefined disk for testing. */ protected static final Guid DISK_ID = new Guid("1b26a52b-b60f-44cb-9f46-3ef333b04a34"); diff --git a/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/ImageDaoTest.java b/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/ImageDaoTest.java index 7553374..a5b567a 100644 --- a/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/ImageDaoTest.java +++ b/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/ImageDaoTest.java @@ -4,6 +4,8 @@ import static org.junit.Assert.assertNotNull; import static org.junit.Assert.fail; +import java.util.Arrays; + import org.junit.Test; import org.ovirt.engine.core.common.businessentities.Image; import org.ovirt.engine.core.common.businessentities.ImageStatus; @@ -71,9 +73,16 @@ @Test public void testUpdateStatus() { dao.updateStatus(EXISTING_IMAGE_ID, ImageStatus.LOCKED); - Image imageFromDb = dao.get(EXISTING_IMAGE_ID); - assertNotNull(imageFromDb); - assertEquals(ImageStatus.LOCKED, imageFromDb.getStatus()); + assertImageNotNullAndStatus(EXISTING_IMAGE_ID, ImageStatus.LOCKED); + } + + @Test + public void testUpdateImagesStatus() { + assertImageNotNullAndStatus(EXISTING_IMAGE_ID, ImageStatus.OK); + assertImageNotNullAndStatus(FixturesTool.IMAGE_ID2, ImageStatus.OK); + dao.updateImagesStatus(Arrays.asList(FixturesTool.IMAGE_ID2, EXISTING_IMAGE_ID), ImageStatus.LOCKED); + assertImageNotNullAndStatus(EXISTING_IMAGE_ID, ImageStatus.LOCKED); + assertImageNotNullAndStatus(FixturesTool.IMAGE_ID2, ImageStatus.LOCKED); } @Test @@ -103,4 +112,10 @@ // check that the new quota is the inserted one assertEquals("quota wasn't changed", quotaId, FixturesTool.DEFAULT_QUOTA_GENERAL); } + + private void assertImageNotNullAndStatus(Guid imageId,ImageStatus status) { + Image imageFromDb = dao.get(imageId); + assertNotNull(imageFromDb); + assertEquals(status, imageFromDb.getStatus()); + } } -- To view, visit http://gerrit.ovirt.org/14569 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I7a3e497e52471f7255e6c17b59a2e3bf362b8783 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Liron Ar <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
