Gilad Chaplik has posted comments on this change.

Change subject: engine: NUMA feature queries and actions validation
......................................................................


Patch Set 6: Code-Review-1

(3 comments)

minor comments

http://gerrit.ovirt.org/#/c/27617/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/numa/vm/AbstractVmNumaNodeCommand.java
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/numa/vm/AbstractVmNumaNodeCommand.java:

Line 17: 
Line 18: public abstract class AbstractVmNumaNodeCommand<T extends 
VmNumaNodeOperationParameters> extends VmCommand<T> {
Line 19: 
Line 20:     // In 1st phase only support pin 2 host NUMA
Line 21:     private boolean pinHost = true;
we need a configuration option for NUMA and migration, it should be allowed if 
user wants it, by default it's disabled.

shouldn't be hard coded.
Line 22: 
Line 23:     public AbstractVmNumaNodeCommand(T parameters) {
Line 24:         super(parameters);
Line 25:     }


Line 38:             // if VM do not contain any NUMA node, skip checking
Line 39:             return true;
Line 40:         }
Line 41:         VM vm = getVm();
Line 42:         Guid vdsId = vm.getDedicatedVmForVds();
use MigrationSupport field, to check if the vm is pinned or not.
Line 43:         if (pinHost && vdsId == null) {
Line 44:             return 
failCanDoAction(VdcBllMessages.VM_NUMA_PINNED_VDS_NOT_EXIST);
Line 45:         }
Line 46:         List<VdsNumaNode> vdsNumaNodes = new ArrayList<>();


http://gerrit.ovirt.org/#/c/27617/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/numa/vm/UpdateVmNumaNodesCommand.java
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/numa/vm/UpdateVmNumaNodesCommand.java:

Line 52:     }
Line 53: 
Line 54:     @Override
Line 55:     protected boolean canDoAction() {
Line 56:         return checkNumaValid();
why not inplemneting it in super class, used in add command as well.
Line 57:     }
Line 58: 
Line 59:     @Override
Line 60:     public List<PermissionSubject> getPermissionCheckSubjects() {


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9c299405ec5d82ada713ed3d220554bf3055c145
Gerrit-PatchSet: 6
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Jason Liao <chuan.l...@hp.com>
Gerrit-Reviewer: Doron Fediuck <dfedi...@redhat.com>
Gerrit-Reviewer: Gilad Chaplik <gchap...@redhat.com>
Gerrit-Reviewer: Jason Liao <chuan.l...@hp.com>
Gerrit-Reviewer: Kobi Ianko <k...@redhat.com>
Gerrit-Reviewer: Xiaolei Shi <xiao-lei....@hp.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

Reply via email to