Bug 4948: Catch RuntimeException from ensureParentByMerge

Made the calls to PUT and POST more robust. Caught the RuntimeException from
 the merge call in the EnsureParentsByMerge. If this fails then started
a new transaction and continue with the original PUT. This ensures when a device
is unable to handle the empty list/container merge it will still succeed with the
original request. Similar change has been made for POST.
This bug was fixed in stable Lithium. I am pushing the same fix on Beryllium.

Added FIXME statements to figure out and catch specific Runtime exceptions
thrown by Netconf instead of RuntimeException.

Change-Id: Id18a97857e46f44014dbd61cbfa86285457a2905
Signed-off-by: Balaji Varadaraju <bvaradar@brocade.com>
Signed-off-by: Ryan Goulding <ryandgoulding@gmail.com>

diff --git a/opendaylight/restconf/sal-rest-connector/src/main/java/org/opendaylight/netconf/sal/restconf/impl/BrokerFacade.java b/opendaylight/restconf/sal-rest-connector/src/main/java/org/opendaylight/netconf/sal/restconf/impl/BrokerFacade.java
index fcf82e5623c142bd41ab261e48ee44eda79e2514..0a5409cb10cde37af69f37fef0f08e450a5295a4 100644 (file)
--- a/opendaylight/restconf/sal-rest-connector/src/main/java/org/opendaylight/netconf/sal/restconf/impl/BrokerFacade.java
+++ b/opendaylight/restconf/sal-rest-connector/src/main/java/org/opendaylight/netconf/sal/restconf/impl/BrokerFacade.java
@@ -111,14 +111,14 @@ public class BrokerFacade {
     public CheckedFuture<Void, TransactionCommitFailedException> commitConfigurationDataPut(
             final SchemaContext globalSchema, final YangInstanceIdentifier path, final NormalizedNode<?, ?> payload) {
         checkPreconditions();
-        return putDataViaTransaction(domDataBroker.newReadWriteTransaction(), CONFIGURATION, path, payload, globalSchema);
+        return putDataViaTransaction(domDataBroker, CONFIGURATION, path, payload, globalSchema);
     }
 
     public CheckedFuture<Void, TransactionCommitFailedException> commitConfigurationDataPut(
             final DOMMountPoint mountPoint, final YangInstanceIdentifier path, final NormalizedNode<?, ?> payload) {
         final Optional<DOMDataBroker> domDataBrokerService = mountPoint.getService(DOMDataBroker.class);
         if (domDataBrokerService.isPresent()) {
-            return putDataViaTransaction(domDataBrokerService.get().newReadWriteTransaction(), CONFIGURATION, path,
+            return putDataViaTransaction(domDataBrokerService.get(), CONFIGURATION, path,
                     payload, mountPoint.getSchemaContext());
         }
         final String errMsg = "DOM data broker service isn't available for mount point " + path;
@@ -130,14 +130,14 @@ public class BrokerFacade {
     public CheckedFuture<Void, TransactionCommitFailedException> commitConfigurationDataPost(
             final SchemaContext globalSchema, final YangInstanceIdentifier path, final NormalizedNode<?, ?> payload) {
         checkPreconditions();
-        return postDataViaTransaction(domDataBroker.newReadWriteTransaction(), CONFIGURATION, path, payload, globalSchema);
+        return postDataViaTransaction(domDataBroker, CONFIGURATION, path, payload, globalSchema);
     }
 
     public CheckedFuture<Void, TransactionCommitFailedException> commitConfigurationDataPost(
             final DOMMountPoint mountPoint, final YangInstanceIdentifier path, final NormalizedNode<?, ?> payload) {
         final Optional<DOMDataBroker> domDataBrokerService = mountPoint.getService(DOMDataBroker.class);
         if (domDataBrokerService.isPresent()) {
-            return postDataViaTransaction(domDataBrokerService.get().newReadWriteTransaction(), CONFIGURATION, path,
+            return postDataViaTransaction(domDataBrokerService.get(), CONFIGURATION, path,
                     payload, mountPoint.getSchemaContext());
         }
         final String errMsg = "DOM data broker service isn't available for mount point " + path;
@@ -212,26 +212,41 @@ public class BrokerFacade {
     }
 
     private CheckedFuture<Void, TransactionCommitFailedException> postDataViaTransaction(
-            final DOMDataReadWriteTransaction rWTransaction, final LogicalDatastoreType datastore,
+            final DOMDataBroker domDataBroker, final LogicalDatastoreType datastore,
             final YangInstanceIdentifier path, final NormalizedNode<?, ?> payload, final SchemaContext schemaContext) {
         // FIXME: This is doing correct post for container and list children
         //        not sure if this will work for choice case
+        DOMDataReadWriteTransaction transaction = domDataBroker.newReadWriteTransaction();
         if(payload instanceof MapNode) {
             LOG.trace("POST " + datastore.name() + " via Restconf: {} with payload {}", path, payload);
             final NormalizedNode<?, ?> emptySubtree = ImmutableNodes.fromInstanceId(schemaContext, path);
-            rWTransaction.merge(datastore, YangInstanceIdentifier.create(emptySubtree.getIdentifier()), emptySubtree);
-            ensureParentsByMerge(datastore, path, rWTransaction, schemaContext);
+            try {
+                transaction.merge(datastore, YangInstanceIdentifier.create(emptySubtree.getIdentifier()), emptySubtree);
+            } catch (RuntimeException e) {
+                // FIXME: Figure out and catch specific RunTimeExceptions thrown by NETCONF instead of generic one.
+                //        to make this cleaner and easier to maintain.
+                transaction.cancel();
+                transaction = domDataBroker.newReadWriteTransaction();
+                LOG.debug("Empty subtree merge failed", e);
+            }
+            if (!ensureParentsByMerge(datastore, path, transaction, schemaContext)) {
+                transaction.cancel();
+                transaction = domDataBroker.newReadWriteTransaction();
+            }
             for(final MapEntryNode child : ((MapNode) payload).getValue()) {
                 final YangInstanceIdentifier childPath = path.node(child.getIdentifier());
-                checkItemDoesNotExists(rWTransaction, datastore, childPath);
-                rWTransaction.put(datastore, childPath, child);
+                checkItemDoesNotExists(transaction, datastore, childPath);
+                transaction.put(datastore, childPath, child);
             }
         } else {
-            checkItemDoesNotExists(rWTransaction,datastore, path);
-            ensureParentsByMerge(datastore, path, rWTransaction, schemaContext);
-            rWTransaction.put(datastore, path, payload);
+            checkItemDoesNotExists(transaction,datastore, path);
+            if(!ensureParentsByMerge(datastore, path, transaction, schemaContext)) {
+                transaction.cancel();
+                transaction = domDataBroker.newReadWriteTransaction();
+            }
+            transaction.put(datastore, path, payload);
         }
-        return rWTransaction.submit();
+        return transaction.submit();
     }
 
     private void checkItemDoesNotExists(final DOMDataReadWriteTransaction rWTransaction,final LogicalDatastoreType store, final YangInstanceIdentifier path) {
@@ -251,12 +266,17 @@ public class BrokerFacade {
     }
 
     private CheckedFuture<Void, TransactionCommitFailedException> putDataViaTransaction(
-            final DOMDataReadWriteTransaction writeTransaction, final LogicalDatastoreType datastore,
-            final YangInstanceIdentifier path, final NormalizedNode<?, ?> payload, final SchemaContext schemaContext) {
+            final DOMDataBroker domDataBroker, final LogicalDatastoreType datastore,
+            final YangInstanceIdentifier path, final NormalizedNode<?, ?> payload, final SchemaContext schemaContext)
+    {
+        DOMDataReadWriteTransaction transaction = domDataBroker.newReadWriteTransaction();
         LOG.trace("Put " + datastore.name() + " via Restconf: {} with payload {}", path, payload);
-        ensureParentsByMerge(datastore, path, writeTransaction, schemaContext);
-        writeTransaction.put(datastore, path, payload);
-        return writeTransaction.submit();
+        if (!ensureParentsByMerge(datastore, path, transaction, schemaContext)) {
+            transaction.cancel();
+            transaction = domDataBroker.newReadWriteTransaction();
+        }
+        transaction.put(datastore, path, payload);
+        return transaction.submit();
     }
 
     private CheckedFuture<Void, TransactionCommitFailedException> deleteDataViaTransaction(
@@ -271,8 +291,10 @@ public class BrokerFacade {
         this.domDataBroker = domDataBroker;
     }
 
-    private void ensureParentsByMerge(final LogicalDatastoreType store,
+    private boolean ensureParentsByMerge(final LogicalDatastoreType store,
                                       final YangInstanceIdentifier normalizedPath, final DOMDataReadWriteTransaction rwTx, final SchemaContext schemaContext) {
+
+        boolean mergeResult = true;
         final List<PathArgument> normalizedPathWithoutChildArgs = new ArrayList<>();
         YangInstanceIdentifier rootNormalizedPath = null;
 
@@ -292,13 +314,35 @@ public class BrokerFacade {
 
         // No parent structure involved, no need to ensure parents
         if(normalizedPathWithoutChildArgs.isEmpty()) {
-            return;
+            return mergeResult;
         }
 
         Preconditions.checkArgument(rootNormalizedPath != null, "Empty path received");
 
         final NormalizedNode<?, ?> parentStructure =
                 ImmutableNodes.fromInstanceId(schemaContext, YangInstanceIdentifier.create(normalizedPathWithoutChildArgs));
-        rwTx.merge(store, rootNormalizedPath, parentStructure);
+        try {
+            rwTx.merge(store, rootNormalizedPath, parentStructure);
+        } catch (RuntimeException e) {
+            /*
+             * Catching the exception here, logging it and proceeding further
+             * for the following reasons.
+             *
+             * 1. For MD-SAL store if it fails we'll go with the next call
+             * anyway and let the failure happen there. 2. For NETCONF devices
+             * that can not handle these calls such as creation of empty lists
+             * etc, instead of failing we'll go with the actual call. Devices
+             * should be able to handle the actual calls made without the need
+             * to create parents. So instead of failing we will give a device a
+             * chance to configure the management entity in question. 3. If this
+             * merge call is handled properly by MD-SAL data store or a Netconf
+             * device this is a no-op.
+             */
+             // FIXME: Figure out and catch specific RunTimeExceptions thrown by NETCONF instead of generic one.
+             //        to make this cleaner and easier to maintain.
+            mergeResult = false;
+            LOG.debug("Exception while creating the parent in ensureParentsByMerge. Proceeding with the actual request", e);
+        }
+        return mergeResult;
     }
 }