Fix incorrect operational state of device configuration

Initialization of clientConfig in NetconfNodeHandler can throw
a RuntimeException, leaving no handler to clean up
the operational datastore. This results in garbage
data upon node deletion.

Made clientConfig nullable and wrapped its initialization in
a try/catch block to handle exceptions. Moved input validation
for host/port from AbstractNetconfTopology#setupConnection()
to DefaultNetconfClientConfigurationBuilderFactory
and included relevant unit tests.

JIRA: NETCONF-1114
Change-Id: Ieb74ee3c1a4e0cbab7f0e01c942d1e8c1d6e20e4
Signed-off-by: Yaroslav Lastivka <yaroslav.lastivka@pantheon.tech>
Signed-off-by: Ivan Hrasko <ivan.hrasko@pantheon.tech>

diff --git a/apps/netconf-topology/src/main/java/org/opendaylight/netconf/topology/spi/AbstractNetconfTopology.java b/apps/netconf-topology/src/main/java/org/opendaylight/netconf/topology/spi/AbstractNetconfTopology.java
index 8fef25b22e022a6a64c3b1b3477a009e591488da..00e6a58ea899cd7692ab639d75c3e8baf678e037 100644 (file)
--- a/apps/netconf-topology/src/main/java/org/opendaylight/netconf/topology/spi/AbstractNetconfTopology.java
+++ b/apps/netconf-topology/src/main/java/org/opendaylight/netconf/topology/spi/AbstractNetconfTopology.java
@@ -133,9 +133,6 @@ public abstract class AbstractNetconfTopology {
         final var netconfNode = configNode.augmentation(NetconfNode.class);
         final var nodeOptional = configNode.augmentation(NetconfNodeAugmentedOptional.class);
 
-        requireNonNull(netconfNode.getHost());
-        requireNonNull(netconfNode.getPort());
-
         // Instantiate the handler ...
         final var deviceId = NetconfNodeUtils.toRemoteDeviceId(nodeId, netconfNode);
         final var deviceSalFacade = createSalFacade(deviceId, netconfNode.requireLockDatastore());

diff --git a/apps/netconf-topology/src/main/java/org/opendaylight/netconf/topology/spi/DefaultNetconfClientConfigurationBuilderFactory.java b/apps/netconf-topology/src/main/java/org/opendaylight/netconf/topology/spi/DefaultNetconfClientConfigurationBuilderFactory.java
index 55ccf74f425cab88b4282d7aa1d7a05f6ace1ef6..6ca870980362d5e7b4c681770f9aa098c6de646f 100644 (file)
--- a/apps/netconf-topology/src/main/java/org/opendaylight/netconf/topology/spi/DefaultNetconfClientConfigurationBuilderFactory.java
+++ b/apps/netconf-topology/src/main/java/org/opendaylight/netconf/topology/spi/DefaultNetconfClientConfigurationBuilderFactory.java
@@ -57,7 +57,10 @@ public final class DefaultNetconfClientConfigurationBuilderFactory implements Ne
     @Override
     public NetconfClientConfigurationBuilder createClientConfigurationBuilder(final NodeId nodeId,
             final NetconfNode node) {
-        final var builder  = NetconfClientConfigurationBuilder.create();
+        final var builder = NetconfClientConfigurationBuilder.create();
+
+        requireNonNull(node.getHost());
+        requireNonNull(node.getPort());
 
         final var protocol = node.getProtocol();
         if (node.requireTcpOnly()) {

diff --git a/apps/netconf-topology/src/main/java/org/opendaylight/netconf/topology/spi/NetconfNodeHandler.java b/apps/netconf-topology/src/main/java/org/opendaylight/netconf/topology/spi/NetconfNodeHandler.java
index 47f980c66039ce8fa1c47e8e1077254aab9e1039..44ab95428eb4376eedbc0c74ec7bbd2f76fc9443 100644 (file)
--- a/apps/netconf-topology/src/main/java/org/opendaylight/netconf/topology/spi/NetconfNodeHandler.java
+++ b/apps/netconf-topology/src/main/java/org/opendaylight/netconf/topology/spi/NetconfNodeHandler.java
@@ -21,6 +21,7 @@ import java.util.concurrent.TimeUnit;
 import org.checkerframework.checker.lock.qual.GuardedBy;
 import org.checkerframework.checker.lock.qual.Holding;
 import org.eclipse.jdt.annotation.NonNull;
+import org.eclipse.jdt.annotation.Nullable;
 import org.opendaylight.mdsal.dom.api.DOMNotification;
 import org.opendaylight.netconf.client.NetconfClientDispatcher;
 import org.opendaylight.netconf.client.conf.NetconfClientConfiguration;
@@ -59,7 +60,6 @@ public final class NetconfNodeHandler extends AbstractRegistration implements Re
 
     private final @NonNull List<SchemaSourceRegistration<?>> yanglibRegistrations;
     private final @NonNull NetconfClientDispatcher clientDispatcher;
-    private final @NonNull NetconfClientConfiguration clientConfig;
     private final @NonNull NetconfDeviceCommunicator communicator;
     private final @NonNull RemoteDeviceHandler delegate;
     private final @NonNull EventExecutor eventExecutor;
@@ -69,6 +69,7 @@ public final class NetconfNodeHandler extends AbstractRegistration implements Re
     private final int minSleep;
     private final double sleepFactor;
 
+    private @Nullable NetconfClientConfiguration clientConfig;
     @GuardedBy("this")
     private long attempts;
     @GuardedBy("this")
@@ -76,6 +77,7 @@ public final class NetconfNodeHandler extends AbstractRegistration implements Re
     @GuardedBy("this")
     private Future<?> currentTask;
 
+    @SuppressWarnings("checkstyle:IllegalCatch")
     public NetconfNodeHandler(final NetconfClientDispatcher clientDispatcher, final EventExecutor eventExecutor,
             final ScheduledExecutorService keepaliveExecutor, final BaseNetconfSchemas baseSchemas,
             final SchemaResourceManager schemaManager, final Executor processingExecutor,
@@ -140,13 +142,23 @@ public final class NetconfNodeHandler extends AbstractRegistration implements Re
         if (keepAliveFacade != null) {
             keepAliveFacade.setListener(communicator);
         }
-
-        clientConfig = builderFactory.createClientConfigurationBuilder(nodeId, node)
-            .withSessionListener(communicator)
-            .build();
+        // FIXME: move this heavy logic out of constructor
+        try {
+            clientConfig = builderFactory.createClientConfigurationBuilder(nodeId, node)
+                .withSessionListener(communicator)
+                .build();
+        } catch (final Exception exception) {
+            LOG.warn("Error initialization configuration for device {}, using null instead.", deviceId, exception);
+            delegate.onDeviceFailed(exception);
+            clientConfig = null;
+        }
     }
 
     public synchronized void connect() {
+        if (clientConfig == null) {
+            LOG.warn("Connection attempts are skipped for device {} because of missing configuration", deviceId);
+            return;
+        }
         attempts = 1;
         lastSleep = minSleep;
         lockedConnect();
@@ -304,4 +316,4 @@ public final class NetconfNodeHandler extends AbstractRegistration implements Re
     synchronized long attempts() {
         return attempts;
     }
-}
\ No newline at end of file
+}

diff --git a/apps/netconf-topology/src/test/java/org/opendaylight/netconf/topology/spi/DefaultNetconfClientConfigurationBuilderFactoryTest.java b/apps/netconf-topology/src/test/java/org/opendaylight/netconf/topology/spi/DefaultNetconfClientConfigurationBuilderFactoryTest.java
index 6e63e1fb191bb17b37e5feb3f8ffe768dcfd81ed..9e75be1d24efaa6ce7d992361c7d32b15ab7cbf1 100644 (file)
--- a/apps/netconf-topology/src/test/java/org/opendaylight/netconf/topology/spi/DefaultNetconfClientConfigurationBuilderFactoryTest.java
+++ b/apps/netconf-topology/src/test/java/org/opendaylight/netconf/topology/spi/DefaultNetconfClientConfigurationBuilderFactoryTest.java
@@ -11,6 +11,7 @@ import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertThrows;
 import static org.mockito.Mockito.doReturn;
 
 import org.junit.Before;
@@ -110,6 +111,18 @@ public class DefaultNetconfClientConfigurationBuilderFactoryTest {
         assertSame(sslHandlerFactory, config.getSslHandlerFactory());
     }
 
+    @Test
+    public void testPort() {
+        assertThrows("Port must be specified.", NullPointerException.class, () ->
+            createConfig(nodeBuilder.setPort(null).build()));
+    }
+
+    @Test
+    public void testHost() {
+        assertThrows("Host must be specified.", NullPointerException.class, () ->
+            createConfig(nodeBuilder.setHost(null).build()));
+    }
+
     private NetconfClientConfiguration createConfig(final NetconfNode netconfNode) {
         return factory.createClientConfigurationBuilder(NODE_ID, netconfNode)
             .withSessionListener(sessionListener)

diff --git a/apps/netconf-topology/src/test/java/org/opendaylight/netconf/topology/spi/NetconfNodeHandlerTest.java b/apps/netconf-topology/src/test/java/org/opendaylight/netconf/topology/spi/NetconfNodeHandlerTest.java
index 3c6da3b4251c2941bb7b1dbdf3a6ce9e36913427..6eff295ee023e173bc6e09100c455f12c852fa5d 100644 (file)
--- a/apps/netconf-topology/src/test/java/org/opendaylight/netconf/topology/spi/NetconfNodeHandlerTest.java
+++ b/apps/netconf-topology/src/test/java/org/opendaylight/netconf/topology/spi/NetconfNodeHandlerTest.java
@@ -195,6 +195,40 @@ public class NetconfNodeHandlerTest {
         assertEquals(2, handler.attempts());
     }
 
+    /**
+     * Tests the initialization of the NetconfNodeHandler's client configuration.
+     *
+     * <p>
+     * Ensures that no connection attempts are made by the handler after calling connect().
+     */
+    @Test
+    public void testClientConfigInitializationFailure() {
+        doNothing().when(delegate).onDeviceFailed(any(Exception.class));
+        // Creates a netconfNode without specifying a Port and Host.
+        final var netconfNode = new NetconfNodeBuilder()
+            .setReconnectOnChangedSchema(true)
+            .setSchemaless(true)
+            .setTcpOnly(true)
+            .setSleepFactor(Decimal64.valueOf("1.5"))
+            .setConcurrentRpcLimit(Uint16.ONE)
+            // One reconnection attempt
+            .setMaxConnectionAttempts(Uint32.TWO)
+            .setDefaultRequestTimeoutMillis(Uint32.valueOf(1000))
+            .setBetweenAttemptsTimeoutMillis(Uint16.valueOf(100))
+            .setKeepaliveDelay(Uint32.valueOf(1000))
+            .setConnectionTimeoutMillis(Uint32.valueOf(1000))
+            .setCredentials(new LoginPasswordBuilder().setUsername("testuser").setPassword("testpassword").build())
+            .build();
+        handler = new NetconfNodeHandler(clientDispatcher, eventExecutor, keepaliveExecutor, BASE_SCHEMAS,
+            schemaManager, processingExecutor, new DefaultNetconfClientConfigurationBuilderFactory(encryptionService,
+            credentialProvider,
+            sslHandlerFactoryProvider), deviceActionFactory, delegate, DEVICE_ID, NODE_ID,
+            netconfNode, null);
+        handler.connect();
+        // Expect 0 connection attempts since the clientConfig has not been initialized.
+        assertEquals(0, handler.attempts());
+    }
+
     @Test
     public void downAfterUpCausesReconnect() {
         // Let's borrow common bits