Doron Fediuck has posted comments on this change.
Change subject: engine: Add validation to cpu pinning
......................................................................
Patch Set 2: I would prefer that you didn't submit this
(5 inline comments)
Noam,
some more work needed here. See inline.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java
Line 329: returnValue = false;
Line 330: }
Line 331:
Line 332: // check cpuPinning
Line 333: if (returnValue) {
It seems that you're adding logic here, so you have both isCpuPinningValid and
the new logic added here.
One place should be enough for pinning validation. I prefer that you extend the
validation method so this line should remain clean and simple.
Line 334: VDS dedicatedVds;
Line 335: VM vmFromParams = getParameters().getVm();
Line 336: if (vmFromParams.getDedicatedVmForVds() != null &&
Line 337: (dedicatedVds =
DbFacade.getInstance().getVdsDao().
....................................................
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;
Same here WRT validation method; put relevant validation code in one place.
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());
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/VdcBllMessages.java
Line 239: VM_CLUSTER_IS_NOT_VALID,
Line 240: VM_CANNOT_REMOVE_VM_WHEN_STATUS_IS_NOT_DOWN,
Line 241: VM_CANNOT_EXPORT_RAW_FORMAT,
Line 242: VM_PINNING_FORMAT_INVALID,
Line 243: VM_PINNING_VCPU_DOESNT_EXIST,
No abbreviations please.
DOESNT => DOES_NOT or VM_PINNING_NO_SUCH_VCPU
Line 244: VM_PINNING_PCPU_DOESNT_EXIST,
Line 245: VM_PINNING_WITHOUT_DEDICATED_VDS,
Line 246: VM_PINNING_DUPLICATE_DEFINITION,
Line 247: VM_PINNING_PINNED_TO_NO_CPU,
Line 240: VM_CANNOT_REMOVE_VM_WHEN_STATUS_IS_NOT_DOWN,
Line 241: VM_CANNOT_EXPORT_RAW_FORMAT,
Line 242: VM_PINNING_FORMAT_INVALID,
Line 243: VM_PINNING_VCPU_DOESNT_EXIST,
Line 244: VM_PINNING_PCPU_DOESNT_EXIST,
DOESNT => DOES_NOT or VM_PINNING_NO_SUCH_PCPU
Line 245: VM_PINNING_WITHOUT_DEDICATED_VDS,
Line 246: VM_PINNING_DUPLICATE_DEFINITION,
Line 247: VM_PINNING_PINNED_TO_NO_CPU,
Line 248: CANNOT_PREIEW_CURRENT_IMAGE,
Line 241: VM_CANNOT_EXPORT_RAW_FORMAT,
Line 242: VM_PINNING_FORMAT_INVALID,
Line 243: VM_PINNING_VCPU_DOESNT_EXIST,
Line 244: VM_PINNING_PCPU_DOESNT_EXIST,
Line 245: VM_PINNING_WITHOUT_DEDICATED_VDS,
There's a configuration which may allow this,
please check that. If needed talk to me.
Line 246: VM_PINNING_DUPLICATE_DEFINITION,
Line 247: VM_PINNING_PINNED_TO_NO_CPU,
Line 248: CANNOT_PREIEW_CURRENT_IMAGE,
Line 249: ACTION_TYPE_FAILED_XP_MEMORY_ERROR,
--
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
