Kobi Ianko has posted comments on this change. Change subject: core: New internal command, VmSlaPolicyCommand ......................................................................
Patch Set 1: (14 comments) http://gerrit.ovirt.org/#/c/27646/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmSlaPolicyCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmSlaPolicyCommand.java: Line 36: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_VM_NOT_EXIST); Line 37: } Line 38: if (getVm().getStatus() != VMStatus.Up) { Line 39: canDo = failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_VM_STATUS_ILLEGAL, Line 40: LocalizedVmStatus.from(getVm().getStatus())); > in the javadoc above you say that this command can run also when the VM is right, the comment is wrong Line 41: } Line 42: Line 43: return canDo; Line 44: } Line 39: canDo = failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_VM_STATUS_ILLEGAL, Line 40: LocalizedVmStatus.from(getVm().getStatus())); Line 41: } Line 42: Line 43: return canDo; > please remove the canDo variable, and return in #39 in a similar way to #36 Done Line 44: } Line 45: Line 46: /** Line 47: * Execution shall perform a call to VDSM to set the SLA parameters. Line 80: } Line 81: Line 82: @Override Line 83: protected void setActionMessageParameters() { Line 84: // addCanDoActionMessage(VdcBllMessages.VAR__ACTION__HOT_SET_CPUS); > missing action & type + remove comments will fix that Line 85: // addCanDoActionMessage(VdcBllMessages.VAR__TYPE__VM); Line 86: // addCanDoActionMessage(String.format("$clusterVersion %1$s", Line 87: // getVm().getVdsGroupCompatibilityVersion() )); Line 88: // addCanDoActionMessage(String.format("$architecture %1$s", http://gerrit.ovirt.org/#/c/27646/1/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/AuditLogType.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/AuditLogType.java: Line 987: // External scheduler Line 988: EXTERNAL_SCHEDULER_PLUGIN_ERROR(10500, AuditLogSeverity.ERROR), Line 989: EXTERNAL_SCHEDULER_ERROR(10501, AuditLogSeverity.ERROR), Line 990: Line 991: VM_SLA_POLICY(10550), > VM_SLA_POLICY_UPDATED? followed the hot set convention, since sla policy cannot be added or deleted only updated, we can skip that... Line 992: FAILED_VM_SLA_POLICY(10551, AuditLogSeverity.ERROR); Line 993: Line 994: private int intValue; Line 995: // indicates time interval in seconds on which identical events from same instance are suppressed. Line 988: EXTERNAL_SCHEDULER_PLUGIN_ERROR(10500, AuditLogSeverity.ERROR), Line 989: EXTERNAL_SCHEDULER_ERROR(10501, AuditLogSeverity.ERROR), Line 990: Line 991: VM_SLA_POLICY(10550), Line 992: FAILED_VM_SLA_POLICY(10551, AuditLogSeverity.ERROR); > FAILED_TO_UPDATE.. /VM_SLA_POLICY_UPDATE_FAILED? same as above Line 993: Line 994: private int intValue; Line 995: // indicates time interval in seconds on which identical events from same instance are suppressed. Line 996: private int eventFloodRate; http://gerrit.ovirt.org/#/c/27646/1/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VdcActionType.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VdcActionType.java: Line 41: DetachDiskFromVm(181, ActionGroup.CONFIGURE_VM_STORAGE, false, QuotaDependency.NONE), Line 42: HotPlugDiskToVm(182, ActionGroup.CONFIGURE_VM_STORAGE, false, QuotaDependency.NONE), Line 43: HotUnPlugDiskFromVm(183, ActionGroup.CONFIGURE_VM_STORAGE, false, QuotaDependency.NONE), Line 44: HotSetNumberOfCpus(184, ActionGroup.EDIT_VM_PROPERTIES, false, QuotaDependency.NONE), Line 45: VmSlaPolicy(185, ActionGroup.EDIT_VM_PROPERTIES, false, QuotaDependency.NONE), > again, I think the name can be improved since sla policy cannot be added or deleted only updated, we can skip that... Line 46: ChangeFloppy(35, QuotaDependency.NONE), Line 47: ImportVm(36, ActionGroup.IMPORT_EXPORT_VM, QuotaDependency.STORAGE), Line 48: RemoveVmFromImportExport(37, ActionGroup.DELETE_VM, QuotaDependency.NONE), Line 49: RemoveVmTemplateFromImportExport(38, ActionGroup.DELETE_TEMPLATE, QuotaDependency.NONE), http://gerrit.ovirt.org/#/c/27646/1/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VmSlaPolicyParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VmSlaPolicyParameters.java: Line 2: Line 3: import org.ovirt.engine.core.common.businessentities.VmStatic; Line 4: Line 5: Line 6: public class VmSlaPolicyParameters extends VmManagementParametersBase { > why having this class if it doesn't add anything to VmManagementParametersB parameters will be added soon... the sla policy should grow... I can remove it, but it will be more easy to other sla team member to know where should their parameters go Line 7: Line 8: private static final long serialVersionUID = 3918909396931144459L; Line 9: Line 10: http://gerrit.ovirt.org/#/c/27646/1/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmBase.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmBase.java: Line 125: @EditableOnTemplate Line 126: private int cpuShares; Line 127: Line 128: @CopyOnNewVersion Line 129: @EditableOnVmStatusField(statuses = { VMStatus.Down, VMStatus.Up }) > if that's a hot-plug like change, replace the annotation with EditableFiel Done Line 130: @EditableOnTemplate Line 131: private int cpuLimit; Line 132: Line 133: @CopyOnNewVersion Line 127: Line 128: @CopyOnNewVersion Line 129: @EditableOnVmStatusField(statuses = { VMStatus.Down, VMStatus.Up }) Line 130: @EditableOnTemplate Line 131: private int cpuLimit; > note that in the UI it is hard-coded that only the CPU can be dynamically c I'll need to keep that in mind when implementing the UI Line 132: Line 133: @CopyOnNewVersion Line 134: @EditableField Line 135: private int priority; http://gerrit.ovirt.org/#/c/27646/1/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/VDSCommandType.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/VDSCommandType.java: Line 157: GetDiskAlignment("org.ovirt.engine.core.vdsbroker.vdsbroker"), Line 158: GlusterTasksList("org.ovirt.engine.core.vdsbroker.gluster"), Line 159: GetGlusterVolumeRemoveBricksStatus("org.ovirt.engine.core.vdsbroker.gluster"), Line 160: SetNumberOfCpus("org.ovirt.engine.core.vdsbroker"), Line 161: UpdateVmPolicy("org.ovirt.engine.core.vdsbroker"), > that's a good name :) :) Line 162: List("org.ovirt.engine.core.vdsbroker.vdsbroker"), // get a list of VMs with status only Line 163: GetVmStats("org.ovirt.engine.core.vdsbroker.vdsbroker"), // get a VM with full data and statistics Line 164: GetAllVmStats("org.ovirt.engine.core.vdsbroker.vdsbroker"); // get a list of VMs with full data and statistics Line 165: http://gerrit.ovirt.org/#/c/27646/1/backend/manager/modules/dal/src/main/resources/bundles/AuditLogMessages.properties File backend/manager/modules/dal/src/main/resources/bundles/AuditLogMessages.properties: Line 795: USER_REMOVED_AFFINITY_GROUP=Affinity Group ${affinityGroupName} was removed. (User: ${UserName}) Line 796: USER_FAILED_TO_REMOVE_AFFINITY_GROUP=Failed to remove Affinity Group ${affinityGroupName}. (User: ${UserName}) Line 797: USER_SET_HOSTED_ENGINE_MAINTENANCE=Hosted Engine HA maintenance mode was updated on host ${VdsName}. Line 798: USER_FAILED_TO_SET_HOSTED_ENGINE_MAINTENANCE=Hosted Engine HA maintenance mode could not be updated on host ${VdsName}. Line 799: VM_SLA_POLICY=VM ${vmName} CPU limit is set to ${cpuLimit} > s/vmName/VmName I should change that to a more general message since sla policy will be more then just cpu limit Line 800: FAILED_VM_SLA_POLICY= Failed to set CPU limit to VM ${vmName}. Underlying error message: ${ErrorMessage} Line 801: Line 802: ISCSI_BOND_ADD_SUCCESS=iSCSI bond '${IscsiBondName}' was successfully created in Data Center '${StoragePoolName}'. Line 803: ISCSI_BOND_ADD_FAILED=Failed to create iSCSI bond '${IscsiBondName}' in Data Center '${StoragePoolName}'. Line 796: USER_FAILED_TO_REMOVE_AFFINITY_GROUP=Failed to remove Affinity Group ${affinityGroupName}. (User: ${UserName}) Line 797: USER_SET_HOSTED_ENGINE_MAINTENANCE=Hosted Engine HA maintenance mode was updated on host ${VdsName}. Line 798: USER_FAILED_TO_SET_HOSTED_ENGINE_MAINTENANCE=Hosted Engine HA maintenance mode could not be updated on host ${VdsName}. Line 799: VM_SLA_POLICY=VM ${vmName} CPU limit is set to ${cpuLimit} Line 800: FAILED_VM_SLA_POLICY= Failed to set CPU limit to VM ${vmName}. Underlying error message: ${ErrorMessage} > s/vmName/VmName Done Line 801: Line 802: ISCSI_BOND_ADD_SUCCESS=iSCSI bond '${IscsiBondName}' was successfully created in Data Center '${StoragePoolName}'. Line 803: ISCSI_BOND_ADD_FAILED=Failed to create iSCSI bond '${IscsiBondName}' in Data Center '${StoragePoolName}'. Line 804: ISCSI_BOND_EDIT_SUCCESS=iSCSI bond '${IscsiBondName}' was successfully updated. http://gerrit.ovirt.org/#/c/27646/1/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/UpdateVmPolicyVDSCommand.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/UpdateVmPolicyVDSCommand.java: Line 30: Line 31: protected void init() { Line 32: struct.put("vmId", getParameters().getVmId().toString()); Line 33: struct.put("vcpuLimit", String.valueOf(getParameters().getCpuLimit())); Line 34: } > how about replacing the class member + init method definitions with a build Does it matter? I copied it from another place in the code. could be handy to have the parameters accessible in all of the class, no? Line 35: public static class Params extends VdsAndVmIDVDSParametersBase{ Line 36: Line 37: private int cpuLimit; Line 38: Line 31: protected void init() { Line 32: struct.put("vmId", getParameters().getVmId().toString()); Line 33: struct.put("vcpuLimit", String.valueOf(getParameters().getCpuLimit())); Line 34: } Line 35: public static class Params extends VdsAndVmIDVDSParametersBase{ > having the parameters class as inner static class was discussed in the past Done Line 36: Line 37: private int cpuLimit; Line 38: Line 39: public Params(Guid vdsId, Guid vmId, int cpuLimit) { -- To view, visit http://gerrit.ovirt.org/27646 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I26b0794c52cfe1d352f26392a835023afcd9efa0 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Kobi Ianko <[email protected]> Gerrit-Reviewer: Arik Hadas <[email protected]> Gerrit-Reviewer: Gilad Chaplik <[email protected]> Gerrit-Reviewer: Jiří Moskovčák <[email protected]> Gerrit-Reviewer: Kobi Ianko <[email protected]> Gerrit-Reviewer: Martin Sivák <[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
