Doron Fediuck has posted comments on this change. Change subject: engine: added EvenGuestDistribution policy ......................................................................
Patch Set 6: (9 comments) Jirka, see inline for some comments. Nothing serious, but needs handling. http://gerrit.ovirt.org/#/c/23103/6//COMMIT_MSG Commit Message: Line 3: AuthorDate: 2014-01-13 14:21:40 +0100 Line 4: Commit: Jiri Moskovcak <jmosk...@redhat.com> Line 5: CommitDate: 2014-01-14 09:51:48 +0100 Line 6: Line 7: engine: added EvenGuestDistribution policy Good name choice! Line 8: Line 9: This change is based on a customer RFE. The new policy Line 10: should make sure that the VMs are evenly distributed Line 11: among all host in the cluster so every host runs http://gerrit.ovirt.org/#/c/23103/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/EvenDistributionBalancePolicyUnit.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/EvenDistributionBalancePolicyUnit.java: Line 49: * && cpuOverCommitMinutes >= CpuOverCommitDurationMinutes Line 50: */ Line 51: List<VDS> overUtilizedHosts = getOverUtilizedHosts(hosts, parameters); Line 52: Line 53: // if no hosts is overutilized, then there is nothing to balance... s/is/are/ Line 54: if (overUtilizedHosts == null || overUtilizedHosts.size() == 0) { Line 55: return null; Line 56: } Line 57: Line 50: */ Line 51: List<VDS> overUtilizedHosts = getOverUtilizedHosts(hosts, parameters); Line 52: Line 53: // if no hosts is overutilized, then there is nothing to balance... Line 54: if (overUtilizedHosts == null || overUtilizedHosts.size() == 0) { Please log, explaining no over-utilization. Line 55: return null; Line 56: } Line 57: Line 58: // returns hosts with utilization lower then the specified threshold Line 54: if (overUtilizedHosts == null || overUtilizedHosts.size() == 0) { Line 55: return null; Line 56: } Line 57: Line 58: // returns hosts with utilization lower then the specified threshold s/then/than/ Line 59: List<VDS> underUtilizedHosts = getUnderUtilizedHosts(cluster, hosts, parameters); Line 60: Line 61: //if no host has a spare power, then there is nothing we can do to balance it.. Line 62: if (underUtilizedHosts == null || underUtilizedHosts.size() == 0) { Line 58: // returns hosts with utilization lower then the specified threshold Line 59: List<VDS> underUtilizedHosts = getUnderUtilizedHosts(cluster, hosts, parameters); Line 60: Line 61: //if no host has a spare power, then there is nothing we can do to balance it.. Line 62: if (underUtilizedHosts == null || underUtilizedHosts.size() == 0) { Please log, explaining no under-utilization. This is a serious issue, since the whole cluster is overloaded. Line 63: return null; Line 64: } Line 65: VDS randomHost = overUtilizedHosts.get(new Random().nextInt(overUtilizedHosts.size())); Line 66: List<VM> migrableVmsOnRandomHost = getMigrableVmsRunningOnVds(randomHost.getId()); http://gerrit.ovirt.org/#/c/23103/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/EvenGuestDistributionBalancePolicyUnit.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/EvenGuestDistributionBalancePolicyUnit.java: Line 10: Line 11: import java.util.List; Line 12: import java.util.Map; Line 13: Line 14: public class EvenGuestDistributionBalancePolicyUnit extends EvenDistributionBalancePolicyUnit { Since you're extending, you may want to make sure any logging is using the right class name, so we won't get confused. Line 15: Line 16: Line 17: public EvenGuestDistributionBalancePolicyUnit (PolicyUnit policyUnit) { Line 18: super(policyUnit); Line 41: occupiedSlots += SPMVMCountGrace; Line 42: Line 43: return occupiedSlots; Line 44: } Line 45: // This method returns the vds running the biggest number of VMs. Line 46: private VDS getWorstVDS(List<VDS> relevantHosts, Map<String, String> parameters) { Line 47: VDS worstVDS = relevantHosts.get(0); Line 48: for (VDS vds: relevantHosts) { Line 49: if (getOccupiedVMSLots(vds, parameters) > getOccupiedVMSLots(worstVDS, parameters)) Line 45: Line 46: private VDS getWorstVDS(List<VDS> relevantHosts, Map<String, String> parameters) { Line 47: VDS worstVDS = relevantHosts.get(0); Line 48: for (VDS vds: relevantHosts) { Line 49: if (getOccupiedVMSLots(vds, parameters) > getOccupiedVMSLots(worstVDS, parameters)) > add parentheses. We force code blocks, so indeed every if/for/etc should open a new code block. Also, since worstVDS may not change in every iteration, we can save a call to getOccupiedVMSLots, and do it only when worstVDS changes. ie- VDS worstVDS = relevantHosts.get(0); int worstVdsSlots = 0; for (...) { if (getOccupiedVMSLots(vds, parameters) > worstVdsSlots) { worstVDS = vds; worstVdsSlots = getOccupiedVMSLots(worstVDS, parameters); } } return worstVDS; Line 50: worstVDS = vds; Line 51: } Line 52: Line 53: return worstVDS; http://gerrit.ovirt.org/#/c/23103/6/backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/model/SchedulingPolicyType.java File backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/model/SchedulingPolicyType.java: Line 32: /** Line 33: * Distributes the guests in a way that every host host has approximately the same Line 34: * number of running guests Line 35: */ Line 36: VM_EVENLY_DISTRIBUTED, Please move VM_EVENLY_DISTRIBUTED to the last position to maintain enum order. None is a real policy and not just a dummy. Line 37: NONE; Line 38: Line 39: public String value() { Line 40: return name().toLowerCase(); -- To view, visit http://gerrit.ovirt.org/23103 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2f2036dfc8a410c787a195d7d56ac3ed4ace81a7 Gerrit-PatchSet: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Jiří Moskovčák <jmosk...@redhat.com> Gerrit-Reviewer: Doron Fediuck <dfedi...@redhat.com> Gerrit-Reviewer: Gilad Chaplik <gchap...@redhat.com> Gerrit-Reviewer: Jiří Moskovčák <jmosk...@redhat.com> Gerrit-Reviewer: Juan Hernandez <juan.hernan...@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
