Roy Golan has posted comments on this change.

Change subject: core: need vds id when build vm numa properties
......................................................................


Patch Set 7:

(3 comments)

http://gerrit.ovirt.org/#/c/27830/7/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/CreateVDSCommand.java
File 
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/CreateVDSCommand.java:

Line 18:         super(parameters, parameters.getVm().getId());
Line 19:         vm = parameters.getVm();
Line 20:         //Use this field to pass the vdsId into builder which will be 
used by buildVmNumaProperties
Line 21:         //It will be set back to null in buildVmNumaProperties since 
the actual runOnVds value will
Line 22:         //be set in CreateVmVDSCommand.executeVdsIdCommand().
instead of using that vm field please just pass it to the builder constructor
  
  new VmInfoBuilder(vm, vdsId, createInfo)
Line 23:         vm.setRunOnVds(getParameters().getVdsId());
Line 24:         createInfo = new HashMap<String, Object>();
Line 25:         builder = createBuilder();
Line 26:     }


http://gerrit.ovirt.org/#/c/27830/7/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VmInfoBuilder.java
File 
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VmInfoBuilder.java:

Line 1039:      * have almost the same libvirt version support
Line 1040:      *
Line 1041:      * @param compatibilityVersion
Line 1042:      */
Line 1043:     private void addNumaSetting(final String compatibilityVersion) {
lots of stuff to unit test here.

please create a numa settings factory and add test to it. this is not straight 
forward to follow.

make sure the factory is receiving the inputs as arguments and don't call 
dbfacade from within

the factory should create the numa structure we want to append to the createInfo

as a hunch I think it should look like

 new NumaSturcture(
       compatVersion,
       vmNodes,
       vdsNumaNodes)
Line 1044:         if (Boolean.TRUE.equals(Config.<Boolean> 
getValue(ConfigValues.CpuPinningEnabled,
Line 1045:                         compatibilityVersion))) {
Line 1046:             NumaTuneMode numaTune = vm.getNumaTuneMode() == null ? 
NumaTuneMode.INTERLEAVE : vm.getNumaTuneMode();
Line 1047:             List<VmNumaNode> vmNumaNodes = 
DbFacade.getInstance().getVmNumaNodeDAO().getAllVmNumaNodeByVmId(vm.getId());


http://gerrit.ovirt.org/#/c/27830/7/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VmInfoBuilderBase.java
File 
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VmInfoBuilderBase.java:

Line 318:     protected abstract void buildVmVirtioScsi();
Line 319: 
Line 320:     protected abstract void buildVmRngDevice();
Line 321: 
Line 322:     protected abstract void buildVmNumaProperties();
thanks
Line 323: 
Line 324:     protected static enum VNIC_PROFILE_PROPERTIES {
Line 325:         PORT_MIRRORING("Port Mirroring"),
Line 326:         CUSTOM_PROPERTIES("Custom Properties"),


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I19be54812d0ebbb54145f33e6b036af84ff1d8ae
Gerrit-PatchSet: 7
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Xiaolei Shi <xiao-lei....@hp.com>
Gerrit-Reviewer: Arik Hadas <aha...@redhat.com>
Gerrit-Reviewer: Gilad Chaplik <gchap...@redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com>
Gerrit-Reviewer: Roy Golan <rgo...@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