Do not start devices with invalid configuration

We have an unfortunate modeling accident, where netconf-node's 'host'
and 'port' leaves cannot be made mandatory unless an incompatible change
is made.

This means invalid configuration may hit the configuration datastore
(rather than being rejected) and we need to deal with that.

Redefine ensureNode() to issue a deleteNode() on error and restructure
internals to capture required state before we make a decision whether
to start a node. This rids us of a source of NPE when the NetconfNode is
completely missing.

JIRA: NETCONF-1114
Change-Id: I2f0fd8a65d1c508cb439c1994cf6c446d051412d
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>
Signed-off-by: Oleksandr Zharov <oleksandr.zharov@pantheon.tech>

diff --git a/apps/netconf-topology-impl/src/test/java/org/opendaylight/netconf/topology/impl/NetconfTopologyImplTest.java b/apps/netconf-topology-impl/src/test/java/org/opendaylight/netconf/topology/impl/NetconfTopologyImplTest.java
index 6616968d03bf36335741b36e08971160dd8afe5f..d3f561b49aa3971a93d7b711a7d960409f4dd090 100644 (file)
--- a/apps/netconf-topology-impl/src/test/java/org/opendaylight/netconf/topology/impl/NetconfTopologyImplTest.java
+++ b/apps/netconf-topology-impl/src/test/java/org/opendaylight/netconf/topology/impl/NetconfTopologyImplTest.java
@@ -113,6 +113,7 @@ class NetconfTopologyImplTest {
         final var node = new NodeBuilder()
             .withKey(key)
             .addAugmentation(new NetconfNodeBuilder()
+                .setLockDatastore(true)
                 .setHost(new Host(new IpAddress(new Ipv4Address("127.0.0.1"))))
                 .setPort(new PortNumber(Uint16.valueOf(9999)))
                 .setReconnectOnChangedSchema(true)

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 00e6a58ea899cd7692ab639d75c3e8baf678e037..73798ff01e00408bfa4735f190598f418072d20a 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
@@ -12,10 +12,10 @@ import static java.util.Objects.requireNonNull;
 import com.google.common.annotations.VisibleForTesting;
 import io.netty.util.concurrent.EventExecutor;
 import java.util.HashMap;
+import java.util.NoSuchElementException;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Executor;
 import java.util.concurrent.ScheduledExecutorService;
-import org.checkerframework.checker.lock.qual.Holding;
 import org.opendaylight.controller.config.threadpool.ScheduledThreadPool;
 import org.opendaylight.controller.config.threadpool.ThreadPool;
 import org.opendaylight.mdsal.binding.api.DataBroker;
@@ -103,9 +103,33 @@ public abstract class AbstractNetconfTopology {
             LOG.info("RemoteDevice{{}} was already configured, disconnecting", nodeId);
             prev.close();
         }
+        final var netconfNode = node.augmentation(NetconfNode.class);
+        if (netconfNode == null) {
+            LOG.warn("RemoteDevice{{}} is missing NETCONF node configuration, not connecting it", nodeId);
+            return;
+        }
+        final RemoteDeviceId deviceId;
+        try {
+            deviceId = NetconfNodeUtils.toRemoteDeviceId(nodeId, netconfNode);
+        } catch (NoSuchElementException e) {
+            LOG.warn("RemoteDevice{{}} has invalid configuration, not connecting it", nodeId, e);
+            return;
+        }
 
         LOG.info("Connecting RemoteDevice{{}}, with config {}", nodeId, hideCredentials(node));
-        setupConnection(nodeId, node);
+
+        // Instantiate the handler ...
+        final var nodeOptional = node.augmentation(NetconfNodeAugmentedOptional.class);
+        final var deviceSalFacade = createSalFacade(deviceId, netconfNode.requireLockDatastore());
+        final var nodeHandler = new NetconfNodeHandler(clientDispatcher, eventExecutor, keepaliveExecutor,
+            baseSchemas, schemaManager, processingExecutor, builderFactory, deviceActionFactory, deviceSalFacade,
+            deviceId, nodeId, netconfNode, nodeOptional);
+
+        // ... record it ...
+        activeConnectors.put(nodeId, nodeHandler);
+
+        // ... and start it
+        nodeHandler.connect();
     }
 
     // Non-final for testing
@@ -128,25 +152,6 @@ public abstract class AbstractNetconfTopology {
         activeConnectors.clear();
     }
 
-    @Holding("this")
-    protected final void setupConnection(final NodeId nodeId, final Node configNode) {
-        final var netconfNode = configNode.augmentation(NetconfNode.class);
-        final var nodeOptional = configNode.augmentation(NetconfNodeAugmentedOptional.class);
-
-        // Instantiate the handler ...
-        final var deviceId = NetconfNodeUtils.toRemoteDeviceId(nodeId, netconfNode);
-        final var deviceSalFacade = createSalFacade(deviceId, netconfNode.requireLockDatastore());
-        final var nodeHandler = new NetconfNodeHandler(clientDispatcher, eventExecutor, keepaliveExecutor,
-            baseSchemas, schemaManager, processingExecutor, builderFactory, deviceActionFactory, deviceSalFacade,
-            deviceId, nodeId, netconfNode, nodeOptional);
-
-        // ... record it ...
-        activeConnectors.put(nodeId, nodeHandler);
-
-        // ... and start it
-        nodeHandler.connect();
-    }
-
     protected RemoteDeviceHandler createSalFacade(final RemoteDeviceId deviceId, final boolean lockDatastore) {
         return new NetconfTopologyDeviceSalFacade(deviceId, mountPointService, lockDatastore, dataBroker);
     }