Bug 6343 - Incorrect handling of configuration failures in SAL netconf connector

Change-Id: I4fbf06038b4a14c0efecfa6564e69671a3d23bcc
Signed-off-by: Jakub Morvay <jmorvay@cisco.com>
(cherry picked from commit 827f758622c4cce82653f3d00507b8a88e7e457a)

diff --git a/netconf/sal-netconf-connector/src/main/java/org/opendaylight/netconf/sal/connect/netconf/sal/tx/AbstractWriteTx.java b/netconf/sal-netconf-connector/src/main/java/org/opendaylight/netconf/sal/connect/netconf/sal/tx/AbstractWriteTx.java
index b9c3f72a60e912e597cb8889cc08ef7dca0b89f2..3269a49eb85e36902893b189a5cd9bd4506335bf 100644 (file)
--- a/netconf/sal-netconf-connector/src/main/java/org/opendaylight/netconf/sal/connect/netconf/sal/tx/AbstractWriteTx.java
+++ b/netconf/sal-netconf-connector/src/main/java/org/opendaylight/netconf/sal/connect/netconf/sal/tx/AbstractWriteTx.java
@@ -10,7 +10,13 @@ package org.opendaylight.netconf.sal.connect.netconf.sal.tx;
 
 import com.google.common.base.Optional;
 import com.google.common.base.Preconditions;
+import com.google.common.collect.Lists;
+import com.google.common.util.concurrent.FutureCallback;
+import com.google.common.util.concurrent.Futures;
 import com.google.common.util.concurrent.ListenableFuture;
+import com.google.common.util.concurrent.SettableFuture;
+import java.util.List;
+import org.opendaylight.controller.config.util.xml.DocumentedException;
 import org.opendaylight.controller.md.sal.common.api.TransactionStatus;
 import org.opendaylight.controller.md.sal.common.api.data.LogicalDatastoreType;
 import org.opendaylight.controller.md.sal.dom.api.DOMDataWriteTransaction;
@@ -19,6 +25,7 @@ import org.opendaylight.netconf.api.NetconfDocumentedException;
 import org.opendaylight.netconf.sal.connect.netconf.util.NetconfBaseOps;
 import org.opendaylight.netconf.sal.connect.util.RemoteDeviceId;
 import org.opendaylight.yangtools.yang.common.RpcResult;
+import org.opendaylight.yangtools.yang.common.RpcResultBuilder;
 import org.opendaylight.yangtools.yang.data.api.ModifyAction;
 import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier;
 import org.opendaylight.yangtools.yang.data.api.schema.DataContainerChild;
@@ -34,6 +41,7 @@ public abstract class AbstractWriteTx implements DOMDataWriteTransaction {
     protected final RemoteDeviceId id;
     protected final NetconfBaseOps netOps;
     protected final boolean rollbackSupport;
+    protected final List<ListenableFuture<DOMRpcResult>> resultsFutures;
     // Allow commit to be called only once
     protected boolean finished = false;
 
@@ -41,6 +49,7 @@ public abstract class AbstractWriteTx implements DOMDataWriteTransaction {
         this.netOps = netOps;
         this.id = id;
         this.rollbackSupport = rollbackSupport;
+        this.resultsFutures = Lists.newArrayList();
         init();
     }
 
@@ -90,8 +99,6 @@ public abstract class AbstractWriteTx implements DOMDataWriteTransaction {
         editConfig(path, Optional.fromNullable(data), editStructure, Optional.of(ModifyAction.NONE), "put");
     }
 
-    protected abstract void handleEditException(YangInstanceIdentifier path, NormalizedNode<?, ?> data, NetconfDocumentedException e, String editType);
-
     @Override
     public synchronized void merge(final LogicalDatastoreType store, final YangInstanceIdentifier path, final NormalizedNode<?, ?> data) {
         checkEditable(store);
@@ -138,4 +145,36 @@ public abstract class AbstractWriteTx implements DOMDataWriteTransaction {
     }
 
     protected abstract void editConfig(final YangInstanceIdentifier path, final Optional<NormalizedNode<?, ?>> data, final DataContainerChild<?, ?> editStructure, final Optional<ModifyAction> defaultOperation, final String operation);
+
+    protected ListenableFuture<RpcResult<TransactionStatus>> resultsToTxStatus() {
+        final SettableFuture<RpcResult<TransactionStatus>> transformed = SettableFuture.create();
+
+        Futures.addCallback(Futures.allAsList(resultsFutures), new FutureCallback<List<DOMRpcResult>>() {
+            @Override
+            public void onSuccess(final List<DOMRpcResult> domRpcResults) {
+                domRpcResults.forEach(domRpcResult -> {
+                    if(!domRpcResult.getErrors().isEmpty() && !transformed.isDone()) {
+                        NetconfDocumentedException exception =
+                                new NetconfDocumentedException(id + ":RPC during tx failed",
+                                        DocumentedException.ErrorType.application,
+                                        DocumentedException.ErrorTag.operation_failed,
+                                        DocumentedException.ErrorSeverity.error);
+                        transformed.setException(exception);
+                    }
+                });
+
+                if(!transformed.isDone()) {
+                    transformed.set(RpcResultBuilder.success(TransactionStatus.COMMITED).build());
+                }
+            }
+
+            @Override
+            public void onFailure(Throwable throwable) {
+                // TODO should we wrap throwable in NetconfDocumentedException
+                transformed.setException(throwable);
+            }
+        });
+
+        return transformed;
+    }
 }

diff --git a/netconf/sal-netconf-connector/src/main/java/org/opendaylight/netconf/sal/connect/netconf/sal/tx/WriteCandidateRunningTx.java b/netconf/sal-netconf-connector/src/main/java/org/opendaylight/netconf/sal/connect/netconf/sal/tx/WriteCandidateRunningTx.java
index a693e8c59c3db5a07fc0538a675e72f550582b86..40a43c1cbe6eb61e1b2873e87cb3dc7117520566 100644 (file)
--- a/netconf/sal-netconf-connector/src/main/java/org/opendaylight/netconf/sal/connect/netconf/sal/tx/WriteCandidateRunningTx.java
+++ b/netconf/sal-netconf-connector/src/main/java/org/opendaylight/netconf/sal/connect/netconf/sal/tx/WriteCandidateRunningTx.java
@@ -23,6 +23,7 @@ import org.slf4j.LoggerFactory;
  *     <li>Running datastore is locked as the first thing and this lock has to succeed</li>
  * </ul>
  */
+//TODO replace custom RPCs future callbacks with NetconfRpcFutureCallback
 public class WriteCandidateRunningTx extends WriteCandidateTx {
 
     private static final Logger LOG  = LoggerFactory.getLogger(WriteCandidateRunningTx.class);
@@ -59,10 +60,9 @@ public class WriteCandidateRunningTx extends WriteCandidateTx {
             @Override
             public void onFailure(Throwable t) {
                 LOG.warn("{}: Failed to lock running. Failed to initialize transaction", id, t);
-                throw new RuntimeException(id + ": Failed to lock running. Failed to initialize transaction", t);
             }
         };
-        netOps.lockRunning(lockRunningCallback);
+        resultsFutures.add(netOps.lockRunning(lockRunningCallback));
     }
 
     /**

diff --git a/netconf/sal-netconf-connector/src/main/java/org/opendaylight/netconf/sal/connect/netconf/sal/tx/WriteCandidateTx.java b/netconf/sal-netconf-connector/src/main/java/org/opendaylight/netconf/sal/connect/netconf/sal/tx/WriteCandidateTx.java
index 4001c3186b385d16b376421256c9ac66c3dba224..489cb7bad1069a862631ce60c7e937a4a563dab8 100644 (file)
--- a/netconf/sal-netconf-connector/src/main/java/org/opendaylight/netconf/sal/connect/netconf/sal/tx/WriteCandidateTx.java
+++ b/netconf/sal-netconf-connector/src/main/java/org/opendaylight/netconf/sal/connect/netconf/sal/tx/WriteCandidateTx.java
@@ -15,6 +15,7 @@ import com.google.common.util.concurrent.CheckedFuture;
 import com.google.common.util.concurrent.FutureCallback;
 import com.google.common.util.concurrent.Futures;
 import com.google.common.util.concurrent.ListenableFuture;
+import javax.annotation.Nullable;
 import org.opendaylight.controller.md.sal.common.api.TransactionStatus;
 import org.opendaylight.controller.md.sal.common.api.data.TransactionCommitFailedException;
 import org.opendaylight.controller.md.sal.dom.api.DOMRpcResult;
@@ -52,6 +53,7 @@ import org.slf4j.LoggerFactory;
  *   <li>Commit and Unlock candidate datastore async</li>
  * </ol>
  */
+//TODO replace custom RPCs future callbacks with NetconfRpcFutureCallback
 public class WriteCandidateTx extends AbstractWriteTx {
 
     private static final Logger LOG  = LoggerFactory.getLogger(WriteCandidateTx.class);
@@ -104,7 +106,7 @@ public class WriteCandidateTx extends AbstractWriteTx {
                 throw new RuntimeException(e);
             }
         };
-        netOps.lockCandidate(lockCandidateCallback);
+        resultsFutures.add(netOps.lockCandidate(lockCandidateCallback));
     }
 
     @Override
@@ -113,13 +115,6 @@ public class WriteCandidateTx extends AbstractWriteTx {
         cleanupOnSuccess();
     }
 
-    @Override
-    protected void handleEditException(final YangInstanceIdentifier path, final NormalizedNode<?, ?> data, final NetconfDocumentedException e, final String editType) {
-        LOG.warn("{}: Error {} data to (candidate){}, data: {}, canceling", id, editType, path, data, e);
-        cancel();
-        throw new RuntimeException(id + ": Error while " + editType + ": (candidate)" + path, e);
-    }
-
     @Override
     public synchronized CheckedFuture<Void, TransactionCommitFailedException> submit() {
         final ListenableFuture<Void> commitFutureAsVoid = Futures.transform(commit(), new Function<RpcResult<TransactionStatus>, Void>() {
@@ -147,28 +142,24 @@ public class WriteCandidateTx extends AbstractWriteTx {
 
     @Override
     public synchronized ListenableFuture<RpcResult<TransactionStatus>> performCommit() {
-        final ListenableFuture<DOMRpcResult> rpcResult = netOps.commit(new NetconfRpcFutureCallback("Commit", id) {
-            @Override
-            public void onSuccess(final DOMRpcResult result) {
-                super.onSuccess(result);
-                LOG.debug("{}: Write successful, transaction: {}. Unlocking", id, getIdentifier());
-                cleanupOnSuccess();
-            }
+        resultsFutures.add(netOps.commit(new NetconfRpcFutureCallback("Commit", id)));
+        ListenableFuture<RpcResult<TransactionStatus>> txResult = resultsToTxStatus();
 
+        Futures.addCallback(txResult, new FutureCallback<RpcResult<TransactionStatus>>() {
             @Override
-            protected void onUnsuccess(final DOMRpcResult result) {
-                LOG.error("{}: Write failed, transaction {}, discarding changes, unlocking: {}", id, getIdentifier(), result.getErrors());
-                cleanup();
+            public void onSuccess(@Nullable RpcResult<TransactionStatus> result) {
+                cleanupOnSuccess();
             }
 
             @Override
-            public void onFailure(final Throwable t) {
-                LOG.error("{}: Write failed, transaction {}, discarding changes, unlocking", id, getIdentifier(), t);
+            public void onFailure(Throwable t) {
+                // TODO If lock is cause of this failure cleanup will issue warning log
+                // cleanup is trying to do unlock, but this will fail
                 cleanup();
             }
         });
 
-        return Futures.transform(rpcResult, RPC_RESULT_TO_TX_STATUS);
+        return txResult;
     }
 
     protected void cleanupOnSuccess() {
@@ -196,15 +187,13 @@ public class WriteCandidateTx extends AbstractWriteTx {
             @Override
             public void onFailure(Throwable t) {
                 LOG.warn("Edit candidate operation failed. {}", t);
-                NetconfDocumentedException e = new NetconfDocumentedException(id + ": Edit candidate operation failed.", NetconfDocumentedException.ErrorType.application,
-                        NetconfDocumentedException.ErrorTag.operation_failed, NetconfDocumentedException.ErrorSeverity.warning);
-                handleEditException(path, data.orNull(), e, operation);
             }
         };
         if (defaultOperation.isPresent()) {
-            netOps.editConfigCandidate(editConfigCallback, editStructure, defaultOperation.get(), rollbackSupport);
+            resultsFutures.add(netOps.editConfigCandidate(
+                    editConfigCallback, editStructure, defaultOperation.get(), rollbackSupport));
         } else {
-            netOps.editConfigCandidate(editConfigCallback, editStructure, rollbackSupport);
+            resultsFutures.add(netOps.editConfigCandidate(editConfigCallback, editStructure, rollbackSupport));
         }
     }
 

diff --git a/netconf/sal-netconf-connector/src/main/java/org/opendaylight/netconf/sal/connect/netconf/sal/tx/WriteRunningTx.java b/netconf/sal-netconf-connector/src/main/java/org/opendaylight/netconf/sal/connect/netconf/sal/tx/WriteRunningTx.java
index 4b3ee7de9f735d41d29a5f15f9c85e4581273262..c249bf7c4ad63c819c6a9b4dbddf79cd7222cea5 100644 (file)
--- a/netconf/sal-netconf-connector/src/main/java/org/opendaylight/netconf/sal/connect/netconf/sal/tx/WriteRunningTx.java
+++ b/netconf/sal-netconf-connector/src/main/java/org/opendaylight/netconf/sal/connect/netconf/sal/tx/WriteRunningTx.java
@@ -17,12 +17,10 @@ import com.google.common.util.concurrent.ListenableFuture;
 import org.opendaylight.controller.md.sal.common.api.TransactionStatus;
 import org.opendaylight.controller.md.sal.common.api.data.TransactionCommitFailedException;
 import org.opendaylight.controller.md.sal.dom.api.DOMRpcResult;
-import org.opendaylight.netconf.api.NetconfDocumentedException;
 import org.opendaylight.netconf.sal.connect.netconf.util.NetconfBaseOps;
 import org.opendaylight.netconf.sal.connect.netconf.util.NetconfRpcFutureCallback;
 import org.opendaylight.netconf.sal.connect.util.RemoteDeviceId;
 import org.opendaylight.yangtools.yang.common.RpcResult;
-import org.opendaylight.yangtools.yang.common.RpcResultBuilder;
 import org.opendaylight.yangtools.yang.data.api.ModifyAction;
 import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier;
 import org.opendaylight.yangtools.yang.data.api.schema.DataContainerChild;
@@ -47,6 +45,7 @@ import org.slf4j.LoggerFactory;
  *   <li>Unlock running datastore on tx commit</li>
  * </ol>
  */
+//TODO replace custom RPCs future callbacks with NetconfRpcFutureCallback
 public class WriteRunningTx extends AbstractWriteTx {
 
     private static final Logger LOG  = LoggerFactory.getLogger(WriteRunningTx.class);
@@ -76,11 +75,10 @@ public class WriteRunningTx extends AbstractWriteTx {
 
             @Override
             public void onFailure(Throwable t) {
-                LOG.warn("Lock running operation failed. {}", t);
-                throw new RuntimeException(id + ": Failed to lock running datastore", t);
+                LOG.warn("{}: Lock running operation failed. {}", id, t);
             }
         };
-        netOps.lockRunning(lockCallback);
+        resultsFutures.add(netOps.lockRunning(lockCallback));
     }
 
     @Override
@@ -88,13 +86,6 @@ public class WriteRunningTx extends AbstractWriteTx {
         unlock();
     }
 
-    @Override
-    protected void handleEditException(final YangInstanceIdentifier path, final NormalizedNode<?, ?> data, final NetconfDocumentedException e, final String editType) {
-        LOG.warn("{}: Error {} data to (running){}, data: {}, canceling", id, editType, path, data, e);
-        cancel();
-        throw new RuntimeException(id + ": Error while " + editType + ": (running)" + path, e);
-    }
-
     @Override
     public synchronized CheckedFuture<Void, TransactionCommitFailedException> submit() {
         final ListenableFuture<Void> commmitFutureAsVoid = Futures.transform(commit(), new Function<RpcResult<TransactionStatus>, Void>() {
@@ -115,7 +106,8 @@ public class WriteRunningTx extends AbstractWriteTx {
     @Override
     public synchronized ListenableFuture<RpcResult<TransactionStatus>> performCommit() {
         unlock();
-        return Futures.immediateFuture(RpcResultBuilder.success(TransactionStatus.COMMITED).build());
+
+        return resultsToTxStatus();
     }
 
     @Override
@@ -138,16 +130,14 @@ public class WriteRunningTx extends AbstractWriteTx {
 
             @Override
             public void onFailure(Throwable t) {
-                LOG.warn("Edit running operation failed. {}", t);
-                NetconfDocumentedException e = new NetconfDocumentedException(id + ": Edit running operation failed.", NetconfDocumentedException.ErrorType.application,
-                        NetconfDocumentedException.ErrorTag.operation_failed, NetconfDocumentedException.ErrorSeverity.warning);
-                handleEditException(path, data.orNull(), e, operation);
+                LOG.warn("{}: Error {} data to (running){}, data: {}", id, operation, path, data.orNull(), t);
             }
         };
         if (defaultOperation.isPresent()) {
-            netOps.editConfigRunning(editConfigCallback, editStructure, defaultOperation.get(), rollbackSupport);
+            resultsFutures.add(
+                    netOps.editConfigRunning(editConfigCallback, editStructure, defaultOperation.get(), rollbackSupport));
         } else {
-            netOps.editConfigRunning(editConfigCallback, editStructure, rollbackSupport);
+            resultsFutures.add(netOps.editConfigRunning(editConfigCallback, editStructure, rollbackSupport));
         }
     }
 

diff --git a/netconf/sal-netconf-connector/src/test/java/org/opendaylight/netconf/sal/connect/netconf/sal/tx/NetconfDeviceWriteOnlyTxTest.java b/netconf/sal-netconf-connector/src/test/java/org/opendaylight/netconf/sal/connect/netconf/sal/tx/NetconfDeviceWriteOnlyTxTest.java
index 18169510972b611b85b92c7a64bb66ab62175482..3efa83719f3caa3ec787c3ee76f1303669259e28 100644 (file)
--- a/netconf/sal-netconf-connector/src/test/java/org/opendaylight/netconf/sal/connect/netconf/sal/tx/NetconfDeviceWriteOnlyTxTest.java
+++ b/netconf/sal-netconf-connector/src/test/java/org/opendaylight/netconf/sal/connect/netconf/sal/tx/NetconfDeviceWriteOnlyTxTest.java
@@ -132,6 +132,7 @@ public class NetconfDeviceWriteOnlyTxTest {
                 false);
 
         tx.delete(LogicalDatastoreType.CONFIGURATION, yangIId);
+        tx.submit();
         // verify discard changes was sent
         final InOrder inOrder = inOrder(rpc);
         inOrder.verify(rpc).invokeRpc(toPath(NetconfMessageTransformUtil.NETCONF_LOCK_QNAME), NetconfBaseOps.getLockContent(NETCONF_RUNNING_QNAME));