Roy Golan has posted comments on this change. Change subject: WIP Update balancers and add memory based load balancing ......................................................................
Patch Set 4: (9 comments) https://gerrit.ovirt.org/#/c/38189/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/CpuAndMemoryBalancingPolicyUnit.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/CpuAndMemoryBalancingPolicyUnit.java: Line 63: * returns list of Hosts with Line 64: * cpuUtilization >= highUtilization Line 65: * && cpuOverCommitMinutes >= CpuOverCommitDurationMinutes Line 66: */ Line 67: List<VDS> overUtilizedCPUHosts = getPrimarySources(cluster, hosts, parameters); few notes regarding the getXXX sources api: * it should return null ever - lets use Collection.emptyList() as the default answer. and we'll get less clutter due to null-checking * name is a bit obscure - you have to read the code to grasp which one is primary and which is secondary. * overUtilzedCPUHosts should be final as well Line 68: Line 69: /* get hosts with not enough free memory */ Line 70: final List<VDS> overUtilizedMemoryHosts = getSecondarySources(cluster, hosts, parameters); Line 71: Line 107: List<VDS> underUtilizedHosts = getSecondaryDestinations(cluster, hosts, parameters); Line 108: Line 109: // if no host has memory to spare, then there is nothing we can do to balance it.. Line 110: if (underUtilizedHosts == null || underUtilizedHosts.size() == 0) { Line 111: log.warn("All hosts are Memory over-utilized, can't Memory balance the cluster '{}'", cluster.getName()); I liked the warn message Line 112: return null; Line 113: } Line 114: Line 115: FindVmAndDestinations.Result result = findVmAndDestinations.invoke(overUtilizedMemoryHosts, underUtilizedHosts, getVmDao()); Line 109: // if no host has memory to spare, then there is nothing we can do to balance it.. Line 110: if (underUtilizedHosts == null || underUtilizedHosts.size() == 0) { Line 111: log.warn("All hosts are Memory over-utilized, can't Memory balance the cluster '{}'", cluster.getName()); Line 112: return null; Line 113: } something to consider: returning an empty Pair if it makes sense Line 114: Line 115: FindVmAndDestinations.Result result = findVmAndDestinations.invoke(overUtilizedMemoryHosts, underUtilizedHosts, getVmDao()); Line 116: if (result != null) { Line 117: destinationHosts = result.getDestinationHosts(); Line 123: || vmToMigrate == null) { Line 124: return null; Line 125: } Line 126: Line 127: List<Guid> destinationHostsKeys = new ArrayList<Guid>(); you can spare that block and use Entities.getIds(destinationHosts) Line 128: for (VDS vds : destinationHosts) { Line 129: destinationHostsKeys.add(vds.getId()); Line 130: } Line 131: Line 149: List<VDS> result = new ArrayList<>(); Line 150: Line 151: for (VDS h: hosts) { Line 152: if (h.getMaxSchedulingMemory() > lowFreeMemoryLimit Line 153: && h.getVmCount() >= minVmCount) { why a host which has less vms than the minumum is not under-utilized? a host with 0 VMs isn't a candidate? or can minVmCount be 0 in that case? Line 154: result.add(h); Line 155: } Line 156: } Line 157: Line 171: List<VDS> result = new ArrayList<>(); Line 172: Line 173: for (VDS h: hosts) { Line 174: if (h.getMaxSchedulingMemory() < highFreeMemoryLimit Line 175: && h.getVmCount() > 1) { I wonder if we have 1 VM and we're > highFreeMemoryLimit, shouldn't we strive to allocate a better host for that VM by marking this host as overUtil? Line 176: result.add(h); Line 177: } Line 178: } Line 179: Line 201: public boolean eval(VDS p) { Line 202: return (p.getUsageCpuPercent() + calcSpmCpuConsumption(p)) >= highUtilization Line 203: && p.getCpuOverCommitTimestamp() != null Line 204: && (new Date().getTime() - p.getCpuOverCommitTimestamp().getTime()) Line 205: >= cpuOverCommitDurationMinutes * 1000L * 60L please use TimeUnit.MINUTES.toMilis Line 206: && p.getVmCount() > 0; Line 207: } Line 208: }); Line 209: Collections.sort(overUtilizedHosts, new ReverseComparator(VdsCpuUsageComparator.INSTANCE)); Line 221: return (p.getUsageCpuPercent() + calcSpmCpuConsumption(p)) < lowUtilization Line 222: && p.getVmCount() >= minVmCount Line 223: && (p.getCpuOverCommitTimestamp() == null Line 224: || (new Date().getTime() - p.getCpuOverCommitTimestamp().getTime()) < Line 225: cpuOverCommitDurationMinutes * 60L * 1000L); same Line 226: } Line 227: }); Line 228: Collections.sort(underUtilizedHosts, VdsCpuUsageComparator.INSTANCE); Line 229: return underUtilizedHosts; Line 242: protected VmDAO getVmDao() { Line 243: return DbFacade.getInstance().getVmDao(); Line 244: } Line 245: Line 246: protected int tryParseWithDefault(String candidate, int defaultValue) { we should move that to some util if not present already Line 247: if (candidate != null) { Line 248: try { Line 249: return Integer.parseInt(candidate); Line 250: } catch (Exception e) { -- To view, visit https://gerrit.ovirt.org/38189 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1fe13267feca89ab6c8fb9d85656f05930d0b333 Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Sivák <msi...@redhat.com> Gerrit-Reviewer: Martin Sivák <msi...@redhat.com> Gerrit-Reviewer: Roy Golan <rgo...@redhat.com> Gerrit-Reviewer: Tomer Saban <tsa...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org 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
