Bug 8832 - rpc-error in keepalive rpc-reply shouldn't bounce the session

Change-Id: I2ef4153b5910e56c3401c08d57465741f1002691
Signed-off-by: Alexis de Talhouët <adetalhouet89@gmail.com>

diff --git a/netconf/sal-netconf-connector/src/main/java/org/opendaylight/netconf/sal/connect/netconf/sal/KeepaliveSalFacade.java b/netconf/sal-netconf-connector/src/main/java/org/opendaylight/netconf/sal/connect/netconf/sal/KeepaliveSalFacade.java
index 428a9b7fa1931ff3236f5510b7727d5671b6e1a3..b77e7269e553ae6dc11837c4ef45ab309e3dccc9 100644 (file)
--- a/netconf/sal-netconf-connector/src/main/java/org/opendaylight/netconf/sal/connect/netconf/sal/KeepaliveSalFacade.java
+++ b/netconf/sal-netconf-connector/src/main/java/org/opendaylight/netconf/sal/connect/netconf/sal/KeepaliveSalFacade.java
@@ -204,9 +204,14 @@ public final class KeepaliveSalFacade implements RemoteDeviceHandler<NetconfSess
 
         @Override
         public void onSuccess(final DOMRpcResult result) {
+            // No matter what response we got, rpc-reply or rpc-error,
+            // we got it from device so the netconf session is OK
             if (result != null && result.getResult() != null) {
                 LOG.debug("{}: Keepalive RPC successful with response: {}", id, result.getResult());
                 scheduleKeepalive();
+            } else if (result != null && result.getErrors() != null) {
+                LOG.warn("{}: Keepalive RPC failed with error: {}", id, result.getErrors());
+                scheduleKeepalive();
             } else {
                 LOG.warn("{} Keepalive RPC returned null with response: {}. Reconnecting netconf session", id, result);
                 reconnect();

diff --git a/netconf/sal-netconf-connector/src/test/java/org/opendaylight/netconf/sal/connect/netconf/sal/KeepaliveSalFacadeTest.java b/netconf/sal-netconf-connector/src/test/java/org/opendaylight/netconf/sal/connect/netconf/sal/KeepaliveSalFacadeTest.java
index f0b791a3b3ef030c4bb8549305db912e550a9deb..3f0b9d8628bacd0414119ce24a85e40a9808fd07 100644 (file)
--- a/netconf/sal-netconf-connector/src/test/java/org/opendaylight/netconf/sal/connect/netconf/sal/KeepaliveSalFacadeTest.java
+++ b/netconf/sal-netconf-connector/src/test/java/org/opendaylight/netconf/sal/connect/netconf/sal/KeepaliveSalFacadeTest.java
@@ -63,6 +63,8 @@ public class KeepaliveSalFacadeTest {
     @Mock
     private ScheduledFuture currentKeepalive;
 
+    private KeepaliveSalFacade keepaliveSalFacade;
+
     @Before
     public void setUp() throws Exception {
         executorServiceSpy = Executors.newScheduledThreadPool(1);
@@ -76,6 +78,7 @@ public class KeepaliveSalFacadeTest {
 
         ScheduledExecutorService executorService = Executors.newScheduledThreadPool(1);
         executorServiceSpy = Mockito.spy(executorService);
+
         doAnswer(
             invocationOnMock -> {
                 invocationOnMock.callRealMethod();
@@ -83,6 +86,10 @@ public class KeepaliveSalFacadeTest {
             }).when(executorServiceSpy).schedule(Mockito.<Runnable>any(), Mockito.anyLong(), Matchers.any());
 
         Mockito.when(currentKeepalive.isDone()).thenReturn(true);
+
+        keepaliveSalFacade =
+                new KeepaliveSalFacade(REMOTE_DEVICE_ID, underlyingSalFacade, executorServiceSpy, 1L, 1L);
+        keepaliveSalFacade.setListener(listener);
     }
 
     @After
@@ -111,18 +118,9 @@ public class KeepaliveSalFacadeTest {
     }
 
     @Test
-    public void testKeepaliveFail() throws Exception {
-        final DOMRpcResult result = new DefaultDOMRpcResult(Builders.containerBuilder().withNodeIdentifier(
-                new YangInstanceIdentifier.NodeIdentifier(NetconfMessageTransformUtil.NETCONF_RUNNING_QNAME)).build());
-
-        RpcError error = mock(RpcError.class);
-        doReturn("Failure").when(error).toString();
-
-        final DOMRpcResult resultFailWithResultAndError = new DefaultDOMRpcResult(mock(NormalizedNode.class), error);
+    public void testKeepaliveRpcFailure() {
 
-        doReturn(Futures.immediateCheckedFuture(result))
-                .doReturn(Futures.immediateCheckedFuture(resultFailWithResultAndError))
-                .doReturn(Futures.immediateFailedCheckedFuture(new IllegalStateException("illegal-state")))
+        doReturn(Futures.immediateFailedCheckedFuture(new IllegalStateException("illegal-state")))
                 .when(deviceRpc).invokeRpc(any(SchemaPath.class), any(NormalizedNode.class));
 
         final KeepaliveSalFacade keepaliveSalFacade =
@@ -134,39 +132,27 @@ public class KeepaliveSalFacadeTest {
         verify(underlyingSalFacade).onDeviceConnected(
                 any(SchemaContext.class), any(NetconfSessionPreferences.class), any(DOMRpcService.class));
 
-        // 1 failed that results in disconnect
+        // Should disconnect the session
         verify(listener, timeout(15000).times(1)).disconnect();
-        // 3 attempts total
-        verify(deviceRpc, times(3)).invokeRpc(any(SchemaPath.class), any(NormalizedNode.class));
-
-        // Reconnect with same keepalive responses
-        doReturn(Futures.immediateCheckedFuture(result))
-                .doReturn(Futures.immediateCheckedFuture(resultFailWithResultAndError))
-                .doReturn(Futures.immediateFailedCheckedFuture(new IllegalStateException("illegal-state")))
-                .when(deviceRpc).invokeRpc(any(SchemaPath.class), any(NormalizedNode.class));
-
-        keepaliveSalFacade.onDeviceConnected(null, null, deviceRpc);
+        verify(deviceRpc, times(1)).invokeRpc(any(SchemaPath.class), any(NormalizedNode.class));
+    }
 
-        // 1 failed that results in disconnect, 2 total with previous fail
-        verify(listener, timeout(15000).times(2)).disconnect();
-        // 6 attempts now total
-        verify(deviceRpc, times(3 * 2)).invokeRpc(any(SchemaPath.class), any(NormalizedNode.class));
+    @Test
+    public void testKeepaliveSuccessWithRpcError() {
 
-        final DOMRpcResult resultFailwithError = new DefaultDOMRpcResult(error);
+        final DOMRpcResult rpcSuccessWithError = new DefaultDOMRpcResult(mock(RpcError.class));
 
-        doReturn(Futures.immediateCheckedFuture(resultFailwithError))
+        doReturn(Futures.immediateCheckedFuture(rpcSuccessWithError))
                 .when(deviceRpc).invokeRpc(any(SchemaPath.class), any(NormalizedNode.class));
 
         keepaliveSalFacade.onDeviceConnected(null, null, deviceRpc);
 
-        // 1 failed that results in disconnect, 3 total with previous fail
-        verify(listener, timeout(15000).times(3)).disconnect();
-
+        verify(underlyingSalFacade).onDeviceConnected(
+                any(SchemaContext.class), any(NetconfSessionPreferences.class), any(DOMRpcService.class));
 
-        Mockito.when(currentKeepalive.isDone()).thenReturn(false);
-        keepaliveSalFacade.onDeviceConnected(null, null, deviceRpc);
-        // 1 failed that results in disconnect, 4 total with previous fail
-        verify(listener, timeout(15000).times(4)).disconnect();
+        // Shouldn't disconnect the session
+        verify(listener, times(0)).disconnect();
+        verify(deviceRpc, timeout(15000).times(1)).invokeRpc(any(SchemaPath.class), any(NormalizedNode.class));
     }
 
     @Test