Netconf stack by default locks the data store before issuing 72/85872/3
authorJakub Toth <jtoth@luminanetworks.com>
Fri, 14 Jun 2019 15:32:00 +0000 (11:32 -0400)
committerRobert Varga <nite@hq.sk>
Tue, 26 Nov 2019 17:34:39 +0000 (17:34 +0000)
edit-config, and then unlocks it.

This poses some scalabilty issues when multiple requests for
the same device arrives. Also some devices automatically
queues the requests at the device itself.

The patch provides an option to allow lock/unlock operations
for edit-config. Default value is true which defaults to today's
behavior.

JIRA: NETCONF-629
Change-Id: I0531b27c0807dfb586f9e80c9b9fc9b189e3cadf
Signed-off-by: Jakub Toth <jtoth@luminanetworks.com>
Signed-off-by: Miroslav Macko <mmacko@luminanetworks.com>
netconf/callhome-provider/src/main/java/org/opendaylight/netconf/callhome/mount/CallHomeTopology.java
netconf/netconf-topology/src/main/java/org/opendaylight/netconf/topology/impl/NetconfTopologyImpl.java
netconf/sal-netconf-connector/src/main/java/org/opendaylight/netconf/sal/connect/netconf/sal/LockChangeListener.java [new file with mode: 0644]
netconf/sal-netconf-connector/src/main/java/org/opendaylight/netconf/sal/connect/netconf/sal/NetconfDeviceDataBroker.java
netconf/sal-netconf-connector/src/main/java/org/opendaylight/netconf/sal/connect/netconf/sal/NetconfDeviceSalFacade.java
netconf/sal-netconf-connector/src/main/java/org/opendaylight/netconf/sal/connect/netconf/sal/tx/AbstractWriteTx.java
netconf/sal-netconf-connector/src/main/java/org/opendaylight/netconf/sal/connect/netconf/sal/tx/WriteCandidateRunningTx.java
netconf/sal-netconf-connector/src/main/java/org/opendaylight/netconf/sal/connect/netconf/sal/tx/WriteCandidateTx.java
netconf/sal-netconf-connector/src/main/java/org/opendaylight/netconf/sal/connect/netconf/sal/tx/WriteRunningTx.java
netconf/sal-netconf-connector/src/test/java/org/opendaylight/netconf/sal/connect/netconf/sal/NetconfDeviceSalFacadeTest.java

index 84c743ea668e494d27223f90d5ac938a4a232995..e032637cd83b2886584013e841edbfea29945d3a 100644 (file)
@@ -48,6 +48,6 @@ public class CallHomeTopology extends BaseCallHomeTopology {
 
     @Override
     protected RemoteDeviceHandler<NetconfSessionPreferences> createSalFacade(final RemoteDeviceId id) {
-        return new NetconfDeviceSalFacade(id, mountPointService, dataBroker);
+        return new NetconfDeviceSalFacade(id, mountPointService, dataBroker, topologyId);
     }
 }
index 94ff04ec40d84ca0cca0482e0fbf05c422b735d2..86ce1f9c02865e4aa3ecb26ca57ea9dba257735f 100644 (file)
@@ -88,7 +88,7 @@ public class NetconfTopologyImpl extends AbstractNetconfTopology
 
     @Override
     protected RemoteDeviceHandler<NetconfSessionPreferences> createSalFacade(final RemoteDeviceId id) {
-        return new NetconfDeviceSalFacade(id, mountPointService, dataBroker);
+        return new NetconfDeviceSalFacade(id, mountPointService, dataBroker, topologyId);
     }
 
     /**
diff --git a/netconf/sal-netconf-connector/src/main/java/org/opendaylight/netconf/sal/connect/netconf/sal/LockChangeListener.java b/netconf/sal-netconf-connector/src/main/java/org/opendaylight/netconf/sal/connect/netconf/sal/LockChangeListener.java
new file mode 100644 (file)
index 0000000..2092057
--- /dev/null
@@ -0,0 +1,52 @@
+/*
+ * Copyright (c) 2019 Lumina Networks, Inc. All rights reserved.
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License v1.0 which accompanies this distribution,
+ * and is available at http://www.eclipse.org/legal/epl-v10.html
+ */
+package org.opendaylight.netconf.sal.connect.netconf.sal;
+
+import java.util.Collection;
+import org.opendaylight.mdsal.binding.api.DataObjectModification;
+import org.opendaylight.mdsal.binding.api.DataTreeChangeListener;
+import org.opendaylight.mdsal.binding.api.DataTreeModification;
+import org.opendaylight.mdsal.dom.api.DOMDataBroker;
+import org.opendaylight.yang.gen.v1.urn.opendaylight.netconf.node.optional.rev190614.netconf.node.fields.optional.topology.node.DatastoreLock;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+final class LockChangeListener implements DataTreeChangeListener<DatastoreLock> {
+
+    private static final Logger LOG = LoggerFactory.getLogger(LockChangeListener.class);
+
+    private final NetconfDeviceDataBroker netconfDeviceDataBroker;
+
+    LockChangeListener(final DOMDataBroker netconfDeviceDataBrokder) {
+        this.netconfDeviceDataBroker = (NetconfDeviceDataBroker)netconfDeviceDataBrokder;
+    }
+
+    @Override
+    public void onDataTreeChanged(final Collection<DataTreeModification<DatastoreLock>> changes) {
+        for (final DataTreeModification<DatastoreLock> change : changes) {
+            final DataObjectModification<DatastoreLock> rootNode = change.getRootNode();
+            switch (rootNode.getModificationType()) {
+                case SUBTREE_MODIFIED:
+                case WRITE:
+                    if (!rootNode.getDataAfter().isDatastoreLockAllowed()) {
+                        LOG.warn("With blocking the lock/unlock operations, the user is coming to"
+                                 + "operate in a manner which is not supported. It must not exist"
+                                 + "any concurrent access to the data store - it may interfere with"
+                                 + "data consistency.");
+                    }
+                    netconfDeviceDataBroker.setLockAllowed(rootNode.getDataAfter().isDatastoreLockAllowed());
+                    break;
+                case DELETE:
+                    netconfDeviceDataBroker.setLockAllowed(true);
+                    break;
+                default:
+                    LOG.debug("Unsupported modification type: {}.", rootNode.getModificationType());
+            }
+        }
+    }
+}
index afb76dcfe4566a1922a0a0b45f761106a153e70c..fabc428331b00778778f35e73a4a82cb4952e885 100644 (file)
@@ -32,11 +32,12 @@ import org.opendaylight.yangtools.yang.model.api.SchemaContext;
 public final class NetconfDeviceDataBroker implements DOMDataBroker {
     private final RemoteDeviceId id;
     private final NetconfBaseOps netconfOps;
-
     private final boolean rollbackSupport;
     private final boolean candidateSupported;
     private final boolean runningWritable;
 
+    private boolean isLockAllowed = true;
+
     public NetconfDeviceDataBroker(final RemoteDeviceId id, final SchemaContext schemaContext,
                                    final DOMRpcService rpc, final NetconfSessionPreferences netconfSessionPreferences) {
         this.id = id;
@@ -65,12 +66,12 @@ public final class NetconfDeviceDataBroker implements DOMDataBroker {
     public DOMDataTreeWriteTransaction newWriteOnlyTransaction() {
         if (candidateSupported) {
             if (runningWritable) {
-                return new WriteCandidateRunningTx(id, netconfOps, rollbackSupport);
+                return new WriteCandidateRunningTx(id, netconfOps, rollbackSupport, isLockAllowed);
             } else {
-                return new WriteCandidateTx(id, netconfOps, rollbackSupport);
+                return new WriteCandidateTx(id, netconfOps, rollbackSupport, isLockAllowed);
             }
         } else {
-            return new WriteRunningTx(id, netconfOps, rollbackSupport);
+            return new WriteRunningTx(id, netconfOps, rollbackSupport, isLockAllowed);
         }
     }
 
@@ -83,4 +84,9 @@ public final class NetconfDeviceDataBroker implements DOMDataBroker {
     public ClassToInstanceMap<DOMDataBrokerExtension> getExtensions() {
         return ImmutableClassToInstanceMap.of();
     }
+
+    void setLockAllowed(boolean isLockAllowedOrig) {
+        this.isLockAllowed = isLockAllowedOrig;
+    }
+
 }
index aaf7db911f4ca16e011395cc58609892b8e80ef6..845378e5e95084bf0c62b8c30eb920ef006e0fab 100644 (file)
@@ -12,9 +12,9 @@ import com.google.common.collect.Lists;
 import java.util.ArrayList;
 import java.util.List;
 import org.opendaylight.mdsal.binding.api.DataBroker;
+import org.opendaylight.mdsal.binding.api.DataTreeIdentifier;
 import org.opendaylight.mdsal.common.api.LogicalDatastoreType;
 import org.opendaylight.mdsal.dom.api.DOMActionService;
-import org.opendaylight.mdsal.dom.api.DOMDataBroker;
 import org.opendaylight.mdsal.dom.api.DOMMountPointService;
 import org.opendaylight.mdsal.dom.api.DOMNotification;
 import org.opendaylight.mdsal.dom.api.DOMRpcService;
@@ -22,8 +22,18 @@ import org.opendaylight.netconf.sal.connect.api.RemoteDeviceHandler;
 import org.opendaylight.netconf.sal.connect.netconf.listener.NetconfDeviceCapabilities;
 import org.opendaylight.netconf.sal.connect.netconf.listener.NetconfSessionPreferences;
 import org.opendaylight.netconf.sal.connect.util.RemoteDeviceId;
+import org.opendaylight.yang.gen.v1.urn.opendaylight.netconf.node.optional.rev190614.NetconfNodeFieldsOptional;
+import org.opendaylight.yang.gen.v1.urn.opendaylight.netconf.node.optional.rev190614.netconf.node.fields.optional.Topology;
+import org.opendaylight.yang.gen.v1.urn.opendaylight.netconf.node.optional.rev190614.netconf.node.fields.optional.TopologyKey;
+import org.opendaylight.yang.gen.v1.urn.opendaylight.netconf.node.optional.rev190614.netconf.node.fields.optional.topology.Node;
+import org.opendaylight.yang.gen.v1.urn.opendaylight.netconf.node.optional.rev190614.netconf.node.fields.optional.topology.NodeKey;
+import org.opendaylight.yang.gen.v1.urn.opendaylight.netconf.node.optional.rev190614.netconf.node.fields.optional.topology.node.DatastoreLock;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.netconf.node.topology.rev150114.NetconfNode;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.netconf.node.topology.rev150114.NetconfNodeConnectionStatus.ConnectionStatus;
+import org.opendaylight.yang.gen.v1.urn.tbd.params.xml.ns.yang.network.topology.rev131021.NodeId;
+import org.opendaylight.yang.gen.v1.urn.tbd.params.xml.ns.yang.network.topology.rev131021.TopologyId;
+import org.opendaylight.yangtools.concepts.ListenerRegistration;
+import org.opendaylight.yangtools.yang.binding.InstanceIdentifier;
 import org.opendaylight.yangtools.yang.model.api.SchemaContext;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -35,17 +45,23 @@ public final class NetconfDeviceSalFacade implements AutoCloseable, RemoteDevice
     private final RemoteDeviceId id;
     private final NetconfDeviceSalProvider salProvider;
     private final List<AutoCloseable> salRegistrations = new ArrayList<>();
+    private final DataBroker dataBroker;
+    private final String topologyId;
+
+    private ListenerRegistration<LockChangeListener> listenerRegistration = null;
 
     public NetconfDeviceSalFacade(final RemoteDeviceId id, final DOMMountPointService mountPointService,
-            final DataBroker dataBroker) {
-        this.id = id;
-        this.salProvider = new NetconfDeviceSalProvider(id, mountPointService, dataBroker);
+            final DataBroker dataBroker, final String topologyId) {
+        this(id, new NetconfDeviceSalProvider(id, mountPointService, dataBroker), dataBroker, topologyId);
     }
 
     @VisibleForTesting
-    NetconfDeviceSalFacade(final RemoteDeviceId id, final NetconfDeviceSalProvider salProvider) {
+    NetconfDeviceSalFacade(final RemoteDeviceId id, final NetconfDeviceSalProvider salProvider,
+            final DataBroker dataBroker, final String topologyId) {
         this.id = id;
         this.salProvider = salProvider;
+        this.dataBroker = dataBroker;
+        this.topologyId = topologyId;
     }
 
     @Override
@@ -58,13 +74,14 @@ public final class NetconfDeviceSalFacade implements AutoCloseable, RemoteDevice
                                                final NetconfSessionPreferences netconfSessionPreferences,
                                                final DOMRpcService deviceRpc, final DOMActionService deviceAction) {
 
-        final DOMDataBroker domBroker =
+        final NetconfDeviceDataBroker netconfDeviceDataBroker =
                 new NetconfDeviceDataBroker(id, schemaContext, deviceRpc, netconfSessionPreferences);
-
+        registerLockListener(netconfDeviceDataBroker);
         final NetconfDeviceNotificationService notificationService = new NetconfDeviceNotificationService();
 
         salProvider.getMountInstance()
-                .onTopologyDeviceConnected(schemaContext, domBroker, deviceRpc, notificationService, deviceAction);
+                .onTopologyDeviceConnected(schemaContext, netconfDeviceDataBroker, deviceRpc, notificationService,
+                        deviceAction);
         salProvider.getTopologyDatastoreAdapter()
                 .updateDeviceData(true, netconfSessionPreferences.getNetconfDeviceCapabilities());
     }
@@ -80,12 +97,14 @@ public final class NetconfDeviceSalFacade implements AutoCloseable, RemoteDevice
     public synchronized void onDeviceDisconnected() {
         salProvider.getTopologyDatastoreAdapter().updateDeviceData(false, new NetconfDeviceCapabilities());
         salProvider.getMountInstance().onTopologyDeviceDisconnected();
+        closeLockChangeListener();
     }
 
     @Override
     public synchronized void onDeviceFailed(final Throwable throwable) {
         salProvider.getTopologyDatastoreAdapter().setDeviceAsFailed(throwable);
         salProvider.getMountInstance().onTopologyDeviceDisconnected();
+        closeLockChangeListener();
     }
 
     @Override
@@ -94,6 +113,7 @@ public final class NetconfDeviceSalFacade implements AutoCloseable, RemoteDevice
             closeGracefully(reg);
         }
         closeGracefully(salProvider);
+        closeLockChangeListener();
     }
 
     @SuppressWarnings("checkstyle:IllegalCatch")
@@ -107,4 +127,23 @@ public final class NetconfDeviceSalFacade implements AutoCloseable, RemoteDevice
         }
     }
 
+    private void closeLockChangeListener() {
+        if (listenerRegistration != null) {
+            listenerRegistration.close();
+        }
+    }
+
+    private void registerLockListener(final NetconfDeviceDataBroker netconfDeviceDataBroker) {
+        listenerRegistration = dataBroker.registerDataTreeChangeListener(
+                DataTreeIdentifier.create(LogicalDatastoreType.CONFIGURATION, createTopologyListPath()),
+                new LockChangeListener(netconfDeviceDataBroker));
+    }
+
+    private InstanceIdentifier<DatastoreLock> createTopologyListPath() {
+        return InstanceIdentifier.create(NetconfNodeFieldsOptional.class)
+                .child(Topology.class, new TopologyKey(new TopologyId(topologyId)))
+                .child(Node.class, new NodeKey(new NodeId(id.getName())))
+                .child(DatastoreLock.class);
+
+    }
 }
index fa94ad0b45bbbdc30809b08f0ffe4101f00bbdfb..51c73147020b508d8582be8569aaf09bd078bf05 100644 (file)
@@ -52,11 +52,14 @@ public abstract class AbstractWriteTx implements DOMDataTreeWriteTransaction {
     private final List<TxListener> listeners = new CopyOnWriteArrayList<>();
     // Allow commit to be called only once
     protected volatile boolean finished = false;
+    protected final boolean isLockAllowed;
 
-    public AbstractWriteTx(final NetconfBaseOps netOps, final RemoteDeviceId id, final boolean rollbackSupport) {
-        this.netOps = netOps;
+    public AbstractWriteTx(final RemoteDeviceId id, final NetconfBaseOps netconfOps, final boolean rollbackSupport,
+            final boolean isLockAllowed) {
+        this.netOps = netconfOps;
         this.id = id;
         this.rollbackSupport = rollbackSupport;
+        this.isLockAllowed = isLockAllowed;
         init();
     }
 
index b5198ec836a2aa7b21c26ddd8ecd9731452a0dae..fefddc5720b269fd92e2cacb56257fb8ec848e94 100644 (file)
@@ -27,7 +27,12 @@ public class WriteCandidateRunningTx extends WriteCandidateTx {
 
     public WriteCandidateRunningTx(final RemoteDeviceId id, final NetconfBaseOps netOps,
                                    final boolean rollbackSupport) {
-        super(id, netOps, rollbackSupport);
+        this(id, netOps, rollbackSupport, true);
+    }
+
+    public WriteCandidateRunningTx(RemoteDeviceId id, NetconfBaseOps netconfOps, boolean rollbackSupport,
+            boolean isLockAllowed) {
+        super(id, netconfOps, rollbackSupport, isLockAllowed);
     }
 
     @Override
@@ -43,7 +48,11 @@ public class WriteCandidateRunningTx extends WriteCandidateTx {
     }
 
     private void lockRunning() {
-        resultsFutures.add(netOps.lockRunning(new NetconfRpcFutureCallback("Lock running", id)));
+        if (isLockAllowed) {
+            resultsFutures.add(netOps.lockRunning(new NetconfRpcFutureCallback("Lock running", id)));
+        } else {
+            LOG.trace("Lock is not allowed: {}", id);
+        }
     }
 
     /**
@@ -51,6 +60,10 @@ public class WriteCandidateRunningTx extends WriteCandidateTx {
      * and its netty threadpool that is really sensitive to blocking calls.
      */
     private void unlockRunning() {
-        netOps.unlockRunning(new NetconfRpcFutureCallback("Unlock running", id));
+        if (isLockAllowed) {
+            netOps.unlockRunning(new NetconfRpcFutureCallback("Unlock running", id));
+        } else {
+            LOG.trace("Unlock is not allowed: {}", id);
+        }
     }
 }
index 1eb47c9775ba9651de88c56556b633f7f3132164..012cbac8c50c78315468fbbba8894045958690f2 100644 (file)
@@ -52,8 +52,13 @@ public class WriteCandidateTx extends AbstractWriteTx {
 
     private static final Logger LOG  = LoggerFactory.getLogger(WriteCandidateTx.class);
 
-    public WriteCandidateTx(final RemoteDeviceId id, final NetconfBaseOps rpc, final boolean rollbackSupport) {
-        super(rpc, id, rollbackSupport);
+    public WriteCandidateTx(final RemoteDeviceId id, final NetconfBaseOps netconfOps, final boolean rollbackSupport) {
+        this(id, netconfOps, rollbackSupport, true);
+    }
+
+    public WriteCandidateTx(RemoteDeviceId id, NetconfBaseOps netconfOps, boolean rollbackSupport,
+            boolean isLockAllowed) {
+        super(id, netconfOps, rollbackSupport, isLockAllowed);
     }
 
     @Override
@@ -63,6 +68,10 @@ public class WriteCandidateTx extends AbstractWriteTx {
     }
 
     private void lock() {
+        if (!isLockAllowed) {
+            LOG.trace("Lock is not allowed.");
+            return;
+        }
         final FutureCallback<DOMRpcResult> lockCandidateCallback = new FutureCallback<DOMRpcResult>() {
             @Override
             public void onSuccess(@Nonnull final DOMRpcResult result) {
@@ -146,7 +155,11 @@ public class WriteCandidateTx extends AbstractWriteTx {
      * and its netty threadpool that is really sensitive to blocking calls.
      */
     private void unlock() {
-        netOps.unlockCandidate(new NetconfRpcFutureCallback("Unlock candidate", id));
+        if (isLockAllowed) {
+            netOps.unlockCandidate(new NetconfRpcFutureCallback("Unlock candidate", id));
+        } else {
+            LOG.trace("Unlock is not allowed: {}", id);
+        }
     }
 
 }
index c9809e0262e88c9d36e6f9d9cddd0d3de8b72081..940ea1a6b9fe6f9799caee811c79bbf9a165ebd3 100644 (file)
@@ -47,7 +47,12 @@ public class WriteRunningTx extends AbstractWriteTx {
 
     public WriteRunningTx(final RemoteDeviceId id, final NetconfBaseOps netOps,
                           final boolean rollbackSupport) {
-        super(netOps, id, rollbackSupport);
+        this(id, netOps, rollbackSupport, true);
+    }
+
+    public WriteRunningTx(RemoteDeviceId id, NetconfBaseOps netconfOps, boolean rollbackSupport,
+            boolean isLockAllowed) {
+        super(id, netconfOps, rollbackSupport, isLockAllowed);
     }
 
     @Override
@@ -56,7 +61,11 @@ public class WriteRunningTx extends AbstractWriteTx {
     }
 
     private void lock() {
-        resultsFutures.add(netOps.lockRunning(new NetconfRpcFutureCallback("Lock running", id)));
+        if (isLockAllowed) {
+            resultsFutures.add(netOps.lockRunning(new NetconfRpcFutureCallback("Lock running", id)));
+        } else {
+            LOG.trace("Lock is not allowed: {}", id);
+        }
     }
 
     @Override
@@ -83,7 +92,11 @@ public class WriteRunningTx extends AbstractWriteTx {
     }
 
     private void unlock() {
-        netOps.unlockRunning(new NetconfRpcFutureCallback("Unlock running", id));
+        if (isLockAllowed) {
+            netOps.unlockRunning(new NetconfRpcFutureCallback("Unlock running", id));
+        } else {
+            LOG.trace("Unlock is not allowed: {}", id);
+        }
     }
 
     private static final class Change {
index 7ff31fbd9e7828848487f9c4c5ec9d60ff30cd19..69f491dfe600c7ec030f4d0fa9779e4e2b9ec485 100644 (file)
@@ -24,6 +24,7 @@ import java.util.List;
 import org.junit.Before;
 import org.junit.Test;
 import org.mockito.Mock;
+import org.opendaylight.mdsal.binding.api.DataBroker;
 import org.opendaylight.mdsal.dom.api.DOMDataBroker;
 import org.opendaylight.mdsal.dom.api.DOMNotification;
 import org.opendaylight.mdsal.dom.api.DOMRpcService;
@@ -41,9 +42,10 @@ public class NetconfDeviceSalFacadeTest {
     private NetconfDeviceTopologyAdapter netconfDeviceTopologyAdapter;
     @Mock
     private NetconfDeviceSalProvider.MountInstance mountInstance;
-
     @Mock
     private NetconfDeviceSalProvider salProvider;
+    @Mock
+    private DataBroker dataBroker;
 
     @Before
     public void setUp() throws Exception {
@@ -51,7 +53,7 @@ public class NetconfDeviceSalFacadeTest {
         final InetSocketAddress address = new InetSocketAddress("127.0.0.1", 8000);
         final RemoteDeviceId remoteDeviceId = new RemoteDeviceId("test", address);
 
-        deviceFacade = new NetconfDeviceSalFacade(remoteDeviceId, salProvider);
+        deviceFacade = new NetconfDeviceSalFacade(remoteDeviceId, salProvider, dataBroker, "mockTopo");
 
         doReturn(netconfDeviceTopologyAdapter).when(salProvider).getTopologyDatastoreAdapter();
         doNothing().when(netconfDeviceTopologyAdapter)