Arik Hadas has posted comments on this change. Change subject: core: clone VM ......................................................................
Patch Set 7: (5 comments) http://gerrit.ovirt.org/#/c/23805/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmAndCloneImageCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmAndCloneImageCommand.java: Line 489: unlockEntities it looks like this method and lockEntities are not used right? if so it should be removed http://gerrit.ovirt.org/#/c/23805/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromSnapshotCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromSnapshotCommand.java: Line 200: } Line 201: Line 202: Map<String, Pair<String, String>> union = new HashMap<>(); Line 203: union.putAll(parentLocks); Line 204: union.putAll(thisLocks); isn't it better to just add the 'thisLocks' to 'parentLocks' map in a similar way it is done for the permission-check-subjects? Line 205: Line 206: return union; Line 207: } Line 208: http://gerrit.ovirt.org/#/c/23805/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CloneVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CloneVmCommand.java: Line 75: LockMessagesMatchUtil.makeLockingPair(LockingGroup.DISK, getDiskSharedLockMessage())); Line 76: } Line 77: Line 78: locks.put(getSourceVmFromDb().getId().toString(), Line 79: LockMessagesMatchUtil.makeLockingPair(LockingGroup.VM, VdcBllMessages.ACTION_TYPE_FAILED_OBJECT_LOCKED)); we shouldn't use this deprecated message anymore especially for locks which are taken for a long time, please replace it with a message that explains the VM is being cloned Line 80: Line 81: return locks; Line 82: } Line 83: Line 97: } Line 98: return diskImagesFromConfiguration; Line 99: } Line 100: Line 101: public Map<String, String> getJobMessageProperties() { missing @overrides Line 102: if (jobProperties == null) { Line 103: jobProperties = super.getJobMessageProperties(); Line 104: jobProperties.put(VdcObjectType.VM.name().toLowerCase(), Line 105: StringUtils.defaultString(getParameters().getNewName())); http://gerrit.ovirt.org/#/c/23805/7/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/VdcActionUtils.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/VdcActionUtils.java: Line 148: VdcActionType.AddVmInterface, VdcActionType.UpdateVmInterface, Line 149: VdcActionType.RemoveVmInterface, VdcActionType.CreateAllSnapshotsFromVm, Line 150: VdcActionType.ExtendImageSize, VdcActionType.RebootVm)); Line 151: vmMatrix.put( Line 152: VMStatus.Suspended, on second thought, it is not that good to enable the clone VM operation for suspended VM because we need to think about the status of the cloned VM: 1. if it created as DOWN, we'll need to remove its hibernation volumes and it might confuse user as they will expect it to be the same as the source (i.e suspended) 2. if it is created as suspended we'll need to clone the hibernation volumes as well - too complicated sorry about the previous comment about it.. Line 153: EnumSet.of(VdcActionType.HibernateVm, VdcActionType.AddVmTemplate, Line 154: VdcActionType.RunVmOnce, VdcActionType.MigrateVm, VdcActionType.ExportVm, VdcActionType.MoveVm, Line 155: VdcActionType.ImportVm, VdcActionType.ChangeDisk, VdcActionType.RemoveVm, Line 156: VdcActionType.AddVmInterface, VdcActionType.UpdateVmInterface, -- To view, visit http://gerrit.ovirt.org/23805 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3038638314e399e9d2390e7204ce4cc00e85e6ae Gerrit-PatchSet: 7 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Tomas Jelinek <[email protected]> Gerrit-Reviewer: Arik Hadas <[email protected]> Gerrit-Reviewer: Omer Frenkel <[email protected]> Gerrit-Reviewer: Tomas Jelinek <[email protected]> Gerrit-Reviewer: Yair Zaslavsky <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
