From f807611cb1efd307031d3bed10914d07c643e344 Mon Sep 17 00:00:00 2001 From: Moiz Raja Date: Thu, 5 Nov 2015 18:21:56 -0800 Subject: [PATCH] BUG 4554 : Ownership is not cleared when all candidates are removed When all candidates for an entity get unregistered at approximately the same time it can create a situation where the owner for the entity is not cleared. Consequently no entity ownership change is raised where hasOwner is false even when there are no owners for the entity. This could be a problem for applications which do some action when there are no candidates for an entity. The openflow application for example relies on the disappearance of all owners to actually remove a switch from inventory. Without this event we have the situation that nodes hang around in inventory. Problem Sequence ---------------- The sequence of events which leads to this problem are as follows. Let's say member-1 owned entity-1 and there are 3 candidates for entity-1 - member-1, member-2 and member-3. Now let's say due to some event all candidates have to unregister. The data transaformations will go like this. delete member-1 delete member-2 delete member-3 delete member-1 succeeds so choose new owner - in this case member-2 make-owner member-2 delete member-2 succeeds - member-2 is not the current owner so do nothing delete member-3 succeeds - member-3 is not the current owner so do nothing make-owner member-2 succeeds. Now we have an owner for entity-1 even though we have not candidates Solution -------- The solution proposed in this patch is to set member to empty when there are no remaining candidates. This changes the above sequence as follows. delete member-1 delete member-2 delete member-3 delete member-1 succeeds so choose new owner - in this case member-2 make-owner member-2 delete member-2 succeeds - member-2 is not the current owner so do nothing delete member-3 succeeds - member-3 is the last candidate so set member to "" make-owner "" make-owner member-2 succeeds. Now we have an owner for entity-1 even though we have not candidates make-owner "" succeeds. Now we have owner for entity-1 set to no one as it should be Change-Id: I583e8c6991742ada5846e87da35db255eeed144e Signed-off-by: Moiz Raja --- .../entityownership/EntityOwnershipShard.java | 2 +- ...ributedEntityOwnershipIntegrationTest.java | 58 +++++++++++++++++++ 2 files changed, 59 insertions(+), 1 deletion(-) 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 d104222b9c..058ab498d9 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 @@ -273,7 +273,7 @@ class EntityOwnershipShard extends Shard { if(isLeader()) { String currentOwner = getCurrentOwner(message.getEntityPath()); - if(message.getRemovedCandidate().equals(currentOwner)){ + if(message.getRemovedCandidate().equals(currentOwner) || message.getRemainingCandidates().size() == 0){ String entityType = EntityOwnersModel.entityTypeFromEntityPath(message.getEntityPath()); writeNewOwner(message.getEntityPath(), newOwner(message.getRemainingCandidates(), entityOwnershipStatistics.byEntityType(entityType), 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 634b92cdb7..2ca128271e 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 @@ -8,8 +8,11 @@ package org.opendaylight.controller.cluster.datastore.entityownership; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; import static org.mockito.AdditionalMatchers.or; import static org.mockito.Mockito.atMost; +import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.reset; import static org.mockito.Mockito.timeout; import static org.mockito.Mockito.verify; @@ -33,14 +36,19 @@ import java.util.concurrent.TimeUnit; import org.junit.After; import org.junit.Before; import org.junit.Test; +import org.mockito.ArgumentCaptor; import org.mockito.Mock; +import org.mockito.Mockito; import org.mockito.MockitoAnnotations; import org.opendaylight.controller.cluster.datastore.DatastoreContext; import org.opendaylight.controller.cluster.datastore.DistributedDataStore; import org.opendaylight.controller.cluster.datastore.IntegrationTestKit; import org.opendaylight.controller.cluster.datastore.entityownership.selectionstrategy.EntityOwnerSelectionStrategyConfig; import org.opendaylight.controller.md.cluster.datastore.model.SchemaContextHelper; +import org.opendaylight.controller.md.sal.common.api.clustering.CandidateAlreadyRegisteredException; import org.opendaylight.controller.md.sal.common.api.clustering.Entity; +import org.opendaylight.controller.md.sal.common.api.clustering.EntityOwnershipCandidateRegistration; +import org.opendaylight.controller.md.sal.common.api.clustering.EntityOwnershipChange; import org.opendaylight.controller.md.sal.common.api.clustering.EntityOwnershipListener; import org.opendaylight.controller.md.sal.common.api.clustering.EntityOwnershipState; import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.controller.md.sal.clustering.entity.owners.rev150804.entity.owners.entity.type.entity.Candidate; @@ -260,6 +268,56 @@ public class DistributedEntityOwnershipIntegrationTest { verify(follower1MockListener, timeout(5000)).ownershipChanged(ownershipChange(ENTITY2, false, false, false)); } + /** + * Reproduces bug 4554 + * + * @throws CandidateAlreadyRegisteredException + */ + @Test + public void testCloseCandidateRegistrationInQuickSuccession() throws CandidateAlreadyRegisteredException { + initDatastores("testCloseCandidateRegistrationInQuickSuccession"); + + leaderEntityOwnershipService.registerListener(ENTITY_TYPE1, leaderMockListener); + follower1EntityOwnershipService.registerListener(ENTITY_TYPE1, follower1MockListener); + follower2EntityOwnershipService.registerListener(ENTITY_TYPE1, follower2MockListener); + + final EntityOwnershipCandidateRegistration candidate1 = leaderEntityOwnershipService.registerCandidate(ENTITY1); + final EntityOwnershipCandidateRegistration candidate2 = follower1EntityOwnershipService.registerCandidate(ENTITY1); + final EntityOwnershipCandidateRegistration candidate3 = follower2EntityOwnershipService.registerCandidate(ENTITY1); + + verify(leaderMockListener, timeout(5000)).ownershipChanged(ownershipChange(ENTITY1, false, true, true)); + + Mockito.reset(leaderMockListener); + + candidate1.close(); + candidate2.close(); + candidate3.close(); + + ArgumentCaptor leaderChangeCaptor = ArgumentCaptor.forClass(EntityOwnershipChange.class); + ArgumentCaptor follower1ChangeCaptor = ArgumentCaptor.forClass(EntityOwnershipChange.class); + ArgumentCaptor follower2ChangeCaptor = ArgumentCaptor.forClass(EntityOwnershipChange.class); + doNothing().when(leaderMockListener).ownershipChanged(leaderChangeCaptor.capture()); + doNothing().when(follower1MockListener).ownershipChanged(follower1ChangeCaptor.capture()); + doNothing().when(follower2MockListener).ownershipChanged(follower2ChangeCaptor.capture()); + + boolean passed = false; + for(int i=0;i<100;i++) { + Uninterruptibles.sleepUninterruptibly(50, TimeUnit.MILLISECONDS); + if(!leaderEntityOwnershipService.getOwnershipState(ENTITY1).get().hasOwner() && + !follower1EntityOwnershipService.getOwnershipState(ENTITY1).get().hasOwner() && + !follower2EntityOwnershipService.getOwnershipState(ENTITY1).get().hasOwner()) { + passed = true; + break; + } + } + + assertTrue("No ownership change message was sent with hasOwner=false", passed); + + assertFalse(leaderChangeCaptor.getAllValues().get(leaderChangeCaptor.getAllValues().size()-1).hasOwner()); + assertFalse(follower1ChangeCaptor.getAllValues().get(follower1ChangeCaptor.getAllValues().size()-1).hasOwner()); + assertFalse(follower2ChangeCaptor.getAllValues().get(follower2ChangeCaptor.getAllValues().size()-1).hasOwner()); + } + private static void verifyGetOwnershipState(DistributedEntityOwnershipService service, Entity entity, boolean isOwner, boolean hasOwner) { Optional state = service.getOwnershipState(entity); -- 2.36.6