Bug 8421 - Revert "Resolve exception from registerClusterSingletonService"

This change causes, that whole topology is closed, when
cluster singleton service registration fails, which is incorrect.

This reverts commit 25e450c382211e0b80d873f7697c7b9807be54af.

Change-Id: I37053d4fda7830ef53dc61fcbda3d149f11bbdbe
Signed-off-by: Andrej Mak <andrej.mak@pantheon.tech>

diff --git a/netconf/netconf-topology-singleton/src/main/java/org/opendaylight/netconf/topology/singleton/impl/NetconfTopologyManager.java b/netconf/netconf-topology-singleton/src/main/java/org/opendaylight/netconf/topology/singleton/impl/NetconfTopologyManager.java
index 1f166247b517661cb5346802207ea3e5b05bf45c..ebaafcf8866a3082a138981b6d33d85fe9799c71 100644 (file)
--- a/netconf/netconf-topology-singleton/src/main/java/org/opendaylight/netconf/topology/singleton/impl/NetconfTopologyManager.java
+++ b/netconf/netconf-topology-singleton/src/main/java/org/opendaylight/netconf/topology/singleton/impl/NetconfTopologyManager.java
@@ -137,11 +137,6 @@ public class NetconfTopologyManager
         context.refresh(createSetup(instanceIdentifier, node));
     }
 
-    // ClusterSingletonServiceRegistration registerClusterSingletonService method throws a Runtime exception if there
-    // are problems with registration and client has to deal with it. Only thing we can do if this error occurs is to
-    // retry registration several times and log the error.
-    // TODO change to a specific documented Exception when changed in ClusterSingletonServiceProvider
-    @SuppressWarnings("checkstyle:IllegalCatch")
     private void startNetconfDeviceContext(final InstanceIdentifier<Node> instanceIdentifier, final Node node) {
         final NetconfNode netconfNode = node.getAugmentation(NetconfNode.class);
         Preconditions.checkNotNull(netconfNode);
@@ -158,26 +153,11 @@ public class NetconfTopologyManager
                 new NetconfTopologyContext(createSetup(instanceIdentifier, node), serviceGroupIdent,
                         actorResponseWaitTime, mountPointService);
 
-        int tries = 3;
-        while (true) {
-            try {
-                final ClusterSingletonServiceRegistration clusterSingletonServiceRegistration =
-                        clusterSingletonServiceProvider.registerClusterSingletonService(newNetconfTopologyContext);
-                clusterRegistrations.put(instanceIdentifier, clusterSingletonServiceRegistration);
-                contexts.put(instanceIdentifier, newNetconfTopologyContext);
-                break;
-            } catch (final RuntimeException e) {
-                LOG.warn("Unable to register cluster singleton service {}, trying again", newNetconfTopologyContext, e);
-
-                if (--tries <= 0) {
-                    LOG.error("Unable to register cluster singleton service {} - done trying, closing topology context",
-                            newNetconfTopologyContext, e);
-                    close();
-                    break;
-                }
-            }
-        }
+        final ClusterSingletonServiceRegistration clusterSingletonServiceRegistration =
+                clusterSingletonServiceProvider.registerClusterSingletonService(newNetconfTopologyContext);
 
+        clusterRegistrations.put(instanceIdentifier, clusterSingletonServiceRegistration);
+        contexts.put(instanceIdentifier, newNetconfTopologyContext);
     }
 
     private void stopNetconfDeviceContext(final InstanceIdentifier<Node> instanceIdentifier) {

diff --git a/netconf/netconf-topology-singleton/src/test/java/org/opendaylight/netconf/topology/singleton/impl/NetconfTopologyManagerTest.java b/netconf/netconf-topology-singleton/src/test/java/org/opendaylight/netconf/topology/singleton/impl/NetconfTopologyManagerTest.java
index 817a58a5eca7eabd5800507c92c0aa00d33e5b83..ffc52a5f4663184c2d9f01e575f378a9af9b4ba2 100644 (file)
--- a/netconf/netconf-topology-singleton/src/test/java/org/opendaylight/netconf/topology/singleton/impl/NetconfTopologyManagerTest.java
+++ b/netconf/netconf-topology-singleton/src/test/java/org/opendaylight/netconf/topology/singleton/impl/NetconfTopologyManagerTest.java
@@ -14,7 +14,6 @@ import static org.junit.Assert.assertTrue;
 import static org.mockito.Matchers.any;
 import static org.mockito.Mockito.doNothing;
 import static org.mockito.Mockito.doReturn;
-import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
@@ -93,83 +92,16 @@ public class NetconfTopologyManagerTest {
 
     @Test
     public void testWriteConfiguration() throws Exception {
-        writeConfiguration(false);
-    }
-
-    @Test
-    public void testWriteConfigurationFail() throws Exception {
-        writeConfiguration(true);
-    }
-
-    @Test
-    public void testRegisterDataTreeChangeListener() {
-
-        final WriteTransaction wtx = mock(WriteTransaction.class);
-
-        doReturn(wtx).when(dataBroker).newWriteOnlyTransaction();
-        doNothing().when(wtx).merge(any(), any(), any());
-        doReturn(Futures.immediateCheckedFuture(null)).when(wtx).submit();
-        doReturn(null).when(dataBroker).registerDataChangeListener(any(), any(), any(), any());
-
-        netconfTopologyManager.init();
-
-        // verify if listener is called with right parameters = registered on right path
-
-        verify(dataBroker, times(1)).registerDataTreeChangeListener(
-                new DataTreeIdentifier<>(LogicalDatastoreType.CONFIGURATION, NetconfTopologyUtils
-                        .createTopologyListPath(topologyId).child(Node.class)), netconfTopologyManager);
-
-    }
-
-    @Test
-    public void testClose() throws Exception {
-
-        final Field fieldContexts = NetconfTopologyManager.class.getDeclaredField("contexts");
-        fieldContexts.setAccessible(true);
-        @SuppressWarnings("unchecked") final Map<InstanceIdentifier<Node>, NetconfTopologyContext> contexts =
-                (Map<InstanceIdentifier<Node>, NetconfTopologyContext>) fieldContexts.get(netconfTopologyManager);
-
-        final Field fieldClusterRegistrations =
-                NetconfTopologyManager.class.getDeclaredField("clusterRegistrations");
-        fieldClusterRegistrations.setAccessible(true);
-        @SuppressWarnings("unchecked")
-        final Map<InstanceIdentifier<Node>, ClusterSingletonServiceRegistration> clusterRegistrations =
-                (Map<InstanceIdentifier<Node>, ClusterSingletonServiceRegistration>)
-                        fieldClusterRegistrations.get(netconfTopologyManager);
-
-        final InstanceIdentifier<Node> instanceIdentifier = NetconfTopologyUtils.createTopologyNodeListPath(
-                new NodeKey(new NodeId("node-id-1")), "topology-1");
-
-
-        final NetconfTopologyContext context = mock(NetconfTopologyContext.class);
-        final ClusterSingletonServiceRegistration clusterRegistration =
-                mock(ClusterSingletonServiceRegistration.class);
-        contexts.put(instanceIdentifier, context);
-        clusterRegistrations.put(instanceIdentifier, clusterRegistration);
-
-        doNothing().when(context).closeFinal();
-        doNothing().when(clusterRegistration).close();
-
-        netconfTopologyManager.close();
-        verify(context, times(1)).closeFinal();
-        verify(clusterRegistration, times(1)).close();
-
-        assertTrue(contexts.isEmpty());
-        assertTrue(clusterRegistrations.isEmpty());
-
-    }
-
-    private void writeConfiguration(final boolean fail) throws Exception {
 
         final ClusterSingletonServiceRegistration clusterRegistration = mock(ClusterSingletonServiceRegistration.class);
 
         final Field fieldContexts = NetconfTopologyManager.class.getDeclaredField("contexts");
         fieldContexts.setAccessible(true);
-        @SuppressWarnings("unchecked") final Map<InstanceIdentifier<Node>, NetconfTopologyContext> contexts =
+        @SuppressWarnings("unchecked")
+        final Map<InstanceIdentifier<Node>, NetconfTopologyContext> contexts =
                 (Map<InstanceIdentifier<Node>, NetconfTopologyContext>) fieldContexts.get(netconfTopologyManager);
 
-        final Field fieldClusterRegistrations =
-                NetconfTopologyManager.class.getDeclaredField("clusterRegistrations");
+        final Field fieldClusterRegistrations = NetconfTopologyManager.class.getDeclaredField("clusterRegistrations");
         fieldClusterRegistrations.setAccessible(true);
         @SuppressWarnings("unchecked")
         final Map<InstanceIdentifier<Node>, ClusterSingletonServiceRegistration> clusterRegistrations =
@@ -179,10 +111,10 @@ public class NetconfTopologyManagerTest {
         final Collection<DataTreeModification<Node>> changes = new ArrayList<>();
 
         final InstanceIdentifier<Node> instanceIdentifier = NetconfTopologyUtils.createTopologyNodeListPath(
-                new NodeKey(new NodeId("node-id-1")), "topology-1");
+                new NodeKey(new NodeId("node-id-1")),"topology-1");
 
         final InstanceIdentifier<Node> instanceIdentifierDiferent = NetconfTopologyUtils.createTopologyNodeListPath(
-                new NodeKey(new NodeId("node-id-2")), "topology-2");
+                new NodeKey(new NodeId("node-id-2")),"topology-2");
 
         final DataTreeIdentifier<Node> rootIdentifier =
                 new DataTreeIdentifier<>(LogicalDatastoreType.CONFIGURATION, instanceIdentifier);
@@ -190,8 +122,8 @@ public class NetconfTopologyManagerTest {
         final DataTreeIdentifier<Node> rootIdentifierDifferent =
                 new DataTreeIdentifier<>(LogicalDatastoreType.CONFIGURATION, instanceIdentifierDiferent);
 
-        @SuppressWarnings("unchecked") final DataObjectModification<Node> objectModification =
-                mock(DataObjectModification.class);
+        @SuppressWarnings("unchecked")
+        final DataObjectModification<Node> objectModification = mock(DataObjectModification.class);
 
         final NetconfNode netconfNode = new NetconfNodeBuilder()
                 .setHost(new Host(new IpAddress(new Ipv4Address("127.0.0.1"))))
@@ -208,68 +140,113 @@ public class NetconfTopologyManagerTest {
 
         final Identifier key = new NodeKey(new NodeId("node-id"));
 
-        @SuppressWarnings("unchecked") final InstanceIdentifier.IdentifiableItem<Node, NodeKey> pathArgument =
+        @SuppressWarnings("unchecked")
+        final InstanceIdentifier.IdentifiableItem<Node, NodeKey> pathArgument =
                 new InstanceIdentifier.IdentifiableItem(Node.class, key);
 
 
         // testing WRITE on two identical rootIdentifiers and one different
-        if (fail) {
-            changes.add(new CustomTreeModification(rootIdentifier, objectModification));
-        } else {
-            changes.add(new CustomTreeModification(rootIdentifier, objectModification));
-            changes.add(new CustomTreeModification(rootIdentifier, objectModification));
-            changes.add(new CustomTreeModification(rootIdentifierDifferent, objectModification));
-        }
+
+        changes.add(new CustomTreeModification(rootIdentifier, objectModification));
+        changes.add(new CustomTreeModification(rootIdentifier, objectModification));
+        changes.add(new CustomTreeModification(rootIdentifierDifferent, objectModification));
+
         doReturn(WRITE).when(objectModification).getModificationType();
         doReturn(node).when(objectModification).getDataAfter();
         doReturn(pathArgument).when(objectModification).getIdentifier();
+        doReturn(clusterRegistration).when(clusterSingletonServiceProvider).registerClusterSingletonService(any());
 
-        if (fail) {
-            doThrow(new RuntimeException("error")).when(clusterSingletonServiceProvider)
-                    .registerClusterSingletonService(any());
-        } else {
-            doReturn(clusterRegistration).when(clusterSingletonServiceProvider).registerClusterSingletonService(any());
-        }
         netconfTopologyManager.onDataTreeChanged(changes);
 
-        if (fail) {
-            verify(clusterSingletonServiceProvider, times(3))
-                    .registerClusterSingletonService(any());
-            assertTrue(contexts.isEmpty());
-            assertTrue(clusterRegistrations.isEmpty());
-        } else {
-            verify(clusterSingletonServiceProvider, times(2))
-                    .registerClusterSingletonService(any());
+        verify(clusterSingletonServiceProvider, times(2)).registerClusterSingletonService(any());
 
-            // only two created contexts
-            assertEquals(2, contexts.size());
-            assertTrue(contexts.containsKey(rootIdentifier.getRootIdentifier()));
-            assertTrue(contexts.containsKey(rootIdentifierDifferent.getRootIdentifier()));
+        // only two created contexts
+        assertEquals(2, contexts.size());
+        assertTrue(contexts.containsKey(rootIdentifier.getRootIdentifier()));
+        assertTrue(contexts.containsKey(rootIdentifierDifferent.getRootIdentifier()));
 
-            // only two created cluster registrations
-            assertEquals(2, clusterRegistrations.size());
-            assertTrue(clusterRegistrations.containsKey(rootIdentifier.getRootIdentifier()));
-            assertTrue(clusterRegistrations.containsKey(rootIdentifierDifferent.getRootIdentifier()));
+        // only two created cluster registrations
+        assertEquals(2, contexts.size());
+        assertTrue(clusterRegistrations.containsKey(rootIdentifier.getRootIdentifier()));
+        assertTrue(clusterRegistrations.containsKey(rootIdentifierDifferent.getRootIdentifier()));
 
-            // after delete there should be no context and clustered registrations
-            doReturn(DELETE).when(objectModification).getModificationType();
+        // after delete there should be no context and clustered registrations
+        doReturn(DELETE).when(objectModification).getModificationType();
+
+        doNothing().when(clusterRegistration).close();
 
-            doNothing().when(clusterRegistration).close();
+        netconfTopologyManager.onDataTreeChanged(changes);
 
-            netconfTopologyManager.onDataTreeChanged(changes);
+        verify(clusterRegistration, times(2)).close();
 
-            verify(clusterRegistration, times(2)).close();
+        // empty map of contexts
+        assertTrue(contexts.isEmpty());
+        assertFalse(contexts.containsKey(rootIdentifier.getRootIdentifier()));
+        assertFalse(contexts.containsKey(rootIdentifierDifferent.getRootIdentifier()));
 
-            // empty map of contexts
-            assertTrue(contexts.isEmpty());
-            assertFalse(contexts.containsKey(rootIdentifier.getRootIdentifier()));
-            assertFalse(contexts.containsKey(rootIdentifierDifferent.getRootIdentifier()));
+        // empty map of clustered registrations
+        assertTrue(clusterRegistrations.isEmpty());
+        assertFalse(clusterRegistrations.containsKey(rootIdentifier.getRootIdentifier()));
+        assertFalse(clusterRegistrations.containsKey(rootIdentifierDifferent.getRootIdentifier()));
+
+    }
+
+    @Test
+    public void testRegisterDataTreeChangeListener() {
+
+        final WriteTransaction wtx = mock(WriteTransaction.class);
+
+        doReturn(wtx).when(dataBroker).newWriteOnlyTransaction();
+        doNothing().when(wtx).merge(any(), any(), any());
+        doReturn(Futures.immediateCheckedFuture(null)).when(wtx).submit();
+        doReturn(null).when(dataBroker).registerDataChangeListener(any(), any(), any(), any());
+
+        netconfTopologyManager.init();
+
+        // verify if listener is called with right parameters = registered on right path
+
+        verify(dataBroker, times(1)).registerDataTreeChangeListener(
+                new DataTreeIdentifier<>(LogicalDatastoreType.CONFIGURATION, NetconfTopologyUtils
+                        .createTopologyListPath(topologyId).child(Node.class)), netconfTopologyManager);
+
+    }
+
+    @Test
+    public void testClose() throws Exception {
+
+        final Field fieldContexts = NetconfTopologyManager.class.getDeclaredField("contexts");
+        fieldContexts.setAccessible(true);
+        @SuppressWarnings("unchecked")
+        final Map<InstanceIdentifier<Node>, NetconfTopologyContext> contexts =
+                (Map<InstanceIdentifier<Node>, NetconfTopologyContext>) fieldContexts.get(netconfTopologyManager);
+
+        final Field fieldClusterRegistrations = NetconfTopologyManager.class.getDeclaredField("clusterRegistrations");
+        fieldClusterRegistrations.setAccessible(true);
+        @SuppressWarnings("unchecked")
+        final Map<InstanceIdentifier<Node>, ClusterSingletonServiceRegistration> clusterRegistrations =
+                (Map<InstanceIdentifier<Node>, ClusterSingletonServiceRegistration>)
+                        fieldClusterRegistrations.get(netconfTopologyManager);
+
+        final InstanceIdentifier<Node> instanceIdentifier = NetconfTopologyUtils.createTopologyNodeListPath(
+                new NodeKey(new NodeId("node-id-1")),"topology-1");
+
+
+        final NetconfTopologyContext context = mock(NetconfTopologyContext.class);
+        final ClusterSingletonServiceRegistration clusterRegistration =
+                mock(ClusterSingletonServiceRegistration.class);
+        contexts.put(instanceIdentifier, context);
+        clusterRegistrations.put(instanceIdentifier, clusterRegistration);
+
+        doNothing().when(context).closeFinal();
+        doNothing().when(clusterRegistration).close();
+
+        netconfTopologyManager.close();
+        verify(context, times(1)).closeFinal();
+        verify(clusterRegistration, times(1)).close();
+
+        assertTrue(contexts.isEmpty());
+        assertTrue(clusterRegistrations.isEmpty());
 
-            // empty map of clustered registrations
-            assertTrue(clusterRegistrations.isEmpty());
-            assertFalse(clusterRegistrations.containsKey(rootIdentifier.getRootIdentifier()));
-            assertFalse(clusterRegistrations.containsKey(rootIdentifierDifferent.getRootIdentifier()));
-        }
     }
 
     private class CustomTreeModification  implements DataTreeModification<Node> {