Do not change NetconfSessionPromise to failure

Setting NetconfSessionPromise to failure when session negotiation fails
prevents recovery from a temporary session negotiation issue.
The state change does not prevent a new connection attempt but the
NETCONF session can never be fully established successfully even if the
negotiation itself is successful because it is tried with the same
promise which has already been set to failure.

Do not change the promise to failure when the session negotiation fails,
and leave the decision to ReconnectStrategy.

JIRA: NETCONF-1067
Change-Id: Id021d268f80f9991a2517a20ed105d3c1d87069c
Signed-off-by: Sangwook Ha <sangwook.ha@verizon.com>
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>
(cherry picked from commit 09c09ffbff1a8cb1dd6926be41464b3fb3a0a24f)

diff --git a/netconf/netconf-netty-util/src/main/java/org/opendaylight/netconf/nettyutil/AbstractNetconfSessionNegotiator.java b/netconf/netconf-netty-util/src/main/java/org/opendaylight/netconf/nettyutil/AbstractNetconfSessionNegotiator.java
index 082d4247cc4dc2fdd6d6fef5e4da26a2e7001eb8..8803b301d37763fd58866e4024cdaa0728e26c69 100644 (file)
--- a/netconf/netconf-netty-util/src/main/java/org/opendaylight/netconf/nettyutil/AbstractNetconfSessionNegotiator.java
+++ b/netconf/netconf-netty-util/src/main/java/org/opendaylight/netconf/nettyutil/AbstractNetconfSessionNegotiator.java
@@ -349,9 +349,8 @@ public abstract class AbstractNetconfSessionNegotiator<S extends AbstractNetconf
     }
 
     protected void negotiationFailed(final Throwable cause) {
-        LOG.debug("Negotiation on channel {} failed", channel, cause);
+        LOG.warn("Negotiation on channel {} failed", channel, cause);
         channel.close();
-        promise.setFailure(cause);
     }
 
     @Override

diff --git a/netconf/netconf-netty-util/src/test/java/org/opendaylight/netconf/nettyutil/AbstractNetconfSessionNegotiatorTest.java b/netconf/netconf-netty-util/src/test/java/org/opendaylight/netconf/nettyutil/AbstractNetconfSessionNegotiatorTest.java
index 19f772514d8077581788d81b2916feee95b1d874..12b96e1c528fa77431385cd5daf298d700396158 100644 (file)
--- a/netconf/netconf-netty-util/src/test/java/org/opendaylight/netconf/nettyutil/AbstractNetconfSessionNegotiatorTest.java
+++ b/netconf/netconf-netty-util/src/test/java/org/opendaylight/netconf/nettyutil/AbstractNetconfSessionNegotiatorTest.java
@@ -11,6 +11,7 @@ import static org.hamcrest.CoreMatchers.instanceOf;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.doNothing;
@@ -22,6 +23,7 @@ import static org.mockito.Mockito.verify;
 import static org.opendaylight.netconf.nettyutil.AbstractChannelInitializer.NETCONF_MESSAGE_AGGREGATOR;
 import static org.opendaylight.netconf.nettyutil.AbstractChannelInitializer.NETCONF_MESSAGE_FRAME_ENCODER;
 
+import io.netty.bootstrap.Bootstrap;
 import io.netty.buffer.ByteBuf;
 import io.netty.buffer.Unpooled;
 import io.netty.channel.ChannelHandlerContext;
@@ -35,7 +37,8 @@ import io.netty.util.Timeout;
 import io.netty.util.Timer;
 import io.netty.util.TimerTask;
 import io.netty.util.concurrent.Future;
-import io.netty.util.concurrent.Promise;
+import io.netty.util.concurrent.GlobalEventExecutor;
+import java.net.InetSocketAddress;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Optional;
@@ -64,13 +67,12 @@ public class AbstractNetconfSessionNegotiatorTest {
     @Mock
     private NetconfSessionListener<TestingNetconfSession> listener;
     @Mock
-    private Promise<TestingNetconfSession> promise;
-    @Mock
     private SslHandler sslHandler;
     @Mock
     private Timer timer;
     @Mock
     private Timeout timeout;
+    private NetconfSessionPromise promise;
     private EmbeddedChannel channel;
     private TestSessionNegotiator negotiator;
     private NetconfHelloMessage hello;
@@ -78,20 +80,9 @@ public class AbstractNetconfSessionNegotiatorTest {
     private NetconfXMLToHelloMessageDecoder xmlToHello;
 
     @Before
-    public void setUp() {
-        channel = new EmbeddedChannel();
-        xmlToHello = new NetconfXMLToHelloMessageDecoder();
-        channel.pipeline().addLast(AbstractChannelInitializer.NETCONF_MESSAGE_ENCODER,
-                new ChannelInboundHandlerAdapter());
-        channel.pipeline().addLast(AbstractChannelInitializer.NETCONF_MESSAGE_DECODER, xmlToHello);
-        channel.pipeline().addLast(NETCONF_MESSAGE_FRAME_ENCODER,
-                FramingMechanismHandlerFactory.createHandler(FramingMechanism.EOM));
-        channel.pipeline().addLast(NETCONF_MESSAGE_AGGREGATOR, new NetconfEOMAggregator());
-        hello = NetconfHelloMessage.createClientHello(Set.of(), Optional.empty());
-        helloBase11 = NetconfHelloMessage.createClientHello(
-            Set.of(XmlNetconfConstants.URN_IETF_PARAMS_NETCONF_BASE_1_1), Optional.empty());
-        doReturn(promise).when(promise).setFailure(any());
-        negotiator = new TestSessionNegotiator(helloBase11, promise, channel, timer, listener, 100L);
+    public void setUp() throws Exception {
+        createNetconfSessionPromise();
+        createNegotiator();
     }
 
     @Test
@@ -178,7 +169,47 @@ public class AbstractNetconfSessionNegotiatorTest {
         negotiator.startNegotiation();
         final RuntimeException cause = new RuntimeException("failure cause");
         channel.pipeline().fireExceptionCaught(cause);
-        verify(promise).setFailure(cause);
+        verify(promise, times(0)).setFailure(cause);
+    }
+
+    @Test
+    public void testRetryNegotiationAfterFailure() {
+        enableTimerTask();
+        negotiator.startNegotiation();
+        negotiator.negotiationFailed(new RuntimeException("failure cause"));
+
+        // Try negotiation after initial failure
+        final var session = mock(TestingNetconfSession.class);
+        doReturn(true).when(session).isSharable();
+        createNegotiator();
+        channel.pipeline().addLast(negotiator);
+        negotiator.startNegotiation();
+        negotiator.negotiationSuccessful(session);
+        assertTrue(promise.isDone());
+        assertTrue(promise.isSuccess());
+    }
+
+    private void createNetconfSessionPromise() throws Exception {
+        var address = mock(InetSocketAddress.class);
+        var reconnectStrategy = mock(ReconnectStrategy.class);
+        var bootstrap = mock(Bootstrap.class);
+        doNothing().when(reconnectStrategy).reconnectSuccessful();
+        promise = spy(new NetconfSessionPromise(GlobalEventExecutor.INSTANCE, address, reconnectStrategy, bootstrap));
+    }
+
+    private void createNegotiator() {
+        channel = new EmbeddedChannel();
+        xmlToHello = new NetconfXMLToHelloMessageDecoder();
+        channel.pipeline().addLast(AbstractChannelInitializer.NETCONF_MESSAGE_ENCODER,
+                new ChannelInboundHandlerAdapter());
+        channel.pipeline().addLast(AbstractChannelInitializer.NETCONF_MESSAGE_DECODER, xmlToHello);
+        channel.pipeline().addLast(NETCONF_MESSAGE_FRAME_ENCODER,
+                FramingMechanismHandlerFactory.createHandler(FramingMechanism.EOM));
+        channel.pipeline().addLast(NETCONF_MESSAGE_AGGREGATOR, new NetconfEOMAggregator());
+        hello = NetconfHelloMessage.createClientHello(Set.of(), Optional.empty());
+        helloBase11 = NetconfHelloMessage.createClientHello(
+            Set.of(XmlNetconfConstants.URN_IETF_PARAMS_NETCONF_BASE_1_1), Optional.empty());
+        negotiator = new TestSessionNegotiator(helloBase11, promise, channel, timer, listener, 100L);
     }
 
     private void enableTimerTask() {