Omer Frenkel has posted comments on this change.
Change subject: engine: Add backend support for VM Reboot
......................................................................
Patch Set 8: (10 inline comments)
partially reviewed.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/FenceVdsBaseCommand.java
Line 417: Backend.getInstance()
Line 418: .getResourceManager()
Line 419: .RunVdsCommand(
Line 420: VDSCommandType.DestroyVm,
Line 421: new
VdsAndVmIDVDSParametersBase(new Guid(vm.getMigratingToVds().toString()),
vm.getId()));
while at it, since vm.getMigratingToVds() is guid,
please change: "new Guid(vm.getMigratingToVds().toString())" to just
vm.getMigratingToVds()
Line 422: log.infoFormat("Stopped migrating vm: {0} on vds:
{1}", vm.getName(), vm.getMigratingToVds());
Line 423: }
Line 424: } catch (RuntimeException ex) {
Line 425: log.infoFormat("Could not stop migrating vm: {0} on
vds: {1}, Error: {2}", vm.getName(),
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/PowerDownCommandBase.java
Line 27: protected boolean canDoAction() {
Line 28: boolean retValue = true;
Line 29: if (getVm() == null) {
Line 30: retValue = false;
Line 31:
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_VM_NOT_FOUND);
please use:
return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_VM_NOT_FOUND);
instead of multiple if..else
Line 32: } else if (!getVm().isRunning() && getVm().getStatus() !=
VMStatus.Paused
Line 33: && getVm().getStatus() != VMStatus.NotResponding &&
getVm().getStatus() != VMStatus.Suspended) {
Line 34: if (getVm().getStatus().isHibernating() ||
getVm().getStatus() == VMStatus.RestoringState) {
Line 35: retValue = false;
Line 61:
Line 62: @Override
Line 63: public void addQuotaPermissionSubject(List<PermissionSubject>
quotaPermissionList) {
Line 64: //
Line 65: }
i dont understand all quota related code above
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RebootVmCommand.java
Line 23: protected boolean canDoAction() {
Line 24: boolean ret = super.canDoAction();
Line 25: if (!ret) {
Line 26: addCanDoActionMessage(VdcBllMessages.VAR__ACTION__RESTART);
Line 27: addCanDoActionMessage(VdcBllMessages.VAR__TYPE__VM);
please override setActionMessageParameters() method to set these messages
Line 28: }
Line 29:
Line 30: return ret;
Line 31: }
Line 31: }
Line 32:
Line 33: @Override
Line 34: protected void Perform() {
Line 35: log.infoFormat("Entered (VM {0}).", getVm().getName());
why do you need this info logged? when commad is executed there is log for it
already (with id and type of related object)
Line 36: final int secondsToWait = getParameters().getGracefulTimeout()
== null ?
Line 37:
Config.<Integer>GetValue(ConfigValues.VmGracefulShutdownTimeout) :
getParameters().getGracefulTimeout();
Line 38:
Line 39: if (hasVmConfigurationChanged() || getVm().isRunOnce()) {
Line 35: log.infoFormat("Entered (VM {0}).", getVm().getName());
Line 36: final int secondsToWait = getParameters().getGracefulTimeout()
== null ?
Line 37:
Config.<Integer>GetValue(ConfigValues.VmGracefulShutdownTimeout) :
getParameters().getGracefulTimeout();
Line 38:
Line 39: if (hasVmConfigurationChanged() || getVm().isRunOnce()) {
i dont think isRunOnce is persisted to the db, so no use checking it here..
Line 40: log.infoFormat("VM {0} configuration has changed.
Destroying and recreating with new configuration.", getVm().getName());
Line 41:
Line 42: Backend.getInstance()
Line 43: .getResourceManager()
Line 38:
Line 39: if (hasVmConfigurationChanged() || getVm().isRunOnce()) {
Line 40: log.infoFormat("VM {0} configuration has changed.
Destroying and recreating with new configuration.", getVm().getName());
Line 41:
Line 42: Backend.getInstance()
you can use runVdsCommand
Line 43: .getResourceManager()
Line 44: .RunVdsCommand(VDSCommandType.ShutdownVm,
Line 45: new
ShutdownVmVDSCommandParameters(getVdsId(), getVmId(), secondsToWait, false,
true))
Line 46: .getReturnValue();
Line 46: .getReturnValue();
Line 47:
Line 48: RunVmParams runVmParams = new RunVmParams(getVmId());
Line 49: runVmParams.setSessionId(getParameters().getSessionId());
Line 50:
Backend.getInstance().runInternalAction(VdcActionType.RunVm, new
RunVmParams(getVmId()));
this will probably fail, as it takes time to stop vm (specially if there is
timeToWait)
Line 51:
Line 52: } else {
Line 53:
Line 54: log.infoFormat("Sending reboot command for VM {0}.",
getVm().getName());
Line 50:
Backend.getInstance().runInternalAction(VdcActionType.RunVm, new
RunVmParams(getVmId()));
Line 51:
Line 52: } else {
Line 53:
Line 54: log.infoFormat("Sending reboot command for VM {0}.",
getVm().getName());
i dont think this log is needed, maybe as debug, but really if you look at the
log of this flow you will see something similar already as i mentioned above
Line 55:
Line 56: // sending a shutdown command to the VM:
Line 57: setActionReturnValue(Backend
Line 58: .getInstance()
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ShutdownVmCommand.java
Line 51: log.infoFormat("Entered (VM {0}).", getVm().getName());
Line 52:
Line 53: VmHandler.UpdateVmGuestAgentVersion(getVm());
Line 54:
Line 55: if (canPowerDownGracefully())
the logic inside canPowerDownGracefully() is not exactly the same to the old
logic, im not sure that being in a new cluster (where the feature will be
supported) is good enough to allow gracefull shutdown (what if no acpi and no
guest agent?)
Line 56: // shutting down desktop and waiting for it in a separate
thread to
Line 57: // become 'down':
Line 58: {
Line 59: log.infoFormat("Sending shutdown command for VM {0}.",
getVm()
--
To view, visit http://gerrit.ovirt.org/16193
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5fd413e4b12cc87f3777cf4bdbce3b0ab98e495f
Gerrit-PatchSet: 8
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Martin Betak <[email protected]>
Gerrit-Reviewer: Arik Hadas <[email protected]>
Gerrit-Reviewer: Martin Betak <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
Gerrit-Reviewer: Roy Golan <[email protected]>
Gerrit-Reviewer: Tomas Jelinek <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
