Resolve Bug:654 - Fix config-persister exception handling. 05/5905/3
authorTomas Olvecky <tolvecky@cisco.com>
Fri, 4 Apr 2014 10:29:19 +0000 (12:29 +0200)
committerTomas Olvecky <tolvecky@cisco.com>
Fri, 4 Apr 2014 14:37:43 +0000 (16:37 +0200)
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>
opendaylight/netconf/config-persister-impl/src/main/java/org/opendaylight/controller/netconf/persist/impl/ConfigPusher.java
opendaylight/netconf/config-persister-impl/src/test/java/org/opendaylight/controller/netconf/persist/impl/osgi/ConfigPersisterTest.java
opendaylight/netconf/netconf-util/src/main/java/org/opendaylight/controller/netconf/util/NetconfUtil.java
opendaylight/netconf/netconf-util/src/test/java/org/opendaylight/controller/netconf/util/NetconfUtilTest.java

index 6dba9ac..ed5a3a9 100644 (file)
@@ -110,6 +110,10 @@ public class ConfigPusher {
     }
 
     private static class NotEnoughCapabilitiesException extends Exception {
+        private NotEnoughCapabilitiesException(String message, Throwable cause) {
+            super(message, cause);
+        }
+
         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 {
-        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();
-            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()
             );
@@ -226,15 +235,13 @@ public class ConfigPusher {
         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);
         }
-        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
index b722496..493ecd9 100644 (file)
@@ -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.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
 
 public class ConfigPersisterTest {
@@ -113,26 +114,29 @@ public class ConfigPersisterTest {
     @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");
     }
 
-    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");
-        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 **");
index b0884ca..29f40ce 100644 (file)
@@ -8,9 +8,7 @@
 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.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;
@@ -19,8 +17,6 @@ import org.slf4j.LoggerFactory;
 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;
@@ -56,31 +52,17 @@ public final class NetconfUtil {
         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());
     }
 
-    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();
-
         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));
index b4b8aba..85eeb30 100644 (file)
@@ -8,7 +8,6 @@
 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;
@@ -25,7 +24,7 @@ public class NetconfUtilTest {
         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"));
         }
     }