Gilad Chaplik has posted comments on this change. Change subject: engine: added EvenGuestDistribution policy ......................................................................
Patch Set 6: (11 comments) minor comments. http://gerrit.ovirt.org/#/c/23103/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/PolicyUnitImpl.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/PolicyUnitImpl.java: Line 191: // TODO Auto-generated method stub Line 192: policyUnit.setEnabled(enabled); Line 193: } Line 194: Line 195: protected int tryParseWithDefault(String candidate, int defaultValue) { who get there first, remove this method and use NumberUtils.toInt(str, defaultValue) :-) Line 196: if (candidate != null) { Line 197: try { Line 198: return Integer.parseInt(candidate); Line 199: } catch (Exception e) { 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 46: // get vds that over committed for the time defined Line 47: /* returns list of Hosts with Line 48: * cpuUtilization >= highUtilization Line 49: * && cpuOverCommitMinutes >= CpuOverCommitDurationMinutes Line 50: */ stick with // 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) { 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 18: super(policyUnit); Line 19: } Line 20: Line 21: private int getSPMGraceDefaultValue() { Line 22: return Config.<Integer> getValue(ConfigValues.SPMVMGraceForEvenGuestDistribute); Use camel case naming. Line 23: } Line 24: Line 25: private int getMigrationThresholdDefault() { Line 26: return Config.<Integer> getValue(ConfigValues.MigrationThresholdForEvenGuestDistribute); Line 27: } Line 28: Line 29: private int getHighVMCountDefaultValue() { Line 30: return Config.<Integer> getValue(ConfigValues.HighVMCountForEvenGuestDistribute); Line 31: } all the above config values can be initialized when class is created. Line 32: Line 33: /* returns the number of running VMS on given VDS Line 34: if the VDS is SPM the return value is the number of running VMS + SPMVMCountGrace Line 35: */ Line 34: if the VDS is SPM the return value is the number of running VMS + SPMVMCountGrace Line 35: */ Line 36: private int getOccupiedVMSLots(VDS vds, Map<String, String> parameters) { Line 37: int occupiedSlots = vds.getVmActive(); Line 38: final int SPMVMCountGrace = tryParseWithDefault(parameters.get("SPMVMCountGrace"), * method vars start with a lowercase letter, and use camel case naming SpmVmCountGrace, more readable. * I guess that this line can move into the if block Line 39: getSPMGraceDefaultValue()); Line 40: if (vds.isSpm()) Line 41: occupiedSlots += SPMVMCountGrace; Line 42: 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. Line 50: worstVDS = vds; Line 51: } Line 52: Line 53: return worstVDS; Line 90: int distance = getOccupiedVMSLots(worstVDS, parameters) - getOccupiedVMSLots(p, parameters); Line 91: return distance < migrationThreshold; Line 92: } Line 93: }); Line 94: } something is bugging me about the number of calls to getOccupiedVMSLots, the result is static, so can't we calculate it once? Line 95: http://gerrit.ovirt.org/#/c/23103/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/EvenGuestDistributionWeightPolicyUnit.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/EvenGuestDistributionWeightPolicyUnit.java: Line 19: } Line 20: Line 21: private int getSPMGraceDefaultValue() { Line 22: return Config.<Integer> getValue(ConfigValues.SPMVMGraceForEvenGuestDistribute); Line 23: } same comment as in previous file Line 24: Line 25: private int getOccupiedVMSLots(VDS vds, Map<String, String> parameters) { Line 26: int occupiedSlots = vds.getVmActive(); Line 27: final int SPMVMCountGrace = tryParseWithDefault(parameters.get("SPMVMCountGrace"), Line 24: Line 25: private int getOccupiedVMSLots(VDS vds, Map<String, String> parameters) { Line 26: int occupiedSlots = vds.getVmActive(); Line 27: final int SPMVMCountGrace = tryParseWithDefault(parameters.get("SPMVMCountGrace"), Line 28: getSPMGraceDefaultValue()); same Line 29: if (vds.isSpm()) Line 30: occupiedSlots += SPMVMCountGrace; Line 31: Line 32: return occupiedSlots; Line 32: return occupiedSlots; Line 33: } Line 34: Line 35: private int calcEvenGuestDistributionScore(VDS vds) { Line 36: return Math.max(0, vds.getVmCount()); prefer we'd have a bug instead of covering it. Line 37: } Line 38: Line 39: @Override Line 40: public List<Pair<Guid, Integer>> score(List<VDS> hosts, VM vm, Map<String, String> parameters) { http://gerrit.ovirt.org/#/c/23103/6/packaging/etc/engine-config/engine-config.properties File packaging/etc/engine-config/engine-config.properties: Line 358: HighVMCount.description="Maximum VM limit. Exceeding it qualifies the host as overloaded" Line 359: MigrationThreshold.type=Integer Line 360: MigrationThreshold.description="Defines a buffer before we start migrating VMs from the host" Line 361: SPMVMGrace.type=Integer Line 362: SPMVMGrace.description="Defines how many slots for VMs should be reserved on SPM hosts" camel case. -- 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
