Doron Fediuck has posted comments on this change. Change subject: core: scheduling: VM Affinity Group filter and weight module ......................................................................
Patch Set 5: (4 comments) The implementation looks good. See inline for specific changes needed. http://gerrit.ovirt.org/#/c/22793/5/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/VmAffinityFilterPolicyUnit.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/VmAffinityFilterPolicyUnit.java: Line 83: // Only one host is allowed for positive affinity, i.e. if the VM contained in a positive Line 84: // affinity group he must run on the host that all the other members are running, if the Line 85: // VMs spread across hosts, the affinity rule isn't applied. Line 86: } else if (acceptableHosts.size() > 1) { Line 87: messages.add(VdcBllMessages.ACTION_TYPE_FAILED_POSITIVE_AFFINITY_GROUP.toString()); Which host / group? How can the user know if we do not add specific details? Line 88: return null; Line 89: } Line 90: // Handle negative affinity Line 91: for (Guid id : allVmIdsNegative) { Line 90: // Handle negative affinity Line 91: for (Guid id : allVmIdsNegative) { Line 92: VM runVm = runningVMsMap.get(id); Line 93: if (runVm != null && runVm.getRunOnVds() != null) { Line 94: acceptableHosts.remove(runVm.getRunOnVds()); Please aggregate the removal into a string which will be outputted in a log in no hosts left at all. ie- removed host X, due to VM Y running there. Line 95: } Line 96: } Line 97: if (acceptableHosts.isEmpty()) { Line 98: if (hasPositiveConstraint) { Line 97: if (acceptableHosts.isEmpty()) { Line 98: if (hasPositiveConstraint) { Line 99: messages.add(VdcBllMessages.ACTION_TYPE_FAILED_MIX_POSITIVE_NEGATIVE_AFFINITY_GROUP.toString()); Line 100: } else { Line 101: messages.add(VdcBllMessages.ACTION_TYPE_FAILED_NEGATIVE_AFFINITY_GROUP.toString()); This should have the list from above. Line 102: } Line 103: return null; Line 104: } Line 105: Line 105: Line 106: List<VDS> retList = new ArrayList<VDS>(); Line 107: for (VDS host : hosts) { Line 108: if (acceptableHosts.contains(host.getId())) { Line 109: retList.add(host); RetainAll should make this loop (and similar) redundant [1] http://docs.oracle.com/javase/7/docs/api/java/util/List.html#retainAll(java.util.Collection) Line 110: } Line 111: } Line 112: Line 113: return retList; -- To view, visit http://gerrit.ovirt.org/22793 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9716523591852635496799412ad387850d6c372d Gerrit-PatchSet: 5 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Gilad Chaplik <gchap...@redhat.com> Gerrit-Reviewer: Doron Fediuck <dfedi...@redhat.com> Gerrit-Reviewer: Gilad Chaplik <gchap...@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
