Arik Hadas has uploaded a new change for review. Change subject: core: acquire locks in standard way in MoveOrCopyDiskCommand ......................................................................
core: acquire locks in standard way in MoveOrCopyDiskCommand In order to solve the race between VM migration and storage commands, we need the storage commands to be able to acquire VM lock for their entire execution process. That thing cannot be done when the locks are acquired using explicit invocation of acquireLockInternal method, we need the LockIdAndNameAttribute annotation. Thus, this patch converts the way MoveOrCopyDiskCommand acquire its locks such that it will be in the standard way. Change-Id: Id4b5ef206581e5f9a1727c85b30f39802859583c Bug-Url: https://bugzilla.redhat.com/952147 Signed-off-by: Arik Hadas <aha...@redhat.com> --- M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyDiskCommand.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/lsm/LiveMigrateDiskCommand.java M backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/MoveOrCopyDiskCommandTest.java 3 files changed, 51 insertions(+), 41 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/23/18523/1 diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyDiskCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyDiskCommand.java index 8dd0a2b..573e00c 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyDiskCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyDiskCommand.java @@ -39,15 +39,31 @@ @DisableInPrepareMode @NonTransactiveCommandAttribute +@LockIdNameAttribute public class MoveOrCopyDiskCommand<T extends MoveOrCopyImageGroupParameters> extends CopyImageGroupCommand<T> implements QuotaStorageDependent { - private Map<String, Pair<String, String>> sharedLockMap; private List<PermissionSubject> cachedPermsList; private List<VM> cachedListVms; public MoveOrCopyDiskCommand(T parameters) { super(parameters); + + defineVmTemplate(); + } + + protected void defineVmTemplate() { + if (getParameters().getOperation() == ImageOperation.Copy) { + setVmTemplate(getTemplateForImage()); + } + } + + protected VmTemplate getTemplateForImage() { + if (getImage() == null) { + return null; + } + Collection<VmTemplate> templates = getVmTemplateDAO().getAllForImage(getImage().getId()).values(); + return !templates.isEmpty() ? templates.iterator().next() : null; } @Override @@ -63,7 +79,6 @@ return isImageExist() && checkOperationIsCorrect() && canFindVmOrTemplate() - && acquireLockInternal() && isImageNotLocked() && isSourceAndDestTheSame() && validateSourceStorageDomain() @@ -341,41 +356,39 @@ */ private boolean canFindVmOrTemplate() { if (getParameters().getOperation() == ImageOperation.Copy) { - Collection<VmTemplate> templates = getVmTemplateDAO().getAllForImage(getImage().getImageId()).values(); - if (templates.isEmpty()) { + if (getVmTemplate() == null) { return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_TEMPLATE_DOES_NOT_EXIST); - } - VmTemplate vmTemplate = templates.iterator().next(); - setVmTemplate(vmTemplate); - sharedLockMap = Collections.singletonMap(vmTemplate.getId().toString(), - LockMessagesMatchUtil.makeLockingPair(LockingGroup.TEMPLATE, VdcBllMessages.ACTION_TYPE_FAILED_OBJECT_LOCKED)); - } else { - List<VM> vmsForDisk = getVmsForDiskId(); - if (!vmsForDisk.isEmpty()) { - Map<String, Pair<String, String>> lockMap = new HashMap<String, Pair<String, String>>(); - for (VM currVm : vmsForDisk) { - lockMap.put(currVm.getId().toString(), - LockMessagesMatchUtil.makeLockingPair(LockingGroup.VM, VdcBllMessages.ACTION_TYPE_FAILED_OBJECT_LOCKED)); - } - lockForMove(lockMap); } } return true; } - protected void lockForMove(Map<String, Pair<String, String>> lockMap) { - sharedLockMap = lockMap; + @Override + protected Map<String, Pair<String, String>> getSharedLocks() { + if (getParameters().getOperation() == ImageOperation.Copy) { + if (!Guid.Empty.equals(getVmTemplateId())) { + return Collections.singletonMap(getVmTemplateId().toString(), + LockMessagesMatchUtil.makeLockingPair(LockingGroup.TEMPLATE, VdcBllMessages.ACTION_TYPE_FAILED_OBJECT_LOCKED)); + } + } else { + List<VM> vmsForDisk = getVmsForDiskId(); + if (!vmsForDisk.isEmpty()) { + Map<String, Pair<String, String>> lockMap = new HashMap<>(); + for (VM currVm : vmsForDisk) { + lockMap.put(currVm.getId().toString(), + LockMessagesMatchUtil.makeLockingPair(LockingGroup.VM, VdcBllMessages.ACTION_TYPE_FAILED_OBJECT_LOCKED)); + } + return lockMap; + } + } + return null; } @Override protected Map<String, Pair<String, String>> getExclusiveLocks() { - return Collections.singletonMap(getImage().getId().toString(), + return Collections.singletonMap( + (getImage() != null ? getImage().getId() : Guid.Empty).toString(), LockMessagesMatchUtil.makeLockingPair(LockingGroup.DISK, VdcBllMessages.ACTION_TYPE_FAILED_OBJECT_LOCKED)); - } - - @Override - protected Map<String, Pair<String, String>> getSharedLocks() { - return sharedLockMap; } public String getDiskAlias() { diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/lsm/LiveMigrateDiskCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/lsm/LiveMigrateDiskCommand.java index a2a8e26..49ca9bc 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/lsm/LiveMigrateDiskCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/lsm/LiveMigrateDiskCommand.java @@ -5,6 +5,7 @@ import java.util.List; import org.ovirt.engine.core.bll.ImagesHandler; +import org.ovirt.engine.core.bll.LockIdNameAttribute; import org.ovirt.engine.core.bll.MoveOrCopyDiskCommand; import org.ovirt.engine.core.bll.NonTransactiveCommandAttribute; import org.ovirt.engine.core.bll.job.ExecutionHandler; @@ -21,6 +22,7 @@ import org.ovirt.engine.core.dal.dbbroker.auditloghandling.AuditLogDirector; @NonTransactiveCommandAttribute +@LockIdNameAttribute public class LiveMigrateDiskCommand<T extends LiveMigrateDiskParameters> extends MoveOrCopyDiskCommand<T> implements TaskHandlerCommand<LiveMigrateDiskParameters> { public LiveMigrateDiskCommand(T parameters) { diff --git a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/MoveOrCopyDiskCommandTest.java b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/MoveOrCopyDiskCommandTest.java index 2b17e28..2a52171 100644 --- a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/MoveOrCopyDiskCommandTest.java +++ b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/MoveOrCopyDiskCommandTest.java @@ -11,7 +11,6 @@ import java.util.ArrayList; import java.util.Collections; import java.util.List; -import java.util.Map; import org.junit.ClassRule; import org.junit.Test; @@ -38,7 +37,6 @@ import org.ovirt.engine.core.dao.StorageDomainDAO; import org.ovirt.engine.core.dao.VmDAO; import org.ovirt.engine.core.dao.VmDeviceDAO; -import org.ovirt.engine.core.dao.VmTemplateDAO; import org.ovirt.engine.core.utils.MockConfigRule; @RunWith(MockitoJUnitRunner.class) @@ -59,8 +57,6 @@ private StorageDomainDAO storageDomainDao; @Mock private VmDAO vmDao; - @Mock - private VmTemplateDAO vmTemplateDao; @Mock private VmDeviceDAO vmDeviceDao; @@ -94,7 +90,8 @@ public void canDoActionWrongDiskImageTypeVm() throws Exception { initializeCommand(ImageOperation.Copy); initVmDiskImage(); - doReturn(vmTemplateDao).when(command).getVmTemplateDAO(); + doReturn(null).when(command).getTemplateForImage(); + command.defineVmTemplate(); assertFalse(command.canDoAction()); assertTrue(command.getReturnValue() .getCanDoActionMessages() @@ -105,8 +102,8 @@ public void canDoActionCanNotFindTemplet() throws Exception { initializeCommand(ImageOperation.Copy); initTemplateDiskImage(); - doReturn(vmTemplateDao).when(command).getVmTemplateDAO(); - when(vmTemplateDao.get(any(Guid.class))).thenReturn(null); + doReturn(null).when(command).getTemplateForImage(); + command.defineVmTemplate(); assertFalse(command.canDoAction()); assertTrue(command.getReturnValue() .getCanDoActionMessages() @@ -158,10 +155,9 @@ initializeCommand(ImageOperation.Copy); initTemplateDiskImage(); command.getImage().setImageStatus(ImageStatus.LOCKED); - doReturn(vmTemplateDao).when(command).getVmTemplateDAO(); + doReturn(new VmTemplate()).when(command).getTemplateForImage(); - Map<Boolean, VmTemplate> map = Collections.singletonMap(true, new VmTemplate()); - doReturn(map).when(vmTemplateDao).getAllForImage(any(Guid.class)); + command.defineVmTemplate(); assertFalse(command.canDoAction()); assertTrue(command.getReturnValue().getCanDoActionMessages().contains( VdcBllMessages.VM_TEMPLATE_IMAGE_IS_LOCKED.toString())); @@ -245,14 +241,13 @@ } @Override - protected DiskImageDAO getDiskImageDao() { - return diskImageDao; + protected VmTemplate getTemplateForImage() { + return null; } @Override - protected boolean acquireLockInternal() { - return true; + protected DiskImageDAO getDiskImageDao() { + return diskImageDao; } - } } -- To view, visit http://gerrit.ovirt.org/18523 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Id4b5ef206581e5f9a1727c85b30f39802859583c Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Arik Hadas <aha...@redhat.com> _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
