Moti Asayag has posted comments on this change.

Change subject: core: add NetworkLinking support to RunVm command.
......................................................................


Patch Set 20: (5 inline comments)

....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RunVmCommand.java
Line 779:         Set<String> clusterNetworksNames = 
Entities.objectNames(clusterNetworks);
Line 780: 
Line 781:         // Checking that the interface are all configured
Line 782:         // and they are attached to networks that are attached to the 
cluster
Line 783:         // and they are attached to VM networks
the comment above should also be updated to state unlinked network might be 
supported (or better, these comments should be deleted from here and be added 
as javadoc for each of the isVmInterface* methods.
Line 784:         return isVmInterfacesConfigured() &&
Line 785:                 
isVmInterfacesAttachedToClusterNetworks(clusterNetworksNames, 
interfaceNetworkNames) &&
Line 786:                 isVmInterfacesAttachedToVmNetworks(clusterNetworks, 
interfaceNetworkNames);
Line 787:     }


Line 789:     /**
Line 790:      * Checking that the interfaces are all configured If network 
linking is supported, interfaces with no network are
Line 791:      * allowed. Otherwise, canDoAction should be thrown.
Line 792:      *
Line 793:      * @param interfaceNetworkNames
the parameter should be removed from the javadoc.
Line 794:      *            VM interface network names
Line 795:      * @return true if all VM network interfaces are attached to 
existing cluster networks
Line 796:      */
Line 797:     private boolean isVmInterfacesConfigured() {


Line 791:      * allowed. Otherwise, canDoAction should be thrown.
Line 792:      *
Line 793:      * @param interfaceNetworkNames
Line 794:      *            VM interface network names
Line 795:      * @return true if all VM network interfaces are attached to 
existing cluster networks
the description of the return isn't accurate: the vNic could be also not 
attached to any network.
Line 796:      */
Line 797:     private boolean isVmInterfacesConfigured() {
Line 798:         for (VmNetworkInterface nic : getVm().getInterfaces()) {
Line 799:             if (nic.getNetworkName() == null) {


....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/VmNicValidator.java
Line 36:         return !networkLinkingSupported(version) && 
nic.getNetworkName() == null
Line 37:                 ? new 
ValidationResult(VdcBllMessages.NULL_NETWORK_IS_NOT_SUPPORTED, clusterVersion())
Line 38:                 : ValidationResult.VALID;
Line 39:     }
Line 40: 
i was wondering when this should come :-)

I think we can take it one step further into a class which contains all logic 
for detecting supported features.

if agreed, you can land the foundation here and later patches could make use of 
it to simplify existing code and new one.
The class could be located in common and could be used also by both clients and 
vdsbroker instead having same code repeated (same as repeated within this 
patch).
Line 41:     public static boolean networkLinkingSupported(String version) {
Line 42:         return Config.<Boolean> 
GetValue(ConfigValues.NetworkLinkingSupported, version);
Line 43:     }
Line 44: 


....................................................
Commit Message
Line 5: CommitDate: 2012-12-20 11:16:09 +0200
Line 6: 
Line 7: core: add NetworkLinking support to RunVm command.
Line 8: 
Line 9: - If "linked" property on VmNetworkInterface is true
commit message should be modify to reflect the changes:

instead of linkState with up/down now linkAction with true/false is sent to 
VDSm.
Line 10:  'linkState-> up' should be sent to the vdsm
Line 11: Otherwise-
Line 12:  'linkState-> down' should be sent to the vdsm
Line 13: 


--
To view, visit http://gerrit.ovirt.org/9518
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9805b18c364685a6533b89cfa31ae0575a00ea5
Gerrit-PatchSet: 20
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alona Kaplan <alkap...@redhat.com>
Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegu...@redhat.com>
Gerrit-Reviewer: Mike Kolesnik <mkole...@redhat.com>
Gerrit-Reviewer: Moti Asayag <masa...@redhat.com>
Gerrit-Reviewer: Muli Salem <msa...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to