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 428c42817ecf56f2fa96577b2f0f9a99b46a48e2..04037b35ea7ff81d8ce25fe02da8e15b16c9156b 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 be82582e06d2e356b4446e0e51d86c4f7a125f7b..d2b4dd3077d60b80d1112534246e7c0cded97c99 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 e57874a3ad58d7c5559a174549e3ded2817d7001..1dfe03d3d22c3aa6aa144e67baee081d83037bd3 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 e5c17f54c6234b777c8175396eb10f8f82f9f249..d15ea39beb705ce9ed46b3ca63be670e911cdd9d 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 4acf572851aa114f7789f3010096a3790d12b35a..dc40f0fa9318424ebe32888c97b7516235de95ad 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));
-    }
 }