Michael Kublin has posted comments on this change.
Change subject: core: add locks to VmTemplate commands (#761515)
......................................................................
Patch Set 3: (12 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:
Code duplication
Line 294: if (!verifyVmTemplateStatusForUse(getVmTemplateId(),
Boolean.TRUE)) {
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 36: }
Line 37:
Line 38: @Override
Line 39: protected Map<String, String> getExclusiveLocks() {
Line 40: Map<String, String> locks = new HashMap<String, String>();
super.getExclusiveLocks() - can be null
map.putAll(null) throws NullPointerException
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();
You should lock a template in DB before super.ExecuteVmCommand();
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:
Missing freeLock(), if status of template changed in DB you should release in
memory lock
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:
These check is useless, it is already done in 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
what is a reason for that method?
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:
If template in export domain how that will help? A template not in DB
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:
These check is useless, it is already done in canDoAction()
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
freeLock()
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:
The template is not located at our DB, how that should help?
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:
such method is code duplication
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: */
such method is code duplication
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 <[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
