Fix issues with LeastLoadedCandidateSelectionStrategy 14/33714/3
authorMoiz Raja <moraja@cisco.com>
Wed, 27 Jan 2016 22:43:39 +0000 (14:43 -0800)
committerMoiz Raja <moraja@cisco.com>
Thu, 10 Mar 2016 20:54:00 +0000 (12:54 -0800)
LLCSS degenerates into a round robin owner allocator when
ownership changes. This patch fixes that issue as follows,

- Consider the statistics that are collected using the DTL
  only as initialStatistics which are passed to the Strategy
  when it is created
- When Leadership changes clear all the strategies so that
  they get freshly created with the right initial statistic
- Modify the newOwner method on Strategy to
    - pass the currentOwner for the entity, for the current
      owner we decrease the ownership statistic
    - remove the statistics passed to it as it would no longer
      be required. Due to this removal we also get rid of all
      the CRUD which we had added to check if the passed in
      stats were actually greater than the local stats which
      anyway did not work.

Change-Id: I754f0459051687a95056857044777ca6eebbcd93
Signed-off-by: Moiz Raja <moraja@cisco.com>
opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/entityownership/EntityOwnershipShard.java
opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/entityownership/selectionstrategy/AbstractEntityOwnerSelectionStrategy.java
opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/entityownership/selectionstrategy/EntityOwnerSelectionStrategy.java
opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/entityownership/selectionstrategy/EntityOwnerSelectionStrategyConfig.java
opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/entityownership/selectionstrategy/FirstCandidateSelectionStrategy.java
opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/entityownership/selectionstrategy/LeastLoadedCandidateSelectionStrategy.java
opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/entityownership/selectionstrategy/EntityOwnerSelectionStrategyConfigReaderTest.java
opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/entityownership/selectionstrategy/LastCandidateSelectionStrategy.java
opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/entityownership/selectionstrategy/LeastLoadedCandidateSelectionStrategyTest.java

index fb7e9e4668b5cf7edf97996cf3629bf96fe150c8..2af106980f5041bba4ca93eabdf40451d5d089a3 100644 (file)
@@ -143,7 +143,7 @@ class EntityOwnershipShard extends Shard {
         String currentOwner = getCurrentOwner(selectOwner.getEntityPath());
         if(Strings.isNullOrEmpty(currentOwner)) {
             String entityType = EntityOwnersModel.entityTypeFromEntityPath(selectOwner.getEntityPath());
-            writeNewOwner(selectOwner.getEntityPath(), newOwner(selectOwner.getAllCandidates(),
+            writeNewOwner(selectOwner.getEntityPath(), newOwner(currentOwner, selectOwner.getAllCandidates(),
                     entityOwnershipStatistics.byEntityType(entityType),
                     selectOwner.getOwnerSelectionStrategy()));
 
@@ -274,6 +274,11 @@ class EntityOwnershipShard extends Shard {
                 newLeader, isLeader);
 
         if(isLeader) {
+
+            // Clear all existing strategies so that they get re-created when we call createStrategy again
+            // This allows the strategies to be re-initialized with existing statistics maintained by
+            // EntityOwnershipStatistics
+            strategyConfig.clearStrategies();
             // We were just elected leader. If the old leader is down, select new owners for the entities
             // owned by the down leader.
 
@@ -300,7 +305,7 @@ class EntityOwnershipShard extends Shard {
             if(message.getRemovedCandidate().equals(currentOwner) || message.getRemainingCandidates().size() == 0){
                 String entityType = EntityOwnersModel.entityTypeFromEntityPath(message.getEntityPath());
                 writeNewOwner(message.getEntityPath(),
-                        newOwner(message.getRemainingCandidates(), entityOwnershipStatistics.byEntityType(entityType),
+                        newOwner(currentOwner, message.getRemainingCandidates(), entityOwnershipStatistics.byEntityType(entityType),
                                 getEntityOwnerElectionStrategy(message.getEntityPath())));
             }
         } else {
@@ -323,7 +328,7 @@ class EntityOwnershipShard extends Shard {
 
     private EntityOwnerSelectionStrategy getEntityOwnerElectionStrategy(YangInstanceIdentifier entityPath) {
         final String entityType = EntityOwnersModel.entityTypeFromEntityPath(entityPath);
-        return strategyConfig.createStrategy(entityType);
+        return strategyConfig.createStrategy(entityType, entityOwnershipStatistics.byEntityType(entityType));
     }
 
     private void onCandidateAdded(CandidateAdded message) {
@@ -348,13 +353,13 @@ class EntityOwnershipShard extends Shard {
         LOG.debug("{}: Using strategy {} to select owner", persistenceId(), strategy);
         if(Strings.isNullOrEmpty(currentOwner)){
             if(strategy.getSelectionDelayInMillis() == 0L) {
-                writeNewOwner(message.getEntityPath(), newOwner(message.getAllCandidates(),
+                writeNewOwner(message.getEntityPath(), newOwner(currentOwner, message.getAllCandidates(),
                         entityOwnershipStatistics.byEntityType(entityType), strategy));
             } else if(message.getAllCandidates().size() == availableMembers) {
                 LOG.debug("{}: Received the maximum candidates requests : {} writing new owner",
                         persistenceId(), availableMembers);
                 cancelOwnerSelectionTask(message.getEntityPath());
-                writeNewOwner(message.getEntityPath(), newOwner(message.getAllCandidates(),
+                writeNewOwner(message.getEntityPath(), newOwner(currentOwner, message.getAllCandidates(),
                         entityOwnershipStatistics.byEntityType(entityType), strategy));
             } else {
                 scheduleOwnerSelection(message.getEntityPath(), message.getAllCandidates(), strategy);
@@ -464,12 +469,12 @@ class EntityOwnershipShard extends Shard {
         }
     }
 
-    private String newOwner(Collection<String> candidates, Map<String, Long> statistics, EntityOwnerSelectionStrategy ownerSelectionStrategy) {
+    private String newOwner(String currentOwner, Collection<String> candidates, Map<String, Long> statistics, EntityOwnerSelectionStrategy ownerSelectionStrategy) {
         Collection<String> viableCandidates = getViableCandidates(candidates);
         if(viableCandidates.size() == 0){
             return "";
         }
-        return ownerSelectionStrategy.newOwner(viableCandidates, statistics);
+        return ownerSelectionStrategy.newOwner(currentOwner, viableCandidates);
     }
 
     private Collection<String> getViableCandidates(Collection<String> candidates) {
index df709c1a95ff05cebc8408cba1a12feb970d0a0c..a757f493b3b42a62b2a1c76b55dbfcba4da29459 100644 (file)
@@ -8,16 +8,24 @@
 
 package org.opendaylight.controller.cluster.datastore.entityownership.selectionstrategy;
 
+import java.util.Map;
+
 public abstract class AbstractEntityOwnerSelectionStrategy implements EntityOwnerSelectionStrategy {
 
     private final long selectionDelayInMillis;
+    private final Map<String, Long> initialStatistics;
 
-    protected AbstractEntityOwnerSelectionStrategy(long selectionDelayInMillis) {
+    protected AbstractEntityOwnerSelectionStrategy(long selectionDelayInMillis, Map<String, Long> initialStatistics) {
         this.selectionDelayInMillis = selectionDelayInMillis;
+        this.initialStatistics = initialStatistics;
     }
 
     @Override
     public long getSelectionDelayInMillis() {
         return selectionDelayInMillis;
     }
+
+    public Map<String, Long> getInitialStatistics() {
+        return initialStatistics;
+    }
 }
index 53b35f65c404c81aabce94fcd0b83406c8258ee3..7cbc07527db878ea458dfa7f8a2bf8517631e982 100644 (file)
@@ -9,7 +9,7 @@
 package org.opendaylight.controller.cluster.datastore.entityownership.selectionstrategy;
 
 import java.util.Collection;
-import java.util.Map;
+import javax.annotation.Nullable;
 
 /**
  * An EntityOwnerSelectionStrategy is to be used by the EntityOwnershipShard to select a new owner from a collection
@@ -24,11 +24,9 @@ public interface EntityOwnerSelectionStrategy {
 
 
     /**
-     *
+     * @param currentOwner the current owner of the entity if any, null otherwise
      * @param viableCandidates the available candidates from which to choose the new owner
-     * @param statistics contains a snapshot of a mapping between candidate names and the number of entities
-     *                   owned by that candidate
      * @return the new owner
      */
-    String newOwner(Collection<String> viableCandidates, Map<String, Long> statistics);
+    String newOwner(@Nullable String currentOwner, Collection<String> viableCandidates);
 }
index c1ddab3c7d67aa351a38654351a93a34673e6d27..5a785fcc00b5610f42e6730f8334fddb0a0edf07 100644 (file)
@@ -30,7 +30,7 @@ public final class EntityOwnerSelectionStrategyConfig {
         return entityTypeToStrategyInfo.get(entityType) != null;
     }
 
-    public EntityOwnerSelectionStrategy createStrategy(final String entityType) {
+    public EntityOwnerSelectionStrategy createStrategy(String entityType, Map<String, Long> initialStatistics){
         final EntityOwnerSelectionStrategy strategy;
         final EntityOwnerSelectionStrategy existingStrategy = entityTypeToOwnerSelectionStrategy.get(entityType);
         if(existingStrategy != null){
@@ -40,7 +40,7 @@ public final class EntityOwnerSelectionStrategyConfig {
             if(strategyInfo == null){
                 strategy = FirstCandidateSelectionStrategy.INSTANCE;
             } else {
-                strategy = strategyInfo.createStrategy();
+                strategy = strategyInfo.createStrategy(initialStatistics);
             }
             entityTypeToOwnerSelectionStrategy.put(entityType, strategy);
         }
@@ -58,6 +58,10 @@ public final class EntityOwnerSelectionStrategyConfig {
      * using it.
      */
     @Deprecated
+    public void clearStrategies() {
+        entityTypeToOwnerSelectionStrategy.clear();
+    }
+
     private static final class StrategyInfo {
         private final Class<? extends EntityOwnerSelectionStrategy> strategyClass;
         private final long delay;
@@ -67,9 +71,9 @@ public final class EntityOwnerSelectionStrategyConfig {
             this.delay = delay;
         }
 
-        public EntityOwnerSelectionStrategy createStrategy({
+        public EntityOwnerSelectionStrategy createStrategy(Map<String, Long> initialStatistics){
             try {
-                return strategyClass.getDeclaredConstructor(long.class).newInstance(delay);
+                return strategyClass.getDeclaredConstructor(long.class, Map.class).newInstance(delay, initialStatistics);
             } catch (InstantiationException | IllegalAccessException | InvocationTargetException | NoSuchMethodException e) {
                 LOG.warn("could not create custom strategy", e);
             }
index d86fcbd24967b59282bb3788c5ad53a6f6dbaa3d..1fe0296165f3b7a558a92f388691b7994a69eda8 100644 (file)
@@ -10,6 +10,7 @@ package org.opendaylight.controller.cluster.datastore.entityownership.selections
 
 import com.google.common.base.Preconditions;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.Map;
 
 /**
@@ -17,14 +18,14 @@ import java.util.Map;
  */
 public class FirstCandidateSelectionStrategy extends AbstractEntityOwnerSelectionStrategy {
 
-    public static final FirstCandidateSelectionStrategy INSTANCE = new FirstCandidateSelectionStrategy(0L);
+    public static final FirstCandidateSelectionStrategy INSTANCE = new FirstCandidateSelectionStrategy(0L, Collections.emptyMap());
 
-    public FirstCandidateSelectionStrategy(long selectionDelayInMillis) {
-        super(selectionDelayInMillis);
+    public FirstCandidateSelectionStrategy(long selectionDelayInMillis, Map<String, Long> initialStatistics) {
+        super(selectionDelayInMillis, initialStatistics);
     }
 
     @Override
-    public String newOwner(Collection<String> viableCandidates, Map<String, Long> statistics) {
+    public String newOwner(String currentOwner, Collection<String> viableCandidates) {
         Preconditions.checkArgument(viableCandidates.size() > 0, "No viable candidates provided");
         return viableCandidates.iterator().next();
     }
index b754046974b2fa48bc641f2c16860e69ec61db43..ee01d95496876ef0c0f5525ba59280898df0cd78 100644 (file)
@@ -8,7 +8,10 @@
 
 package org.opendaylight.controller.cluster.datastore.entityownership.selectionstrategy;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.MoreObjects;
+import com.google.common.base.Preconditions;
+import com.google.common.base.Strings;
 import java.util.Collection;
 import java.util.HashMap;
 import java.util.Map;
@@ -24,27 +27,25 @@ public class LeastLoadedCandidateSelectionStrategy extends AbstractEntityOwnerSe
 
     private Map<String, Long> localStatistics = new HashMap<>();
 
-    protected LeastLoadedCandidateSelectionStrategy(long selectionDelayInMillis) {
-        super(selectionDelayInMillis);
+    protected LeastLoadedCandidateSelectionStrategy(long selectionDelayInMillis, Map<String, Long> initialStatistics) {
+        super(selectionDelayInMillis, initialStatistics);
+
+        localStatistics.putAll(initialStatistics);
     }
 
     @Override
-    public String newOwner(Collection<String> viableCandidates, Map<String, Long> statistics) {
+    public String newOwner(String currentOwner, Collection<String> viableCandidates) {
+        Preconditions.checkArgument(viableCandidates.size() > 0);
         String leastLoadedCandidate = null;
         long leastLoadedCount = Long.MAX_VALUE;
 
+        if(!Strings.isNullOrEmpty(currentOwner)){
+            long localVal = MoreObjects.firstNonNull(localStatistics.get(currentOwner), 0L);
+            localStatistics.put(currentOwner, localVal - 1);
+        }
+
         for(String candidateName : viableCandidates){
-            long val = MoreObjects.firstNonNull(statistics.get(candidateName), 0L);
-            long localVal = MoreObjects.firstNonNull(localStatistics.get(candidateName), 0L);
-            if(val < localVal){
-                LOG.debug("Local statistic higher - Candidate : {}, local statistic : {}, provided statistic : {}",
-                        candidateName, localVal, val);
-                val = localVal;
-            } else {
-                LOG.debug("Provided statistic higher - Candidate : {}, local statistic : {}, provided statistic : {}",
-                        candidateName, localVal, val);
-                localStatistics.put(candidateName, val);
-            }
+            long val = MoreObjects.firstNonNull(localStatistics.get(candidateName), 0L);
             if(val < leastLoadedCount){
                 leastLoadedCount = val;
                 leastLoadedCandidate = candidateName;
@@ -58,4 +59,9 @@ public class LeastLoadedCandidateSelectionStrategy extends AbstractEntityOwnerSe
         localStatistics.put(leastLoadedCandidate, leastLoadedCount + 1);
         return leastLoadedCandidate;
     }
+
+    @VisibleForTesting
+    Map<String, Long> getLocalStatistics(){
+        return localStatistics;
+    }
 }
index 8b6e4e42cbe7c79f4ab636cb3fea20f19b04b4a8..f16a90532d35bc4a9a0daa0c1ac6d2cfeb738755 100644 (file)
@@ -9,10 +9,12 @@ package org.opendaylight.controller.cluster.datastore.entityownership.selections
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertTrue;
 import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.doThrow;
 import java.io.IOException;
+import java.util.Collections;
 import java.util.Hashtable;
 import org.junit.Before;
 import org.junit.Test;
@@ -61,9 +63,17 @@ public class EntityOwnerSelectionStrategyConfigReaderTest {
 
         assertTrue(config.isStrategyConfigured("test"));
 
-        EntityOwnerSelectionStrategy strategy = config.createStrategy("test");
-        assertTrue(strategy instanceof LastCandidateSelectionStrategy);
+        EntityOwnerSelectionStrategy strategy = config.createStrategy("test", Collections.<String, Long>emptyMap());
+        assertTrue(strategy.toString(), strategy instanceof LastCandidateSelectionStrategy);
         assertEquals(100L, strategy.getSelectionDelayInMillis());
+
+        final EntityOwnerSelectionStrategy strategy1 = config.createStrategy("test", Collections.<String, Long>emptyMap());
+        assertEquals(strategy, strategy1);
+
+        config.clearStrategies();
+
+        final EntityOwnerSelectionStrategy strategy2 = config.createStrategy("test", Collections.<String, Long>emptyMap());
+        assertNotEquals(strategy1, strategy2);
     }
 
     @Test
@@ -123,8 +133,8 @@ public class EntityOwnerSelectionStrategyConfigReaderTest {
 
         EntityOwnerSelectionStrategyConfig config = loadStrategyConfig();
 
-        assertEquals(100, config.createStrategy("test").getSelectionDelayInMillis());
-        assertEquals(0, config.createStrategy("test2").getSelectionDelayInMillis());
+        assertEquals(100, config.createStrategy("test", Collections.<String, Long>emptyMap()).getSelectionDelayInMillis());
+        assertEquals(0, config.createStrategy("test2", Collections.<String, Long>emptyMap()).getSelectionDelayInMillis());
     }
 
 }
\ No newline at end of file
index 73236c234c25ac34ed635f385bb17e25b1f2079e..3c22cda79934a8ed6cf6ae52598e483686018618 100644 (file)
@@ -13,12 +13,12 @@ import java.util.Collection;
 import java.util.Map;
 
 public class LastCandidateSelectionStrategy extends AbstractEntityOwnerSelectionStrategy {
-    public LastCandidateSelectionStrategy(final long selectionDelayInMillis) {
-        super(selectionDelayInMillis);
+    public LastCandidateSelectionStrategy(long selectionDelayInMillis, Map<String, Long> initialStatistics) {
+        super(selectionDelayInMillis, initialStatistics);
     }
 
     @Override
-    public String newOwner(final Collection<String> viableCandidates, final Map<String, Long> statistics) {
+    public String newOwner(String currentOwner, Collection<String> viableCandidates) {
         return Iterables.getLast(viableCandidates);
     }
 }
index 1054f346f9ba5f77dee9a97ab8c6c2fe251b4c3e..d0cd32f1b01f7ec529b2874fa6db023a56f0a0b0 100644 (file)
@@ -10,6 +10,7 @@ package org.opendaylight.controller.cluster.datastore.entityownership.selections
 import static org.junit.Assert.assertEquals;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
 import org.junit.Test;
@@ -18,23 +19,48 @@ public class LeastLoadedCandidateSelectionStrategyTest {
 
     @Test
     public void testLeastLoadedStrategy(){
-        LeastLoadedCandidateSelectionStrategy strategy = new LeastLoadedCandidateSelectionStrategy(0L);
+        LeastLoadedCandidateSelectionStrategy strategy = new LeastLoadedCandidateSelectionStrategy(0L, Collections.<String, Long>emptyMap());
 
-        String owner = strategy.newOwner(prepareViableCandidates(3), new HashMap<String, Long>());
+        String owner = strategy.newOwner(null, prepareViableCandidates(3));
         assertEquals("member-1", owner);
 
+        Map<String, Long> localStatistics = strategy.getLocalStatistics();
+        assertEquals(1L, (long) localStatistics.get("member-1"));
+
         // member-2 has least load
-        owner = strategy.newOwner(prepareViableCandidates(3), prepareStatistics(5,2,4));
+        strategy = new LeastLoadedCandidateSelectionStrategy(0L, prepareStatistics(5,2,4));
+        owner = strategy.newOwner(null, prepareViableCandidates(3));
         assertEquals("member-2", owner);
 
+        assertStatistics(strategy.getLocalStatistics(), 5,3,4);
+
         // member-3 has least load
-        owner = strategy.newOwner(prepareViableCandidates(3), prepareStatistics(5,7,4));
+        strategy = new LeastLoadedCandidateSelectionStrategy(0L, prepareStatistics(5,7,4));
+        owner = strategy.newOwner(null, prepareViableCandidates(3));
         assertEquals("member-3", owner);
 
+        assertStatistics(strategy.getLocalStatistics(), 5,7,5);
+
         // member-1 has least load
-        owner = strategy.newOwner(prepareViableCandidates(3), prepareStatistics(1,7,4));
+        strategy = new LeastLoadedCandidateSelectionStrategy(0L, prepareStatistics(1,7,4));
+        owner = strategy.newOwner(null, prepareViableCandidates(3));
+        assertEquals("member-1", owner);
+
+        assertStatistics(strategy.getLocalStatistics(), 2,7,4);
+
+        // Let member-3 become the owner
+        strategy = new LeastLoadedCandidateSelectionStrategy(0L, prepareStatistics(3,3,0));
+        owner = strategy.newOwner(null, prepareViableCandidates(3));
+        assertEquals("member-3", owner);
+
+        assertStatistics(strategy.getLocalStatistics(), 3,3,1);
+
+        // member-3 is no longer viable so choose a new owner
+        owner = strategy.newOwner("member-3", prepareViableCandidates(2));
         assertEquals("member-1", owner);
 
+        assertStatistics(strategy.getLocalStatistics(), 4,3,0);
+
     }
 
     private static Map<String, Long> prepareStatistics(long... count){
@@ -52,4 +78,10 @@ public class LeastLoadedCandidateSelectionStrategyTest {
         }
         return viableCandidates;
     }
+
+    private void assertStatistics(Map<String, Long> statistics, long... count){
+        for(int i=0;i<count.length;i++){
+            assertEquals(count[i], (long) statistics.get("member-" + (i+1)));
+        }
+    }
 }
\ No newline at end of file