Gilad Chaplik has posted comments on this change. Change subject: core:Added affinity Group collision validation. ......................................................................
Patch Set 8: (12 comments) minor comments, leaves the algorithm part to Martin :) https://gerrit.ovirt.org/#/c/41805/8//COMMIT_MSG Commit Message: Line 7: core:Added affinity Group collision validation. Line 8: Line 9: CRUD Affinity Group Command's validateParameters() now have a test Line 10: that checks for affinity rules collision, To prevent the user from Line 11: creating problematic Affinity Rules from the first place. /s/from/in Line 12: Line 13: Change-Id: I56c483023ed14e238c8a67ec0890812158ecd0b2 Line 14: Bug-Url: https://bugzilla.redhat.com/1057531 https://gerrit.ovirt.org/#/c/41805/8/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/commands/AffinityGroupCRUDCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/commands/AffinityGroupCRUDCommand.java: Line 34: protected boolean validateParameters() { Line 35: if (getVdsGroup() == null) { Line 36: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_INVALID_CLUSTER_FOR_AFFINITY_GROUP); Line 37: } Line 38: line Line 39: if (getParameters().getAffinityGroup().getEntityIds() != null) { Line 40: VmStatic vmStatic = null; Line 41: Set<Guid> vmSet = new HashSet<>(); Line 42: for (Guid vmId : getParameters().getAffinityGroup().getEntityIds()) { Line 54: } Line 55: } Line 56: } Line 57: Line 58: return hasAffinityCollisions(getParameters()); no need to pass getParam..s() Line 59: } Line 60: Line 61: private boolean hasAffinityCollisions(AffinityGroupCRUDParameters parameters) { Line 62: List<AffinityGroup> affinityGroups = getAllAffinityGroups(parameters.getAffinityGroup().getClusterId()); Line 58: return hasAffinityCollisions(getParameters()); Line 59: } Line 60: Line 61: private boolean hasAffinityCollisions(AffinityGroupCRUDParameters parameters) { Line 62: List<AffinityGroup> affinityGroups = getAllAffinityGroups(parameters.getAffinityGroup().getClusterId()); There's no cluster id validation till now.. check how bad it can behave via REST (it's me to blame ;-)) Line 63: affinityGroups.add(parameters.getAffinityGroup()); Line 64: return getUnifiedAffinityGroups(affinityGroups); Line 65: } Line 66: Line 64: return getUnifiedAffinityGroups(affinityGroups); Line 65: } Line 66: Line 67: private List<AffinityGroup> getAllAffinityGroups(Guid clusterId) { Line 68: return DbFacade.getInstance().getAffinityGroupDao().getAllAffinityGroupsByClusterId(clusterId); inject the DAO or at least have a getter, anyway I always prefer to avoid a single line method. Line 69: } Line 70: Line 71: /** Line 72: # UAG = {{vm} for each vm} Line 68: return DbFacade.getInstance().getAffinityGroupDao().getAllAffinityGroupsByClusterId(clusterId); Line 69: } Line 70: Line 71: /** Line 72: # UAG = {{vm} for each vm} https://en.wikipedia.org/wiki/UAG ;-) and please try to improve comment. Line 73: # For each (+) affinity group(Sorted by group id): Line 74: ## Merge VMs from the group in UAG(Sorted by vm id). Line 75: ## For each (-) affinity group(Sorted by group id): Line 76: ### For each group in UAG(Sorted by first vm uuid): Line 77: #### if size of the intersection of group from UAG and the negative group is > 1: Line 78: ##### throw exception “Affinity group contradiction detected” (With associated groups). Line 79: Line 80: */ Line 81: private boolean getUnifiedAffinityGroups(List<AffinityGroup> affinityGroups) { the method returns a boolean and named "getSomething()". Line 82: Set<Set<Guid>> uag = new HashSet(); Line 83: Line 84: //uag = {{vm} for each vm in any affinity group(Either negative or positive)} Line 85: for(AffinityGroup ag: affinityGroups) { Line 80: */ Line 81: private boolean getUnifiedAffinityGroups(List<AffinityGroup> affinityGroups) { Line 82: Set<Set<Guid>> uag = new HashSet(); Line 83: Line 84: //uag = {{vm} for each vm in any affinity group(Either negative or positive)} format (all over) Line 85: for(AffinityGroup ag: affinityGroups) { Line 86: Set<Guid> temp = new HashSet<>(); Line 87: temp.addAll(ag.getEntityIds()); Line 88: uag.add(temp); Line 118: intersection.retainAll(positiveGroup); Line 119: Line 120: if(intersection.size() > 1) { Line 121: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_AFFINITY_RULES_COLLISION, Line 122: String.format("$UAG %s", positiveGroup.toString()), would someone who checks out "UAG" will get to the same wiki page as me :-P Line 123: String.format("$negativeAR %s", ag.getEntityIds().toString())); Line 124: } Line 125: } Line 126: } Line 132: for(Set<Guid> s : uag) { Line 133: if(s.contains(id)) { Line 134: uag.remove(s); Line 135: return s; Line 136: } iterator.remove is a much nicer approach Line 137: } Line 138: Line 139: return new HashSet<>(); Line 140: } Line 135: return s; Line 136: } Line 137: } Line 138: Line 139: return new HashSet<>(); return Collections.<Guid>emptySet(); Line 140: } Line 141: Line 142: protected AffinityGroup getAffinityGroup() { Line 143: if (affinityGroup == null) { https://gerrit.ovirt.org/#/c/41805/8/frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/AppErrors.java File frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/AppErrors.java: Line 3697: Line 3698: @DefaultStringValue("VM is associated with both positive and negative Affinity Groups, please reconfigure VM's affinity groups") Line 3699: String ACTION_TYPE_FAILED_MIX_POSITIVE_NEGATIVE_AFFINITY_GROUP(); Line 3700: Line 3701: @DefaultStringValue("Affinity Group contradiction detected between unified affinity group:\n${UAG}\nand negative affinity group:\n${negativeAR}") /s/contradiction between/detect collision in/ work a bit on phrasing here. Line 3702: String ACTION_TYPE_FAILED_AFFINITY_RULES_COLLISION(); Line 3703: Line 3704: @DefaultStringValue("iSCSI bond name must not exceed 50 characters") Line 3705: String VALIDATION_ISCSI_BOND_NAME_MAX(); -- To view, visit https://gerrit.ovirt.org/41805 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I56c483023ed14e238c8a67ec0890812158ecd0b2 Gerrit-PatchSet: 8 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Tomer Saban <tsa...@redhat.com> Gerrit-Reviewer: Gilad Chaplik <gchap...@redhat.com> Gerrit-Reviewer: Jenkins CI 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-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
