Arik Hadas has posted comments on this change.
Change subject: core: move network validations on run vm to RunVmValidator
......................................................................
Patch Set 1:
(3 comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/RunVmValidator.java
Line 292: }
Line 293: return map;
Line 294: }
Line 295:
Line 296: protected NetworkDao getNetworkDAO() {
it was copied from AuditLogableBase.. but np, I'll change it
Line 297: return DbFacade.getInstance().getNetworkDao();
Line 298: }
Line 299:
Line 300: protected VdsDAO getVdsDao() {
Line 392:
Line 393: /**
Line 394: * @return true if all VM network interfaces are valid
Line 395: */
Line 396: public ValidationResult validateNetworkInterfaces(VM vm) {
There's a work in progress to improve this class, few related patches were
merged recently.
I'm not sure about the 'state' you suggested though - on the one hand, yes it's
redundant to pass the VM to each validation method and we can save it as class
field, but on the other hand if we want to preserve the ability to invoke each
validation method separately from outside of this class, we'll need to supply
the validation method all the information it needs.
I'm still considering if the best way to do it. there're two options I see:
1. to keep it stateless, where every method gets all the information it needs
and remove the canRunVm method which got too many parameters. then we'll use it
much like SnapshotsValidator is used at CreateAllSnapshotsFromVm#canDoAction in
RunVmCommand and VmPoolCommandBase
2. to have RunVmValidatorBase in which all those validations method will reside
and we'll have two class that extend it: RunVmValidator and RunPoolVmValidator
which have methods like canRunVm
Line 397: Map<String, VmNetworkInterface> interfaceNetworkMap =
Entities.vmInterfacesByNetworkName(vm.getInterfaces());
Line 398: Set<String> interfaceNetworkNames =
interfaceNetworkMap.keySet();
Line 399: List<Network> clusterNetworks =
getNetworkDAO().getAllForCluster(vm.getVdsGroupId());
Line 400: Set<String> clusterNetworksNames =
Entities.objectNames(clusterNetworks);
Line 398: Set<String> interfaceNetworkNames =
interfaceNetworkMap.keySet();
Line 399: List<Network> clusterNetworks =
getNetworkDAO().getAllForCluster(vm.getVdsGroupId());
Line 400: Set<String> clusterNetworksNames =
Entities.objectNames(clusterNetworks);
Line 401:
Line 402: ValidationResult validationResult =
isVmInterfacesConfigured(vm);
yes, I noticed that as well. I thought of a different way to solve it: the
private methods will return boolean, and the 'validateNetworkInterfaces' method
will call them, and if a method returns false it will return a ValidationResult
with a proper error.
I mean, I don't see a reason for invoking each sub-method separately. we'll
probably want to validate that network interfaces are valid and we'll need all
of them for that right? if so, we can keep exposing one method as public and it
will call the others.
what do you think?
Line 403: if (!validationResult.isValid()) {
Line 404: return validationResult;
Line 405: }
Line 406:
--
To view, visit http://gerrit.ovirt.org/17971
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6aff70b45617e90deac826f2a873c1604c9a4241
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Arik Hadas <aha...@redhat.com>
Gerrit-Reviewer: Arik Hadas <aha...@redhat.com>
Gerrit-Reviewer: Moti Asayag <masa...@redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com>
Gerrit-Reviewer: Roy Golan <rgo...@redhat.com>
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
