Tomer Saban has posted comments on this change. Change subject: core: Added affinity rules enforcement manager ......................................................................
Patch Set 7: (15 comments) https://gerrit.ovirt.org/#/c/41092/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/InitBackendServicesOnStartupBean.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/InitBackendServicesOnStartupBean.java: Line 48: @Inject Line 49: private Instance<BackendService> services; Line 50: Line 51: @Inject Line 52: private AffinityRulesEnforcementManager aremManager; > I want it as a backendService. and load it at InitBackendServicesOnStartupB Done Line 53: Line 54: /** Line 55: * This method is called upon the bean creation as part Line 56: * of the management Service bean life cycle. https://gerrit.ovirt.org/#/c/41092/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/AffinityRulesEnforcementManager.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/AffinityRulesEnforcementManager.java: Line 40: Line 41: @Singleton Line 42: public class AffinityRulesEnforcementManager { Line 43: //Definitions Line 44: //Fields > redundant comment? Done Line 45: Map<VDSGroup, AffinityRulesEnforcementPerCluster> perClusterMap; Line 46: Iterator<AffinityRulesEnforcementPerCluster> AreClusterIterator = Collections.emptyIterator(); Line 47: Integer currentInterval; Line 48: Line 41: @Singleton Line 42: public class AffinityRulesEnforcementManager { Line 43: //Definitions Line 44: //Fields Line 45: Map<VDSGroup, AffinityRulesEnforcementPerCluster> perClusterMap; > private members, please Done Line 46: Iterator<AffinityRulesEnforcementPerCluster> AreClusterIterator = Collections.emptyIterator(); Line 47: Integer currentInterval; Line 48: Line 49: private String jobId; Line 42: public class AffinityRulesEnforcementManager { Line 43: //Definitions Line 44: //Fields Line 45: Map<VDSGroup, AffinityRulesEnforcementPerCluster> perClusterMap; Line 46: Iterator<AffinityRulesEnforcementPerCluster> AreClusterIterator = Collections.emptyIterator(); > same Done Line 47: Integer currentInterval; Line 48: Line 49: private String jobId; Line 50: Line 43: //Definitions Line 44: //Fields Line 45: Map<VDSGroup, AffinityRulesEnforcementPerCluster> perClusterMap; Line 46: Iterator<AffinityRulesEnforcementPerCluster> AreClusterIterator = Collections.emptyIterator(); Line 47: Integer currentInterval; > same Done Line 48: Line 49: private String jobId; Line 50: Line 51: @Inject Line 53: @Inject Line 54: protected VdsDAO vdsDao; Line 55: @Inject Line 56: protected VdsGroupDAO vdsGroupDao; Line 57: > redundant line Done Line 58: @Inject Line 59: Instance<AffinityRulesEnforcementPerCluster> perClusterProvider; Line 60: Line 61: @PostConstruct Line 55: @Inject Line 56: protected VdsGroupDAO vdsGroupDao; Line 57: Line 58: @Inject Line 59: Instance<AffinityRulesEnforcementPerCluster> perClusterProvider; > private Done Line 60: Line 61: @PostConstruct Line 62: protected void wakeup() { Line 63: long regularInterval = getRegularInterval(); Line 61: @PostConstruct Line 62: protected void wakeup() { Line 63: long regularInterval = getRegularInterval(); Line 64: long initialDelay = getInitialInterval(); Line 65: currentInterval = new Long(regularInterval).intValue(); > use longValue() Done Line 66: perClusterMap = new HashMap<>(); Line 67: Line 68: //Check that AffinityRulesEnforcementPerCluster exist in perClusterMap for each cluster in the engine. Line 69: List<VDSGroup> vdsGroups = getClusters(); Line 103: private Integer getMaximumMigrationTries() { Line 104: return Config.<Integer>getValue(ConfigValues.AffinityRulesEnforcementManagerMaximumMigrationTries); Line 105: } Line 106: Line 107: private Integer getLongInterval() { > name is a bit misleading. what do you refer as Long interval? There is the regular interval which is used when the manager is checking affinity rules and migrating VMs and there is the long interval which is used in two cases: 1. All affinity rules are enforced. 2. Too many migrations failed and we don't want to overload the system with migrations. We want to give the system time to adjust itself and maybe allow migrations again. I will add a javadoc describing their purposes. Line 108: return Config.<Integer> getValue(ConfigValues.AffinityRulesEnforcementManagerLongInterval); Line 109: } Line 110: Line 111: //Called each interval. Line 107: private Integer getLongInterval() { Line 108: return Config.<Integer> getValue(ConfigValues.AffinityRulesEnforcementManagerLongInterval); Line 109: } Line 110: Line 111: //Called each interval. > replace with proper Java doc Done Line 112: @OnTimerMethodAnnotation("refresh") Line 113: public void refresh() { Line 114: AuditLogableBase logable = new AuditLogableBase(); Line 115: auditLogDirector.log(logable, AuditLogType.AFFINITY_RULES_ENFORCEMENT_MANAGER_INTERVAL_REACHED); Line 111: //Called each interval. Line 112: @OnTimerMethodAnnotation("refresh") Line 113: public void refresh() { Line 114: AuditLogableBase logable = new AuditLogableBase(); Line 115: auditLogDirector.log(logable, AuditLogType.AFFINITY_RULES_ENFORCEMENT_MANAGER_INTERVAL_REACHED); > you meant regular log maybe? this is the event log which goes in the event It is better be in the engine log. Because, it adds too many messages to the Audit Log. It was removed and moved to the event log. Line 116: Line 117: if (AreClusterIterator == null || !AreClusterIterator.hasNext()) { Line 118: AreClusterIterator = perClusterMap.values().iterator(); Line 119: } https://gerrit.ovirt.org/#/c/41092/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/arem/AffinityRulesEnforcementPerCluster.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/arem/AffinityRulesEnforcementPerCluster.java: Line 27: import java.util.Map; Line 28: import java.util.Set; Line 29: Line 30: /** Line 31: * Created by tsaban on 5/18/15. > Please replace with proper Javadoc. (And change the IDEA template :) Done Line 32: */ Line 33: public class AffinityRulesEnforcementPerCluster { Line 34: //Definitions Line 35: //Fields Line 287: Set<Guid> mergedSet = new HashSet<>(); Line 288: Line 289: for(Guid id : ag.getEntityIds()) { Line 290: Set<Guid> vmGroup = popVmGroupByGuid(uag, id); Line 291: mergedSet.addAll(vmGroup); > The same as the other patch. Remove the old groups from uag. It is already done(See popVmGroupByGuid(). Also the name starts with pop which gives a clue about what the method is doing). Line 292: } Line 293: Line 294: uag.add(mergedSet); Line 295: } https://gerrit.ovirt.org/#/c/41092/7/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 57: Line 58: return hasAffinityCollisions(getParameters()); Line 59: } Line 60: Line 61: private Boolean hasAffinityCollisions(AffinityGroupCRUDParameters parameters) { > boolean Done Line 62: List<AffinityGroup> affinity_groups = getAllAffinityGroups(parameters.getAffinityGroup().getClusterId()); Line 63: affinity_groups.add(parameters.getAffinityGroup()); Line 64: return getUnifiedAffinityGroups(affinity_groups); Line 65: } 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: private Boolean getUnifiedAffinityGroups(List<AffinityGroup> affinityGroups) { > Again? Isn't this part of the canDoAction patch? Done (Removed unrelated code). Line 81: Set<Set<VM>> uag = new HashSet(); Line 82: Line 83: VmDAO vm_dao = DbFacade.getInstance().getVmDao(); Line 84: -- To view, visit https://gerrit.ovirt.org/41092 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I674666a4e84ca0f4bb3ed7eb96654f2362020d4d Gerrit-PatchSet: 7 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Tomer Saban <tsa...@redhat.com> Gerrit-Reviewer: Dudi Maroshi <d...@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
