Resolve Bug:654 - Fix config-persister exception handling.

Modify NetconfUtil to fail on any <rpc-error>, not detecting transaction conflicts. Move
exception handling to ConfigPusher.
Also fix bug when config-netconf-connector detects discrepancy between yang store and
config manager factories: persister will retry as if some capabilities were not found.

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

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 6dba9ac64e22e1e5d5f528aa6b8c830aa78d9f17..ed5a3a97a4c67809e75857ba77409eb695e00535 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
@@ -110,6 +110,10 @@ public class ConfigPusher {
     }
 
     private static class NotEnoughCapabilitiesException extends Exception {
     }
 
     private static class NotEnoughCapabilitiesException extends Exception {

+        private NotEnoughCapabilitiesException(String message, Throwable cause) {
+            super(message, cause);
+        }
+

         private NotEnoughCapabilitiesException(String message) {
             super(message);
         }
         private NotEnoughCapabilitiesException(String message) {
             super(message);
         }


@@ -123,13 +127,18 @@ public class ConfigPusher {
      * @return service if capabilities are present, otherwise absent value
      */
     private NetconfOperationService getOperationService(Set<String> expectedCapabilities, String idForReporting) throws NotEnoughCapabilitiesException {
      * @return service if capabilities are present, otherwise absent value
      */
     private NetconfOperationService getOperationService(Set<String> expectedCapabilities, String idForReporting) throws NotEnoughCapabilitiesException {

-        NetconfOperationService serviceCandidate = configNetconfConnector.createService(idForReporting);
+        NetconfOperationService serviceCandidate;
+        try {
+            serviceCandidate = configNetconfConnector.createService(idForReporting);
+        } catch(RuntimeException e) {
+            throw new NotEnoughCapabilitiesException("Netconf service not stable for " + idForReporting, e);
+        }

         Set<String> notFoundDiff = computeNotFoundCapabilities(expectedCapabilities, serviceCandidate);
         if (notFoundDiff.isEmpty()) {
             return serviceCandidate;
         } else {
             serviceCandidate.close();
         Set<String> notFoundDiff = computeNotFoundCapabilities(expectedCapabilities, serviceCandidate);
         if (notFoundDiff.isEmpty()) {
             return serviceCandidate;
         } else {
             serviceCandidate.close();

-            logger.debug("Netconf server did not provide required capabilities for {} " +
+            logger.trace("Netconf server did not provide required capabilities for {} " +

                             "Expected but not found: {}, all expected {}, current {}",
                     idForReporting, notFoundDiff, expectedCapabilities, serviceCandidate.getCapabilities()
             );
                             "Expected but not found: {}, all expected {}, current {}",
                     idForReporting, notFoundDiff, expectedCapabilities, serviceCandidate.getCapabilities()
             );


@@ -226,15 +235,13 @@ public class ConfigPusher {
         try {
             response = operation.handle(request.getDocument(), NetconfOperationChainedExecution.EXECUTION_TERMINATION_POINT);
         } catch (NetconfDocumentedException | RuntimeException e) {
         try {
             response = operation.handle(request.getDocument(), NetconfOperationChainedExecution.EXECUTION_TERMINATION_POINT);
         } catch (NetconfDocumentedException | RuntimeException e) {

+            if (e instanceof NetconfDocumentedException && e.getCause() instanceof ConflictingVersionException) {
+                throw (ConflictingVersionException) e.getCause();
+            }

             throw new IllegalStateException("Failed to send " + operationNameForReporting +
                     " for configuration " + configIdForReporting, e);
         }
             throw new IllegalStateException("Failed to send " + operationNameForReporting +
                     " for configuration " + configIdForReporting, e);
         }

-        try {
-            return NetconfUtil.checkIsMessageOk(response);
-        } catch (ConflictingVersionException e) {
-            logger.trace("conflicting version detected: {} while committing {}", e.toString(), configIdForReporting);
-            throw e;
-        }
+        return NetconfUtil.checkIsMessageOk(response);

     }
 
     // load editConfig.xml template, populate /rpc/edit-config/config with parameter
     }
 
     // load editConfig.xml template, populate /rpc/edit-config/config with parameter

diff --git a/opendaylight/netconf/config-persister-impl/src/test/java/org/opendaylight/controller/netconf/persist/impl/osgi/ConfigPersisterTest.java b/opendaylight/netconf/config-persister-impl/src/test/java/org/opendaylight/controller/netconf/persist/impl/osgi/ConfigPersisterTest.java
index b722496142e4f2014b82e776317488e02ddb4527..493ecd9250262959e78523c4ed1dc49c7d177474 100644 (file)
--- a/opendaylight/netconf/config-persister-impl/src/test/java/org/opendaylight/controller/netconf/persist/impl/osgi/ConfigPersisterTest.java
+++ b/opendaylight/netconf/config-persister-impl/src/test/java/org/opendaylight/controller/netconf/persist/impl/osgi/ConfigPersisterTest.java
@@ -33,6 +33,7 @@ import static org.mockito.Matchers.any;
 import static org.mockito.Matchers.anyString;
 import static org.mockito.Mockito.doNothing;
 import static org.mockito.Mockito.doReturn;
 import static org.mockito.Matchers.anyString;
 import static org.mockito.Mockito.doNothing;
 import static org.mockito.Mockito.doReturn;

+import static org.mockito.Mockito.doThrow;

 import static org.mockito.Mockito.mock;
 
 public class ConfigPersisterTest {
 import static org.mockito.Mockito.mock;
 
 public class ConfigPersisterTest {


@@ -113,26 +114,29 @@ public class ConfigPersisterTest {
     @Test
     public void testPersisterConflictingVersionException() throws Exception {
         setUpContextAndStartPersister("cap1");
     @Test
     public void testPersisterConflictingVersionException() throws Exception {
         setUpContextAndStartPersister("cap1");

-        NetconfOperationService service = getWorkingService(getConflictVersionDocument());
-        doReturn(service).when(ctx.serviceFactory).createService(anyString());
+
+        doReturn(getConflictingService()).when(ctx.serviceFactory).createService(anyString());

         Thread.sleep(2000);
         handler.assertException(IllegalStateException.class, "Max wait for conflicting version stabilization timeout");
     }
 
         Thread.sleep(2000);
         handler.assertException(IllegalStateException.class, "Max wait for conflicting version stabilization timeout");
     }
 

-    private Document getConflictVersionDocument() throws SAXException, IOException {
-        return XmlUtil.readXmlToDocument(
-                "<rpc-reply message-id=\"1\" xmlns=\"urn:ietf:params:xml:ns:netconf:base:1.0\">\n" +
-                        "<rpc-error><error-info><error>" +
-                        ConflictingVersionException.class.getCanonicalName() +
-                        "</error></error-info></rpc-error>\n" +
-                        "</rpc-reply>"
-        );
+    private NetconfOperationService getConflictingService() throws Exception {
+        NetconfOperationService service =  getWorkingService(getOKDocument());
+        ConflictingVersionException cve = new ConflictingVersionException("");
+        try {
+            NetconfDocumentedException.wrap(cve);
+            throw new AssertionError("Should throw an exception");
+        }catch(NetconfDocumentedException e) {
+            NetconfOperation mockedOperation = service.getNetconfOperations().iterator().next();
+            doThrow(e).when(mockedOperation).handle(any(Document.class), any(NetconfOperationChainedExecution.class));
+            return service;
+        }

     }
 
     @Test
     public void testSuccessConflictingVersionException() throws Exception {
         setUpContextAndStartPersister("cap1");
     }
 
     @Test
     public void testSuccessConflictingVersionException() throws Exception {
         setUpContextAndStartPersister("cap1");

-        doReturn(getWorkingService(getConflictVersionDocument())).when(ctx.serviceFactory).createService(anyString());
+        doReturn(getConflictingService()).when(ctx.serviceFactory).createService(anyString());

         Thread.sleep(500);
         // working service:
         logger.info("Switching to working service **");
         Thread.sleep(500);
         // working service:
         logger.info("Switching to working service **");

diff --git a/opendaylight/netconf/netconf-util/src/main/java/org/opendaylight/controller/netconf/util/NetconfUtil.java b/opendaylight/netconf/netconf-util/src/main/java/org/opendaylight/controller/netconf/util/NetconfUtil.java
index b0884ca2fb4bc648183841241967660c3f9fc9f3..29f40ce6242e7fcada70574b774a56ff1a5fab99 100644 (file)
--- a/opendaylight/netconf/netconf-util/src/main/java/org/opendaylight/controller/netconf/util/NetconfUtil.java
+++ b/opendaylight/netconf/netconf-util/src/main/java/org/opendaylight/controller/netconf/util/NetconfUtil.java
@@ -8,9 +8,7 @@
 package org.opendaylight.controller.netconf.util;
 
 import com.google.common.base.Preconditions;
 package org.opendaylight.controller.netconf.util;
 
 import com.google.common.base.Preconditions;

-import org.opendaylight.controller.config.api.ConflictingVersionException;

 import org.opendaylight.controller.netconf.api.NetconfMessage;
 import org.opendaylight.controller.netconf.api.NetconfMessage;

-import org.opendaylight.controller.netconf.util.xml.XMLNetconfUtil;

 import org.opendaylight.controller.netconf.util.xml.XmlElement;
 import org.opendaylight.controller.netconf.util.xml.XmlNetconfConstants;
 import org.opendaylight.controller.netconf.util.xml.XmlUtil;
 import org.opendaylight.controller.netconf.util.xml.XmlElement;
 import org.opendaylight.controller.netconf.util.xml.XmlNetconfConstants;
 import org.opendaylight.controller.netconf.util.xml.XmlUtil;


@@ -19,8 +17,6 @@ import org.slf4j.LoggerFactory;
 import org.w3c.dom.Document;
 import org.xml.sax.SAXException;
 
 import org.w3c.dom.Document;
 import org.xml.sax.SAXException;
 

-import javax.xml.xpath.XPathConstants;
-import javax.xml.xpath.XPathExpression;

 import java.io.File;
 import java.io.FileInputStream;
 import java.io.FileNotFoundException;
 import java.io.File;
 import java.io.FileInputStream;
 import java.io.FileNotFoundException;


@@ -56,31 +52,17 @@ public final class NetconfUtil {
         return (doc == null) ? null : new NetconfMessage(doc);
     }
 
         return (doc == null) ? null : new NetconfMessage(doc);
     }
 

-    public static Document checkIsMessageOk(NetconfMessage responseMessage) throws ConflictingVersionException {
+    public static Document checkIsMessageOk(NetconfMessage responseMessage) {

         return checkIsMessageOk(responseMessage.getDocument());
     }
 
         return checkIsMessageOk(responseMessage.getDocument());
     }
 

-    public static Document checkIsMessageOk(Document response) throws ConflictingVersionException {
+    public static Document checkIsMessageOk(Document response) {

         XmlElement element = XmlElement.fromDomDocument(response);
         Preconditions.checkState(element.getName().equals(XmlNetconfConstants.RPC_REPLY_KEY));
         element = element.getOnlyChildElement();
         XmlElement element = XmlElement.fromDomDocument(response);
         Preconditions.checkState(element.getName().equals(XmlNetconfConstants.RPC_REPLY_KEY));
         element = element.getOnlyChildElement();

-

         if (element.getName().equals(XmlNetconfConstants.OK)) {
             return response;
         }
         if (element.getName().equals(XmlNetconfConstants.OK)) {
             return response;
         }

-
-        if (element.getName().equals(XmlNetconfConstants.RPC_ERROR)) {
-            logger.warn("Can not load last configuration, operation failed");
-            // is it ConflictingVersionException ?
-            XPathExpression xPathExpression = XMLNetconfUtil.compileXPath("/netconf:rpc-reply/netconf:rpc-error/netconf:error-info/netconf:error");
-            String error = (String) XmlUtil.evaluateXPath(xPathExpression, element.getDomElement(), XPathConstants.STRING);
-            if (error!=null && error.contains(ConflictingVersionException.class.getCanonicalName())) {
-                throw new ConflictingVersionException(error);
-            }
-            throw new IllegalStateException("Can not load last configuration, operation failed: "
-                    + XmlUtil.toString(response));
-        }
-

         logger.warn("Can not load last configuration. Operation failed.");
         throw new IllegalStateException("Can not load last configuration. Operation failed: "
                 + XmlUtil.toString(response));
         logger.warn("Can not load last configuration. Operation failed.");
         throw new IllegalStateException("Can not load last configuration. Operation failed: "
                 + XmlUtil.toString(response));

diff --git a/opendaylight/netconf/netconf-util/src/test/java/org/opendaylight/controller/netconf/util/NetconfUtilTest.java b/opendaylight/netconf/netconf-util/src/test/java/org/opendaylight/controller/netconf/util/NetconfUtilTest.java
index b4b8aba81fc0513e5d7debf0aca749f1a592129b..85eeb30a07339c3b379bd756328f7ed96926fbc2 100644 (file)
--- a/opendaylight/netconf/netconf-util/src/test/java/org/opendaylight/controller/netconf/util/NetconfUtilTest.java
+++ b/opendaylight/netconf/netconf-util/src/test/java/org/opendaylight/controller/netconf/util/NetconfUtilTest.java
@@ -8,7 +8,6 @@
 package org.opendaylight.controller.netconf.util;
 
 import org.junit.Test;
 package org.opendaylight.controller.netconf.util;
 
 import org.junit.Test;

-import org.opendaylight.controller.config.api.ConflictingVersionException;

 import org.opendaylight.controller.netconf.api.NetconfMessage;
 import org.opendaylight.controller.netconf.util.xml.XmlUtil;
 import org.w3c.dom.Document;
 import org.opendaylight.controller.netconf.api.NetconfMessage;
 import org.opendaylight.controller.netconf.util.xml.XmlUtil;
 import org.w3c.dom.Document;


@@ -25,7 +24,7 @@ public class NetconfUtilTest {
         try{
             NetconfUtil.checkIsMessageOk(new NetconfMessage(document));
             fail();
         try{
             NetconfUtil.checkIsMessageOk(new NetconfMessage(document));
             fail();

-        }catch(ConflictingVersionException e){
+        }catch(IllegalStateException e){

             assertThat(e.getMessage(), containsString("Optimistic lock failed. Expected parent version 21, was 18"));
         }
     }
             assertThat(e.getMessage(), containsString("Optimistic lock failed. Expected parent version 21, was 18"));
         }
     }