From: Tom Pantelis Date: Thu, 16 Feb 2017 09:20:11 +0000 (-0500) Subject: Bug 7746: Fix intermittent EOS test failure and synchronization X-Git-Tag: release/carbon~260 X-Git-Url: https://git.opendaylight.org/gerrit/gitweb?p=controller.git;a=commitdiff_plain;h=7a602c554a1421ceb0da9f1ae297c17f705f663f Bug 7746: Fix intermittent EOS test failure and synchronization 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 --- diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/entityownership/EntityOwnerChangeListener.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/entityownership/EntityOwnerChangeListener.java index 428c42817e..04037b35ea 100644 --- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/entityownership/EntityOwnerChangeListener.java +++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/entityownership/EntityOwnerChangeListener.java @@ -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 index 0000000000..eb1e188f50 --- /dev/null +++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/entityownership/EntityOwnershipChangePublisher.java @@ -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(); +} diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/entityownership/EntityOwnershipListenerSupport.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/entityownership/EntityOwnershipListenerSupport.java index be82582e06..d2b4dd3077 100644 --- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/entityownership/EntityOwnershipListenerSupport.java +++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/entityownership/EntityOwnershipListenerSupport.java @@ -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 listenerActorMap = new IdentityHashMap<>(); - private final Set entitiesWithCandidateSet = new HashSet<>(); + + @GuardedBy("listenerLock") private final Multimap 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 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 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 listeners) { + Collection 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)); diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/entityownership/EntityOwnershipShard.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/entityownership/EntityOwnershipShard.java index e57874a3ad..1dfe03d3d2 100644 --- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/entityownership/EntityOwnershipShard.java +++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/entityownership/EntityOwnershipShard.java @@ -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); diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/entityownership/DistributedEntityOwnershipIntegrationTest.java b/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/entityownership/DistributedEntityOwnershipIntegrationTest.java index e5c17f54c6..d15ea39beb 100644 --- a/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/entityownership/DistributedEntityOwnershipIntegrationTest.java +++ b/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/entityownership/DistributedEntityOwnershipIntegrationTest.java @@ -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"); diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/entityownership/EntityOwnershipListenerSupportTest.java b/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/entityownership/EntityOwnershipListenerSupportTest.java index 4acf572851..dc40f0fa93 100644 --- a/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/entityownership/EntityOwnershipListenerSupportTest.java +++ b/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/entityownership/EntityOwnershipListenerSupportTest.java @@ -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)); - } }