Liron Aravot has posted comments on this change.
Change subject: core: add locks to VmTemplate commands (#761515)
......................................................................
Patch Set 3: (11 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromTemplateCommand.java
Line 36: }
Line 37:
Line 38: @Override
Line 39: protected Map<String, String> getExclusiveLocks() {
Line 40: Map<String, String> locks = new HashMap<String, String>();
Done, although i think that it can't actually be null as we can't create vm
without name
Line 41: locks.putAll(super.getExclusiveLocks());
Line 42: locks.put(getVmTemplateId().toString(),
LockingGroup.TEMPLATE.name());
Line 43: return locks;
Line 44: }
Line 46: @Override
Line 47: protected void ExecuteVmCommand() {
Line 48: super.ExecuteVmCommand();
Line 49: // override template id to blank
Line 50: getParameters().OriginalTemplate = getVm().getvmt_guid();
I dont think that it's necessarily make a difference as the template is locked
in the memory, the db lock is needed while the async tasks are executed - we
will fail on acquireLock on the other commands that will try to take the lock.
Line 51:
VmTemplateHandler.lockVmTemplateInTransaction(getParameters().OriginalTemplate,
getCompensationContext());
Line 52: getVm().setvmt_guid(VmTemplateHandler.BlankVmTemplateId);
Line 53:
getVm().getStaticData().setQuotaId(getParameters().getVmStaticData().getQuotaId());
Line 54:
DbFacade.getInstance().getVmStaticDAO().update(getVm().getStaticData());
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ExportVmTemplateCommand.java
Line 73: return null;
Line 74: }
Line 75: });
Line 76: }
Line 77:
it will be freed when the command execution is done (Execute()), why to release
it when i can still use it and prevent DB calls? that way we will fail even
before the can do action and won't perform unneeded db query.
Line 78: @Override
Line 79: protected Map<String, String> getExclusiveLocks() {
Line 80: return Collections.singletonMap(getVmTemplateId().toString(),
LockingGroup.TEMPLATE.name());
Line 81: }
Line 84: protected boolean canDoAction() {
Line 85: if (getVmTemplate() != null) {
Line 86: setDescription(getVmTemplateName());
Line 87: }
Line 88:
this is the canDoAction method, can you elaborate?
Line 89: if
(!VmTemplateHandler.verifyVmTemplateStatusForUse(getVmTemplateId(), false)) {
Line 90: getReturnValue().getCanDoActionMessages()
Line 91:
.add(VdcBllMessages.VM_TEMPLATE_IMAGE_IS_LOCKED.toString());
Line 92: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmTemplateCommand.java
Line 77: protected ImportVmTemplateCommand(Guid commandId) {
Line 78: super(commandId);
Line 79: }
Line 80:
Line 81: @Override
so we can't import the template and on the same time delete it for example.
Line 82: protected Map<String, String> getExclusiveLocks() {
Line 83: return Collections.singletonMap(getVmTemplateId().toString(),
LockingGroup.TEMPLATE.name());
Line 84: }
Line 85:
Line 90: retVal = false;
Line 91: } else {
Line 92: setDescription(getVmTemplateName());
Line 93: }
Line 94:
not following,what do you mean by not in DB? can you please elaborate?
Line 95: if (retVal &&
!VmTemplateHandler.verifyVmTemplateStatusForUse(getVmTemplateId(), false)) {
Line 96: getReturnValue().getCanDoActionMessages()
Line 97:
.add(VdcBllMessages.VM_TEMPLATE_IMAGE_IS_LOCKED.toString());
Line 98: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmTemplateCommand.java
Line 69: if (VmTemplateHandler.BlankVmTemplateId.equals(vmTemplateId)) {
Line 70:
addCanDoActionMessage(VdcBllMessages.VMT_CANNOT_REMOVE_BLANK_TEMPLATE);
Line 71: return false;
Line 72: }
Line 73:
this is the candoaction method, can you please elaborate?
Line 74: if
(!VmTemplateHandler.verifyVmTemplateStatusForUse(getVmTemplateId(), false)) {
Line 75: getReturnValue().getCanDoActionMessages()
Line 76:
.add(VdcBllMessages.VM_TEMPLATE_IMAGE_IS_LOCKED.toString());
Line 77: }
Line 185: @Override
Line 186: protected void executeCommand() {
Line 187: // Set VM to lock status immediately, for reducing race
condition.
Line 188:
VmTemplateHandler.lockVmTemplateInTransaction(getVmTemplateId(),
getCompensationContext());
Line 189: // if for some reason template doesn't have images, remove it
now and not in end action
same comment as in preview class, why free the lock now and now when the
executeCommand end (as it's implemented) and prevent DB query?
Line 190: final boolean hasImages = imageTemplates.size() > 0;
Line 191: if
(RemoveTemplateInSpm(getVmTemplate().getstorage_pool_id().getValue(),
getVmTemplateId())) {
Line 192: if (hasImages) {
Line 193: TransactionSupport.executeInNewTransaction(new
TransactionMethod<Void>() {
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmTemplateFromImportExportCommand.java
Line 50: if (getVmTemplate() == null) {
Line 51: retVal = false;
Line 52:
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_TEMPLATE_DOES_NOT_EXIST);
Line 53: }
Line 54:
so that it cannot be removed during import of it.
Line 55: if (retVal &&
!VmTemplateHandler.verifyVmTemplateStatusForUse(getVmTemplateId(), false)) {
Line 56: getReturnValue().getCanDoActionMessages()
Line 57:
.add(VdcBllMessages.VM_TEMPLATE_IMAGE_IS_LOCKED.toString());
Line 58: retVal = false;
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmTemplateCommand.java
Line 176: @Override
Line 177: protected String getDescription() {
Line 178: return getVmTemplateName();
Line 179: }
Line 180:
Done
Line 181: protected boolean verifyVmTemplateStatusForUse(Guid templateId,
boolean checkIfBlankTemplate) {
Line 182: return
VmTemplateHandler.verifyVmTemplateStatusForUse(templateId,
checkIfBlankTemplate);
Line 183: }
Line 184:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmTemplateHandler.java
Line 127: * Returns boolean indicating if the template with the given
templateId is free for use.
Line 128: * @param templateId
Line 129: * @param checkIfBlankTemplate
Line 130: * @return
Line 131: */
Done
Line 132: public static boolean verifyVmTemplateStatusForUse(Guid
templateId, boolean checkIfBlankTemplate) {
Line 133: return !((checkIfBlankTemplate ||
!VmTemplateHandler.BlankVmTemplateId.equals(templateId))
Line 134: &&
!VmTemplateHandler.isTemplateStatusIsNotLocked(templateId));
Line 135: }
--
To view, visit http://gerrit.ovirt.org/8013
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d5b6518bbf19077ae5822b9359a6147c87d0f46
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Liron Aravot <lara...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Ayal Baron <aba...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Liron Aravot <lara...@redhat.com>
Gerrit-Reviewer: Michael Kublin <mkub...@redhat.com>
Gerrit-Reviewer: Tal Nisan <tni...@redhat.com>
Gerrit-Reviewer: Vered Volansky <vvola...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
