Omer Frenkel has posted comments on this change.
Change subject: core: Move RunVm validation logic to RunVmCommand (1/15)
......................................................................
Patch Set 2: I would prefer that you didn't submit this
(4 inline comments)
i dont like this approach, shared code should be in shared place, this patch
cause small and nice canDoAction method to become huge and unreadable,
in my opinion it would be better to:
first and most important - make sure we have tests for each check and scenario,
when we have test we can start refactor:
split huge check methods to small and focused by check,
move all common checks to common place
call checks from a clean and nice canDoAction method
please check out example:ActivateStorageDomainCommand
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RunVmCommand.java
Line 683: // if VM is paused, it was already checked before that it is
capable to run
Line 684: if (vm.getStatus() == VMStatus.Paused) {
Line 685: return true;
Line 686: }
Line 687: /********* VmRunHandler.canRunVm START **********/
the above comment make no sense
Line 688:
Line 689: ArrayList<String> messages =
getReturnValue().getCanDoActionMessages();
Line 690: boolean canDoAction = true;
Line 691: List<VmPropertiesUtils.ValidationError> validationErrors =
getVmPropertiesUtils().validateVMProperties(
Line 818: }
Line 819: }
Line 820: }
Line 821:
Line 822: /********* VmRunHandler.canRunVm END **********/
this one as well
Line 823: canDoAction &= validateNetworkInterfaces();
Line 824:
Line 825: // check for Vm Payload
Line 826: if (canDoAction && getParameters().getVmPayload() != null) {
Line 819: }
Line 820: }
Line 821:
Line 822: /********* VmRunHandler.canRunVm END **********/
Line 823: canDoAction &= validateNetworkInterfaces();
i am pretty sure this is a bug, validateNetworkInterfaces() will be called even
if canDoAction is false...
you need to do canDoAction = canDoAction && validateNetworkInterfaces()
to get what you want
Line 824:
Line 825: // check for Vm Payload
Line 826: if (canDoAction && getParameters().getVmPayload() != null) {
Line 827: canDoAction = checkPayload(getParameters().getVmPayload(),
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmPoolCommandBase.java
Line 266: true,
Line 267: new VdsFreeMemoryChecker(new
NonWaitingDelayer()));
Line 268:
Line 269: // TODO: temporary until this logic will be extracted.
Line 270: RunVmCommand<RunVmParams> runVmCommand = new
RunVmCommand<RunVmParams>(runVmParams);
you should not create commands like this, for sure not just to run its
canDoAction,
better approach is to move shared checks to some shared placed and call it
Line 271:
runVmCommand.getReturnValue().setCanDoActionMessages(messages);
Line 272: return runVmCommand.canDoAction();
Line 273: }
Line 274:
--
To view, visit http://gerrit.ovirt.org/13111
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id4f65185ac3c3e2b6d65dd772268dbccaf9168a8
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Gilad Chaplik <[email protected]>
Gerrit-Reviewer: Arik Hadas <[email protected]>
Gerrit-Reviewer: Doron Fediuck <[email protected]>
Gerrit-Reviewer: Gilad Chaplik <[email protected]>
Gerrit-Reviewer: Laszlo Hornyak <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
Gerrit-Reviewer: Roy Golan <[email protected]>
Gerrit-Reviewer: noam slomianko <[email protected]>
Gerrit-Reviewer: ofri masad <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
