Mike Kolesnik has posted comments on this change.
Change subject: engine: Support interface QoS override in Setup Networks
......................................................................
Patch Set 4: Code-Review-1
(4 comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/SetupNetworksCommand.java
Line 31: import org.ovirt.engine.core.utils.log.LogFactory;
Line 32: import org.ovirt.engine.core.utils.transaction.TransactionMethod;
Line 33: import org.ovirt.engine.core.utils.transaction.TransactionSupport;
Line 34:
Line 35: @NonTransactiveCommandAttribute
You should add a check that the user has a CONFIGURE_STORAGE_POOL_NETWORK
permission if he is trying to set a specific QoS for any of the NICs.
Line 36: public class SetupNetworksCommand<T extends SetupNetworksParameters>
extends VdsCommand<T> {
Line 37:
Line 38: /** Time between polling attempts, to prevent flooding the
host/network. */
Line 39: private static final long POLLING_BREAK = 500;
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/SetupNetworksHelper.java
Line 329:
Line 330: if (existingIface != null &&
existingIface.getNetworkImplementationDetails() != null
Line 331: &&
!existingIface.getNetworkImplementationDetails().isInSync()) {
Line 332: if (networkShouldBeSynced(networkName)) {
Line 333: modifiedNetworks.add(network);
You should also handle this case?
Line 334: } else if (networkWasModified(iface)) {
Line 335:
addViolation(VdcBllMessages.NETWORKS_NOT_IN_SYNC, networkName);
Line 336: }
Line 337: } else {
....................................................
File
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/host/SetupNetworksHelperTest.java
Line 1684: return createHelper(params, vds);
Line 1685: }
Line 1686:
Line 1687: private SetupNetworksHelper createHelper(SetupNetworksParameters
params, VDS vds) {
Line 1688:
when(vds.getVdsGroupCompatibilityVersion()).thenReturn(Version.v3_3);
Why add here and not in the specific methods you need it?
Either way it's not related to the helper..
Line 1689:
Line 1690: SetupNetworksHelper helper = spy(new
SetupNetworksHelper(params, vds));
Line 1691:
Line 1692:
when(helper.getVmInterfaceManager()).thenReturn(vmInterfaceManager);
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/NetworkUtils.java
Line 171: public static boolean isNetworkInSync(VdsNetworkInterface iface,
Network network, NetworkQoS qos) {
Line 172: return (network.getMtu() == 0 || iface.getMtu() ==
network.getMtu())
Line 173: && Objects.equals(iface.getVlanId(),
network.getVlanId())
Line 174: && iface.isBridged() == network.isVmNetwork()
Line 175: && (Objects.equals(iface.getQos(), qos) ||
iface.isQosOverridden());
As I stated, the syncness should not be affected by the fact the user decided
to override the QoS. It is entirely possible that this will get out of sync
even if he decided to put his own QoS definition.
Line 176: }
Line 177:
Line 178: /**
Line 179: * Returns true if a given network is non-VM network with no Vlan
tagging, else false.
--
To view, visit http://gerrit.ovirt.org/22766
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I22a4fefac1c5e80c98a72507623152ea4d30ef07
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Lior Vernia <[email protected]>
Gerrit-Reviewer: Mike Kolesnik <[email protected]>
Gerrit-Reviewer: Moti Asayag <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
