Michael Kublin has posted comments on this change.

Change subject: core: add locks to VmTemplate commands (#761515)
......................................................................


Patch Set 3: (6 inline comments)

....................................................
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: 
Because of when the entity is updated in DB we don't need to keep a lock, so it 
should be freed in order to have a correct canDoactionMessage.
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: 
If you will check where is used VdcBllMessages.VM_TEMPLATE_IMAGE_IS_LOCKED, you 
can see that is called from  VmTemplateCommand.isVmTemplateImagesReady(), and 
these is called at canDoAction()
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
And how locking mechanism will know that it should run? Also, why I will lock a 
template that is located at export domain, and not located at my DB? Also why 
lock is exclusive I can also import template and clone it.
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: 
Code duplication,  VmTemplateCommand.isVmTemplateImagesReady
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 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
because of status of object is updated in DB, no need for lock. Idea of lock is 
a prevent a race and not make code running in sequence.
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: 
It is not located at our DB, what you are checking?
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;


--
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

Reply via email to