Omer Frenkel has posted comments on this change. Change subject: core: add support to update running vm ......................................................................
Patch Set 7: (5 comments) http://gerrit.ovirt.org/#/c/26758/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 554: */ Line 555: private boolean isRunningConfigurationNeeded() { Line 556: return !VmHandler.isUpdateValid(getVm().getStaticData(), Line 557: getParameters().getVmStaticData(), Line 558: getVm().getStatus()) || getVm().isNextRunConfigurationExists(); > minor: can we check the isNextRunConfigurationExists first? Done Line 559: } Line 560: Line 561: @Override Line 562: public List<PermissionSubject> getPermissionCheckSubjects() { Line 629: return Collections.singletonMap( Line 630: getVmId().toString(), Line 631: LockMessagesMatchUtil.makeLockingPair( Line 632: LockingGroup.VM, Line 633: VdcBllMessages.ACTION_TYPE_FAILED_OBJECT_LOCKED)); > should be short, I know, but don't you prefer to have an informative messag Done Line 634: } Line 635: Line 636: @Override Line 637: public List<QuotaConsumptionParameter> getQuotaVdsConsumptionParameters() { http://gerrit.ovirt.org/#/c/26758/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmPoolHandler.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmPoolHandler.java: Line 59: Line 60: QuotaManager.getInstance().rollbackQuotaByVmId(vmId); Line 61: VmHandler.removeStatelessVmUnmanagedDevices(vmId); Line 62: Line 63: ApplyNextRunConfiguration(vmId); > it reminds me the stateless snapshot handling, don't we need to handle a ca actually since this is a backend operation only, we can run it always, also there is an exclusive lock on vm name in updateVmCommand that sort the potential race as well i added a comment explaining this below Line 64: } Line 65: Line 66: /** Line 67: * Update vm configuration with NEXT_RUN configuration, if exists Line 118: updateVmParams.setVmPayload(new VmPayload(VmDeviceType.getByName(device.getDevice()), device.getSpecParams())); Line 119: } Line 120: break; Line 121: case CONSOLE: Line 122: updateVmParams.setConsoleEnabled(true); > need to add empty 'default' clause Done Line 123: } Line 124: } Line 125: return updateVmParams; Line 126: } http://gerrit.ovirt.org/#/c/26758/7/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ObjectIdentityChecker.java File backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ObjectIdentityChecker.java: Line 132: } Line 133: srcCls = srcCls.getSuperclass(); Line 134: dstCls = dstCls.getSuperclass(); Line 135: } Line 136: return true; > I think we can use only one Class object, assuming both objects are from th Done Line 137: } Line 138: Line 139: public final boolean IsUpdateValid(Object source, Object destination) { Line 140: if (source.getClass() != destination.getClass()) { -- To view, visit http://gerrit.ovirt.org/26758 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie433352852f53f62e58349ce85475ee89e37ce89 Gerrit-PatchSet: 7 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Roy Golan <rgo...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
