Ayal Baron has posted comments on this change.
Change subject: core: block vm migration if cpu pinning enabled
......................................................................
Patch Set 15: (12 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java
Line 540: if (vmStaticData != null) {
while you're at it, why not change this to positive and return false early?
Line 546:
reasons.add(VdcBllMessages.ACTION_TYPE_FAILED_VM_CANNOT_BE_HIGHLY_AVAILABLE_AND_PINNED_TO_HOST
unless the other functions add anything to VdcBllMessages (doesn't look like it
here, but I could be wrong) then there is no point in going on, just return
false.
Line 550: if(!returnValue) {
why have the same condition twice in a row?
looks like you meant if (returnValue) ?
Line 555: returnValue = false;
again, return false.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmCommand.java
Line 284:
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_VM_CANNOT_BE_PINNED_TO_CPU_AND_MIGRATABLE);
you already add a can do action message inside isPinningAndMigrationValid (not
that I understand why an 'is' function would do that), so why do it again here?
Line 285: return false;
you're adding code to a function which is already over 120 loc.
Why not move pinning logic into a separate function to be called from here?
(both isCpuPinningValid and isPinningAndMigrationValid)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmManagementCommandBase.java
Line 77: && StringUtils.isNotEmpty(cpuPinning)) {
I would check isNotEmpty(cpuPinning) before MigrationSupport as it is more
likely to return false and is probably also just a tad faster
Line 78:
reasons.add(VdcBllMessages.ACTION_TYPE_FAILED_VM_CANNOT_BE_PINNED_TO_CPU_AND_MIGRATABLE.toString());
an 'is' function should not mess around with vdcBllMessages.
either this should be validatePinning... or the message should be added
somewhere else.
Also, as noted in calling function, having 2 messages added for same thing
seems quite redundant.
....................................................
File backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties
Line 758:
ACTION_TYPE_FAILED_VM_CANNOT_BE_PINNED_TO_CPU_AND_MIGRATABLE=Migratable VM's
cannot be pinned to CPU.
"cannot" is incorrect, as a simple config change would make this possible.
should be something like 'Current configuration prevents pinning migratable
VMs. To change this set CpuPinMigrationEnabled to True'
....................................................
Commit Message
Line 9: Blocks VM migration of cpu pinning is enabled.
s/of/if/
....................................................
File
frontend/webadmin/modules/userportal-gwtp/src/main/resources/org/ovirt/engine/ui/frontend/AppErrors.properties
Line 154:
ACTION_TYPE_FAILED_VM_CANNOT_BE_PINNED_TO_CPU_AND_MIGRATABLE=Migratable VM's
cannot be pinned to CPU.
same as previous
....................................................
File
frontend/webadmin/modules/webadmin/src/main/resources/org/ovirt/engine/ui/frontend/AppErrors.properties
Line 702:
ACTION_TYPE_FAILED_VM_CANNOT_BE_PINNED_TO_CPU_AND_MIGRATABLE=Migratable VM's
cannot be pinned to CPU.
and again
--
To view, visit http://gerrit.ovirt.org/5065
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I38e7585d96ff0c5d5ea29222495b8d987f6aeb7b
Gerrit-PatchSet: 15
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Laszlo Hornyak <lhorn...@redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfedi...@redhat.com>
Gerrit-Reviewer: Laszlo Hornyak <lhorn...@redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
