Omer Frenkel has posted comments on this change. Change subject: core, engine, webadmin: Shutdown/power off operations on a VM would ask for an optional reason ......................................................................
Patch Set 11: (7 comments) http://gerrit.ovirt.org/#/c/25633/11/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmCommand.java: Line 58: Line 59: public VmCommand(T parameters) { Line 60: super(parameters); Line 61: setVmId(parameters.getVmId()); Line 62: setReason(parameters.getReason()); why this is in vmCommand? should be in stopBase command Line 63: } Line 64: Line 65: @Override Line 66: public Guid getStoragePoolId() { http://gerrit.ovirt.org/#/c/25633/11/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VmOperationParameterBase.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VmOperationParameterBase.java: Line 5: Line 6: public class VmOperationParameterBase extends VdcActionParametersBase implements Serializable { Line 7: private static final long serialVersionUID = -6248335374537898949L; Line 8: private Guid quotaId; Line 9: private String reason; this is a specific parameter to stop commands, its not nice to put it in the general vm commands params, please create a new param class for stop and put it there Line 10: Line 11: public VmOperationParameterBase() { Line 12: vmId = Guid.Empty; Line 13: } http://gerrit.ovirt.org/#/c/25633/11/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VDSGroup.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VDSGroup.java: Line 72: private boolean trustedService; Line 73: Line 74: private boolean haReservation; Line 75: Line 76: private boolean optionalReason; i think a better name would be 'vmStopReasonRequired', because it will make it more clear what this field is about, also the get/set methods will have standard java names: isVmStopReasonRequired/setVmStopReasonRequired Line 77: Line 78: private Guid clusterPolicyId; Line 79: Line 80: private String clusterPolicyName; http://gerrit.ovirt.org/#/c/25633/11/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VM.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VM.java: Line 213: public void setComment(String value) { Line 214: this.vmStatic.setComment(value); Line 215: } Line 216: Line 217: public String getReason() { please rename to StopReason Line 218: return this.vmDynamic.getReason(); Line 219: } Line 220: Line 221: public void setReason(String value) { http://gerrit.ovirt.org/#/c/25633/11/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmDynamic.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmDynamic.java: Line 66: private boolean runOnce; Line 67: @UnchangeableByVdsm Line 68: private String cpuName; Line 69: private String currentCd; Line 70: private String reason; should be @UnchangeableByVdsm Line 71: Line 72: public static final String APPLICATIONS_LIST_FIELD_NAME = "appList"; Line 73: public static final String STATUS_FIELD_NAME = "status"; Line 74: http://gerrit.ovirt.org/#/c/25633/11/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsUpdateRunTimeInfo.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsUpdateRunTimeInfo.java: Line 172: } Line 173: } Line 174: Line 175: for (VmDynamic vmDynamic : _vmDynamicToSave.values()) { Line 176: if (vmDynamic.getStatus() == VMStatus.Up) { why not clearing this on start command? think of this scenario: user stop vm with some reason, then start it and it fails to start, now the vm is down with the old reason, even that the user didnt stop the vm again. Line 177: // clear the reason for VM shutdown Line 178: vmDynamic.setReason(""); Line 179: } Line 180: } http://gerrit.ovirt.org/#/c/25633/11/packaging/dbscripts/create_views.sql File packaging/dbscripts/create_views.sql: Line 633: WHERE vm_static.entity_type = 'VM'; Line 634: Line 635: Line 636: Line 637: CREATE OR REPLACE VIEW vms_with_tags i think should be added here as well Line 638: AS Line 639: SELECT vms.vm_name, vms.vm_mem_size_mb, vms.nice_level, vms.cpu_shares, vms.vmt_guid, vms.vm_os, vms.vm_description, vms.vm_comment, Line 640: vms.vds_group_id, vms.vm_creation_date, vms.auto_startup, vms.is_stateless, vms.is_smartcard_enabled, vms.is_delete_protected, Line 641: vms.sso_method, vms.dedicated_vm_for_vds, vms.fail_back, vms.default_boot_sequence, vms.vm_type, -- To view, visit http://gerrit.ovirt.org/25633 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I17645e5bc97a1a4d460956ec45f88524465dfd7b Gerrit-PatchSet: 11 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: Barak Azulay <bazu...@redhat.com> Gerrit-Reviewer: Einav Cohen <eco...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Gilad Chaplik <gchap...@redhat.com> Gerrit-Reviewer: Itamar Heim <ih...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: Tomas Jelinek <tjeli...@redhat.com> Gerrit-Reviewer: Yair Zaslavsky <yzasl...@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
