Arik Hadas has posted comments on this change.

Change subject: engine: clear the pending memory when destroying paused VM
......................................................................


Patch Set 1: Code-Review-1

(2 comments)

http://gerrit.ovirt.org/#/c/25271/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/StopVmCommandBase.java
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/StopVmCommandBase.java:

Line 81:                             new DestroyVmVDSCommandParameters(new 
Guid(getVm().getMigratingToVds().toString()),
Line 82:                                     getVmId(), true, false, 0));
Line 83:         }
Line 84: 
Line 85:         if (getVm().getStatus() == VMStatus.Paused) {
you should check the pause-status as well (VmDynamic#getPauseStatus) because if 
the VM is paused due to some error, we shouldn't decrease the pending mem/cpu 
when it stops. The pending resources should be decreased only if the 
pause-status is NOERR (which means the VM was started in pause mode and the 
pending resources were not decreased yet)
Line 86:             VmStatic vmStatic = getVm().getStaticData();
Line 87:             decreasePendingVms(getVm().getRunOnVds(), 
vmStatic.getNumOfCpus(),
Line 88:                     vmStatic.getMinAllocatedMem(), vmStatic.getName());
Line 89:         }


http://gerrit.ovirt.org/#/c/25271/1/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 575:      * @return {@link org.ovirt.engine.core.vdsbroker.VdsMonitor} for 
signaling on thread actions
Line 576:      */
Line 577:     private VdsMonitor getMonitor(Guid vdsId) {
Line 578:         return 
ResourceManager.getInstance().GetVdsManager(vdsId).getVdsMonitor();
Line 579:     }
Extracting those methods from RunVmCommand is good, but VmCommand is not the 
right place for them. I understand that you moved them here so they will be 
accessible but they are not related to VmCommand (most of the commands that 
extend VmCommand don't need it). I think they should move to VmHandler which 
include such "utility" methods and is also accessible to run & stop commands.


-- 
To view, visit http://gerrit.ovirt.org/25271
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1de34ac8b0b6e48d360161d8f0ff8d1fe4f6fd04
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Jiří Moskovčák <[email protected]>
Gerrit-Reviewer: Arik Hadas <[email protected]>
Gerrit-Reviewer: Doron Fediuck <[email protected]>
Gerrit-Reviewer: Gilad Chaplik <[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

Reply via email to