Omer Frenkel has posted comments on this change. Change subject: backend: Add RebootVmCommand ......................................................................
Patch Set 4: (7 comments) http://gerrit.ovirt.org/#/c/22744/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RebootVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RebootVmCommand.java: Line 24: } Line 25: Line 26: @Override Line 27: protected void perform() { Line 28: log.infoFormat("Sending reboot command for VM {0}.", getVm().getName()); this log is not needed, the infrastructure already print every command that is run, you should see something like: 2014-01-13 12:20:27,160 INFO [org.ovirt.engine.core.bll.StopVmCommand] (org.ovirt.thread.pool-6-thread-10) [5dfd99b1] Running command: StopVmCommand internal: false. Entities affected : ID: 1a6b9b46-f96b-4fb3-9260-cbab58e81da3 Type: VM i know its only id, but this is the infra and its ok for all other commands so i think its ok here as well Line 29: final VDSReturnValue returnValue = runVdsCommand(VDSCommandType.RebootVm, new VdsAndVmIDVDSParametersBase(getVdsId(), getVmId())); Line 30: setActionReturnValue(returnValue.getReturnValue()); Line 31: setSucceeded(returnValue.getSucceeded()); Line 32: } Line 28: log.infoFormat("Sending reboot command for VM {0}.", getVm().getName()); Line 29: final VDSReturnValue returnValue = runVdsCommand(VDSCommandType.RebootVm, new VdsAndVmIDVDSParametersBase(getVdsId(), getVmId())); Line 30: setActionReturnValue(returnValue.getReturnValue()); Line 31: setSucceeded(returnValue.getSucceeded()); Line 32: } missing: canDoAction - making sure getVm() is not null, making sure vm is in correct status for reboot? (vmStatus.isRunningOrPaused() i guess), something else? audit-log - you need to override getAuditLogTypeValue with value for success and failure, you can look at SetVmTicketCommand for example http://gerrit.ovirt.org/#/c/22744/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmHandler.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmHandler.java: Line 113: COMMANDS_ALLOWED_ON_EXTERNAL_VMS.add(VdcActionType.VmLogon); Line 114: COMMANDS_ALLOWED_ON_EXTERNAL_VMS.add(VdcActionType.StopVm); Line 115: COMMANDS_ALLOWED_ON_EXTERNAL_VMS.add(VdcActionType.ShutdownVm); Line 116: COMMANDS_ALLOWED_ON_EXTERNAL_VMS.add(VdcActionType.RemoveVm); Line 117: COMMANDS_ALLOWED_ON_EXTERNAL_VMS.add(VdcActionType.RebootVm); add it to the rebootVm canDoAction as well Line 118: COMMANDS_ALLOWED_ON_HOSTED_ENGINE.add(VdcActionType.MigrateVm); Line 119: COMMANDS_ALLOWED_ON_HOSTED_ENGINE.add(VdcActionType.MigrateVmToServer); Line 120: COMMANDS_ALLOWED_ON_HOSTED_ENGINE.add(VdcActionType.InternalMigrateVm); Line 121: COMMANDS_ALLOWED_ON_HOSTED_ENGINE.add(VdcActionType.CancelMigrateVm); http://gerrit.ovirt.org/#/c/22744/4/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/FeatureSupported.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/FeatureSupported.java: Line 206: * @return <code>true</code> if VM reboot is supported for the given version Line 207: */ Line 208: public static boolean reboot(Version version) { Line 209: return supportedInConfig(ConfigValues.RebootSupported, version); Line 210: } i see that you are using the action_version_map table, which is great! so why you need this here? are you using it somewhere? http://gerrit.ovirt.org/#/c/22744/4/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/config/ConfigValues.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/config/ConfigValues.java: Line 1570: MaxNumOfTriesToRunFailedAutoStartVm, Line 1571: Line 1572: @TypeConverterAttribute(Boolean.class) Line 1573: @DefaultValueAttribute("false") Line 1574: RebootSupported, if you choose to keep this config (if you still need it for UI or something like that) than its better to have the default value as 'true' as this is the value from this version and on, so when 3.5 is out, and someone forget to add a specific entry in the db, this value will be used. Line 1575: Line 1576: Invalid; http://gerrit.ovirt.org/#/c/22744/4/packaging/dbscripts/upgrade/03_04_0390_add_reboot_command.sql File packaging/dbscripts/upgrade/03_04_0390_add_reboot_command.sql: Line 1: insert into action_version_map values (6, '3.4', '*'); on your next rebase please update the file name for the latest upgrade script http://gerrit.ovirt.org/#/c/22744/4/packaging/dbscripts/upgrade/pre_upgrade/0000_config.sql File packaging/dbscripts/upgrade/pre_upgrade/0000_config.sql: Line 200: select fn_db_add_config_value('CloudInitSupported','false','3.2'); Line 201: select fn_db_add_config_value('HotPlugDiskSnapshotSupported','false','3.0'); Line 202: select fn_db_add_config_value('HotPlugDiskSnapshotSupported','false','3.1'); Line 203: select fn_db_add_config_value('HotPlugDiskSnapshotSupported','false','3.2'); Line 204: select fn_db_add_config_value('RebootSupported','true','3.4'); the practice is to have here the value for old versions, and in the default the value for this version and on. Line 205: Line 206: -- by default use no proxy Line 207: select fn_db_add_config_value('SpiceProxyDefault','','general'); Line 208: -- To view, visit http://gerrit.ovirt.org/22744 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0693943c01e204da877268c02957f3b936cf3fa2 Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Betak <mbe...@redhat.com> Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com> Gerrit-Reviewer: Martin Betak <mbe...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Roy Golan <rgo...@redhat.com> 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
