Bug 7746: Fix intermittent EOS test failure and synchronization 41/51941/3
authorTom Pantelis <tpanteli@brocade.com>
Thu, 16 Feb 2017 09:20:11 +0000 (04:20 -0500)
committerTom Pantelis <tpanteli@brocade.com>
Fri, 17 Feb 2017 11:05:17 +0000 (11:05 +0000)
Modified the EntityOwnershipListenerSupport class to be thread-safe
utilizing a ReadWriteLock to guard access to the listenerActorMap
and entityTypeListenerMap. While updates are only done by the
EntityOwnershipShard and thus aren't concurrent, read access occurs
concurrently via the EntityOwnershipChangeListener which runs in its
own actor sandbox. I also factored out an
EntityOwnershipChangePublisher interface with the read-only access
methods used by EntityOwnershipChangeListener to make it clear
that EntityOwnershipShard is the only mutator.

The testFunctionalityWithThreeNodes case failed intermittently b/c
it's possible for the follower2MockListener to get notified of a
pre-existing entity on registration twice if the prior owner change
is replicated and the EntityOwnershipChangeListener triggers concurrently
with the registration and the timing is just right. I added verification
of ownership sync to follower2 pior to registration to make it
deterministic and avoid the sporadic timing failures.

Change-Id: Icc197d0c23135ca69b56eac1702f249e8e60e66e
Signed-off-by: Tom Pantelis <tpanteli@brocade.com>
opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/entityownership/EntityOwnerChangeListener.java
opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/entityownership/EntityOwnershipChangePublisher.java [new file with mode: 0644]
opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/entityownership/EntityOwnershipListenerSupport.java
opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/entityownership/EntityOwnershipShard.java
opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/entityownership/DistributedEntityOwnershipIntegrationTest.java
opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/entityownership/EntityOwnershipListenerSupportTest.java

index 428c428..04037b3 100644 (file)
@@ -33,11 +33,11 @@ class EntityOwnerChangeListener extends AbstractEntityOwnerChangeListener {
     private static final Logger LOG = LoggerFactory.getLogger(EntityOwnerChangeListener.class);
 
     private final String localMemberName;
-    private final EntityOwnershipListenerSupport listenerSupport;
+    private final EntityOwnershipChangePublisher publisher;
 
-    EntityOwnerChangeListener(final MemberName localMemberName, final EntityOwnershipListenerSupport listenerSupport) {
+    EntityOwnerChangeListener(final MemberName localMemberName, final EntityOwnershipChangePublisher publisher) {
         this.localMemberName = Verify.verifyNotNull(localMemberName.getName());
-        this.listenerSupport = Preconditions.checkNotNull(listenerSupport);
+        this.publisher = Preconditions.checkNotNull(publisher);
     }
 
     @Override
@@ -70,12 +70,12 @@ class EntityOwnerChangeListener extends AbstractEntityOwnerChangeListener {
                     "{}: Calling notifyEntityOwnershipListeners: entity: {}, wasOwner: {}, isOwner: {}, hasOwner: {}",
                     logId(), entity, wasOwner, isOwner, hasOwner);
 
-                listenerSupport.notifyEntityOwnershipListeners(entity, wasOwner, isOwner, hasOwner);
+                publisher.notifyEntityOwnershipListeners(entity, wasOwner, isOwner, hasOwner);
             }
         }
     }
 
     private String logId() {
-        return listenerSupport.getLogId();
+        return publisher.getLogId();
     }
 }
diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/entityownership/EntityOwnershipChangePublisher.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/entityownership/EntityOwnershipChangePublisher.java
new file mode 100644 (file)
index 0000000..eb1e188
--- /dev/null
@@ -0,0 +1,21 @@
+/*
+ * Copyright (c) 2017 Brocade Communications Systems, Inc. and others.  All rights reserved.
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License v1.0 which accompanies this distribution,
+ * and is available at http://www.eclipse.org/legal/epl-v10.html
+ */
+package org.opendaylight.controller.cluster.datastore.entityownership;
+
+import org.opendaylight.mdsal.eos.dom.api.DOMEntity;
+
+/**
+ * Abstract base for notifying EntityOwnershipListeners.
+ *
+ * @author Thomas Pantelis
+ */
+abstract class EntityOwnershipChangePublisher {
+    abstract void notifyEntityOwnershipListeners(DOMEntity entity, boolean wasOwner, boolean isOwner, boolean hasOwner);
+
+    abstract String getLogId();
+}
index be82582..d2b4dd3 100644 (file)
@@ -11,13 +11,16 @@ import akka.actor.ActorContext;
 import akka.actor.ActorRef;
 import akka.actor.PoisonPill;
 import com.google.common.collect.HashMultimap;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Multimap;
 import java.util.Collection;
-import java.util.Collections;
-import java.util.HashSet;
 import java.util.IdentityHashMap;
 import java.util.Map;
-import java.util.Set;
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+import java.util.stream.Collectors;
+import javax.annotation.concurrent.GuardedBy;
+import javax.annotation.concurrent.ThreadSafe;
 import org.opendaylight.mdsal.eos.common.api.EntityOwnershipChangeState;
 import org.opendaylight.mdsal.eos.dom.api.DOMEntity;
 import org.opendaylight.mdsal.eos.dom.api.DOMEntityOwnershipChange;
@@ -30,14 +33,20 @@ import org.slf4j.LoggerFactory;
  *
  * @author Thomas Pantelis
  */
-class EntityOwnershipListenerSupport {
+@ThreadSafe
+class EntityOwnershipListenerSupport extends EntityOwnershipChangePublisher {
     private static final Logger LOG = LoggerFactory.getLogger(EntityOwnershipListenerSupport.class);
 
     private final String logId;
     private final ActorContext actorContext;
+    private final ReadWriteLock listenerLock = new ReentrantReadWriteLock();
+
+    @GuardedBy("listenerLock")
     private final Map<DOMEntityOwnershipListener, ListenerActorRefEntry> listenerActorMap = new IdentityHashMap<>();
-    private final Set<DOMEntity> entitiesWithCandidateSet = new HashSet<>();
+
+    @GuardedBy("listenerLock")
     private final Multimap<String, DOMEntityOwnershipListener> entityTypeListenerMap = HashMultimap.create();
+
     private volatile boolean inJeopardy = false;
 
     EntityOwnershipListenerSupport(ActorContext actorContext, String logId) {
@@ -61,52 +70,80 @@ class EntityOwnershipListenerSupport {
         return wasInJeopardy;
     }
 
-    boolean hasCandidateForEntity(DOMEntity entity) {
-        return entitiesWithCandidateSet.contains(entity);
-    }
-
-    void setHasCandidateForEntity(DOMEntity entity) {
-        entitiesWithCandidateSet.add(entity);
-    }
-
-    void unsetHasCandidateForEntity(DOMEntity entity) {
-        entitiesWithCandidateSet.remove(entity);
-    }
-
     void addEntityOwnershipListener(String entityType, DOMEntityOwnershipListener listener) {
         LOG.debug("{}: Adding EntityOwnershipListener {} for entity type {}", logId, listener, entityType);
 
-        addListener(listener, entityType);
+        listenerLock.writeLock().lock();
+        try {
+            if (entityTypeListenerMap.put(entityType, listener)) {
+                ListenerActorRefEntry listenerEntry = listenerActorMap.get(listener);
+                if (listenerEntry == null) {
+                    listenerActorMap.put(listener, new ListenerActorRefEntry(listener));
+                } else {
+                    listenerEntry.referenceCount++;
+                }
+            }
+        } finally {
+            listenerLock.writeLock().unlock();
+        }
     }
 
     void removeEntityOwnershipListener(String entityType, DOMEntityOwnershipListener listener) {
         LOG.debug("{}: Removing EntityOwnershipListener {} for entity type {}", logId, listener, entityType);
 
-        removeListener(listener, entityType);
+        listenerLock.writeLock().lock();
+        try {
+            if (entityTypeListenerMap.remove(entityType, listener)) {
+                ListenerActorRefEntry listenerEntry = listenerActorMap.get(listener);
+
+                LOG.debug("{}: Found {}", logId, listenerEntry);
+
+                listenerEntry.referenceCount--;
+                if (listenerEntry.referenceCount <= 0) {
+                    listenerActorMap.remove(listener);
+
+                    if (listenerEntry.actorRef != null) {
+                        LOG.debug("Killing EntityOwnershipListenerActor {}", listenerEntry.actorRef);
+                        listenerEntry.actorRef.tell(PoisonPill.getInstance(), ActorRef.noSender());
+                    }
+                }
+            }
+        } finally {
+            listenerLock.writeLock().unlock();
+        }
     }
 
+    @Override
     void notifyEntityOwnershipListeners(DOMEntity entity, boolean wasOwner, boolean isOwner, boolean hasOwner) {
-        notifyListeners(entity, entity.getType(), wasOwner, isOwner, hasOwner);
+        listenerLock.readLock().lock();
+        try {
+            Collection<DOMEntityOwnershipListener> listeners = entityTypeListenerMap.get(entity.getType());
+            if (!listeners.isEmpty()) {
+                notifyListeners(entity, wasOwner, isOwner, hasOwner, listeners.stream().map(
+                    listener -> listenerActorMap.get(listener)).collect(Collectors.toList()));
+            }
+        } finally {
+            listenerLock.readLock().unlock();
+        }
     }
 
     void notifyEntityOwnershipListener(DOMEntity entity, boolean wasOwner, boolean isOwner, boolean hasOwner,
             DOMEntityOwnershipListener listener) {
-        notifyListeners(entity, wasOwner, isOwner, hasOwner, Collections.singleton(listener));
-    }
-
-    private void notifyListeners(DOMEntity entity, String mapKey, boolean wasOwner, boolean isOwner, boolean hasOwner) {
-        Collection<DOMEntityOwnershipListener> listeners = entityTypeListenerMap.get(mapKey);
-        if (!listeners.isEmpty()) {
-            notifyListeners(entity, wasOwner, isOwner, hasOwner, listeners);
+        listenerLock.readLock().lock();
+        try {
+            notifyListeners(entity, wasOwner, isOwner, hasOwner, ImmutableList.of(listenerActorMap.get(listener)));
+        } finally {
+            listenerLock.readLock().unlock();
         }
     }
 
+    @GuardedBy("listenerLock")
     private void notifyListeners(DOMEntity entity, boolean wasOwner, boolean isOwner, boolean hasOwner,
-            Collection<DOMEntityOwnershipListener> listeners) {
+            Collection<ListenerActorRefEntry> listenerEntries) {
         DOMEntityOwnershipChange changed = new DOMEntityOwnershipChange(entity,
                 EntityOwnershipChangeState.from(wasOwner, isOwner, hasOwner), inJeopardy);
-        for (DOMEntityOwnershipListener listener: listeners) {
-            ActorRef listenerActor = listenerActorFor(listener);
+        for (ListenerActorRefEntry entry: listenerEntries) {
+            ActorRef listenerActor = entry.actorFor();
 
             LOG.debug("{}: Notifying EntityOwnershipListenerActor {} with {}", logId, listenerActor, changed);
 
@@ -114,44 +151,20 @@ class EntityOwnershipListenerSupport {
         }
     }
 
-    private void addListener(DOMEntityOwnershipListener listener, String mapKey) {
-        if (entityTypeListenerMap.put(mapKey, listener)) {
-            ListenerActorRefEntry listenerEntry = listenerActorMap.get(listener);
-            if (listenerEntry == null) {
-                listenerActorMap.put(listener, new ListenerActorRefEntry());
-            } else {
-                listenerEntry.referenceCount++;
-            }
-        }
-    }
-
-    private void removeListener(DOMEntityOwnershipListener listener, String mapKey) {
-        if (entityTypeListenerMap.remove(mapKey, listener)) {
-            ListenerActorRefEntry listenerEntry = listenerActorMap.get(listener);
+    private class ListenerActorRefEntry {
+        final DOMEntityOwnershipListener listener;
 
-            LOG.debug("{}: Found {}", logId, listenerEntry);
+        @GuardedBy("listenerLock")
+        ActorRef actorRef;
 
-            listenerEntry.referenceCount--;
-            if (listenerEntry.referenceCount <= 0) {
-                listenerActorMap.remove(listener);
+        @GuardedBy("listenerLock")
+        int referenceCount = 1;
 
-                if (listenerEntry.actorRef != null) {
-                    LOG.debug("Killing EntityOwnershipListenerActor {}", listenerEntry.actorRef);
-                    listenerEntry.actorRef.tell(PoisonPill.getInstance(), ActorRef.noSender());
-                }
-            }
+        ListenerActorRefEntry(final DOMEntityOwnershipListener listener) {
+            this.listener = listener;
         }
-    }
-
-    private ActorRef listenerActorFor(DOMEntityOwnershipListener listener) {
-        return listenerActorMap.get(listener).actorFor(listener);
-    }
-
-    private class ListenerActorRefEntry {
-        ActorRef actorRef;
-        int referenceCount = 1;
 
-        ActorRef actorFor(DOMEntityOwnershipListener listener) {
+        ActorRef actorFor() {
             if (actorRef == null) {
                 actorRef = actorContext.actorOf(EntityOwnershipListenerActor.props(listener));
 
index e57874a..1dfe03d 100644 (file)
@@ -169,8 +169,6 @@ class EntityOwnershipShard extends Shard {
     private void onRegisterCandidateLocal(final RegisterCandidateLocal registerCandidate) {
         LOG.debug("{}: onRegisterCandidateLocal: {}", persistenceId(), registerCandidate);
 
-        listenerSupport.setHasCandidateForEntity(registerCandidate.getEntity());
-
         NormalizedNode<?, ?> entityOwners = entityOwnersWithCandidate(registerCandidate.getEntity().getType(),
                 registerCandidate.getEntity().getIdentifier(), localMemberName.getName());
         commitCoordinator.commitModification(new MergeModification(ENTITY_OWNERS_PATH, entityOwners), this);
@@ -182,8 +180,6 @@ class EntityOwnershipShard extends Shard {
         LOG.debug("{}: onUnregisterCandidateLocal: {}", persistenceId(), unregisterCandidate);
 
         DOMEntity entity = unregisterCandidate.getEntity();
-        listenerSupport.unsetHasCandidateForEntity(entity);
-
         YangInstanceIdentifier candidatePath = candidatePath(entity.getType(), entity.getIdentifier(),
                 localMemberName.getName());
         commitCoordinator.commitModification(new DeleteModification(candidatePath), this);
index e5c17f5..d15ea39 100644 (file)
@@ -11,6 +11,7 @@ import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
+import static org.mockito.AdditionalMatchers.or;
 import static org.mockito.Mockito.doNothing;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.reset;
@@ -184,6 +185,7 @@ public class DistributedEntityOwnershipIntegrationTest {
         follower1EntityOwnershipService.registerCandidate(ENTITY1);
         verifyCandidates(leaderDistributedDataStore, ENTITY1, "member-1", "member-2");
         verifyOwner(leaderDistributedDataStore, ENTITY1, "member-1");
+        verifyOwner(follower2Node.configDataStore(), ENTITY1, "member-1");
         Uninterruptibles.sleepUninterruptibly(300, TimeUnit.MILLISECONDS);
         verify(leaderMockListener, never()).ownershipChanged(ownershipChange(ENTITY1));
         verify(follower1MockListener, never()).ownershipChanged(ownershipChange(ENTITY1));
@@ -194,13 +196,14 @@ public class DistributedEntityOwnershipIntegrationTest {
                 follower1EntityOwnershipService.registerCandidate(ENTITY2);
         verify(follower1MockListener, timeout(5000)).ownershipChanged(ownershipChange(ENTITY2, false, true, true));
         verify(leaderMockListener, timeout(5000)).ownershipChanged(ownershipChange(ENTITY2, false, false, true));
+        verifyOwner(follower2Node.configDataStore(), ENTITY2, "member-2");
         reset(leaderMockListener, follower1MockListener);
 
         // Register follower2 candidate for entity2 and verify it gets added but doesn't become owner
 
         follower2EntityOwnershipService.registerListener(ENTITY_TYPE1, follower2MockListener);
-        verify(follower2MockListener, timeout(5000)).ownershipChanged(ownershipChange(ENTITY2, false, false, true));
-        verify(follower2MockListener, timeout(5000)).ownershipChanged(ownershipChange(ENTITY1, false, false, true));
+        verify(follower2MockListener, timeout(5000).times(2)).ownershipChanged(or(
+                ownershipChange(ENTITY1, false, false, true), ownershipChange(ENTITY2, false, false, true)));
 
         follower2EntityOwnershipService.registerCandidate(ENTITY2);
         verifyCandidates(leaderDistributedDataStore, ENTITY2, "member-2", "member-3");
index 4acf572..dc40f0f 100644 (file)
@@ -161,25 +161,4 @@ public class EntityOwnershipListenerSupportTest extends AbstractEntityOwnershipT
         support.addEntityOwnershipListener(entityType1, mockListener2);
         support.removeEntityOwnershipListener(entityType1, mockListener2);
     }
-
-    @Test
-    public void testHasCandidateForEntity() {
-        EntityOwnershipListenerSupport support = new EntityOwnershipListenerSupport(actorContext, "test");
-        DOMEntity entity = new DOMEntity("type", YangInstanceIdentifier.of(QName.create("test", "id")));
-
-        assertEquals("hasCandidateForEntity", false, support.hasCandidateForEntity(entity));
-
-        support.setHasCandidateForEntity(entity);
-        support.setHasCandidateForEntity(entity); // set again - should be noop
-        assertEquals("hasCandidateForEntity", true, support.hasCandidateForEntity(entity));
-
-        support.unsetHasCandidateForEntity(entity);
-        assertEquals("hasCandidateForEntity", false, support.hasCandidateForEntity(entity));
-
-        support.unsetHasCandidateForEntity(entity); // unset again - should be noop
-        assertEquals("hasCandidateForEntity", false, support.hasCandidateForEntity(entity));
-
-        support.setHasCandidateForEntity(entity);
-        assertEquals("hasCandidateForEntity", true, support.hasCandidateForEntity(entity));
-    }
 }

©2013 OpenDaylight, A Linux Foundation Collaborative Project. All Rights Reserved.
OpenDaylight is a registered trademark of The OpenDaylight Project, Inc.
Linux Foundation and OpenDaylight are registered trademarks of the Linux Foundation.
Linux is a registered trademark of Linus Torvalds.