Liron Aravot has posted comments on this change.
Change subject: engine: Add validation to cpu pinning
......................................................................
Patch Set 2: (8 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java
Line 330: }
Line 331:
Line 332: // check cpuPinning
Line 333: if (returnValue) {
Line 334: VDS dedicatedVds;
1. the if clause is not so clear now in my opinion.
2. the user might get unclear message, if the vds is deleted then the user will
get 'VM_PINNING_WITHOUT_DEDICATED_VDS' although vds was chosen and he might try
again to press 'OK' without getting correct message about what went wrong.
3. I think that it'll be better to load VdsDynamic rather then VDS here -
loading vds is much 'heavier' query which involves few joins, no reason to
perform it.
Line 335: VM vmFromParams = getParameters().getVm();
Line 336: if (vmFromParams.getDedicatedVmForVds() != null &&
Line 337: (dedicatedVds =
DbFacade.getInstance().getVdsDao().
Line 338: get(vmFromParams.getDedicatedVmForVds()))
!= null) {
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmCommand.java
Line 286: return false;
Line 287: }
Line 288:
Line 289: // check cpuPinning
Line 290: VDS dedicatedVds;
1. the if clause is not so clear now in my opinion.
2. the user might get unclear message, if the vds is deleted then the user will
get 'VM_PINNING_WITHOUT_DEDICATED_VDS' although vds was chosen and he might try
again to press 'OK' without getting correct message about what went wrong.
3. I think that it'll be better to load VdsDynamic rather then VDS here -
loading vds is much 'heavier' query which involves few joins, no reason to
perform it.
Line 291: if (vmFromParams.getDedicatedVmForVds() != null &&
Line 292: (dedicatedVds =
DbFacade.getInstance().getVdsDao().get(vmFromParams.getDedicatedVmForVds())) !=
null) {
Line 293: VdcBllMessages returnMsg =
isCpuPinningValid(vmFromParams.getCpuPinning(),
Line 294: vmFromParams.getNumOfCpus(),
dedicatedVds.getcpu_cores() * dedicatedVds.getcpu_sockets());
Line 291: if (vmFromParams.getDedicatedVmForVds() != null &&
Line 292: (dedicatedVds =
DbFacade.getInstance().getVdsDao().get(vmFromParams.getDedicatedVmForVds())) !=
null) {
Line 293: VdcBllMessages returnMsg =
isCpuPinningValid(vmFromParams.getCpuPinning(),
Line 294: vmFromParams.getNumOfCpus(),
dedicatedVds.getcpu_cores() * dedicatedVds.getcpu_sockets());
Line 295: if (returnMsg != VdcBllMessages.Unassigned) {
you can use here failCanDoAction instead of those two lines
Line 296: addCanDoActionMessage(returnMsg);
Line 297: return false;
Line 298: }
Line 299: } else {
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmManagementCommandBase.java
Line 79: * @param maxPcpus
Line 80: * Number of pCPUs in the host
Line 81: * @return Error message or Unassigned if string is valid
Line 82: */
Line 83: static VdcBllMessages isCpuPinningValid(final String cpuPinning,
int maxVcpus, int maxPcpus) {
access modifier?
Line 84: if (StringUtils.isEmpty(cpuPinning)) {
Line 85: return VdcBllMessages.Unassigned;
Line 86: }
Line 87:
Line 107: }
Line 108:
Line 109:
Line 110:
Line 111: TreeSet<Integer> currPcpus =
parseCpuPinningNumbers(splitRule[1]);
why treeset?
Line 112: if (currPcpus == null) {
Line 113: return VdcBllMessages.VM_PINNING_FORMAT_INVALID;
Line 114: }
Line 115: if (currPcpus.size() == 0) {
Line 125:
Line 126: return VdcBllMessages.Unassigned;
Line 127: }
Line 128:
Line 129: private static TreeSet<Integer> parseCpuPinningNumbers(final
String text) {
weird identation all over this method
Line 130: try {
Line 131: TreeSet<Integer> include = new TreeSet<Integer>();
Line 132: TreeSet<Integer> exclude = new TreeSet<Integer>();
Line 133: String[] splitText = text.split(",");
Line 128:
Line 129: private static TreeSet<Integer> parseCpuPinningNumbers(final
String text) {
Line 130: try {
Line 131: TreeSet<Integer> include = new TreeSet<Integer>();
Line 132: TreeSet<Integer> exclude = new TreeSet<Integer>();
why treeset?
Line 133: String[] splitText = text.split(",");
Line 134: for (String section : splitText) {
Line 135: if (section.startsWith("^")) {
Line 136:
exclude.add(Integer.parseInt(section.substring(1)));
....................................................
File
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/VmManagementCommandBaseTest.java
Line 9: @Test
Line 10: public void isCpuPinningValid() {
Line 11: Assert.assertTrue("null value must be accepted",
Line 12: VmManagementCommandBase.isCpuPinningValid(null, 0, 0)
== VdcBllMessages.Unassigned);
Line 13:
assertEquals would be nicer here..
Line 14: Assert.assertTrue("empty string must be accepted",
Line 15: VmManagementCommandBase.isCpuPinningValid("", 0, 0) ==
VdcBllMessages.Unassigned);
Line 16:
Line 17:
Assert.assertTrue(VmManagementCommandBase.isCpuPinningValid("intentionally
invalid", 0, 0) == VdcBllMessages.VM_PINNING_FORMAT_INVALID);
--
To view, visit http://gerrit.ovirt.org/10364
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I690b60fcde6bf337058246ef9b1ee24fa3b7220a
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Noam Slomianko <nslom...@redhat.com>
Gerrit-Reviewer: Andrew Cathrow <and...@cathrow.com>
Gerrit-Reviewer: Doron Fediuck <dfedi...@redhat.com>
Gerrit-Reviewer: Einav Cohen <eco...@redhat.com>
Gerrit-Reviewer: Liron Aravot <lara...@redhat.com>
Gerrit-Reviewer: Noam Slomianko <nslom...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
