Fix connection closing before initialization

Check if connection is fully initialized when closing
objects created during initialization phase to prevent
NPEs, what can cause incorrect close of connection
and later program unstability.

Also, increase timeout to propagate role to 10 seconds
in case of slower machine.

Resolves: bug 5271
See also: bug 6672

Change-Id: I0f009edf5fe3b382e2bfee64f72036ba599ccc5d
Signed-off-by: Tomas Slusny <tomas.slusny@pantheon.tech>
Signed-off-by: Shuva Kar <shuva.jyoti.kar@ericsson.com>

diff --git a/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/device/DeviceContextImpl.java b/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/device/DeviceContextImpl.java
index 6eb240a174d460d248e161ba3c5e5549bc607df6..c7d60e6f9a06260992149bd406792475f50e01f2 100644 (file)
--- a/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/device/DeviceContextImpl.java
+++ b/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/device/DeviceContextImpl.java
@@ -18,7 +18,6 @@ import java.math.BigInteger;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
-import java.util.Objects;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicInteger;
@@ -102,9 +101,6 @@ import org.opendaylight.yangtools.yang.binding.KeyedInstanceIdentifier;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-/**
- *
- */
 public class DeviceContextImpl implements DeviceContext, ExtensionConverterProviderKeeper{
 
     private static final Logger LOG = LoggerFactory.getLogger(DeviceContextImpl.class);
@@ -184,7 +180,9 @@ public class DeviceContextImpl implements DeviceContext, ExtensionConverterProvi
 
     @Override
     public void initialSubmitTransaction() {
-        transactionChainManager.initialSubmitWriteTransaction();
+        if (initialized) {
+            transactionChainManager.initialSubmitWriteTransaction();
+        }
     }
 
     @Override
@@ -219,7 +217,7 @@ public class DeviceContextImpl implements DeviceContext, ExtensionConverterProvi
     public <T extends DataObject> void writeToTransaction(final LogicalDatastoreType store,
                                                           final InstanceIdentifier<T> path,
                                                           final T data){
-        if (Objects.nonNull(transactionChainManager)) {
+        if (initialized) {
             transactionChainManager.writeToTransaction(store, path, data, false);
         }
     }
@@ -228,21 +226,21 @@ public class DeviceContextImpl implements DeviceContext, ExtensionConverterProvi
     public <T extends DataObject> void writeToTransactionWithParentsSlow(final LogicalDatastoreType store,
                                                                          final InstanceIdentifier<T> path,
                                                                          final T data){
-        if (Objects.nonNull(transactionChainManager)) {
+        if (initialized) {
             transactionChainManager.writeToTransaction(store, path, data, true);
         }
     }
 
     @Override
-    public <T extends DataObject> void addDeleteToTxChain(final LogicalDatastoreType store, final InstanceIdentifier<T> path) {
-        if (Objects.nonNull(transactionChainManager)) {
+    public <T extends DataObject> void addDeleteToTxChain(final LogicalDatastoreType store, final InstanceIdentifier<T> path) throws TransactionChainClosedException {
+        if (initialized) {
             transactionChainManager.addDeleteOperationTotTxChain(store, path);
         }
     }
 
     @Override
     public boolean submitTransaction() {
-        return Objects.nonNull(transactionChainManager) && transactionChainManager.submitWriteTransaction();
+        return initialized && transactionChainManager.submitWriteTransaction();
     }
 
     @Override
@@ -557,26 +555,29 @@ public class DeviceContextImpl implements DeviceContext, ExtensionConverterProvi
             LOG.debug("ConnectionCtx for Node {} is in RIP state.", getDeviceInfo().getLOGValue());
             return;
         }
-        /* Terminate Auxiliary Connection */
+
+        // Terminate Auxiliary Connection
         for (final ConnectionContext connectionContext : auxiliaryConnectionContexts.values()) {
             LOG.debug("Closing auxiliary connection {}", connectionContext.getNodeId());
             connectionContext.closeConnection(false);
         }
-        /* Terminate Primary Connection */
+
+        // Terminate Primary Connection
         getPrimaryConnectionContext().closeConnection(true);
-        /* Close all Group Registry */
-        deviceGroupRegistry.close();
-        deviceFlowRegistry.close();
-        deviceMeterRegistry.close();
+
+        // Close all datastore registries
+        if (initialized) {
+            deviceGroupRegistry.close();
+            deviceFlowRegistry.close();
+            deviceMeterRegistry.close();
+        }
     }
 
     @Override
     public ListenableFuture<Void> shuttingDownDataStoreTransactions() {
-        ListenableFuture<Void> future = Futures.immediateFuture(null);
-        if (Objects.nonNull(this.transactionChainManager)) {
-            future = this.transactionChainManager.shuttingDown();
-        }
-        return future;
+        return initialized
+                ? this.transactionChainManager.shuttingDown()
+                : Futures.immediateFuture(null);
     }
 
     @VisibleForTesting
@@ -601,11 +602,9 @@ public class DeviceContextImpl implements DeviceContext, ExtensionConverterProvi
 
     @Override
     public ListenableFuture<Void> stopClusterServices(boolean deviceDisconnected) {
-        ListenableFuture<Void> future = Futures.immediateFuture(null);
-        if (Objects.nonNull(this.transactionChainManager)) {
-            future = this.transactionChainManager.deactivateTransactionManager();
-        }
-        return future;
+        return initialized
+                ? this.transactionChainManager.deactivateTransactionManager()
+                : Futures.immediateFuture(null);
     }
 
     @Override
@@ -620,7 +619,7 @@ public class DeviceContextImpl implements DeviceContext, ExtensionConverterProvi
 
     @Override
     public void putLifecycleServiceIntoTxChainManager(final LifecycleService lifecycleService){
-        if (Objects.nonNull(this.transactionChainManager)) {
+        if (initialized) {
             this.transactionChainManager.setLifecycleService(lifecycleService);
         }
     }

diff --git a/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/lifecycle/LifecycleServiceImpl.java b/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/lifecycle/LifecycleServiceImpl.java
index abbe25802d1ccb8c7baf2080a9dc63e3f6ad05c9..7b6cbe58eed8d29efae56bc0d415260872e158d1 100644 (file)
--- a/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/lifecycle/LifecycleServiceImpl.java
+++ b/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/lifecycle/LifecycleServiceImpl.java
@@ -79,6 +79,7 @@ public class LifecycleServiceImpl implements LifecycleService {
     @Override
     public void close() throws Exception {
         if (registration != null) {
+            LOG.info("Unregistering clustering MASTER services for node {}", this.deviceContext.getDeviceInfo().getLOGValue());
             registration.close();
             registration = null;
         }
@@ -86,6 +87,8 @@ public class LifecycleServiceImpl implements LifecycleService {
 
     @Override
     public void registerService(final ClusterSingletonServiceProvider singletonServiceProvider) {
+        LOG.info("Registering clustering MASTER services for node {}", this.deviceContext.getDeviceInfo().getLOGValue());
+
         //lifecycle service -> device context -> statistics context -> rpc context -> role context -> lifecycle service
         this.clusterInitializationPhaseHandler = deviceContext;
         this.deviceContext.setLifecycleInitializationPhaseHandler(this.statContext);

diff --git a/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/role/RoleContextImpl.java b/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/role/RoleContextImpl.java
index b636c9fb3021aa379c9651fca999c8ff84be70ca..58d8ab212837a24175546514c31646b083498ce0 100644 (file)
--- a/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/role/RoleContextImpl.java
+++ b/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/role/RoleContextImpl.java
@@ -49,6 +49,9 @@ class RoleContextImpl implements RoleContext {
 
     private static final Logger LOG = LoggerFactory.getLogger(RoleContextImpl.class);
 
+    // Timeout in seconds after what we will give up on propagating role
+    private static final int SET_ROLE_TIMEOUT = 10;
+
     private SalRoleService salRoleService = null;
     private final HashedWheelTimer hashedWheelTimer;
     private final DeviceInfo deviceInfo;
@@ -162,7 +165,7 @@ class RoleContextImpl implements RoleContext {
                     setRoleOutputFuture.cancel(true);
                 }
             };
-            hashedWheelTimer.newTimeout(timerTask, 5, TimeUnit.SECONDS);
+            hashedWheelTimer.newTimeout(timerTask, SET_ROLE_TIMEOUT, TimeUnit.SECONDS);
         } else {
             LOG.info("Device: {} with version: {} does not support role", deviceInfo.getLOGValue(), deviceInfo.getVersion());
             return Futures.immediateFuture(null);

diff --git a/openflowplugin-impl/src/test/java/org/opendaylight/openflowplugin/impl/lifecycle/LifecycleServiceImplTest.java b/openflowplugin-impl/src/test/java/org/opendaylight/openflowplugin/impl/lifecycle/LifecycleServiceImplTest.java
index 21f3f9460b838c67d883b7fefc6e57ca11ea84f9..96f4dcecdf92d386fbe9693b396b8e142d2ceac1 100644 (file)
--- a/openflowplugin-impl/src/test/java/org/opendaylight/openflowplugin/impl/lifecycle/LifecycleServiceImplTest.java
+++ b/openflowplugin-impl/src/test/java/org/opendaylight/openflowplugin/impl/lifecycle/LifecycleServiceImplTest.java
@@ -54,12 +54,6 @@ public class LifecycleServiceImplTest {
 
     @Before
     public void setUp() {
-        lifecycleService = new LifecycleServiceImpl();
-        lifecycleService.setDeviceContext(deviceContext);
-        lifecycleService.setRpcContext(rpcContext);
-        lifecycleService.setRoleContext(roleContext);
-        lifecycleService.setStatContext(statContext);
-        lifecycleService.registerService(clusterSingletonServiceProvider);
         Mockito.when(deviceContext.getDeviceInfo()).thenReturn(deviceInfo);
         Mockito.when(deviceContext.getPrimaryConnectionContext()).thenReturn(connectionContext);
         Mockito.when(deviceContext.getDeviceFlowRegistry()).thenReturn(deviceFlowRegistry);
@@ -67,6 +61,13 @@ public class LifecycleServiceImplTest {
         Mockito.when(deviceFlowRegistry.fill()).thenReturn(Futures.immediateFuture(null));
         Mockito.when(connectionContext.getConnectionState()).thenReturn(ConnectionContext.CONNECTION_STATE.WORKING);
         Mockito.when(deviceInfo.getLOGValue()).thenReturn(TEST_NODE);
+
+        lifecycleService = new LifecycleServiceImpl();
+        lifecycleService.setDeviceContext(deviceContext);
+        lifecycleService.setRpcContext(rpcContext);
+        lifecycleService.setRoleContext(roleContext);
+        lifecycleService.setStatContext(statContext);
+        lifecycleService.registerService(clusterSingletonServiceProvider);
     }
 
     @Test