Gilad Chaplik has posted comments on this change. Change subject: engine: HA VN Reservation feature ......................................................................
Patch Set 2: (22 comments) .................................................... Commit Message Line 6: Line 7: engine: HA VN Reservation feature Line 8: Line 9: Adding a feature HA VN Reservation feature, Line 10: The HA VM reservation feature ensure the safety of /s/ensure/ensures btw, not sure about ensuring, maybe notifies. Line 11: HA VMs in case of host failover, Line 12: without negatively impacting performance. Line 13: Line 14: Added a checkbox to the Cluster popup. Line 15: A new weight function Line 16: A new balance function Line 17: Rest api updated Line 18: A new scheduled task Line 19: A new alert pls add link to feature page... Line 20: Line 21: Change-Id: I44e698e58ff5e4a0b74249d3fe3cf4acf042c324 Line 22: Bug-Url: https://bugzilla.redhat.com/1036753 .................................................... File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/HaReservationHandling.java Line 15: import org.ovirt.engine.core.utils.linq.Predicate; Line 16: Line 17: /** Line 18: * A halper class for the scheduling mechanism for checking the HA Reservation status of a Cluster Line 19: * @author kianku pls remove author. not part of our code convetions. Line 20: */ Line 21: public class HaReservationHandling { Line 22: Line 23: /** Line 31: public boolean checkHaReservationStatusForCluster(VDSGroup cluster, List<VDS> failedHosts) { Line 32: List<VDS> hosts = DbFacade.getInstance().getVdsDao().getAllForVdsGroup(cluster.getId()); Line 33: Line 34: // No hosts, return true Line 35: if (hosts == null) { null or empty Line 36: return true; Line 37: } Line 38: // HA Reservation is not possible with less the 2 hosts Line 39: if (hosts.size() < 2) { Line 55: @Override Line 56: public boolean eval(VM v) { Line 57: return v.isAutoStartup(); Line 58: } Line 59: }); can be done using stored procedure. that will give us more flexibility in handled VMs. Line 60: Line 61: Map<Guid, List<VM>> hostToHaVmsMapping = mapVmToHost(vmInCluster); Line 62: Line 63: for (VDS host : hosts) { Line 64: boolean isHaSafe = Line 65: findReplacementForHost(cluster, host, Line 66: hostToHaVmsMapping.get(host.getId()), Line 67: hostsUnutilizedResources); Line 68: if (!isHaSafe) { var isHaSafe can be removed. Line 69: failedHosts.add(host); Line 70: } Line 71: } Line 72: Line 69: failedHosts.add(host); Line 70: } Line 71: } Line 72: Line 73: if (failedHosts.size() == 0) { or failedHosts.isEmpty() Line 74: return true; Line 75: } else { Line 76: return false; Line 77: } Line 84: Map<Guid, Pair<Integer, Integer>> additionalHostsUtilizedResources = Line 85: new HashMap<Guid, Pair<Integer, Integer>>(); Line 86: Line 87: for (VM vm : vmList) { Line 88: int curVmMemSize = (int) Math.round(vm.getMemSizeMb() * (vm.getUsageMemPercent() / 100.0)); getUsageMemPercent() can be null. please check nullity for all VM's non native objects Line 89: int curVmCpuPercent = Line 90: vm.getUsageCpuPercent() * vm.getNumOfCpus() Line 91: / SlaValidator.getEffectiveCpuCores(host, cluster.getCountThreadsAsCores()); Line 92: Line 116: // Found a place for current vm, add the RAM and CPU size to additionalHostsUtilizedResources Line 117: if (!additionalHostsUtilizedResources.containsKey(hostData.getFirst())) { Line 118: Pair<Integer, Long> cpuRamPair = new Pair<Integer, Long>(); Line 119: cpuRamPair.setSecond(new Long(curVmMemSize)); Line 120: cpuRamPair.setFirst(curVmCpuPercent); add the pair to the map? and use the ctor(first, second) to set the properties. matter of code style but I won't use containsKey here: value = map.get(key) if(value != null){ value.setBla }else { map.put(key, new pair(first, second) } Line 121: } else { Line 122: Pair<Integer, Integer> cpuRamPair = Line 123: additionalHostsUtilizedResources.get(hostData.getFirst()); Line 124: cpuRamPair.setSecond(cpuRamPair.getSecond() + curVmMemSize); Line 145: private Map<Guid, List<VM>> mapVmToHost(List<VM> vms) { Line 146: Map<Guid, List<VM>> hostToHaVmsMapping = new HashMap<Guid, List<VM>>(); Line 147: Line 148: for (VM vm : vms) { Line 149: if (vm.getRunOnVds() != null) { to be on the safe side use Guid.isNullOrEmpty() Line 150: if (!hostToHaVmsMapping.containsKey(vm.getRunOnVds())) { Line 151: List<VM> vmsOfHost = new ArrayList<VM>(); Line 152: vmsOfHost.add(vm); Line 153: hostToHaVmsMapping.put(vm.getRunOnVds(), vmsOfHost); Line 160: } Line 161: Line 162: private List<Pair<Guid, Pair<Integer, Integer>>> getUnutilizedResources(List<VDS> hosts) { Line 163: List<Pair<Guid, Pair<Integer, Integer>>> hostsUnutilizedResources = Line 164: new ArrayList<Pair<Guid, Pair<Integer, Integer>>>(); it's a good place to use diamond operator and drop the type. Line 165: for (VDS host : hosts) { Line 166: Pair<Integer, Integer> innerUnutilizedCpuRamPair = new Pair<Integer, Integer>(); Line 167: innerUnutilizedCpuRamPair.setFirst(100 - host.getUsageCpuPercent()); Line 168: .................................................... File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/SchedulingManager.java Line 767: for (VDSGroup cluster : clusters) { Line 768: if (cluster.supportsHaReservation()) { Line 769: List<VDS> returnedFailedHosts = new ArrayList<VDS>(); Line 770: boolean status = Line 771: new HaReservationHandling().checkHaReservationStatusForCluster(cluster, returnedFailedHosts); why creating new instance for each cluster? Line 772: if (!status) { Line 773: // create Alert using returnedFailedHosts Line 774: AuditLogableBase logable = new AuditLogableBase(); Line 775: logable.setVdsGroupId(cluster.getId()); Line 783: if (commaCount > 0) { Line 784: failedHostsStr.append(", "); Line 785: commaCount--; Line 786: } Line 787: } use StringUtils.join Line 788: logable.addCustomValue("Hosts", failedHostsStr.toString()); Line 789: AlertDirector.Alert(logable, AuditLogType.CLUSTER_ALERT_HA_RESERVATION); Line 790: } Line 791: } .................................................... File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/HaReservationBalancePolicyUnit.java Line 52: int optimalHaDistribution = (int) Math.ceil(((double) haVmsInCluster / hosts.size())); Line 53: Line 54: int overUtilizationParam = 100; Line 55: if (parameters.get("OverUtilization") != null) { Line 56: overUtilizationParam = Integer.parseInt(parameters.get("OverUtilization")); consider using NumberUtils.toInt (with default) Line 57: } else { Line 58: overUtilizationParam = Config.<Integer> getValue(ConfigValues.OverUtilizationForHaReservation); Line 59: } Line 60: Line 60: Line 61: int overUtilizationThreshold = (int) Math.ceil(optimalHaDistribution * (overUtilizationParam / 100)); Line 62: Line 63: List<VDS> overUtilizedHosts = getOverUtilizedHosts(hosts, hostId2HaVmMapping, optimalHaDistribution); Line 64: if (overUtilizedHosts.size() == 0) { isEmpty() Line 65: return null; Line 66: } Line 67: Line 68: List<VDS> underUtilizedHosts = getUnderUtilizedHosts(hosts, hostId2HaVmMapping, overUtilizationParam); Line 90: return new Pair<List<Guid>, Guid>(underUtilizedHostsKeys, vm.getId()); Line 91: Line 92: } Line 93: Line 94: private int countHaVmsInCluster(Map<Guid, List<VM>> hostId2HaVmMapping) { I think that you can count the vms when you get them. Line 95: int result = 0; Line 96: for (Entry<Guid, List<VM>> entry : hostId2HaVmMapping.entrySet()) { Line 97: result += entry.getValue().size(); Line 98: } Line 119: } Line 120: Line 121: private List<VDS> getOverUtilizedHosts(List<VDS> hosts, Line 122: Map<Guid, List<VM>> hostId2HaVmMapping, Line 123: int overUtilizationThreshold) { same code like getUnderUtilizedHosts except comparator, consider passing it as a method parameter. Line 124: List<VDS> overUtilizedHosts = new ArrayList<VDS>(); Line 125: Line 126: for (VDS host : hosts) { Line 127: int count = 0; Line 143: vms = LinqUtils.filter(vms, new Predicate<VM>() { Line 144: @Override Line 145: public boolean eval(VM v) { Line 146: return v.getMigrationSupport() == MigrationSupport.MIGRATABLE Line 147: && !hostId.equals(v.getDedicatedVmForVds()); Migratable is enough. Line 148: } Line 149: }); Line 150: Line 151: return vms; Line 150: Line 151: return vms; Line 152: } Line 153: Line 154: private Map<Guid, List<VM>> mapHaVmToHostByCluster(Guid clusterId) { looks like the same code in Ha..Handling, please reuse Line 155: Line 156: Map<Guid, List<VM>> result = new HashMap<Guid, List<VM>>(); Line 157: List<VM> vms = DbFacade.getInstance().getVmDao().getAllForVdsGroup(clusterId); Line 158: if (vms != null) { .................................................... File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/HaReservationWeightPolicyUnit.java Line 84: return scores; Line 85: } Line 86: Line 87: Line 88: private Map<Guid, List<VM>> mapHaVmToHostByCluster(Guid clusterId) { code reuse. Line 89: Line 90: Map<Guid, List<VM>> result = new HashMap<Guid, List<VM>>(); Line 91: List<VM> vms = DbFacade.getInstance().getVmDao().getAllForVdsGroup(clusterId); Line 92: if (vms != null) { .................................................... File packaging/dbscripts/upgrade/pre_upgrade/0000_config.sql Line 613: -- Cluster HA Reservation Line 614: select fn_db_add_config_value('OverUtilizationForHaReservation','200','3.4'); Line 615: select fn_db_add_config_value('ScaleDownForHaReservation','1','3.4'); Line 616: select fn_db_add_config_value('EnableVdsHaReservation','true','3.4'); Line 617: select fn_db_add_config_value('VdsHaReservationIntervalInMinutes','5','3.4'); why 3.4 and not general? Line 618: Line 619: ------------------------------------------------------------------------------------ Line 620: -- Update with override section Line 621: ------------------------------------------------------------------------------------ .................................................... File packaging/dbscripts/vds_groups_sp.sql Line 45: Line 46: Line 47: Line 48: Line 49: what about insert? Line 50: Create or replace FUNCTION UpdateVdsGroup(v_description VARCHAR(4000) , Line 51: v_free_text_comment text, Line 52: v_name VARCHAR(40), Line 53: v_vds_group_id UUID, -- To view, visit http://gerrit.ovirt.org/22929 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I44e698e58ff5e4a0b74249d3fe3cf4acf042c324 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Kobi Ianko <k...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Doron Fediuck <dfedi...@redhat.com> Gerrit-Reviewer: Gilad Chaplik <gchap...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Roy Golan <rgo...@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
