Martin Sivák has posted comments on this change. Change subject: PendingResourceManager for tracking resources in WaitForLaunch ......................................................................
Patch Set 17: (20 comments) https://gerrit.ovirt.org/#/c/40136/17/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/pending/AbstractPendingResource.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/pending/AbstractPendingResource.java: Line 11: * It is also a good idea to implement static method that talks to the Line 12: * PendingResourceManager and collects a summary value per Host or VM Line 13: * depending on what is needed. Line 14: */ Line 15: public abstract class AbstractPendingResource { > the class is already abstract so just PendingResource is fine Done Line 16: protected VDS host; Line 17: protected VM vm; Line 18: Line 19: public AbstractPendingResource(Guid host, VM vm) { Line 12: * PendingResourceManager and collects a summary value per Host or VM Line 13: * depending on what is needed. Line 14: */ Line 15: public abstract class AbstractPendingResource { Line 16: protected VDS host; > why do we need the whole VDS object? VdsDynamic is a better fit. there's a Guid will do. Line 17: protected VM vm; Line 18: Line 19: public AbstractPendingResource(Guid host, VM vm) { Line 20: this.host = new VDS(); Line 57: * (size can change and host assignment as well) Line 58: * - Pending NIC should use Host and the NIC name Line 59: * (NIC is tied to a host, the allocation to VM can change) Line 60: */ Line 61: public abstract boolean equals(Object other); > just put PendingResource Doesn't equals expect Object according to the contract? Line 62: public abstract int hashCode(); https://gerrit.ovirt.org/#/c/40136/17/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/pending/PendingCpuCores.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/pending/PendingCpuCores.java: Line 8: * Represents a cpu core allocation that is going to be used Line 9: * by not yet started VM on a specified host Line 10: */ Line 11: public class PendingCpuCores extends AbstractPendingResource { Line 12: long coreCount; > int is enough Done Line 13: Line 14: public PendingCpuCores(VDS host, VM vm, long coreCount) { Line 15: super(host, vm); Line 16: this.coreCount = coreCount; https://gerrit.ovirt.org/#/c/40136/17/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/pending/PendingMemory.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/pending/PendingMemory.java: Line 49: } Line 50: Line 51: public static int collectForHost(PendingResourceManager manager, VDS host) { Line 52: int sumMb = 0; Line 53: for (PendingMemory resource: manager.pendingHostResources(host, PendingMemory.class)) { > again, the whole VDS object here looks like too much. try narrowing it. Done Line 54: sumMb += resource.getSizeInMb(); Line 55: } Line 56: Line 57: return sumMb; https://gerrit.ovirt.org/#/c/40136/17/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/pending/PendingResourceManager.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/pending/PendingResourceManager.java: Line 25: private static final Logger log = LoggerFactory.getLogger(PendingResourceManager.class); Line 26: Line 27: // All internal structures have to be thread-safe for concurrent access Line 28: final Map<Guid, Set<AbstractPendingResource>> resourcesByHost = new ConcurrentHashMap<>(); Line 29: final Map<Guid, Set<AbstractPendingResource>> resourcesByVm = new ConcurrentHashMap<>(); > proposing a unified structure with the VM guid as the key. The two resourcesBy* serve as indexes. Both can be removed, but then we need to iterate over the whole pending set all the time. resourcesByHost is needed for clearHost flows (like maintenance), but is not used atm. pendingResources has to stay for reasons explained in addPending (dealing with multiple addition of the same resource). And if two of these are still available then we need the locks here. The locks allow arbitrary read access, but protect the index consistency during write. Line 30: final Map<AbstractPendingResource, AbstractPendingResource> pendingResources = new ConcurrentHashMap<>(); Line 31: Line 32: final ResourceManager resourceManager; Line 33: Line 83: * This is supposed to be used during maintenance flows to clear possible stale data. Line 84: * Line 85: * @param host VDS with valid getId() Line 86: */ Line 87: public void clearHost(VDS host) { > again I think a single structure will handle both. It really won't unless you want to iterate over everything. Line 88: synchronized (this) { Line 89: if (!resourcesByHost.containsKey(host.getId())) { Line 90: return; Line 91: } Line 156: * @return Iterable object with the requested resources Line 157: */ Line 158: public <T extends AbstractPendingResource> Iterable<T> pendingHostResources(VDS vds, Class<T> type) { Line 159: if (!resourcesByHost.containsKey(vds.getId())) { Line 160: return new ArrayList<>(0); > Use Collections.emptyList() instead. its immutable by the way so don't plan Done Line 161: } Line 162: Line 163: List<T> list = new ArrayList<>(); Line 164: for (AbstractPendingResource resource: resourcesByHost.get(vds.getId())) { Line 162: Line 163: List<T> list = new ArrayList<>(); Line 164: for (AbstractPendingResource resource: resourcesByHost.get(vds.getId())) { Line 165: if (resource.getClass().equals(type)) { Line 166: list.add((T)resource); > lets use java's type system. getClass() is a code smell and usually mean so I am using the type system :) I am not using this to check whether it has a proper parent. I am using it to get only instances of an exact specified type. Line 167: } Line 168: } Line 169: Line 170: return list; Line 174: * Return all currently pending resources of type "type". Line 175: * @param type Class object identifying the type of pending resources we are interested in Line 176: * @return Iterable object with the requested resources Line 177: */ Line 178: public <T extends AbstractPendingResource> Iterable<T> pendingResources(Class<T> type) { > probably extracted that for line 163, I guess It is used in affinity group policy units. Line 179: List<T> list = new ArrayList<>(); Line 180: for (AbstractPendingResource resource: pendingResources.values()) { Line 181: if (resource.getClass().equals(type)) { Line 182: list.add((T)resource); Line 192: * for calling it after finished with adding all new pending resources for a VM. Line 193: * @param hostId - it of the affected host Line 194: */ Line 195: public void notifyHostManagers(Guid hostId) { Line 196: if (resourceManager == null) return; > missing curly braces. //TODO CDI this class Done Line 197: Line 198: VdsManager vdsManager = resourceManager.GetVdsManager(hostId); Line 199: Line 200: VDS vds = new VDS(); Line 197: Line 198: VdsManager vdsManager = resourceManager.GetVdsManager(hostId); Line 199: Line 200: VDS vds = new VDS(); Line 201: vds.setId(hostId); > why we need a new VDS instance just to calc the it? Not needed anymore, I moved to Guids. Line 202: Line 203: int pendingCpus = PendingCpuCores.collectForHost(this, vds); Line 204: int pendingMemory = PendingMemory.collectForHost(this, vds); Line 205: https://gerrit.ovirt.org/#/c/40136/17/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/EvenDistributionWeightPolicyUnit.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/EvenDistributionWeightPolicyUnit.java: Line 37: int vcpu = Config.<Integer> getValue(ConfigValues.VcpuConsumptionPercentage); Line 38: int spmCpu = (vds.getSpmStatus() == VdsSpmStatus.None) ? 0 : Config Line 39: .<Integer> getValue(ConfigValues.SpmVCpuConsumption); Line 40: double hostCpu = vds.getUsageCpuPercent(); Line 41: double pendingVcpus = PendingCpuCores.collectForHost(getPendingResourceManager(), vds); > that's an int. It is double intentionally (even in the old code). It is then the only double field in (which needs to return a double): (pendingVcpus + vm.getNumOfCpus() + spmCpu) / hostCores Line 42: Line 43: return (hostCpu / vcpu) + (pendingVcpus + vm.getNumOfCpus() + spmCpu) / hostCores; Line 44: } Line 45: https://gerrit.ovirt.org/#/c/40136/17/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/PinToHostPolicyUnit.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/PinToHostPolicyUnit.java: Line 16: public class PinToHostPolicyUnit extends PolicyUnitImpl { Line 17: Line 18: public PinToHostPolicyUnit(PolicyUnit policyUnit, Line 19: PendingResourceManager pendingResourceManager) { Line 20: super(policyUnit, pendingResourceManager); > tough needed in few of the units, all the constructors have been changed to It will be needed in much more units.. and all PolicyUnits inherit from PolicyUnitImpl which gets that from the SchedulingManager.. Line 21: } Line 22: Line 23: @Override Line 24: public List<VDS> filter(List<VDS> hosts, VM vm, Map<String, String> parameters, PerHostMessages messages) { https://gerrit.ovirt.org/#/c/40136/17/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/PowerSavingBalancePolicyUnit.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/PowerSavingBalancePolicyUnit.java: Line 144: for (VDS vds: allHosts) { Line 145: if (vds.getStatus() == VDSStatus.Up Line 146: && vds.getVmCount() == 0 Line 147: && vds.getVmMigrating() == 0 Line 148: && PendingVM.collectForHost(getPendingResourceManager(), vds).size() == 0) { > ok it took me a while cause the static method on the Resource is confusing. Well I wanted to make the API the same everywhere. And you need to specify what you are counting. VMs, CPUs, Memory. And all can have a different return type (list, int..). There was not too many possible ways of doing that. Line 149: emptyHosts.add(vds); Line 150: } Line 151: else if (vds.isPowerManagementControlledByPolicy() && !vds.isDisablePowerManagementPolicy()) { Line 152: if (vds.getStatus() == VDSStatus.Maintenance) { https://gerrit.ovirt.org/#/c/40136/17/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VDS.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VDS.java: Line 896: * Line 897: * This field is a cache, use for reporting only. Line 898: * The authoritative source for current value is the Line 899: * SchedulingManager.getInstance().getPendingResourceManager() Line 900: */ > replace with plain link reference[1] to the class - one day (soon) RendingR Done Line 901: public Integer getPendingVcpusCount() { Line 902: return vdsDynamic.getPendingVcpusCount(); Line 903: } Line 904: Line 911: * assigned to a running VM. Line 912: * Line 913: * This field is a cache, use for reporting only. Line 914: * The authoritative source for current value is the Line 915: * SchedulingManager.getInstance().getPendingResourceManager() > same Done Line 916: */ Line 917: public int getPendingVmemSize() { Line 918: return vdsDynamic.getPendingVmemSize(); Line 919: } Line 1207: maxSchedulingMemory = maxSchedulingMemory > 0 ? maxSchedulingMemory : 0; Line 1208: } Line 1209: } Line 1210: Line 1211: public float getFreeVirtualMemory() { > why is it a float? split mega is meaningless Because of the fractions in overcommitment. It was always float, there is no change here (except splitting it out of calculateFreeMemoryCache. Line 1212: if (getMemCommited() != null && getPhysicalMemMb() != null && getReservedMem() != null) { Line 1213: float freeMemory = (getMaxVdsMemoryOverCommit() * getPhysicalMemMb() / 100.0f) Line 1214: - getMemCommited() Line 1215: - getReservedMem(); https://gerrit.ovirt.org/#/c/40136/17/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VdsDynamic.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VdsDynamic.java: Line 72: * as part of not yet running VM. Line 73: * Line 74: * Do not use for anything critical as scheduling, ask for the Line 75: * exact data using SchedulingManager.getInstance().getPendingResourceManager(). Line 76: */ > kudos for the javadc again, and same comment about the reference Done Line 77: private Integer pendingVcpusCount; Line 78: Line 79: private Integer cpuSockets; Line 80: Line 135: * Cached (best effort) amount of memory scheduled Line 136: * as part of not yet running VM. Line 137: * Line 138: * Do not use for anything critical as scheduling, ask for the Line 139: * exact data using SchedulingManager.getInstance().getPendingResourceManager(). > same Done Line 140: */ Line 141: private Integer pendingVmemSize; Line 142: Line 143: private RpmVersion rpmVersion; -- To view, visit https://gerrit.ovirt.org/40136 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie741b8e244acb64e470af83252d90ec134ba7f8e Gerrit-PatchSet: 17 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Sivák <msi...@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: Jenkins CI Gerrit-Reviewer: Martin Betak <mbe...@redhat.com> Gerrit-Reviewer: Martin Peřina <mper...@redhat.com> Gerrit-Reviewer: Martin Sivák <msi...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Roy Golan <rgo...@redhat.com> Gerrit-Reviewer: Tomer Saban <tsa...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
