From e5fe596438a744080f625d12e9c23f4e419da402 Mon Sep 17 00:00:00 2001
From: =?utf8?q?Alexis=20de=20Talhou=C3=ABt?= <adetalhouet89@gmail.com>
Date: Mon, 10 Jul 2017 13:30:10 -0400
Subject: [PATCH] Bug 8832 - rpc-error in keepalive rpc-reply shouldn't bounce
 the session
MIME-Version: 1.0
Content-Type: text/plain; charset=utf8
Content-Transfer-Encoding: 8bit

Change-Id: I2ef4153b5910e56c3401c08d57465741f1002691
Signed-off-by: Alexis de Talhouët <adetalhouet89@gmail.com>
---
 .../netconf/sal/KeepaliveSalFacade.java       |  5 ++
 .../netconf/sal/KeepaliveSalFacadeTest.java   | 56 +++++++------------
 2 files changed, 26 insertions(+), 35 deletions(-)

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 428a9b7fa1..b77e7269e5 100644
--- 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 f0b791a3b3..3f0b9d8628 100644
--- 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
-- 
2.36.6