Mike Kolesnik has posted comments on this change.
Change subject: engine: Include network QoS in Setup Networks
......................................................................
Patch Set 15:
(7 comments)
....................................................
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);
I would prefer that you use the QoS entity method of "equalValues" somehow,
since the general equals is relying on other properties which aren't
interesting for us to check in this case and therefore shouldn't influence our
decision.
Line 176: }
Line 177:
Line 178: /**
Line 179: * Returns true if a given network is non-VM network with no Vlan
tagging, else false.
....................................................
File
backend/manager/modules/utils/src/test/java/org/ovirt/engine/core/utils/NetworkUtilsTest.java
Line 67: iface.getNetworkName(),
Line 68: iface.isBridged(),
Line 69: iface.getMtu(),
Line 70: iface.getVlanId(),
Line 71: null);
You should also check when QoS is not null and NIC should be in sync with
network, i.e. all relevant fields are the same (and all irrelevant fields
should have random values).
Line 72: }
Line 73:
Line 74: @Test
Line 75: public void
calculateNetworkImplementationDetailsNetworkIsSyncWithMtuUnset() throws
Exception {
Line 142: iface.getNetworkName(),
Line 143: iface.isBridged(),
Line 144: iface.getMtu(),
Line 145: iface.getVlanId(),
Line 146: new NetworkQoS());
Please add more granular checks for the specific values of the QoS entity.
Line 147: }
Line 148:
Line 149: @Test
Line 150: public void interfaceBasedOn() {
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/SetupNetworksVDSCommand.java
Line 58: if (network.isVmNetwork()) {
Line 59: opts.put(VdsProperties.STP, network.getStp() ? "yes" :
"no");
Line 60: }
Line 61:
Line 62: if
(FeatureSupported.HostNetworkQos(getVds().getVdsGroupCompatibilityVersion())) {
Shouldn't you first check if a QoS ID is set on the network?
I think it would be more clear this way.
Line 63: NetworkQosMapper qosMapper =
Line 64: new NetworkQosMapper(opts,
VdsProperties.HOST_QOS_INBOUND, VdsProperties.HOST_QOS_OUTBOUND);
Line 65: qosMapper.serialize(qosDao.get(network.getQosId()));
Line 66: }
....................................................
File
backend/manager/modules/vdsbroker/src/test/java/org/ovirt/engine/core/vdsbroker/vdsbroker/SetupNetworksVDSCommandTest.java
Line 156: NetworkQoS expectedQos,
Line 157: Version clusterCompatibilityVersion) {
Line 158:
Line 159:
configRule.mockConfigValue(ConfigValues.HostNetworkQosSupported, Version.v3_3,
false);
Line 160:
configRule.mockConfigValue(ConfigValues.HostNetworkQosSupported, new Version(3,
4), true);
You don't have to use real versions, since you're mocking it anyway you can
just set fake values (even mocks).
Anyway, I'd expect it to be more like:
configRule.mockConfigValue(ConfigValues.HostNetworkQosSupported,
clusterCompatibilityVersion, mockQosSupport);
Where mockQosSupport is a parameter stating if QoS should be supported or not.
Line 161:
when(host.getVdsGroupCompatibilityVersion()).thenReturn(clusterCompatibilityVersion);
Line 162:
Line 163: SetupNetworksVdsCommandParameters parameters =
Line 164: new SetupNetworksVdsCommandParameters(Guid.newGuid(),
Line 173: verifyMethodPassedToHost();
Line 174: Map<String, Object> networkStruct =
assertNeworkWasSent(network);
Line 175: NetworkQosMapper qosMapper =
Line 176: new NetworkQosMapper(networkStruct,
VdsProperties.HOST_QOS_INBOUND, VdsProperties.HOST_QOS_OUTBOUND);
Line 177: assertEquals(qosMapper.deserialize(), expectedQos);
When assert*()ing, the expected value is always first.
Line 178: }
Line 179:
Line 180: @Test
Line 181: public void qosNotSupported() {
Line 337: private NetworkQoS createQos() {
Line 338: NetworkQoS qos = new NetworkQoS();
Line 339: qos.setInboundAverage(30);
Line 340: qos.setInboundPeak(30);
Line 341: qos.setInboundBurst(30);
You should set random values if they have no special meaning
Line 342: return qos;
Line 343: }
--
To view, visit http://gerrit.ovirt.org/22604
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5f0d2c3fe430961f7a402518ce1358938ed52063
Gerrit-PatchSet: 15
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Lior Vernia <lver...@redhat.com>
Gerrit-Reviewer: Lior Vernia <lver...@redhat.com>
Gerrit-Reviewer: Mike Kolesnik <mkole...@redhat.com>
Gerrit-Reviewer: Moti Asayag <masa...@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
