Michael Kublin has posted comments on this change.
Change subject: core: add locks to VmTemplate commands (#761515)
......................................................................
Patch Set 6: I would prefer that you didn't submit this
(9 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java
Line 289: if (getVmTemplate() == null) {
Line 290:
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_TEMPLATE_DOES_NOT_EXIST);
Line 291: return false;
Line 292: }
Line 293:
These check is already done, I already point on method where is done. These is
a new design pattern double check?
Line 294: if (!verifyVmTemplateStatusForUse(getVmTemplateId())) {
Line 295: getReturnValue().getCanDoActionMessages()
Line 296:
.add(VdcBllMessages.VM_TEMPLATE_IMAGE_IS_LOCKED.toString());
Line 297: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromTemplateCommand.java
Line 35: return true;
Line 36: }
Line 37:
Line 38: @Override
Line 39: protected Map<String, String> getExclusiveLocks() {
These lock should be shared. Like I said we can make couple of vms from the
same template simultaneously.
Line 40: Map<String, String> locks = new HashMap<String, String>();
Line 41: Map<String, String> parentLocks = super.getExclusiveLocks();
Line 42: if (parentLocks != null) {
Line 43: locks.putAll(parentLocks);
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ExportVmTemplateCommand.java
Line 84: protected boolean canDoAction() {
Line 85: if (getVmTemplate() != null) {
Line 86: setDescription(getVmTemplateName());
Line 87: }
Line 88:
These check is already done. Code duplication
Line 89: if
(!VmTemplateHandler.isTemplateStatusIsNotLocked(getVmTemplateId())) {
Line 90: getReturnValue().getCanDoActionMessages()
Line 91:
.add(VdcBllMessages.VM_TEMPLATE_IMAGE_IS_LOCKED.toString());
Line 92: }
Line 87: }
Line 88:
Line 89: if
(!VmTemplateHandler.isTemplateStatusIsNotLocked(getVmTemplateId())) {
Line 90: getReturnValue().getCanDoActionMessages()
Line 91:
.add(VdcBllMessages.VM_TEMPLATE_IMAGE_IS_LOCKED.toString());
By the way u don't saw that there addCanDoActionMessage() method?
Line 92: }
Line 93:
Line 94: StorageDomainValidator storageDomainValidator = new
StorageDomainValidator(getStorageDomain());
Line 95: boolean retVal =
storageDomainValidator.isDomainExistAndActive(getReturnValue().getCanDoActionMessages());
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmTemplateCommand.java
Line 90: retVal = false;
Line 91: } else {
Line 92: setDescription(getVmTemplateName());
Line 93: }
Line 94:
Please explain what is a reason for that check? Template is not in our DB,
because of it located on import/export domain. Also why lock it and by
exclusive lock?
Line 95: if (retVal &&
!VmTemplateHandler.isTemplateStatusIsNotLocked(getVmTemplateId())) {
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:
These check is already done and it is useless.
Line 74: if
(!VmTemplateHandler.isTemplateStatusIsNotLocked(getVmTemplateId())) {
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
freeLock(); Add these, I think u don't understand a reason, so a more simple
thsese is design of the locking mechanism feature.
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:
Great, we are checking a status of template that is not located at our DB. What
for?
Did you try to run these code?
Line 55: if (retVal &&
!VmTemplateHandler.isTemplateStatusIsNotLocked(getVmTemplateId())) {
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 177: protected String getDescription() {
Line 178: return getVmTemplateName();
Line 179: }
Line 180:
Line 181:
And these for what?
Line 182: /**
Line 183: * This method create OVF for each template in list and call
updateVm in SPM
Line 184: *
Line 185: * @param storagePoolId
--
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: 6
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Liron Aravot <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Liron Aravot <[email protected]>
Gerrit-Reviewer: Michael Kublin <[email protected]>
Gerrit-Reviewer: Tal Nisan <[email protected]>
Gerrit-Reviewer: Vered Volansky <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
