From f93b17ba71874d70af4c243017eed23c3a36e63c Mon Sep 17 00:00:00 2001 From: Amit Mandke Date: Tue, 29 Mar 2016 10:25:29 -0700 Subject: [PATCH] Bug 5613 - unregister candidates on node restart When EntityOwnershipShard receives CandidateAdded for local candidate before any local registration happes it means a restart of a local node must have happned and the candiates are not registered yet. So this change removes candidate for such case. The corresponding test reproduces the issue if the change is not applied. Fixed other test failures. Change-Id: I0e8e675530c93dca172ca661fa4c5e1250f40150 Signed-off-by: Amit Mandke --- .../entityownership/EntityOwnershipShard.java | 10 ++ .../AbstractEntityOwnershipTest.java | 2 +- ...ributedEntityOwnershipIntegrationTest.java | 130 +++++++++++++++++- .../EntityOwnershipShardTest.java | 84 ++++++++++- 4 files changed, 218 insertions(+), 8 deletions(-) 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 2af106980f..665099f44f 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 @@ -333,6 +333,16 @@ class EntityOwnershipShard extends Shard { private void onCandidateAdded(CandidateAdded message) { if(!isLeader()){ + Entity entity = createEntity(message.getEntityPath()); + if(!listenerSupport.hasCandidateForEntity(entity) && localMemberName.equals(message.getNewCandidate())) { + // This means CandidateAdded is received for local candidate before any local registration happens. + // This will happen only on a restart of a local node. So remove this candidate for now, it will be + // added back when a local registration is received again. + LOG.debug("Remove Candidate {} for {} as not registered as candidate yet locally.", + message.getNewCandidate(), entity.getId()); + YangInstanceIdentifier candidatePath = candidatePath(entity.getType(), entity.getId(), localMemberName); + commitCoordinator.commitModification(new DeleteModification(candidatePath), this); + } return; } diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/entityownership/AbstractEntityOwnershipTest.java b/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/entityownership/AbstractEntityOwnershipTest.java index 9315cc2dde..9b4f9e97cc 100644 --- a/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/entityownership/AbstractEntityOwnershipTest.java +++ b/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/entityownership/AbstractEntityOwnershipTest.java @@ -122,13 +122,13 @@ public class AbstractEntityOwnershipTest extends AbstractActorTest { Stopwatch sw = Stopwatch.createStarted(); while(sw.elapsed(TimeUnit.MILLISECONDS) <= 5000) { try { + Uninterruptibles.sleepUninterruptibly(100, TimeUnit.MILLISECONDS); NormalizedNode node = reader.apply(entityPath); Assert.assertNotNull("Owner was not set for entityId: " + entityId, node); Assert.assertEquals("Entity owner", expected, node.getValue().toString()); return; } catch(AssertionError e) { lastError = e; - Uninterruptibles.sleepUninterruptibly(100, TimeUnit.MILLISECONDS); } } 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 a664fb7ece..a80a46ae9d 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 @@ -34,6 +34,7 @@ import com.google.common.util.concurrent.Uninterruptibles; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; +import java.util.HashSet; import java.util.List; import java.util.concurrent.TimeUnit; import org.junit.After; @@ -108,6 +109,9 @@ public class DistributedEntityOwnershipIntegrationTest { @Mock private EntityOwnershipListener follower2MockListener; + @Mock + private EntityOwnershipListener member1Listener, member2Listener, member3Listener; + @Before public void setUp() { MockitoAnnotations.initMocks(this); @@ -343,12 +347,132 @@ public class DistributedEntityOwnershipIntegrationTest { verifyOwner(follower1Node.configDataStore(), ENTITY2, "member-3"); } + /** + * Tests if candidate(s) get unregistered for restarted nodes, in case majority of nodes restart including the node + * that was leader for entity-ownership shard. + * Tests for the bug 4554 * * @throws CandidateAlreadyRegisteredException */ - @Test + public void testCloseCandidateRegistrationInQuickSuccession() throws CandidateAlreadyRegisteredException { String name = "testCloseCandidateRegistrationInQuickSuccession"; MemberNode leaderNode = MemberNode.builder(memberNodes).akkaConfig("Member1").testName(name ). @@ -507,12 +631,12 @@ public class DistributedEntityOwnershipIntegrationTest { entityPath(entity.getType(), entity.getId()).node(Candidate.QNAME)).get(5, TimeUnit.SECONDS); try { assertEquals("Candidates not found for " + entity, true, possible.isPresent()); - Collection actual = new ArrayList<>(); + Collection actual = new HashSet<>(); for(MapEntryNode candidate: ((MapNode)possible.get()).getValue()) { actual.add(candidate.getChild(CANDIDATE_NAME_NODE_ID).get().getValue().toString()); } - assertEquals("Candidates for " + entity, Arrays.asList(expCandidates), actual); + assertEquals("Candidates for " + entity, new HashSet(Arrays.asList(expCandidates)), actual); return; } catch (AssertionError e) { lastError = e; diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/entityownership/EntityOwnershipShardTest.java b/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/entityownership/EntityOwnershipShardTest.java index 155c4650c8..951ec0ade5 100644 --- a/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/entityownership/EntityOwnershipShardTest.java +++ b/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/entityownership/EntityOwnershipShardTest.java @@ -432,24 +432,34 @@ public class EntityOwnershipShardTest extends AbstractEntityOwnershipTest { kit.expectMsgClass(SuccessReply.class); verifyCommittedEntityCandidate(leader, ENTITY_TYPE, ENTITY_ID1, LOCAL_MEMBER_NAME); + peer2.tell(new RegisterCandidateLocal(new Entity(ENTITY_TYPE, ENTITY_ID1)), kit.getRef()); + kit.expectMsgClass(SuccessReply.class); commitModification(leader, entityOwnersWithCandidate(ENTITY_TYPE, ENTITY_ID1, peerMemberName2), kit); verifyCommittedEntityCandidate(leader, ENTITY_TYPE, ENTITY_ID1, peerMemberName2); + peer1.tell(new RegisterCandidateLocal(new Entity(ENTITY_TYPE, ENTITY_ID1)), kit.getRef()); + kit.expectMsgClass(SuccessReply.class); commitModification(leader, entityOwnersWithCandidate(ENTITY_TYPE, ENTITY_ID1, peerMemberName1), kit); verifyCommittedEntityCandidate(leader, ENTITY_TYPE, ENTITY_ID1, peerMemberName1); verifyOwner(leader, ENTITY_TYPE, ENTITY_ID1, LOCAL_MEMBER_NAME); // Add candidates for entity2 with peerMember2 as the owner + peer2.tell(new RegisterCandidateLocal(new Entity(ENTITY_TYPE, ENTITY_ID2)), kit.getRef()); + kit.expectMsgClass(SuccessReply.class); commitModification(leader, entityOwnersWithCandidate(ENTITY_TYPE, ENTITY_ID2, peerMemberName2), kit); verifyCommittedEntityCandidate(leader, ENTITY_TYPE, ENTITY_ID2, peerMemberName2); + peer1.tell(new RegisterCandidateLocal(new Entity(ENTITY_TYPE, ENTITY_ID2)), kit.getRef()); + kit.expectMsgClass(SuccessReply.class); commitModification(leader, entityOwnersWithCandidate(ENTITY_TYPE, ENTITY_ID2, peerMemberName1), kit); verifyCommittedEntityCandidate(leader, ENTITY_TYPE, ENTITY_ID2, peerMemberName1); verifyOwner(leader, ENTITY_TYPE, ENTITY_ID2, peerMemberName2); // Add candidates for entity3 with peerMember2 as the owner. + peer2.tell(new RegisterCandidateLocal(new Entity(ENTITY_TYPE, ENTITY_ID3)), kit.getRef()); + kit.expectMsgClass(SuccessReply.class); commitModification(leader, entityOwnersWithCandidate(ENTITY_TYPE, ENTITY_ID3, peerMemberName2), kit); verifyCommittedEntityCandidate(leader, ENTITY_TYPE, ENTITY_ID3, peerMemberName2); @@ -457,18 +467,24 @@ public class EntityOwnershipShardTest extends AbstractEntityOwnershipTest { kit.expectMsgClass(SuccessReply.class); verifyCommittedEntityCandidate(leader, ENTITY_TYPE, ENTITY_ID3, LOCAL_MEMBER_NAME); + peer1.tell(new RegisterCandidateLocal(new Entity(ENTITY_TYPE, ENTITY_ID3)), kit.getRef()); + kit.expectMsgClass(SuccessReply.class); commitModification(leader, entityOwnersWithCandidate(ENTITY_TYPE, ENTITY_ID3, peerMemberName1), kit); verifyCommittedEntityCandidate(leader, ENTITY_TYPE, ENTITY_ID3, peerMemberName1); verifyOwner(leader, ENTITY_TYPE, ENTITY_ID3, peerMemberName2); // Add only candidate peerMember2 for entity4. + peer2.tell(new RegisterCandidateLocal(new Entity(ENTITY_TYPE, ENTITY_ID4)), kit.getRef()); + kit.expectMsgClass(SuccessReply.class); commitModification(leader, entityOwnersWithCandidate(ENTITY_TYPE, ENTITY_ID4, peerMemberName2), kit); verifyCommittedEntityCandidate(leader, ENTITY_TYPE, ENTITY_ID4, peerMemberName2); verifyOwner(leader, ENTITY_TYPE, ENTITY_ID4, peerMemberName2); // Add only candidate peerMember1 for entity5. + peer1.tell(new RegisterCandidateLocal(new Entity(ENTITY_TYPE, ENTITY_ID5)), kit.getRef()); + kit.expectMsgClass(SuccessReply.class); commitModification(leader, entityOwnersWithCandidate(ENTITY_TYPE, ENTITY_ID5, peerMemberName1), kit); verifyCommittedEntityCandidate(leader, ENTITY_TYPE, ENTITY_ID5, peerMemberName1); verifyOwner(leader, ENTITY_TYPE, ENTITY_ID5, peerMemberName1); @@ -511,8 +527,18 @@ public class EntityOwnershipShardTest extends AbstractEntityOwnershipTest { verifyOwner(leader, ENTITY_TYPE, ENTITY_ID1, LOCAL_MEMBER_NAME); verifyOwner(leader, ENTITY_TYPE, ENTITY_ID4, ""); - // Add back candidate peerMember2 for entities 1, 2, & 3. + peer2.tell(new PeerAddressResolved(peerId1.toString(), peer1.path().toString()), ActorRef.noSender()); + peer2.tell(new PeerAddressResolved(leaderId.toString(), leader.path().toString()), ActorRef.noSender()); + peer2.tell(new PeerUp(LOCAL_MEMBER_NAME, leaderId.toString()), ActorRef.noSender()); + peer2.tell(new PeerUp(peerMemberName1, peerId1.toString()), ActorRef.noSender()); + // Add back candidate peerMember2 for entities 1, 2, & 3. + peer2.tell(new RegisterCandidateLocal(new Entity(ENTITY_TYPE, ENTITY_ID1)), kit.getRef()); + kit.expectMsgClass(SuccessReply.class); + peer2.tell(new RegisterCandidateLocal(new Entity(ENTITY_TYPE, ENTITY_ID2)), kit.getRef()); + kit.expectMsgClass(SuccessReply.class); + peer2.tell(new RegisterCandidateLocal(new Entity(ENTITY_TYPE, ENTITY_ID3)), kit.getRef()); + kit.expectMsgClass(SuccessReply.class); commitModification(leader, entityOwnersWithCandidate(ENTITY_TYPE, ENTITY_ID1, peerMemberName2), kit); commitModification(leader, entityOwnersWithCandidate(ENTITY_TYPE, ENTITY_ID2, peerMemberName2), kit); commitModification(leader, entityOwnersWithCandidate(ENTITY_TYPE, ENTITY_ID3, peerMemberName2), kit); @@ -559,9 +585,6 @@ public class EntityOwnershipShardTest extends AbstractEntityOwnershipTest { // Kill the local leader and elect peer2 the leader. This should cause a new owner to be selected for // the entities (1 and 3) previously owned by the local leader member. - peer2.tell(new PeerAddressResolved(peerId1.toString(), peer1.path().toString()), ActorRef.noSender()); - peer2.tell(new PeerUp(LOCAL_MEMBER_NAME, leaderId.toString()), ActorRef.noSender()); - peer2.tell(new PeerUp(peerMemberName1, peerId1.toString()), ActorRef.noSender()); leader.tell(PoisonPill.getInstance(), ActorRef.noSender()); peer2.tell(new PeerDown(LOCAL_MEMBER_NAME, leaderId.toString()), ActorRef.noSender()); @@ -575,6 +598,59 @@ public class EntityOwnershipShardTest extends AbstractEntityOwnershipTest { verifyOwner(peer2, ENTITY_TYPE, ENTITY_ID1, peerMemberName2); } + @Test + public void testCandidateRemovedWhenCandidateNotRegisteredLocally() throws Exception { + ShardTestKit kit = new ShardTestKit(getSystem()); + + dataStoreContextBuilder.shardHeartbeatIntervalInMillis(500).shardElectionTimeoutFactor(10000); + + String peerMemberName1 = "peerMember1"; + String peerMemberName2 = "peerMember2"; + + ShardIdentifier leaderId = newShardId(LOCAL_MEMBER_NAME); + ShardIdentifier peerId1 = newShardId(peerMemberName1); + ShardIdentifier peerId2 = newShardId(peerMemberName2); + + TestActorRef peer1 = actorFactory.createTestActor(newShardProps(peerId1, + ImmutableMap.builder().put(leaderId.toString(), "").put(peerId2.toString(), "").build(), + peerMemberName1, EntityOwnerSelectionStrategyConfig.newBuilder().build()).withDispatcher(Dispatchers.DefaultDispatcherId()), peerId1.toString()); + + TestActorRef peer2 = actorFactory.createTestActor(newShardProps(peerId2, + ImmutableMap.builder().put(leaderId.toString(), "").put(peerId1.toString(), "").build(), + peerMemberName2, EntityOwnerSelectionStrategyConfig.newBuilder().build()).withDispatcher(Dispatchers.DefaultDispatcherId()), peerId2.toString()); + + TestActorRef leader = actorFactory.createTestActor(newShardProps(leaderId, + ImmutableMap.builder().put(peerId1.toString(), peer1.path().toString()). + put(peerId2.toString(), peer2.path().toString()).build(), LOCAL_MEMBER_NAME, EntityOwnerSelectionStrategyConfig.newBuilder().build()). + withDispatcher(Dispatchers.DefaultDispatcherId()), leaderId.toString()); + leader.tell(new ElectionTimeout(), leader); + + kit.waitUntilLeader(leader); + + peer2.tell(new PeerAddressResolved(peerId1.toString(), peer1.path().toString()), ActorRef.noSender()); + peer2.tell(new PeerAddressResolved(leaderId.toString(), leader.path().toString()), ActorRef.noSender()); + peer1.tell(new PeerAddressResolved(peerId2.toString(), peer2.path().toString()), ActorRef.noSender()); + peer1.tell(new PeerAddressResolved(leaderId.toString(), leader.path().toString()), ActorRef.noSender()); + + leader.tell(new RegisterCandidateLocal(new Entity(ENTITY_TYPE, ENTITY_ID1)), kit.getRef()); + kit.expectMsgClass(SuccessReply.class); + verifyCommittedEntityCandidate(leader, ENTITY_TYPE, ENTITY_ID1, LOCAL_MEMBER_NAME); + + peer2.tell(new RegisterCandidateLocal(new Entity(ENTITY_TYPE, ENTITY_ID1)), kit.getRef()); + kit.expectMsgClass(SuccessReply.class); + commitModification(leader, entityOwnersWithCandidate(ENTITY_TYPE, ENTITY_ID1, peerMemberName2), kit); + verifyCommittedEntityCandidate(leader, ENTITY_TYPE, ENTITY_ID1, peerMemberName2); + + // Try adding peerMemberName1 as candidate without registering locally + commitModification(leader, entityOwnersWithCandidate(ENTITY_TYPE, ENTITY_ID1, peerMemberName1), kit); + + verifyOwner(leader, ENTITY_TYPE, ENTITY_ID1, LOCAL_MEMBER_NAME); + + // confirm peerMemberName1 is not candidate as was not registered locally + verifyNoEntityCandidate(leader, ENTITY_TYPE, ENTITY_ID1, peerMemberName1); + } + + @Test public void testLocalCandidateRemovedWithCandidateRegistered() throws Exception { ShardTestKit kit = new ShardTestKit(getSystem()); -- 2.36.6