Yevgeny Zaspitsky has posted comments on this change. Change subject: engine: Allow a VM trunk network be mixed with VLAN tagged ones ......................................................................
Patch Set 9: (11 comments) https://gerrit.ovirt.org/#/c/40784/9/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/network/LegacyNetworkExclusivenessValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/network/LegacyNetworkExclusivenessValidator.java: Line 18: the interface > s/the interface/the base interface Done Line 18: than > s/than/then Done Line 22: Vy > Vy ? Done https://gerrit.ovirt.org/#/c/40784/9/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/network/NetworkExclusivenessValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/network/NetworkExclusivenessValidator.java: Line 3: import java.util.List; Line 4: Line 5: import org.ovirt.engine.core.common.errors.VdcBllMessages; Line 6: Line 7: public interface NetworkExclusivenessValidator { > please format. there should be space line between the methods. Done Line 8: boolean isNetworkExclusive(List<NetworkType> networksOnIface); Line 9: VdcBllMessages getViolationMessage(); https://gerrit.ovirt.org/#/c/40784/9/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/network/VlanUntaggedNetworkExclusivenessValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/network/VlanUntaggedNetworkExclusivenessValidator.java: Line 24: this.untaggedNetworkPredicate = untaggedNetworkPredicate; Line 25: } Line 26: Line 27: /** Line 28: * Make sure that not more that at most one vlan-untagged network is attached to a given interface. > can you rephrase the sentence ? It should say that the method validates the Done Line 29: * Line 30: * @return true for a valid configuration, otherwise false. Line 31: */ Line 32: public boolean isNetworkExclusive(List<NetworkType> networksOnIface) { https://gerrit.ovirt.org/#/c/40784/9/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/NetworkAttachmentsValidatorTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/NetworkAttachmentsValidatorTest.java: Line 66: Line 67: networkMap = new BusinessEntityMap<>( Line 68: Arrays.asList(vlanNetwork, vmNetwork1, vmNetwork2, nonVmNetwork1, nonVmNetwork2)); Line 69: Line 70: Mockito.when(networkExclusivenessValidator.getViolationMessage()) > please use static import Done Line 71: .thenReturn(NETWORK_INTERFACES_NOT_EXCLUSIVELY_USED_BY_NETWORK); Line 72: } Line 73: Line 74: private Network createNetworkWithId() { https://gerrit.ovirt.org/#/c/40784/9/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/network/NetworkExclusivenessValidatorResolverTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/network/NetworkExclusivenessValidatorResolverTest.java: > Please use static import for Assert Done Line 1: package org.ovirt.engine.core.bll.validator.network; Line 2: Line 3: import static org.hamcrest.CoreMatchers.sameInstance; Line 4: import static org.ovirt.engine.core.common.config.ConfigValues.NetworkExclusivenessPermissiveValidation; https://gerrit.ovirt.org/#/c/40784/9/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/network/VlanUntaggedNetworkExclusivenessValidatorTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/network/VlanUntaggedNetworkExclusivenessValidatorTest.java: > please use static import Done Line 1: package org.ovirt.engine.core.bll.validator.network; Line 2: Line 3: import static org.hamcrest.CoreMatchers.is; Line 4: import static org.junit.Assert.assertFalse; Line 34: Mockito.when(mockUntaggedNetworkPredicate.evaluate(VLAN)).thenReturn(false); Line 35: Mockito.when(mockUntaggedNetworkPredicate.evaluate(VM)).thenReturn(true); Line 36: } Line 37: Line 38: @Test > you could have simplified it by using Parameterized runner (except for test I prefer named tests, when it fails the name should give a hint what gone wrong. Line 39: public void testIsNetworkExclusiveInvalid() { Line 40: assertFalse(underTest.isNetworkExclusive(Arrays.asList(VM, VLAN, VLAN, VM))); Line 41: } Line 42: https://gerrit.ovirt.org/#/c/40784/9/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/FeatureSupported.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/FeatureSupported.java: Line 553: Line 554: /** Line 555: * @param version Line 556: * Compatibility version to check for. Line 557: * @return <code>true</code> if SR-IOV network interfaces are supported for the given version > doesn't look like a SR-IOV to me ;-) Done Line 558: */ Line 559: public static boolean networkExclusivenessPermissiveValidation(Version version) { Line 560: return supportedInConfig(ConfigValues.NetworkExclusivenessPermissiveValidation, version); Line 561: } https://gerrit.ovirt.org/#/c/40784/9/backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties File backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties: Line 645: NETWORKS_DONT_EXIST_IN_CLUSTER=Cannot ${action} ${type}. The following Logical Networks do not exist in the Host's Cluster: ${NETWORKS_DONT_EXIST_IN_CLUSTER_LIST}. Line 646: NETWORKS_DONT_EXIST_IN_DATA_CENTER=Cannot ${action} ${type}. The following Logical Networks: ${networkIds} do not exist in the Data Center: ${dataCenterId}. Line 647: NETWORK_BONDS_INVALID_SLAVE_COUNT=Cannot ${action} ${type}. The following bonds consist of less than two Network Interfaces: ${NETWORK_BONDS_INVALID_SLAVE_COUNT_LIST}. Line 648: NETWORK_INTERFACES_NOT_EXCLUSIVELY_USED_BY_NETWORK=Cannot ${action} ${type}. The following Network Interfaces can have only a single VM Logical Network, or at most one non-VM Logical Network and/or several VLAN Logical Networks: ${NETWORK_INTERFACES_NOT_EXCLUSIVELY_USED_BY_NETWORK_LIST}. Line 649: NETWORK_INTERFACES_NOT_EXCLUSIVELY_USED_BY_UNTAGGED_NETWORK=Cannot ${action} ${type}. The following Network Interfaces can at most one VLAN-untagged Logical Network, optionally in conjunction with several VLAN Logical Networks: ${NETWORK_INTERFACES_NOT_EXCLUSIVELY_USED_BY_NETWORK_LIST}. > as on that method javadoc, this should be rephrased to simplified the messa Done Line 650: NETWORK_CANNOT_DETACH_NETWORK_USED_BY_VMS=Cannot ${action} ${type}. The following VMs are actively using the Logical Network: ${NETWORK_CANNOT_DETACH_NETWORK_USED_BY_VMS_LIST}. Please stop the VMs and try again. Line 651: NON_VM_NETWORK_CANNOT_SUPPORT_STP=Cannot ${action} ${type}. STP can only be enabled on VM Networks. Line 652: NETWORK_MTU_DIFFERENCES=Cannot ${action} ${type}. The following Logical Networks do not have the same MTU value: ${NETWORK_MTU_DIFFERENCES_LIST}. Line 653: NETWORK_MTU_OVERRIDE_NOT_SUPPORTED=Cannot ${action} ${type}. Overriding MTU is not supported for this Data Center's compatibility version. -- To view, visit https://gerrit.ovirt.org/40784 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3f0b52ad6bb91947848cb03d516b2b014f876769 Gerrit-PatchSet: 9 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yevgeny Zaspitsky <yzasp...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Yevgeny Zaspitsky <yzasp...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
