Omer Frenkel has posted comments on this change. Change subject: core: adjustments needed to support instance types ......................................................................
Patch Set 12: (14 comments) not fully reviewed, please see my comments about checking cluster for existing vms, before i continue to look on the rest of the patch http://gerrit.ovirt.org/#/c/23828/12//COMMIT_MSG Commit Message: Line 10: - permissions: the instance types need a bit different permissions than the Line 11: template. Since they are not cluster dependent they don't need that Line 12: permissions. But at the same time, they need a system level permission to Line 13: create template. Line 14: - devices: when creating an instance type, the permissions are not copied from the permissions -> the devices ? Line 15: a VM (since there is no base VM). Instead a blank template's devices are used Line 16: - Since the instace types are stored to vm_static but they are not cluster Line 17: dependent, the vds_groups_vm_static had to be removed. Since from now the Line 18: VdsGroup can be null, the guarding that it is not where it is not allowed to Line 24: guards into the code (mostly canDoAction). An exception is the Line 25: RemoveAllVmImagesCommand which can be run also after the VM has been Line 26: deleted Line 27: - Template related ones: The vdsGroupId can be null if the templateType is Line 28: instance type what about image type? Line 29: Line 30: It has been tested using the http://gerrit.ovirt.org/#/c/21821 Line 31: Line 32: Change-Id: Ifaebd849061efa1637f9fa21d8584212ba0e51f2 http://gerrit.ovirt.org/#/c/23828/12/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java: Line 263: VmStatic vmStaticFromParams = getParameters().getVmStaticData(); Line 264: boolean returnValue = Line 265: canAddVm(reasons, vmStaticFromParams.getName(), getStoragePoolId(), vmStaticFromParams.getPriority()); Line 266: Line 267: returnValue = returnValue && getVdsGroup() != null; why this check is needed here as well? we already checked cluster is ok before calling this method Line 268: Line 269: if (returnValue) { Line 270: List<ValidationError> validationErrors = validateCustomProperties(vmStaticFromParams, getVdsGroup().getcompatibility_version()); Line 271: if (!validationErrors.isEmpty()) { Line 760: } Line 761: Line 762: protected static boolean isLegalClusterId(Guid clusterId, List<String> reasons) { Line 763: // check given cluster id Line 764: if (clusterId == null) { i dont think its needed, doesnt dao.get(null) return null ? Line 765: reasons.add(VdcBllErrors.VM_INVALID_SERVER_CLUSTER_ID.toString()); Line 766: return false; Line 767: } Line 768: http://gerrit.ovirt.org/#/c/23828/12/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 306: protected boolean canDoAction() { Line 307: if (getVdsGroup() == null) { Line 308: failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_CLUSTER_CAN_NOT_BE_EMPTY); Line 309: return false; Line 310: } this is covered already in the call to super.canDoAction(), that is in the end of this method, no? Line 311: Line 312: SnapshotsValidator snapshotsValidator = createSnapshotsValidator(); Line 313: Line 314: // If snapshot does not exist or is broken, there is not point in checking any of the VM related checks http://gerrit.ovirt.org/#/c/23828/12/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmTemplateCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmTemplateCommand.java: Line 250: getParameters().isConsoleEnabled(), Line 251: getParameters().isVirtioScsiEnabled(), Line 252: VmDeviceUtils.isBalloonEnabled(getVmId()), Line 253: false); Line 254: } else if (getVm() != null) { iirc, isVmInDb == (getVm() != null) making the code below unreachable, this code used to handle create of template from non-existing vm, what was the issue with it? Line 255: // sending true for isVm in order to create basic devices needed Line 256: VmDeviceUtils.copyVmDevices(getVmId(), Line 257: getVmTemplateId(), Line 258: getVm(), Line 281: return null; Line 282: } Line 283: }); Line 284: Line 285: if (getParameters().getTemplateType() != VmEntityType.INSTANCE_TYPE) { what about image type? isn't it safer to check if getVdsGroup() != null ? Line 286: VmHandler.warnMemorySizeLegal(getVmTemplate(), getVdsGroup().getcompatibility_version()); Line 287: } Line 288: Line 289: // means that there are no asynchronous tasks to execute and that we can Line 377: } Line 378: Line 379: if (isInstanceType) { Line 380: // no additional checks needed Line 381: return true; i dont like this, if someone will add a new check in the future, he will probably not see this code, and when you add new check, you usually add it in the end.. maybe, instead of checking isInstanceType all the time, extract cluster related checks to a method with a clear name, and don't call it if isInstanceType ? Line 382: } Line 383: return imagesRelatedChecks() && AddVmCommand.checkCpuSockets(getParameters().getMasterVm().getNumOfSockets(), Line 384: getParameters().getMasterVm().getCpuPerSocket(), getVdsGroup() Line 385: .getcompatibility_version().toString(), getReturnValue().getCanDoActionMessages()); Line 677: @Override Line 678: public List<PermissionSubject> getPermissionCheckSubjects() { Line 679: if (permissionCheckSubject == null) { Line 680: permissionCheckSubject = new ArrayList<PermissionSubject>(); Line 681: if (getParameters().getTemplateType() != VmEntityType.INSTANCE_TYPE) { what about image type? Line 682: Guid storagePoolId = getVdsGroup() == null ? null : getVdsGroup().getStoragePoolId(); Line 683: permissionCheckSubject.add(new PermissionSubject(storagePoolId, Line 684: VdcObjectType.StoragePool, Line 685: getActionType().getActionGroup())); Line 694: } Line 695: } else { Line 696: permissionCheckSubject.add(new PermissionSubject(Guid.SYSTEM, Line 697: VdcObjectType.System, Line 698: ActionGroup.CREATE_TEMPLATE)); please use getActionType().getActionGroup() Line 699: } Line 700: } Line 701: Line 702: return permissionCheckSubject; http://gerrit.ovirt.org/#/c/23828/12/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AttachDiskToVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AttachDiskToVmCommand.java: Line 52: } Line 53: Line 54: @Override Line 55: protected boolean canDoAction() { Line 56: if (getVdsGroup() == null) { why do you need to check this here? i think we can use the approach of "guarding the gates" only: we can assume vm has cluster (since we check cluster is ok in add/changeCluster/import), and here we check vm exist, so nothing to check (this command doesn't handle templates) what do you think? Line 57: failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_CLUSTER_CAN_NOT_BE_EMPTY); Line 58: return false; Line 59: } Line 60: http://gerrit.ovirt.org/#/c/23828/12/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AttachUserToVmFromPoolAndRunCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AttachUserToVmFromPoolAndRunCommand.java: Line 58: @Override Line 59: protected boolean canDoAction() { Line 60: boolean returnValue = true; Line 61: Line 62: if (vmToAttach != null) { also here, assuming vm has cluster makes this check not needed Line 63: VM vm = getVmDAO().get(vmToAttach); Line 64: if (vm.getVdsGroupId() == null) { Line 65: addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_CLUSTER_CAN_NOT_BE_EMPTY); Line 66: return false; http://gerrit.ovirt.org/#/c/23828/12/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ChangeVMClusterCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ChangeVMClusterCommand.java: Line 40: if (!super.canDoAction()) { Line 41: return false; Line 42: } Line 43: Line 44: if (getVm().getVdsGroupId() == null) { not needed, current cluster cant be null for existing vm Line 45: failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_CLUSTER_CAN_NOT_BE_EMPTY); Line 46: return false; Line 47: } Line 48: http://gerrit.ovirt.org/#/c/23828/12/packaging/dbscripts/upgrade/03_04_0610_remove_vds_groups_vm_static_constraint.sql File packaging/dbscripts/upgrade/03_04_0610_remove_vds_groups_vm_static_constraint.sql: Line 1: ALTER TABLE vm_static DROP CONSTRAINT vds_groups_vm_static; why do you need to drop the foreign key? -- To view, visit http://gerrit.ovirt.org/23828 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifaebd849061efa1637f9fa21d8584212ba0e51f2 Gerrit-PatchSet: 12 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Tomas Jelinek <[email protected]> Gerrit-Reviewer: Omer Frenkel <[email protected]> Gerrit-Reviewer: Roy Golan <[email protected]> Gerrit-Reviewer: Tomas Jelinek <[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
