From 0d7444fae6e9873e7af11a762aeb59d833d18573 Mon Sep 17 00:00:00 2001 From: Tomas Olvecky Date: Fri, 29 Nov 2013 12:17:06 +0100 Subject: [PATCH] Fix race conditions between config-manager and persister. Config-manager commits a blank transaction each time new ModuleFactory is discovered. At the same time persister might try to push configuration. Add retries to both routines to overcome optimistic lock failure. Change-Id: Ia45505d285b2eb6cc7fa949289a3fbeb4c62f9e7 Signed-off-by: Tomas Olvecky --- .../osgi/BlankTransactionServiceTracker.java | 29 ++++++-- .../netconf/config-persister-impl/pom.xml | 3 + .../ConfigPersisterNotificationHandler.java | 68 ++++++++++++------- ...onfigPersisterNotificationHandlerTest.java | 34 ++++++++++ .../resources/conflictingVersionResponse.xml | 13 ++++ 5 files changed, 116 insertions(+), 31 deletions(-) create mode 100644 opendaylight/netconf/config-persister-impl/src/test/java/org/opendaylight/controller/netconf/persist/impl/ConfigPersisterNotificationHandlerTest.java create mode 100644 opendaylight/netconf/config-persister-impl/src/test/resources/conflictingVersionResponse.xml diff --git a/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/BlankTransactionServiceTracker.java b/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/BlankTransactionServiceTracker.java index de1a425ce6..3d0decb93d 100644 --- a/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/BlankTransactionServiceTracker.java +++ b/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/BlankTransactionServiceTracker.java @@ -7,6 +7,7 @@ */ package org.opendaylight.controller.config.manager.impl.osgi; +import org.opendaylight.controller.config.api.ConflictingVersionException; import org.opendaylight.controller.config.api.jmx.CommitStatus; import org.opendaylight.controller.config.manager.impl.ConfigRegistryImpl; import org.opendaylight.controller.config.spi.ModuleFactory; @@ -19,7 +20,7 @@ import javax.management.ObjectName; /** * Every time factory is added or removed, blank transaction is triggered to handle - * {@link org.opendaylight.controller.config.spi.ModuleFactory#getDefaultModules(org.opendaylight.controller.config.api.DependencyResolverFactory)} + * {@link org.opendaylight.controller.config.spi.ModuleFactory#getDefaultModules(org.opendaylight.controller.config.api.DependencyResolverFactory, org.osgi.framework.BundleContext)} * functionality. */ public class BlankTransactionServiceTracker implements ServiceTrackerCustomizer { @@ -38,14 +39,30 @@ public class BlankTransactionServiceTracker implements ServiceTrackerCustomizer< } private synchronized void blankTransaction() { - // create transaction - ObjectName tx = configRegistry.beginConfig(); - CommitStatus commitStatus = configRegistry.commitConfig(tx); - logger.debug("Committed blank transaction with status {}", commitStatus); + // race condition check: config-persister might push new configuration while server is starting up. + ConflictingVersionException lastException = null; + for (int i = 0; i < 10; i++) { + try { + // create transaction + ObjectName tx = configRegistry.beginConfig(); + CommitStatus commitStatus = configRegistry.commitConfig(tx); + logger.debug("Committed blank transaction with status {}", commitStatus); + return; + } catch (ConflictingVersionException e) { + lastException = e; + try { + Thread.sleep(1000); + } catch (InterruptedException interruptedException) { + Thread.currentThread().interrupt(); + throw new IllegalStateException(interruptedException); + } + } + } + throw lastException; } @Override - public void modifiedService(ServiceReference moduleFactoryServiceReference, Object o) { + public void modifiedService(ServiceReference moduleFactoryServiceReference, Object o) { blankTransaction(); } diff --git a/opendaylight/netconf/config-persister-impl/pom.xml b/opendaylight/netconf/config-persister-impl/pom.xml index ce25de215e..dce858fce6 100644 --- a/opendaylight/netconf/config-persister-impl/pom.xml +++ b/opendaylight/netconf/config-persister-impl/pom.xml @@ -89,6 +89,9 @@ org.slf4j, org.w3c.dom, org.xml.sax, + javax.xml.namespace, + javax.xml.xpath, + org.opendaylight.controller.config.api diff --git a/opendaylight/netconf/config-persister-impl/src/main/java/org/opendaylight/controller/netconf/persist/impl/ConfigPersisterNotificationHandler.java b/opendaylight/netconf/config-persister-impl/src/main/java/org/opendaylight/controller/netconf/persist/impl/ConfigPersisterNotificationHandler.java index a569f90538..a777ea4099 100644 --- a/opendaylight/netconf/config-persister-impl/src/main/java/org/opendaylight/controller/netconf/persist/impl/ConfigPersisterNotificationHandler.java +++ b/opendaylight/netconf/config-persister-impl/src/main/java/org/opendaylight/controller/netconf/persist/impl/ConfigPersisterNotificationHandler.java @@ -13,6 +13,7 @@ import com.google.common.base.Preconditions; import com.google.common.collect.Sets; import io.netty.channel.EventLoopGroup; import io.netty.channel.nio.NioEventLoopGroup; +import org.opendaylight.controller.config.api.ConflictingVersionException; import org.opendaylight.controller.config.persist.api.ConfigSnapshotHolder; import org.opendaylight.controller.config.persist.api.Persister; import org.opendaylight.controller.netconf.api.NetconfMessage; @@ -21,6 +22,7 @@ import org.opendaylight.controller.netconf.api.jmx.DefaultCommitOperationMXBean; import org.opendaylight.controller.netconf.api.jmx.NetconfJMXNotification; import org.opendaylight.controller.netconf.client.NetconfClient; import org.opendaylight.controller.netconf.client.NetconfClientDispatcher; +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; @@ -37,6 +39,8 @@ import javax.management.Notification; import javax.management.NotificationListener; import javax.management.ObjectName; import javax.net.ssl.SSLContext; +import javax.xml.xpath.XPathConstants; +import javax.xml.xpath.XPathExpression; import java.io.Closeable; import java.io.IOException; import java.io.InputStream; @@ -66,7 +70,7 @@ public class ConfigPersisterNotificationHandler implements NotificationListener, private final Persister persister; private final MBeanServerConnection mbeanServer; - private Long currentSessionId; + private final ObjectName on = DefaultCommitOperationMXBean.objectName; @@ -96,16 +100,25 @@ public class ConfigPersisterNotificationHandler implements NotificationListener, if (maybeConfig.isPresent()) { logger.debug("Last config found {}", persister); - - registerToNetconf(maybeConfig.get().getCapabilities()); - - final String configSnapshot = maybeConfig.get().getConfigSnapshot(); - logger.trace("Pushing following xml to netconf {}", configSnapshot); - try { - pushLastConfig(XmlUtil.readXmlToElement(configSnapshot)); - } catch (SAXException | IOException e) { - throw new IllegalStateException("Unable to load last config", e); + ConflictingVersionException lastException = null; + int maxAttempts = 30; + for(int i = 0 ; i < maxAttempts; i++) { + registerToNetconf(maybeConfig.get().getCapabilities()); + + final String configSnapshot = maybeConfig.get().getConfigSnapshot(); + logger.trace("Pushing following xml to netconf {}", configSnapshot); + try { + pushLastConfig(XmlUtil.readXmlToElement(configSnapshot)); + } catch(ConflictingVersionException e) { + closeClientAndDispatcher(netconfClient, netconfClientDispatcher); + lastException = e; + Thread.sleep(1000); + } catch (SAXException | IOException e) { + throw new IllegalStateException("Unable to load last config", e); + } } + throw new IllegalStateException("Failed to push configuration, maximum attempt count has been reached: " + + maxAttempts, lastException); } else { // this ensures that netconf is initialized, this is first @@ -130,9 +143,9 @@ public class ConfigPersisterNotificationHandler implements NotificationListener, int attempt = 0; - while (true) { + long deadline = pollingStart + timeout; + while (System.currentTimeMillis() < deadline) { attempt++; - netconfClientDispatcher = new NetconfClientDispatcher(Optional.absent(), nettyThreadgroup, nettyThreadgroup); try { netconfClient = new NetconfClient(this.toString(), address, delay, netconfClientDispatcher); @@ -146,14 +159,12 @@ public class ConfigPersisterNotificationHandler implements NotificationListener, if (isSubset(currentCapabilities, expectedCaps)) { logger.debug("Hello from netconf stable with {} capabilities", currentCapabilities); - currentSessionId = netconfClient.getSessionId(); + long currentSessionId = netconfClient.getSessionId(); logger.info("Session id received from netconf server: {}", currentSessionId); return currentSessionId; } - if (System.currentTimeMillis() > pollingStart + timeout) { - break; - } + logger.debug("Polling hello from netconf, attempt {}, capabilities {}", attempt, currentCapabilities); @@ -245,7 +256,7 @@ public class ConfigPersisterNotificationHandler implements NotificationListener, return maybeConfigElement; } - private synchronized void pushLastConfig(Element xmlToBePersisted) { + private synchronized void pushLastConfig(Element xmlToBePersisted) throws ConflictingVersionException { logger.info("Pushing last configuration to netconf"); StringBuilder response = new StringBuilder("editConfig response = {"); @@ -276,19 +287,26 @@ public class ConfigPersisterNotificationHandler implements NotificationListener, logger.trace("Detailed message {}", response); } - private void checkIsOk(XmlElement element, NetconfMessage responseMessage) { + static void checkIsOk(XmlElement element, NetconfMessage responseMessage) throws ConflictingVersionException { if (element.getName().equals(XmlNetconfConstants.OK)) { return; - } else { - if (element.getName().equals(XmlNetconfConstants.RPC_ERROR)) { - logger.warn("Can not load last configuration, operation failed"); - throw new IllegalStateException("Can not load last configuration, operation failed: " - + XmlUtil.toString(responseMessage.getDocument())); + } + + 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); } - logger.warn("Can not load last configuration. Operation failed."); - throw new IllegalStateException("Can not load last configuration. Operation failed: " + throw new IllegalStateException("Can not load last configuration, operation failed: " + XmlUtil.toString(responseMessage.getDocument())); } + + logger.warn("Can not load last configuration. Operation failed."); + throw new IllegalStateException("Can not load last configuration. Operation failed: " + + XmlUtil.toString(responseMessage.getDocument())); } private static NetconfMessage createEditConfigMessage(Element dataElement, String editConfigResourcename) { diff --git a/opendaylight/netconf/config-persister-impl/src/test/java/org/opendaylight/controller/netconf/persist/impl/ConfigPersisterNotificationHandlerTest.java b/opendaylight/netconf/config-persister-impl/src/test/java/org/opendaylight/controller/netconf/persist/impl/ConfigPersisterNotificationHandlerTest.java new file mode 100644 index 0000000000..6c45c9c011 --- /dev/null +++ b/opendaylight/netconf/config-persister-impl/src/test/java/org/opendaylight/controller/netconf/persist/impl/ConfigPersisterNotificationHandlerTest.java @@ -0,0 +1,34 @@ +/* + * Copyright (c) 2013 Cisco Systems, Inc. and others. All rights reserved. + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License v1.0 which accompanies this distribution, + * and is available at http://www.eclipse.org/legal/epl-v10.html + */ +package org.opendaylight.controller.netconf.persist.impl; + +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.XmlElement; +import org.opendaylight.controller.netconf.util.xml.XmlUtil; +import org.w3c.dom.Document; + +import static org.junit.Assert.assertThat; +import static org.junit.Assert.fail; +import static org.junit.matchers.JUnitMatchers.containsString; + +public class ConfigPersisterNotificationHandlerTest { + + @Test + public void testConflictingVersionDetection() throws Exception { + Document document = XmlUtil.readXmlToDocument(getClass().getResourceAsStream("/conflictingVersionResponse.xml")); + try{ + ConfigPersisterNotificationHandler.checkIsOk(XmlElement.fromDomDocument(document).getOnlyChildElement(), new NetconfMessage(document)); + fail(); + }catch(ConflictingVersionException e){ + assertThat(e.getMessage(), containsString("Optimistic lock failed. Expected parent version 21, was 18")); + } + } + +} diff --git a/opendaylight/netconf/config-persister-impl/src/test/resources/conflictingVersionResponse.xml b/opendaylight/netconf/config-persister-impl/src/test/resources/conflictingVersionResponse.xml new file mode 100644 index 0000000000..f65e0e0bc2 --- /dev/null +++ b/opendaylight/netconf/config-persister-impl/src/test/resources/conflictingVersionResponse.xml @@ -0,0 +1,13 @@ + + + application + operation-failed + error + + + + + org.opendaylight.controller.config.api.ConflictingVersionException: Optimistic lock failed. Expected parent version 21, was 18 + + + -- 2.36.6