Tomas Jelinek has posted comments on this change. Change subject: core: adjustments needed to support instance types ......................................................................
Patch Set 12: (15 comments) http://gerrit.ovirt.org/#/c/23828/12//COMMIT_MSG Commit Message: Line 9: Three things had to be changed: 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. > so as a cluster admin i can't create an instance type in scope of my cluste according to our discussion on engine-devel for now we will only have system level instance types with an ability to enable/disable them on cluster level (with system level default). Line 14: - devices: when creating an instance type, the permissions are not copied from 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 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 ? right 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? images are cluster level 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 bef Done 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 ? Done 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 Done 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) the issue is that if the getVm() == null than this part fails on NPE because of getVm().isBalloonEnabled() and than inside the copyVmDevices(). But you are right, the isVmInDb = (getVm() == null), changing accordingly. 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 ? image type is on cluster so if it is image type and the getVdsGroup == null than it will not get here because of canDoAction() 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 pr Done 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? image type is on cluster 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() Done 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? Done 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 Done 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 Done 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? right, no need to drop since the problem is only the "not null" constraint... -- 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: Itamar Heim <[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
