Omer Frenkel has posted comments on this change.
Change subject: engine: Ability to assign multiple VMs from a pool to single
user
......................................................................
Patch Set 3: (5 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmPoolCommand.java
Line 27: protected boolean canDoAction() {
Line 28: boolean returnValue = super.canDoAction();
Line 29: if (returnValue) {
Line 30: VmPool pool = getParameters().getVmPool();
Line 31: if (pool.getMaxAssignedVmsPerUser() < 1 ||
pool.getMaxAssignedVmsPerUser() > pool.getAssignedVmsCount()) {
first, why this if is added here and not in CommonVmPoolWithVmsCommand? isn't
it relevant for update as well?
second part:
getAssignedVmsCount() reflects the number of vms currently assigned to users,
since the pool hasn't been created yet, it will always be 0
you should probably use parameters.getVmsCount() which represent the number of
vms that asked to be in the pool, but i think this is available only in
CommonVmPoolWithVmsCommand, so another excuse to put it there, basically i
think this class is useless
Line 32:
addCanDoActionMessage(VdcBllMessages.VM_POOL_NUMBER_OF_ASSIGNED_VMS_OUT_OF_RANGE);
Line 33: returnValue = false;
Line 34: }
Line 35: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AttachUserToVmFromPoolAndRunCommand.java
Line 73: vmCount++;
Line 74: }
Line 75: }
Line 76:
Line 77: int limit =
DbFacade.getInstance().getVmPoolDao().get(getVmPoolId()).getMaxAssignedVmsPerUser();
please use getVmPool() instead of db call
Line 78: if (vmCount >= limit) {
Line 79:
addCanDoActionMessage(VdcBllMessages.VM_POOL_CANNOT_ATTACH_TO_MORE_VMS_FROM_POOL);
Line 80: returnValue = false;
Line 81: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/DetachUserFromVmFromPoolCommand.java
Line 11: import org.ovirt.engine.core.compat.TransactionScopeOption;
Line 12: import org.ovirt.engine.core.dal.dbbroker.DbFacade;
Line 13:
Line 14: @DisableInPrepareMode
Line 15: public class DetachUserFromVmFromPoolCommand<T extends
VmPoolSimpleUserParameters> extends
in manual vm-pool flow, when admin want to "return" the vm to the pool, he
removes the user permission from the vm, this is why there is a call to this
command from removePermissionCommand, please handle this call as well
Line 16: VmPoolSimpleUserCommandBase<T> {
Line 17:
Line 18: /**
Line 19: * Constructor for command creation when compensation is applied
on startup
Line 31:
Line 32: protected void DetachVmFromUser() {
Line 33: VM vm = DbFacade.getInstance().getVmDao().get(getVmId());
Line 34:
Line 35: if (getVmPoolId().equals(vm.getVmPoolId())) {
please verify vm is not null
Line 36: permissions perm = DbFacade
Line 37: .getInstance()
Line 38: .getPermissionDao()
Line 39: .getForRoleAndAdElementAndObject(
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmPoolSimpleUserCommandBase.java
Line 68:
Line 69: @Override
Line 70: public Guid getVmId() {
Line 71: return getParameters().getVmId();
Line 72: }
i think its better to call setVmId instead of overriding the getter, which is
confusing as this is not the way its used all over the project
Line 73:
Line 74: @Override
Line 75: public void setVmId(Guid value) {
Line 76: getParameters().setVmId(value);
--
To view, visit http://gerrit.ovirt.org/14005
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3a1cc836db48d7094c2e7a03ee3a60b0b34a52a9
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Martin Beták <mbe...@redhat.com>
Gerrit-Reviewer: Martin Beták <mbe...@redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com>
Gerrit-Reviewer: Tomas Jelinek <tjeli...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
