Eldan Shachar has posted comments on this change. Change subject: core:(2) Editing of template version for pools ......................................................................
Patch Set 7: (7 comments) https://gerrit.ovirt.org/#/c/36508/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/DetachUserFromVmFromPoolCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/DetachUserFromVmFromPoolCommand.java: Line 45: if (perm != null) { Line 46: DbFacade.getInstance().getPermissionDao().remove(perm.getId()); Line 47: if (getParameters().getIsRestoreStateless()) { Line 48: VM vm = DbFacade.getInstance().getVmDao().get(getParameters().getVmId()); Line 49: if (vm != null) { > not sure i understand what you did here (except adding the parameter, looks Main changes: - Move the permission removal to be independent of the vm object existence, that is, command should remove permissions even if getVm fails. this is the reason for the change in the commands order. - "getVmPoolId().equals(vm.getVmPoolId())" - it seems like legacy from before 0bb3bfb18bf50f4c4ece1eb364ce4a27e0915158 where the code iterated over all the VMs of the user and needed to know which ones actually belongs to the pool. Line 50: RestoreVmFromBaseSnapshot(vm); Line 51: } Line 52: } Line 53: } https://gerrit.ovirt.org/#/c/36508/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetAllPoolVmsQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetAllPoolVmsQuery.java: Line 17: Line 18: @Override Line 19: protected void executeQueryCommand() { Line 20: List<VM> vmsList = getDbFacade() Line 21: .getVmDao().getAllForVmPool(getParameters().getId()); > since this is defined as a user query (is it used by user portal?), you nee removed the user definition from VdcQueryType. Line 22: for (VM vm : vmsList) { Line 23: VmHandler.updateVmGuestAgentVersion(vm); Line 24: } Line 25: getQueryReturnValue().setReturnValue(vmsList); https://gerrit.ovirt.org/#/c/36508/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetLatestTemplateInChainQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetLatestTemplateInChainQuery.java: Line 11: Line 12: @Override Line 13: protected void executeQueryCommand() { Line 14: VmTemplate vmt; Line 15: vmt = DbFacade.getInstance().getVmTemplateDao().getTemplateWithLatestVersionInChain(getParameters().getId()); > user query issue changed vdcQueryType. Line 16: if (vmt != null) { Line 17: VmTemplateHandler.updateDisksFromDb(vmt); Line 18: VmHandler.updateVmInitFromDB(vmt, true); Line 19: } https://gerrit.ovirt.org/#/c/36508/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetVmTemplatesByBaseTemplateIdQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetVmTemplatesByBaseTemplateIdQuery.java: Line 15: Line 16: @Override Line 17: protected void executeQueryCommand() { Line 18: List<VmTemplate> templateList = Line 19: DbFacade.getInstance().getVmTemplateDao().getTemplateVersionsForBaseTemplate(getParameters().getId()); > user query issue changed vdcQueryType. (*this query is used by a function which is common for admin and user portal. but the flow which calls the query is only used by admin portal.) Line 20: if (templateList != null) { Line 21: VmTemplate baseTemplate = DbFacade.getInstance().getVmTemplateDao().get(getParameters().getId(), getUserID(), getParameters().isFiltered()); Line 22: if (baseTemplate != null) { Line 23: templateList.add(baseTemplate); https://gerrit.ovirt.org/#/c/36508/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmCommand.java: Line 186: setSucceeded(true); Line 187: } Line 188: Line 189: private void updateVmTemplateVersion() { Line 190: if (getVm().getStatus() == VMStatus.Down) { // if vm is down -> updateVmVersionCommand will handle the rest. if it's not, a NEXT_RUN snapshot will be created. > maybe move this comment to java doc for the method, and add little more inf Done Line 191: VdcReturnValueBase result = Line 192: runInternalActionWithTasksContext( Line 193: VdcActionType.UpdateVmVersion, Line 194: new UpdateVmVersionParameters(getVmId(), https://gerrit.ovirt.org/#/c/36508/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmVersionCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmVersionCommand.java: Line 60: super(parameters, cmdContext); Line 61: parameters.setEntityInfo(new EntityInfo(VdcObjectType.VM, parameters.getVmId())); Line 62: Line 63: if (getVm() != null) { Line 64: if(parameters.getNewTemplateVersion() != null) { > missing space "if (" Done Line 65: setVmTemplate(getVmTemplateDAO().get(parameters.getNewTemplateVersion())); Line 66: } else { Line 67: setVmTemplate(getVmTemplateDAO().getTemplateWithLatestVersionInChain(getVm().getVmtGuid())); Line 68: } https://gerrit.ovirt.org/#/c/36508/7/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/UpdateVmVersionParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/UpdateVmVersionParameters.java: Line 7: private Guid vmPoolId; Line 8: /** The ID of disk operator of one of the disks the VM had before the update */ Line 9: private Guid previousDiskOperatorAuthzPrincipalDbId; Line 10: private Guid newTemplateVersion; Line 11: private Boolean LatestStatus; > why not boolean? also maybe rename to useLatestVersion? what status means? Until now UpdateVmVersion copied the original\source VM static data to the new VM and isUseLatest was set accordingly. But when changing a template-version from latest to regular(or vice versa) the command shouldn't rely on the original VM latest property, so this field allows to override the default behavior and set the field explicitly. but there are still places which need the old behavior, so null is used to signal this(i.e. field is null -> no override was done -> use original vm property). Fixed the variable name and added explanation. Line 12: Line 13: public UpdateVmVersionParameters() { Line 14: } Line 15: -- To view, visit https://gerrit.ovirt.org/36508 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2249cfea12ffbab008c162d52928df782f5f9d85 Gerrit-PatchSet: 7 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Eldan Shachar <[email protected]> Gerrit-Reviewer: Arik Hadas <[email protected]> Gerrit-Reviewer: Eldan Shachar <[email protected]> Gerrit-Reviewer: Omer Frenkel <[email protected]> Gerrit-Reviewer: Shahar Havivi <[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
