Liron Aravot has uploaded a new change for review. Change subject: core: add locks to VmTemplate commands (#761515) ......................................................................
core: add locks to VmTemplate commands (#761515) https://bugzilla.redhat.com/761515 Currently the different VmTemplate commands make use of DB lock only, which causes to different race conditions (as the lock is check and only then acquired - non atomic operation). This patch adds to the different VmTemplate commands the use of in-memory lock and canDoAction() checks in order to prevent those race conditions and possible corruption. Change-Id: I2d5b6518bbf19077ae5822b9359a6147c87d0f46 Signed-off-by: Liron Aravot <lara...@redhat.com> --- M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromTemplateCommand.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ExportVmTemplateCommand.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmTemplateCommand.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyTemplateCommand.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmTemplateCommand.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmTemplateFromImportExportCommand.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmTemplateCommand.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmTemplateHandler.java M backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddVmCommandTest.java 10 files changed, 111 insertions(+), 26 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/13/8013/1 diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java index eb81bc6..dd113f5 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java @@ -279,6 +279,10 @@ addCanDoActionMessage(VdcBllMessages.VAR__TYPE__VM); } + protected boolean verifyVmTemplateStatusForUse(Guid templateId, Boolean checkIfBlankTemplate) { + return VmTemplateHandler.verifyVmTemplateStatusForUse(templateId, checkIfBlankTemplate); + } + @Override protected boolean canDoAction() { boolean returnValue = true; @@ -286,6 +290,12 @@ addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_TEMPLATE_DOES_NOT_EXIST); return false; } + + if (!verifyVmTemplateStatusForUse(getVmTemplateId(), Boolean.TRUE)) { + getReturnValue().getCanDoActionMessages() + .add(VdcBllMessages.VM_TEMPLATE_IMAGE_IS_LOCKED.toString()); + } + returnValue = buildAndCheckDestStorageDomains(); if (returnValue) { storageToDisksMap = diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromTemplateCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromTemplateCommand.java index 4faa6ea..b08d5a0 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromTemplateCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromTemplateCommand.java @@ -1,6 +1,8 @@ package org.ovirt.engine.core.bll; +import java.util.HashMap; import java.util.List; +import java.util.Map; import org.ovirt.engine.core.bll.job.ExecutionHandler; import org.ovirt.engine.core.common.action.AddVmFromTemplateParameters; @@ -12,6 +14,7 @@ import org.ovirt.engine.core.common.businessentities.VMStatus; import org.ovirt.engine.core.common.errors.VdcBLLException; import org.ovirt.engine.core.common.errors.VdcBllErrors; +import org.ovirt.engine.core.common.locks.LockingGroup; import org.ovirt.engine.core.compat.Guid; import org.ovirt.engine.core.dal.dbbroker.DbFacade; @@ -33,6 +36,14 @@ } @Override + protected Map<String, String> getExclusiveLocks() { + Map<String, String> locks = new HashMap<String, String>(); + locks.putAll(super.getExclusiveLocks()); + locks.put(getVmTemplateId().toString(), LockingGroup.TEMPLATE.name()); + return locks; + } + + @Override protected void ExecuteVmCommand() { super.ExecuteVmCommand(); // override template id to blank diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ExportVmTemplateCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ExportVmTemplateCommand.java index ede9c86..d94dc2f 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ExportVmTemplateCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ExportVmTemplateCommand.java @@ -1,6 +1,7 @@ package org.ovirt.engine.core.bll; import java.util.Arrays; +import java.util.Collections; import java.util.Map; import org.ovirt.engine.core.bll.job.ExecutionHandler; @@ -15,11 +16,13 @@ import org.ovirt.engine.core.common.businessentities.DiskImage; import org.ovirt.engine.core.common.businessentities.StorageDomainType; import org.ovirt.engine.core.common.errors.VdcBLLException; +import org.ovirt.engine.core.common.locks.LockingGroup; import org.ovirt.engine.core.compat.Guid; import org.ovirt.engine.core.dal.VdcBllMessages; import org.ovirt.engine.core.utils.transaction.TransactionMethod; import org.ovirt.engine.core.utils.transaction.TransactionSupport; +@LockIdNameAttribute public class ExportVmTemplateCommand<T extends MoveOrCopyParameters> extends MoveOrCopyTemplateCommand<T> { public ExportVmTemplateCommand(T parameters) { @@ -73,10 +76,21 @@ } @Override + protected Map<String, String> getExclusiveLocks() { + return Collections.singletonMap(getVmTemplateId().toString(), LockingGroup.TEMPLATE.name()); + } + + @Override protected boolean canDoAction() { if (getVmTemplate() != null) { setDescription(getVmTemplateName()); } + + if (!VmTemplateHandler.verifyVmTemplateStatusForUse(getVmTemplateId(), false)) { + getReturnValue().getCanDoActionMessages() + .add(VdcBllMessages.VM_TEMPLATE_IMAGE_IS_LOCKED.toString()); + } + StorageDomainValidator storageDomainValidator = new StorageDomainValidator(getStorageDomain()); boolean retVal = storageDomainValidator.isDomainExistAndActive(getReturnValue().getCanDoActionMessages()); diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmTemplateCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmTemplateCommand.java index 05eed07..043dad2 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmTemplateCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmTemplateCommand.java @@ -2,6 +2,7 @@ import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -42,6 +43,7 @@ import org.ovirt.engine.core.common.businessentities.storage_domains; import org.ovirt.engine.core.common.errors.VdcBLLException; import org.ovirt.engine.core.common.errors.VdcBllErrors; +import org.ovirt.engine.core.common.locks.LockingGroup; import org.ovirt.engine.core.common.queries.DiskImageList; import org.ovirt.engine.core.common.queries.GetAllFromExportDomainQueryParameters; import org.ovirt.engine.core.common.queries.VdcQueryReturnValue; @@ -77,6 +79,11 @@ } @Override + protected Map<String, String> getExclusiveLocks() { + return Collections.singletonMap(getVmTemplateId().toString(), LockingGroup.TEMPLATE.name()); + } + + @Override protected boolean canDoAction() { boolean retVal = true; if (getVmTemplate() == null) { @@ -84,6 +91,11 @@ } else { setDescription(getVmTemplateName()); } + + if (retVal && !VmTemplateHandler.verifyVmTemplateStatusForUse(getVmTemplateId(), false)) { + getReturnValue().getCanDoActionMessages() + .add(VdcBllMessages.VM_TEMPLATE_IMAGE_IS_LOCKED.toString()); + } // check that the storage pool is valid retVal = retVal && checkStoragePool(); diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyTemplateCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyTemplateCommand.java index 5be693f..7ac4cd6 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyTemplateCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyTemplateCommand.java @@ -156,15 +156,13 @@ @Override protected void executeCommand() { - if (VmTemplateHandler.isTemplateStatusIsNotLocked(getVmTemplateId())) { - VmTemplateHandler.lockVmTemplateInTransaction(getVmTemplateId(), getCompensationContext()); - if (!getTemplateDisks().isEmpty()) { - MoveOrCopyAllImageGroups(); - } else { - endVmTemplateRelatedOps(); - } - setSucceeded(true); + VmTemplateHandler.lockVmTemplateInTransaction(getVmTemplateId(), getCompensationContext()); + if (!getTemplateDisks().isEmpty()) { + MoveOrCopyAllImageGroups(); + } else { + endVmTemplateRelatedOps(); } + setSucceeded(true); } protected void MoveOrCopyAllImageGroups() { @@ -219,6 +217,10 @@ } } + protected boolean verifyVmTemplateStatusForUse(Guid templateId, boolean checkIfBlankTemplate) { + return VmTemplateHandler.verifyVmTemplateStatusForUse(templateId, checkIfBlankTemplate); + } + protected boolean checkIfDisksExist(Iterable<DiskImage> disksList) { for (DiskImage disk : disksList) { VDSReturnValue runVdsCommand = getBackend() diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmTemplateCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmTemplateCommand.java index 02ff06e..57f0c70 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmTemplateCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmTemplateCommand.java @@ -1,6 +1,7 @@ package org.ovirt.engine.core.bll; import java.util.ArrayList; +import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -19,6 +20,7 @@ import org.ovirt.engine.core.common.businessentities.VM; import org.ovirt.engine.core.common.businessentities.VmTemplate; import org.ovirt.engine.core.common.businessentities.storage_domain_static; +import org.ovirt.engine.core.common.locks.LockingGroup; import org.ovirt.engine.core.common.vdscommands.IrsBaseVDSCommandParameters; import org.ovirt.engine.core.common.vdscommands.VDSCommandType; import org.ovirt.engine.core.compat.Guid; @@ -29,6 +31,7 @@ import org.ovirt.engine.core.utils.transaction.TransactionSupport; @NonTransactiveCommandAttribute(forceCompensation = true) +@LockIdNameAttribute public class RemoveVmTemplateCommand<T extends VmTemplateParametersBase> extends VmTemplateCommand<T> implements Quotable { @@ -66,6 +69,11 @@ if (VmTemplateHandler.BlankVmTemplateId.equals(vmTemplateId)) { addCanDoActionMessage(VdcBllMessages.VMT_CANNOT_REMOVE_BLANK_TEMPLATE); return false; + } + + if (!VmTemplateHandler.verifyVmTemplateStatusForUse(getVmTemplateId(), false)) { + getReturnValue().getCanDoActionMessages() + .add(VdcBllMessages.VM_TEMPLATE_IMAGE_IS_LOCKED.toString()); } // check storage pool valid @@ -176,28 +184,32 @@ @Override protected void executeCommand() { - if (VmTemplateHandler.isTemplateStatusIsNotLocked(getVmTemplateId())) { - // Set VM to lock status immediately, for reducing race condition. - VmTemplateHandler.lockVmTemplateInTransaction(getVmTemplateId(), getCompensationContext()); - // if for some reason template doesn't have images, remove it now and not in end action - final boolean hasImages = imageTemplates.size() > 0; - if (RemoveTemplateInSpm(getVmTemplate().getstorage_pool_id().getValue(), getVmTemplateId())) { - if (hasImages) { - TransactionSupport.executeInNewTransaction(new TransactionMethod<Void>() { + // Set VM to lock status immediately, for reducing race condition. + VmTemplateHandler.lockVmTemplateInTransaction(getVmTemplateId(), getCompensationContext()); + // if for some reason template doesn't have images, remove it now and not in end action + final boolean hasImages = imageTemplates.size() > 0; + if (RemoveTemplateInSpm(getVmTemplate().getstorage_pool_id().getValue(), getVmTemplateId())) { + if (hasImages) { + TransactionSupport.executeInNewTransaction(new TransactionMethod<Void>() { - @Override - public Void runInTransaction() { - if (RemoveVmTemplateImages()) { - setSucceeded(true); - } - return null; + @Override + public Void runInTransaction() { + if (RemoveVmTemplateImages()) { + setSucceeded(true); } - }); - } else { - HandleEndAction(); - } + return null; + } + }); + } else { + HandleEndAction(); } } + + } + + @Override + protected Map<String, String> getExclusiveLocks() { + return Collections.singletonMap(getVmTemplateId().toString(), LockingGroup.TEMPLATE.name()); } private void RemoveTemplateFromDb() { diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmTemplateFromImportExportCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmTemplateFromImportExportCommand.java index 95be26f..63139be 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmTemplateFromImportExportCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmTemplateFromImportExportCommand.java @@ -31,6 +31,7 @@ import org.ovirt.engine.core.utils.linq.Predicate; @NonTransactiveCommandAttribute +@LockIdNameAttribute public class RemoveVmTemplateFromImportExportCommand<T extends VmTemplateImportExportParameters> extends RemoveVmTemplateCommand<T> { @@ -50,6 +51,13 @@ retVal = false; addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_TEMPLATE_DOES_NOT_EXIST); } + + if (retVal && !VmTemplateHandler.verifyVmTemplateStatusForUse(getVmTemplateId(), false)) { + getReturnValue().getCanDoActionMessages() + .add(VdcBllMessages.VM_TEMPLATE_IMAGE_IS_LOCKED.toString()); + retVal = false; + } + if (retVal) { DiskImageList images = templatesFromExport.get(LinqUtils.firstOrNull(templatesFromExport.keySet(), new Predicate<VmTemplate>() { diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmTemplateCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmTemplateCommand.java index ad78350..99ccd75 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmTemplateCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmTemplateCommand.java @@ -178,6 +178,10 @@ return getVmTemplateName(); } + protected boolean verifyVmTemplateStatusForUse(Guid templateId, boolean checkIfBlankTemplate) { + return VmTemplateHandler.verifyVmTemplateStatusForUse(templateId, checkIfBlankTemplate); + } + /** * This method create OVF for each template in list and call updateVm in SPM * diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmTemplateHandler.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmTemplateHandler.java index 4e0f1c0..4876b74 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmTemplateHandler.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmTemplateHandler.java @@ -123,5 +123,16 @@ } } + /** + * Returns boolean indicating if the template with the given templateId is free for use. + * @param templateId + * @param checkIfBlankTemplate + * @return + */ + public static boolean verifyVmTemplateStatusForUse(Guid templateId, boolean checkIfBlankTemplate) { + return !((checkIfBlankTemplate || !VmTemplateHandler.BlankVmTemplateId.equals(templateId)) + && !VmTemplateHandler.isTemplateStatusIsNotLocked(templateId)); + } + private static Log log = LogFactory.getLog(VmTemplateHandler.class); } diff --git a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddVmCommandTest.java b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddVmCommandTest.java index 7be5659..e28ab00 100644 --- a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddVmCommandTest.java +++ b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddVmCommandTest.java @@ -469,6 +469,7 @@ private static void mockVerifyAddVM(AddVmCommand<?> cmd) { doReturn(true).when(cmd).verifyAddVM(anyListOf(String.class), any(Guid.class), anyInt()); + doReturn(true).when(cmd).verifyVmTemplateStatusForUse(any(Guid.class), any(Boolean.class)); } private void mockConfig() { -- To view, visit http://gerrit.ovirt.org/8013 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I2d5b6518bbf19077ae5822b9359a6147c87d0f46 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Liron Aravot <lara...@redhat.com> _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
