BUG-7505: Conflict Modification

When closing Peer and Rib, can cause
race condition between the removal
of the peer and the rib when updating DS.
Fix by close peers on blocking mode, and once
they are closed proceed with rib.
Same its applied in case that peer/rib is updated.
First we close it and then once DS is updated
we proceed with creating the new instance.

Change-Id: Ibe70e0324ae12bcbb88b2e7f039671141447ddb1
Signed-off-by: Claudio D. Gasparini <claudio.gasparini@pantheon.tech>

diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/RIBImpl.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/RIBImpl.java
index d1a788b75cebb1fddeb3b31f1c46618b454b1ddd..d792e0458927baf7afa58c9812c3522209a5dacf 100755 (executable)
--- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/RIBImpl.java
+++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/RIBImpl.java
@@ -13,6 +13,7 @@ import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.util.concurrent.CheckedFuture;
+import com.google.common.util.concurrent.Futures;
 import com.google.common.util.concurrent.ListenableFuture;
 import java.util.HashMap;
 import java.util.HashSet;
@@ -21,6 +22,7 @@ import java.util.Map;
 import java.util.Map.Entry;
 import java.util.Set;
 import javax.annotation.Nonnull;
+import javax.annotation.concurrent.GuardedBy;
 import javax.annotation.concurrent.ThreadSafe;
 import org.opendaylight.controller.md.sal.common.api.data.AsyncTransaction;
 import org.opendaylight.controller.md.sal.common.api.data.LogicalDatastoreType;
@@ -114,6 +116,8 @@ public final class RIBImpl extends BGPRIBStateImpl implements ClusterSingletonSe
     private final Map<TablesKey, ExportPolicyPeerTracker> exportPolicyPeerTrackerMap;
 
     private DOMTransactionChain domChain;
+    @GuardedBy("this")
+    private boolean isServiceInstantiated;
 
     public RIBImpl(final ClusterSingletonServiceProvider provider, final RibId ribId, final AsNumber localAs, final BgpId localBgpId,
         final ClusterIdentifier clusterId, final RIBExtensionConsumerContext extensions, final BGPDispatcher dispatcher,
@@ -311,7 +315,8 @@ public final class RIBImpl extends BGPRIBStateImpl implements ClusterSingletonSe
     }
 
     @Override
-    public void instantiateServiceInstance() {
+    public synchronized void instantiateServiceInstance() {
+        this.isServiceInstantiated = true;
         this.domChain = this.domDataBroker.createTransactionChain(this);
         if(this.configurationWriter != null) {
             this.configurationWriter.apply();
@@ -350,7 +355,14 @@ public final class RIBImpl extends BGPRIBStateImpl implements ClusterSingletonSe
 
     @Override
     public synchronized ListenableFuture<Void> closeServiceInstance() {
+        if(!this.isServiceInstantiated) {
+            LOG.trace("RIB Singleton Service {} already closed, RIB {}", getIdentifier().getValue(),
+                this.ribId.getValue());
+            return Futures.immediateFuture(null);
+        }
         LOG.info("Close RIB Singleton Service {}, RIB {}", getIdentifier().getValue(), this.ribId.getValue());
+        this.isServiceInstantiated = false;
+
         this.txChainToLocRibWriter.values().forEach(LocRibWriter::close);
         this.txChainToLocRibWriter.clear();
 

diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/config/AppPeer.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/config/AppPeer.java
index 7d9fe8f76db2ada0f2f1943aee1b782c1f4d0c9d..4891504c9d80596850f386b1076515f254031f64 100644 (file)
--- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/config/AppPeer.java
+++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/config/AppPeer.java
@@ -10,8 +10,10 @@ package org.opendaylight.protocol.bgp.rib.impl.config;
 
 import com.google.common.base.Preconditions;
 import com.google.common.base.Strings;
+import com.google.common.util.concurrent.Futures;
 import com.google.common.util.concurrent.ListenableFuture;
 import java.util.Objects;
+import javax.annotation.concurrent.GuardedBy;
 import org.opendaylight.controller.md.sal.common.api.data.LogicalDatastoreType;
 import org.opendaylight.controller.md.sal.dom.api.DOMDataTreeChangeService;
 import org.opendaylight.controller.md.sal.dom.api.DOMDataTreeIdentifier;
@@ -72,6 +74,15 @@ public final class AppPeer implements PeerBean, BGPPeerStateConsumer {
         }
     }
 
+    @Override
+    public ListenableFuture<Void> closeServiceInstance() {
+        if (this.bgpAppPeerSingletonService != null) {
+            return this.bgpAppPeerSingletonService.closeServiceInstance();
+        }
+
+        return Futures.immediateFuture(null);
+    }
+
     @Override
     public Boolean containsEqualConfiguration(final Neighbor neighbor) {
         return Objects.equals(this.currentConfiguration.getKey(), neighbor.getKey())
@@ -103,6 +114,8 @@ public final class AppPeer implements PeerBean, BGPPeerStateConsumer {
         private ClusterSingletonServiceRegistration singletonServiceRegistration;
         private final ServiceGroupIdentifier serviceGroupIdentifier;
         private final WriteConfiguration configurationWriter;
+        @GuardedBy("this")
+        private boolean isServiceInstantiated;
 
         BgpAppPeerSingletonService(final RIB rib, final ApplicationRibId appRibId, final Ipv4Address neighborAddress,
             final WriteConfiguration configurationWriter) {
@@ -126,7 +139,8 @@ public final class AppPeer implements PeerBean, BGPPeerStateConsumer {
         }
 
         @Override
-        public void instantiateServiceInstance() {
+        public synchronized void instantiateServiceInstance() {
+            this.isServiceInstantiated = true;
             if(this.configurationWriter != null) {
                 this.configurationWriter.apply();
             }
@@ -139,9 +153,15 @@ public final class AppPeer implements PeerBean, BGPPeerStateConsumer {
         }
 
         @Override
-        public ListenableFuture<Void> closeServiceInstance() {
+        public synchronized ListenableFuture<Void> closeServiceInstance() {
+            if(!this.isServiceInstantiated) {
+                LOG.trace("Application Peer Singleton Service {} instance already closed, Application peer {}",
+                    getIdentifier().getValue(), this.appRibId.getValue());
+                return Futures.immediateFuture(null);
+            }
             LOG.info("Application Peer Singleton Service {} instance closed, Application peer {}",
                 getIdentifier().getValue(), this.appRibId.getValue());
+            this.isServiceInstantiated = false;
             return this.applicationPeer.close();
         }
 

diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/config/BgpDeployerImpl.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/config/BgpDeployerImpl.java
index 9939b4667b971bea0fc3b8e3a6f577d8eef97b31..52c7a6ecf84cacc8c2ac7524722b30e61f2ff44b 100644 (file)
--- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/config/BgpDeployerImpl.java
+++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/config/BgpDeployerImpl.java
@@ -8,10 +8,12 @@
 
 package org.opendaylight.protocol.bgp.rib.impl.config;
 
+import static com.google.common.util.concurrent.Futures.transform;
 import static org.opendaylight.protocol.bgp.rib.impl.config.OpenConfigMappingUtil.getNeighborInstanceIdentifier;
 import static org.opendaylight.protocol.bgp.rib.impl.config.OpenConfigMappingUtil.getNeighborInstanceName;
 import static org.opendaylight.protocol.bgp.rib.impl.config.OpenConfigMappingUtil.getRibInstanceName;
 
+import com.google.common.base.Function;
 import com.google.common.base.Preconditions;
 import com.google.common.util.concurrent.CheckedFuture;
 import com.google.common.util.concurrent.FutureCallback;
@@ -24,13 +26,13 @@ import java.util.HashMap;
 import java.util.Hashtable;
 import java.util.List;
 import java.util.Map;
+import java.util.stream.Collectors;
 import javax.annotation.concurrent.GuardedBy;
 import org.opendaylight.controller.md.sal.binding.api.ClusteredDataTreeChangeListener;
 import org.opendaylight.controller.md.sal.binding.api.DataBroker;
 import org.opendaylight.controller.md.sal.binding.api.DataObjectModification;
 import org.opendaylight.controller.md.sal.binding.api.DataTreeIdentifier;
 import org.opendaylight.controller.md.sal.binding.api.DataTreeModification;
-import org.opendaylight.controller.md.sal.binding.api.ReadWriteTransaction;
 import org.opendaylight.controller.md.sal.binding.api.WriteTransaction;
 import org.opendaylight.controller.md.sal.common.api.data.LogicalDatastoreType;
 import org.opendaylight.controller.md.sal.common.api.data.TransactionCommitFailedException;
@@ -128,11 +130,30 @@ public final class BgpDeployerImpl implements BgpDeployer, ClusteredDataTreeChan
     public synchronized void close() throws Exception {
         LOG.info("Closing BGP Deployer.");
         this.registration.close();
-        this.peers.values().forEach(PeerBean::close);
-        this.peers.clear();
-        this.ribs.values().forEach(RibImpl::close);
-        this.ribs.clear();
         this.closed = true;
+
+        final List<ListenableFuture<Void>> futurePeerCloseList = this.peers.values().stream()
+            .map(PeerBean::closeServiceInstance).collect(Collectors.toList());
+        final ListenableFuture<List<Void>> futureOfRelevance = Futures.allAsList(futurePeerCloseList);
+
+        final ListenableFuture<ListenableFuture<List<Void>>> maxRelevanceFuture = transform(futureOfRelevance,
+            (Function<List<Void>, ListenableFuture<List<Void>>>) futurePeersClose -> {
+                this.peers.values().forEach(PeerBean::close);
+                BgpDeployerImpl.this.peers.clear();
+
+                final List<ListenableFuture<Void>> futureRIBCloseList = BgpDeployerImpl.this.ribs.values().stream()
+                    .map(RibImpl::closeServiceInstance).collect(Collectors.toList());
+                return Futures.allAsList(futureRIBCloseList);
+            });
+
+        final ListenableFuture<Void> ribFutureClose = transform(maxRelevanceFuture,
+            (Function<ListenableFuture<List<Void>>, Void>) futurePeersClose -> {
+                BgpDeployerImpl.this.ribs.values().forEach(RibImpl::close);
+                this.ribs.clear();
+                return null;
+            });
+
+        ribFutureClose.get();
     }
 
     private static CheckedFuture<Void, TransactionCommitFailedException> initializeNetworkInstance(
@@ -173,7 +194,13 @@ public final class BgpDeployerImpl implements BgpDeployer, ClusteredDataTreeChan
     private synchronized List<PeerBean> closeAllBindedPeers(final InstanceIdentifier<Bgp> rootIdentifier) {
         final List<PeerBean> filtered = new ArrayList<>();
         this.peers.entrySet().stream().filter(entry -> entry.getKey().firstIdentifierOf(Bgp.class)
-            .contains(rootIdentifier)).forEach(entry -> {final PeerBean peer = entry.getValue();
+            .contains(rootIdentifier)).forEach(entry -> {
+            final PeerBean peer = entry.getValue();
+            try {
+                peer.closeServiceInstance().get();
+            } catch (final Exception e) {
+                LOG.error("Peer instance failed to close service instance", e);
+            }
             peer.close();
             filtered.add(peer);
         });
@@ -193,6 +220,11 @@ public final class BgpDeployerImpl implements BgpDeployer, ClusteredDataTreeChan
         final RibImpl ribImpl, final WriteConfiguration configurationWriter) {
         LOG.debug("Modifying RIB instance with configuration: {}", global);
         final List<PeerBean> closedPeers = closeAllBindedPeers(rootIdentifier);
+        try {
+            ribImpl.closeServiceInstance().get();
+        } catch (final Exception e) {
+            LOG.error("RIB instance failed to close service instance", e);
+        }
         ribImpl.close();
         initiateRibInstance(rootIdentifier, global, ribImpl, configurationWriter);
         closedPeers.forEach(peer -> peer.restart(ribImpl, this.tableTypeRegistry));
@@ -205,6 +237,8 @@ public final class BgpDeployerImpl implements BgpDeployer, ClusteredDataTreeChan
         final RibImpl ribImpl = this.ribs.remove(rootIdentifier);
         if (ribImpl != null) {
             LOG.debug("RIB instance removed {}", ribImpl);
+            closeAllBindedPeers(rootIdentifier);
+            ribImpl.closeServiceInstance();
             ribImpl.close();
         }
     }

diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/config/BgpPeer.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/config/BgpPeer.java
index 135381098d6fcb2c06c5b7e2f2bbc6481ec296ff..d3de0299d078284866938d3027c00d474c81b8a4 100644 (file)
--- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/config/BgpPeer.java
+++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/config/BgpPeer.java
@@ -15,6 +15,7 @@ import static org.opendaylight.protocol.bgp.rib.impl.config.OpenConfigMappingUti
 import com.google.common.base.Optional;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.Iterables;
+import com.google.common.util.concurrent.Futures;
 import com.google.common.util.concurrent.ListenableFuture;
 import io.netty.util.concurrent.Future;
 import java.net.InetSocketAddress;
@@ -23,6 +24,7 @@ import java.util.Collections;
 import java.util.List;
 import java.util.Objects;
 import java.util.Set;
+import javax.annotation.concurrent.GuardedBy;
 import org.opendaylight.controller.config.yang.bgp.rib.impl.BGPPeerRuntimeMXBean;
 import org.opendaylight.controller.config.yang.bgp.rib.impl.BgpPeerState;
 import org.opendaylight.controller.config.yang.bgp.rib.impl.BgpSessionState;
@@ -100,6 +102,14 @@ public final class BgpPeer implements PeerBean, BGPPeerStateConsumer, BGPPeerRun
         }
     }
 
+    @Override
+    public ListenableFuture<Void> closeServiceInstance() {
+        if (this.bgpPeerSingletonService != null) {
+            return this.bgpPeerSingletonService.closeServiceInstance();
+        }
+        return Futures.immediateFuture(null);
+    }
+
     private void closeSingletonService() {
         try {
             this.bgpPeerSingletonService.close();
@@ -217,6 +227,8 @@ public final class BgpPeer implements PeerBean, BGPPeerStateConsumer, BGPPeerRun
         private final IpAddress neighborAddress;
         private final BGPSessionPreferences prefs;
         private Future<Void> connection;
+        @GuardedBy("this")
+        private boolean isServiceInstantiated;
 
         private BgpPeerSingletonService(final RIB rib, final Neighbor neighbor,
             final BGPTableTypeRegistryConsumer tableTypeRegistry, final WriteConfiguration configurationWriter) {
@@ -252,7 +264,8 @@ public final class BgpPeer implements PeerBean, BGPPeerStateConsumer, BGPPeerRun
         }
 
         @Override
-        public void instantiateServiceInstance() {
+        public synchronized void instantiateServiceInstance() {
+            this.isServiceInstantiated = true;
             if(this.configurationWriter != null) {
                 this.configurationWriter.apply();
             }
@@ -265,8 +278,14 @@ public final class BgpPeer implements PeerBean, BGPPeerStateConsumer, BGPPeerRun
         }
 
         @Override
-        public ListenableFuture<Void> closeServiceInstance() {
+        public synchronized ListenableFuture<Void> closeServiceInstance() {
+            if(!this.isServiceInstantiated) {
+                LOG.info("Peer Singleton Service {} already closed, Peer {}", getIdentifier().getValue(),
+                    this.neighborAddress);
+                return Futures.immediateFuture(null);
+            }
             LOG.info("Close Peer Singleton Service {}, Peer {}", getIdentifier().getValue(), this.neighborAddress);
+            this.isServiceInstantiated = false;
             if (this.connection != null) {
                 this.connection.cancel(true);
                 this.connection = null;

diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/config/PeerBean.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/config/PeerBean.java
index b8f0bf5c7b68195c5402711975704068d942677c..d53c59ea3b76ab7c0c79408887d8800852650c67 100644 (file)
--- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/config/PeerBean.java
+++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/config/PeerBean.java
@@ -8,6 +8,7 @@
 
 package org.opendaylight.protocol.bgp.rib.impl.config;
 
+import com.google.common.util.concurrent.ListenableFuture;
 import org.opendaylight.protocol.bgp.openconfig.spi.BGPTableTypeRegistryConsumer;
 import org.opendaylight.protocol.bgp.rib.impl.spi.BgpDeployer.WriteConfiguration;
 import org.opendaylight.protocol.bgp.rib.impl.spi.RIB;
@@ -26,5 +27,7 @@ public interface PeerBean extends AutoCloseable {
     @Override
     void close();
 
+    ListenableFuture<Void> closeServiceInstance();
+
     Boolean containsEqualConfiguration(Neighbor neighbor);
 }

diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/config/RibImpl.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/config/RibImpl.java
index 38cee4c16267a6c6edbab7444c2e7d724d97412e..64076a2cd8eca681f55f16ecb0aa3e30a96d87de 100644 (file)
--- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/config/RibImpl.java
+++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/config/RibImpl.java
@@ -13,6 +13,8 @@ import static org.opendaylight.protocol.bgp.rib.impl.config.OpenConfigMappingUti
 import static org.opendaylight.protocol.bgp.rib.impl.config.OpenConfigMappingUtil.toTableTypes;
 
 import com.google.common.base.Preconditions;
+import com.google.common.util.concurrent.Futures;
+import com.google.common.util.concurrent.ListenableFuture;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -165,6 +167,13 @@ public final class RibImpl implements RIB, BGPRIBStateConsumer, AutoCloseable {
         return this.ribImpl.getService();
     }
 
+    public ListenableFuture<Void> closeServiceInstance() {
+        if (this.ribImpl != null) {
+            return this.ribImpl.closeServiceInstance();
+        }
+        return Futures.immediateFuture(null);
+    }
+
     @Override
     public void close() {
         if (this.ribImpl != null) {

diff --git a/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/config/BgpDeployerImplTest.java b/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/config/BgpDeployerImplTest.java
index e5230fd52f24d4cbd085736932f867416063055a..e983dec39b5961d16f1a908fb7681e706eb49f3a 100644 (file)
--- a/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/config/BgpDeployerImplTest.java
+++ b/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/config/BgpDeployerImplTest.java
@@ -277,7 +277,7 @@ public class BgpDeployerImplTest {
         verifyPrivate(spyDeployer).invoke("onGlobalCreated", any(InstanceIdentifier.class), any(Global.class),
             any(BgpDeployer.WriteConfiguration.class));
         verifyPrivate(spyDeployer).invoke("onGlobalUpdated", any(InstanceIdentifier.class), any(Global.class), any(RibImpl.class), any(BgpDeployer.WriteConfiguration.class));
-        verifyPrivate(spyDeployer).invoke("closeAllBindedPeers", any(InstanceIdentifier.class));
+        verifyPrivate(spyDeployer, times(2)).invoke("closeAllBindedPeers", any(InstanceIdentifier.class));
         verifyPrivate(spyDeployer, times(2)).invoke("initiateRibInstance", any(InstanceIdentifier.class), any(Global.class), any(RibImpl.class), any
             (BgpDeployer.WriteConfiguration.class));
         verifyPrivate(spyDeployer, times(2)).invoke("registerRibInstance", any(RibImpl.class), anyString());