Fix config transaction handling in netconf.

When config manager fails a transaction because of optimistic lock failure, netconf
must clean up this transaction as well. Fix TransactionProvider to abort transaction
if commit fails. Only exception is ValidationException where user should
have an option to fix the configuration and commit again.

Turn off netconf client reuse in persister: this narrows the possibility of
hitting concurrent modification with other config transactions. Each retry
to push a snapshot is now done with a newly obtained client, so new transaction
will be started each time.

Change-Id: I4d49fea10a682e4d7cd85ab49f2b78be63a84c02
Signed-off-by: Tomas Olvecky <tolvecky@cisco.com>

diff --git a/opendaylight/netconf/config-netconf-connector/src/main/java/org/opendaylight/controller/netconf/confignetconfconnector/transactions/TransactionProvider.java b/opendaylight/netconf/config-netconf-connector/src/main/java/org/opendaylight/controller/netconf/confignetconfconnector/transactions/TransactionProvider.java
index 0fa005e04a7930df7b1e85fc3aa933da4db98ac4..cd3b44852c352cdf9ca905980790ae329e6143ba 100644 (file)
--- a/opendaylight/netconf/config-netconf-connector/src/main/java/org/opendaylight/controller/netconf/confignetconfconnector/transactions/TransactionProvider.java
+++ b/opendaylight/netconf/config-netconf-connector/src/main/java/org/opendaylight/controller/netconf/confignetconfconnector/transactions/TransactionProvider.java
@@ -95,15 +95,29 @@ public class TransactionProvider implements AutoCloseable {
      * Commit and notification send must be atomic
      */
     public synchronized CommitStatus commitTransaction() throws NetconfDocumentedException {
-        final Optional<ObjectName> taON = getTransaction();
-        Preconditions.checkState(taON.isPresent(), "No transaction found for session " + netconfSessionIdForReporting);
-        CommitStatus status = configRegistryClient.commitConfig(taON.get());
-        allOpenedTransactions.remove(transaction);
-        transaction = null;
-        return status;
+        final Optional<ObjectName> maybeTaON = getTransaction();
+        Preconditions.checkState(maybeTaON.isPresent(), "No transaction found for session " + netconfSessionIdForReporting);
+        ObjectName taON = maybeTaON.get();
+        try {
+            CommitStatus status = configRegistryClient.commitConfig(taON);
+            // clean up
+            allOpenedTransactions.remove(transaction);
+            transaction = null;
+            return status;
+        } catch (ValidationException validationException) {
+            // no clean up: user can reconfigure and recover this transaction
+            logger.warn("Transaction {} failed on {}", taON, validationException.toString());
+            throw validationException;
+        } catch (Exception e) {
+            logger.error("Exception while commit of {}, aborting transaction", taON, e);
+            // clean up
+            abortTransaction();
+            throw e;
+        }
     }
 
     public synchronized void abortTransaction() {
+        logger.debug("Aborting current transaction");
         Optional<ObjectName> taON = getTransaction();
         Preconditions.checkState(taON.isPresent(), "No transaction found for session " + netconfSessionIdForReporting);
 
@@ -114,6 +128,7 @@ public class TransactionProvider implements AutoCloseable {
     }
 
     public synchronized void abortTestTransaction(ObjectName testTx) {
+        logger.debug("Aborting transaction {}", testTx);
         ConfigTransactionClient transactionClient = configRegistryClient.getConfigTransactionClient(testTx);
         allOpenedTransactions.remove(testTx);
         transactionClient.abortConfig();

diff --git a/opendaylight/netconf/config-persister-impl/src/main/java/org/opendaylight/controller/netconf/persist/impl/ConfigPusher.java b/opendaylight/netconf/config-persister-impl/src/main/java/org/opendaylight/controller/netconf/persist/impl/ConfigPusher.java
index 6e62a982d1588217215f999011b6cfba4a443e46..3c968f8e2013423d4c405e1e95b8c4c711e5640e 100644 (file)
--- a/opendaylight/netconf/config-persister-impl/src/main/java/org/opendaylight/controller/netconf/persist/impl/ConfigPusher.java
+++ b/opendaylight/netconf/config-persister-impl/src/main/java/org/opendaylight/controller/netconf/persist/impl/ConfigPusher.java
@@ -72,7 +72,9 @@ public class ConfigPusher {
     }
 
     private synchronized NetconfClient pushAllConfigs(List<ConfigSnapshotHolder> configs) throws InterruptedException {
+        // first just make sure we can connect to netconf, even if nothing is being pushed
         NetconfClient netconfClient = makeNetconfConnection(Collections.<String>emptySet(), Optional.<NetconfClient>absent());
+        // start pushing snapshots:
         for (ConfigSnapshotHolder configSnapshotHolder: configs){
             netconfClient = pushSnapshotWithRetries(configSnapshotHolder, Optional.of(netconfClient));
         }
@@ -106,21 +108,16 @@ public class ConfigPusher {
     /**
      * @param expectedCaps capabilities that server hello must contain. Will retry until all are found or throws RuntimeException.
      *                     If empty set is provided, will only make sure netconf client successfuly connected to the server.
-     * @param oldClientForPossibleReuse if present, try to get expected capabilities from it before closing it and retrying with
-     *                                  new client connection.
+     * @param maybeOldClient if present, close it.
      * @return NetconfClient that has all required capabilities from server.
      */
     private synchronized NetconfClient makeNetconfConnection(Set<String> expectedCaps,
-                                                             Optional<NetconfClient> oldClientForPossibleReuse)
+                                                             Optional<NetconfClient> maybeOldClient)
             throws InterruptedException {
 
-        if (oldClientForPossibleReuse.isPresent()) {
-            NetconfClient oldClient = oldClientForPossibleReuse.get();
-            if (Util.isSubset(oldClient, expectedCaps)) {
-                return oldClient;
-            } else {
-                Util.closeClientAndDispatcher(oldClient);
-            }
+        if (maybeOldClient.isPresent()) {
+            NetconfClient oldClient = maybeOldClient.get();
+            Util.closeClientAndDispatcher(oldClient);
         }
 
         // TODO think about moving capability subset check to netconf client