Alona Kaplan has posted comments on this change. Change subject: engine: Enforce maximal QoS commitment on interface ......................................................................
Patch Set 10: (4 comments) http://gerrit.ovirt.org/#/c/34133/10/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/SetupNetworksHelper.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/SetupNetworksHelper.java: Line 274: .get(network.getQosId()); Line 275: Line 276: Float baseInterfaceCommitment = relativeCommitmentByBaseInterface.get(baseIfaceName); Line 277: if (baseInterfaceCommitment == null) { Line 278: baseInterfaceCommitment = new Float(0); why not- 0f? Line 279: } Line 280: Line 281: Integer subInterfaceCommitment = qos.getOutAverageRealtime(); Line 282: if (subInterfaceCommitment != null) { Line 330: if (speed != null) { Line 331: return speed; Line 332: } Line 333: Line 334: // not cached, compute and cache for future reference In case of a bond that was unchanged, you can use the speed value provided by the vdsm instead of calculating it. Line 335: boolean chooseMinimum = isBondModeFailover(iface.getBondType()); Line 336: speed = chooseMinimum ? Integer.MAX_VALUE : 0; Line 337: for (VdsNetworkInterface slave : slaves) { Line 338: speed = chooseMinimum ? Math.min(speed, slave.getSpeed()) : speed + slave.getSpeed(); Line 331: return speed; Line 332: } Line 333: Line 334: // not cached, compute and cache for future reference Line 335: boolean chooseMinimum = isBondModeFailover(iface.getBondType()); 1. As I see the bond type is never used or initialized, so you need to check the bondOptions not the type. 2. What is mode 3? 3. We need to think if we want (and know how) the backend to calculate new/changed bond's speed. What about custom bond mode? Line 336: speed = chooseMinimum ? Integer.MAX_VALUE : 0; Line 337: for (VdsNetworkInterface slave : slaves) { Line 338: speed = chooseMinimum ? Math.min(speed, slave.getSpeed()) : speed + slave.getSpeed(); Line 339: } http://gerrit.ovirt.org/#/c/34133/10/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/host/SetupNetworksHelperTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/host/SetupNetworksHelperTest.java: Line 471: return qosCommitmentSetup("nic0", totalCommitment); Line 472: } Line 473: Line 474: @Test Line 475: public void qosValidCommitment() { Please add tests for different types of bonds. Line 476: SetupNetworksHelper helper = qosCommitmentSetup(DEFAULT_SPEED / 2); Line 477: validateAndExpectNoViolations(helper); Line 478: } Line 479: -- To view, visit http://gerrit.ovirt.org/34133 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Icc89f4193d6b7b590c6e1e053af74ab23bc77a11 Gerrit-PatchSet: 10 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Lior Vernia <[email protected]> Gerrit-Reviewer: Alona Kaplan <[email protected]> Gerrit-Reviewer: Lior Vernia <[email protected]> Gerrit-Reviewer: [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
