Arik Hadas 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 down, but then you return an error, so how should it be? if it is hot-plug only, then I would rename the command to reflect it 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 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 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? 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? 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 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 VmManagementParametersBase? 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 EditableField 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 changed (hot-plug/unplug), so if this field is that way as well and can be changed from the edit-vm dialog, that hard-coded thing should be updated 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 should it be something like "The CPU limit of VM {..} is set.." ? 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 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 method that creates the map and return it? 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 and the conclusion was that it is better to have it as a separate class (matter of conventions if I recall correctly), so please extract it to separate class 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 <k...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Gilad Chaplik <gchap...@redhat.com> Gerrit-Reviewer: Jiří Moskovčák <jmosk...@redhat.com> Gerrit-Reviewer: Kobi Ianko <k...@redhat.com> Gerrit-Reviewer: Martin Sivák <msi...@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
