Refactor shutdown-impl: add parameter to RPC, remove secret validation and masking. 75/4275/1
authorTomas Olvecky <tolvecky@cisco.com>
Wed, 15 Jan 2014 16:56:49 +0000 (17:56 +0100)
committerTomas Olvecky <tolvecky@cisco.com>
Wed, 15 Jan 2014 17:04:35 +0000 (18:04 +0100)
Remove old secret as it does not work well with config-persister.
Add parameter maxWaitTime to shutdown RPC that specifies amount of time before
server is forcibly shut down.
Work around buggy yuma message sending xml element without inner text
node if blank value is specified.

Change-Id: Ief3fea74107349c5ff425ffdc184346981254bc4
Signed-off-by: Tomas Olvecky <tolvecky@cisco.com>
12 files changed:
opendaylight/config/config-api/src/main/java/org/opendaylight/controller/config/api/ValidationException.java
opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/BlankTransactionServiceTracker.java
opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/ConfigManagerActivator.java
opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/ExtenderBundleTracker.java
opendaylight/config/shutdown-api/src/main/java/org/opendaylight/controller/config/shutdown/ShutdownService.java
opendaylight/config/shutdown-impl/src/main/java/org/opendaylight/controller/config/yang/shutdown/impl/ShutdownModule.java
opendaylight/config/shutdown-impl/src/main/java/org/opendaylight/controller/config/yang/shutdown/impl/ShutdownModuleFactory.java
opendaylight/config/shutdown-impl/src/main/java/org/opendaylight/controller/config/yang/shutdown/impl/ShutdownServiceImpl.java
opendaylight/config/shutdown-impl/src/main/yang/shutdown-impl.yang
opendaylight/config/shutdown-impl/src/test/java/org/opendaylight/controller/config/yang/shutdown/impl/ShutdownTest.java
opendaylight/netconf/config-netconf-connector/src/main/java/org/opendaylight/controller/netconf/confignetconfconnector/mapping/attributes/fromxml/SimpleAttributeReadingStrategy.java
opendaylight/netconf/netconf-util/src/main/java/org/opendaylight/controller/netconf/util/xml/XmlElement.java

index da54a83..f27d123 100644 (file)
@@ -140,8 +140,7 @@ public class ValidationException extends RuntimeException {
 
         @Override
         public String toString() {
-            return "ExceptionMessageWithStackTrace [message=" + message
-                    + ", stackTrace=" + stackTrace + "]";
+            return message;
         }
 
     }
index ec1a290..b973b92 100644 (file)
@@ -38,7 +38,7 @@ public class BlankTransactionServiceTracker implements ServiceTrackerCustomizer<
         return null;
     }
 
-    private synchronized void blankTransaction() {
+    synchronized void blankTransaction() {
         // race condition check: config-persister might push new configuration while server is starting up.
         ConflictingVersionException lastException = null;
         for (int i = 0; i < 10; i++) {
index 1ee6cca..be602e5 100644 (file)
@@ -46,11 +46,11 @@ public class ConfigManagerActivator implements BundleActivator {
         configRegistryJMXRegistrator.registerToJMX(configRegistry);
 
         // track bundles containing factories
-        extenderBundleTracker = new ExtenderBundleTracker(context);
+        BlankTransactionServiceTracker blankTransactionServiceTracker = new BlankTransactionServiceTracker(configRegistry);
+        extenderBundleTracker = new ExtenderBundleTracker(context, blankTransactionServiceTracker);
         extenderBundleTracker.open();
 
-        BlankTransactionServiceTracker customizer = new BlankTransactionServiceTracker(configRegistry);
-        ServiceTracker<?, ?> serviceTracker = new ServiceTracker(context, ModuleFactory.class, customizer);
+        ServiceTracker<?, ?> serviceTracker = new ServiceTracker(context, ModuleFactory.class, blankTransactionServiceTracker);
         serviceTracker.open();
     }
 
index 6663cdd..b55f313 100644 (file)
@@ -34,11 +34,12 @@ import org.slf4j.LoggerFactory;
  * Code based on http://www.toedter.com/blog/?p=236
  */
 public class ExtenderBundleTracker extends BundleTracker<Object> {
-
+    private final BlankTransactionServiceTracker blankTransactionServiceTracker;
     private static final Logger logger = LoggerFactory.getLogger(ExtenderBundleTracker.class);
 
-    public ExtenderBundleTracker(BundleContext context) {
+    public ExtenderBundleTracker(BundleContext context, BlankTransactionServiceTracker blankTransactionServiceTracker) {
         super(context, Bundle.ACTIVE, null);
+        this.blankTransactionServiceTracker = blankTransactionServiceTracker;
         logger.trace("Registered as extender with context {}", context);
     }
 
@@ -64,6 +65,8 @@ public class ExtenderBundleTracker extends BundleTracker<Object> {
     @Override
     public void removedBundle(Bundle bundle, BundleEvent event, Object object) {
         super.removedBundle(bundle,event,object);
+        // workaround for service tracker not getting removed service event
+        blankTransactionServiceTracker.blankTransaction();
     }
 
     // TODO:test
index 1042417..5cbe493 100644 (file)
@@ -17,5 +17,5 @@ public interface ShutdownService {
      * @param inputSecret must match configured secret of the implementation
      * @param reason Optional string to be logged while shutting down
      */
-    void shutdown(String inputSecret, Optional<String> reason);
+    void shutdown(String inputSecret, Long maxWaitTime, Optional<String> reason);
 }
index f6937f9..d58ebf2 100644 (file)
@@ -16,13 +16,11 @@ import org.osgi.framework.Bundle;
 
 public final class ShutdownModule extends AbstractShutdownModule {
     private final Bundle systemBundle;
-    private final ShutdownModule nullableOldModule;
 
     public ShutdownModule(ModuleIdentifier identifier, Bundle systemBundle) {
         super(identifier, null);
         singletonCheck(identifier);
         this.systemBundle = systemBundle;
-        this.nullableOldModule = null;
     }
 
     public ShutdownModule(ModuleIdentifier identifier, ShutdownModule oldModule, java.lang.AutoCloseable oldInstance,
@@ -30,7 +28,6 @@ public final class ShutdownModule extends AbstractShutdownModule {
         super(identifier, null, oldModule, oldInstance);
         singletonCheck(identifier);
         this.systemBundle = systemBundle;
-        this.nullableOldModule = oldModule;
     }
 
     private static void singletonCheck(ModuleIdentifier identifier) {
@@ -52,40 +49,13 @@ public final class ShutdownModule extends AbstractShutdownModule {
         throw new UnsupportedOperationException();
     }
 
-    @Override
-    public String getSecret() {
-        throw new UnsupportedOperationException();
-    }
-
-    @Override
-    public String getOldSecret() {
-        throw new UnsupportedOperationException();
-    }
-
-    String getActualSecret() {
-        return super.getSecret();
-    }
-
-    String getActualOldSecret() {
-        return super.getOldSecret();
-    }
-
     @Override
     protected void customValidation() {
-        JmxAttributeValidationException.checkNotNull(super.getOldSecret(), oldSecretJmxAttribute);
         JmxAttributeValidationException.checkNotNull(super.getSecret(), secretJmxAttribute);
-        if (nullableOldModule != null) {
-            // if nothing changed, remain valid
-            boolean sameAsOldModule = isSame(nullableOldModule);
-            if (sameAsOldModule == false) {
-                boolean valid = getActualOldSecret().equals(nullableOldModule.getActualSecret());
-                JmxAttributeValidationException.checkCondition(valid, "Invalid old secret", oldSecretJmxAttribute);
-            }
-        }
     }
 
     @Override
     public java.lang.AutoCloseable createInstance() {
-        return new ShutdownServiceImpl(getActualSecret(), systemBundle, getRootRuntimeBeanRegistratorWrapper());
+        return new ShutdownServiceImpl(getSecret(), systemBundle, getRootRuntimeBeanRegistratorWrapper());
     }
 }
index 637395b..0517faf 100644 (file)
 package org.opendaylight.controller.config.yang.shutdown.impl;
 
 import org.opendaylight.controller.config.api.DependencyResolver;
+import org.opendaylight.controller.config.api.DependencyResolverFactory;
 import org.opendaylight.controller.config.api.ModuleIdentifier;
 import org.osgi.framework.Bundle;
 import org.osgi.framework.BundleContext;
 
-public class ShutdownModuleFactory extends AbstractShutdownModuleFactory {
-
-    @Override
-    public org.opendaylight.controller.config.spi.Module createModule(String instanceName, org.opendaylight.controller.config.api.DependencyResolver dependencyResolver, org.opendaylight.controller.config.api.DynamicMBeanWithInstance old, org.osgi.framework.BundleContext bundleContext) throws Exception {
-        org.opendaylight.controller.config.yang.shutdown.impl.ShutdownModule oldModule = null;
-        try {
-            oldModule = (org.opendaylight.controller.config.yang.shutdown.impl.ShutdownModule) old.getModule();
-        } catch(Exception e) {
-            return handleChangedClass(old);
-        }
-        org.opendaylight.controller.config.yang.shutdown.impl.ShutdownModule module = instantiateModule(instanceName, dependencyResolver, oldModule, old.getInstance(), bundleContext);
-
-        module.setOldSecret(oldModule.getActualOldSecret());
-        module.setSecret(oldModule.getActualSecret());
-
-        return module;
-    }
+import java.util.Arrays;
+import java.util.Set;
 
+public class ShutdownModuleFactory extends AbstractShutdownModuleFactory {
 
     public ShutdownModule instantiateModule(String instanceName, DependencyResolver dependencyResolver,
                                             ShutdownModule oldModule, AutoCloseable oldInstance,
@@ -46,4 +33,12 @@ public class ShutdownModuleFactory extends AbstractShutdownModuleFactory {
         Bundle systemBundle = bundleContext.getBundle(0);
         return new ShutdownModule(new ModuleIdentifier(NAME, instanceName), systemBundle);
     }
+
+    @Override
+    public Set<ShutdownModule> getDefaultModules(DependencyResolverFactory dependencyResolverFactory, BundleContext bundleContext) {
+        ModuleIdentifier id = new ModuleIdentifier(NAME, NAME);
+        DependencyResolver dependencyResolver = dependencyResolverFactory.createDependencyResolver(id);
+        ShutdownModule shutdownModule = instantiateModule(NAME, dependencyResolver, bundleContext);
+        return new java.util.HashSet<>(Arrays.asList(shutdownModule));
+    }
 }
index e5b95c8..6cdcf60 100644 (file)
@@ -14,6 +14,9 @@ import org.osgi.framework.BundleException;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import java.lang.management.ManagementFactory;
+import java.lang.management.ThreadInfo;
+
 public class ShutdownServiceImpl implements ShutdownService, AutoCloseable {
     private final ShutdownService impl;
     private final ShutdownRuntimeRegistration registration;
@@ -28,8 +31,8 @@ public class ShutdownServiceImpl implements ShutdownService, AutoCloseable {
     }
 
     @Override
-    public void shutdown(String inputSecret, Optional<String> reason) {
-        impl.shutdown(inputSecret, reason);
+    public void shutdown(String inputSecret, Long maxWaitTime, Optional<String> reason) {
+        impl.shutdown(inputSecret, maxWaitTime, reason);
     }
 
     @Override
@@ -49,7 +52,7 @@ class Impl implements ShutdownService {
     }
 
     @Override
-    public void shutdown(String inputSecret, Optional<String> reason) {
+    public void shutdown(String inputSecret, Long maxWaitTime, Optional<String> reason) {
         logger.warn("Shutdown issued with secret {} and reason {}", inputSecret, reason);
         try {
             Thread.sleep(1000); // prevent brute force attack
@@ -60,22 +63,15 @@ class Impl implements ShutdownService {
         if (this.secret.equals(inputSecret)) {
             logger.info("Server is shutting down");
 
-            Thread stopSystemBundle = new Thread() {
-                @Override
-                public void run() {
-                    try {
-                        // wait so that JMX response is received
-                        Thread.sleep(1000);
-                        systemBundle.stop();
-                    } catch (BundleException e) {
-                        logger.warn("Can not stop OSGi server", e);
-                    } catch (InterruptedException e) {
-                        logger.warn("Shutdown process interrupted", e);
-                    }
-                }
-            };
-            stopSystemBundle.start();
-
+            // actual work:
+            Thread stopSystemBundleThread = new StopSystemBundleThread(systemBundle);
+            stopSystemBundleThread.start();
+            if (maxWaitTime != null && maxWaitTime > 0) {
+                Thread systemExitThread = new CallSystemExitThread(maxWaitTime);
+                logger.debug("Scheduling {}", systemExitThread);
+                systemExitThread.start();
+            }
+            // end
         } else {
             logger.warn("Unauthorized attempt to shut down server");
             throw new IllegalArgumentException("Invalid secret");
@@ -84,6 +80,70 @@ class Impl implements ShutdownService {
 
 }
 
+class StopSystemBundleThread extends Thread {
+    private static final Logger logger = LoggerFactory.getLogger(StopSystemBundleThread.class);
+    private final Bundle systemBundle;
+
+    StopSystemBundleThread(Bundle systemBundle) {
+        super("stop-system-bundle");
+        this.systemBundle = systemBundle;
+    }
+
+    @Override
+    public void run() {
+        try {
+            // wait so that JMX response is received
+            Thread.sleep(1000);
+            systemBundle.stop();
+        } catch (BundleException e) {
+            logger.warn("Can not stop OSGi server", e);
+        } catch (InterruptedException e) {
+            logger.warn("Shutdown process interrupted", e);
+        }
+    }
+}
+
+class CallSystemExitThread extends Thread {
+    private static final Logger logger = LoggerFactory.getLogger(CallSystemExitThread.class);
+    private final long maxWaitTime;
+    CallSystemExitThread(long maxWaitTime) {
+        super("call-system-exit-daemon");
+        setDaemon(true);
+        if (maxWaitTime <= 0){
+            throw new IllegalArgumentException("Cannot schedule to zero or negative time:" + maxWaitTime);
+        }
+        this.maxWaitTime = maxWaitTime;
+    }
+
+    @Override
+    public String toString() {
+        return "CallSystemExitThread{" +
+                "maxWaitTime=" + maxWaitTime +
+                '}';
+    }
+
+    @Override
+    public void run() {
+        try {
+            // wait specified time
+            Thread.sleep(maxWaitTime);
+            logger.error("Since some threads are still running, server is going to shut down via System.exit(1) !");
+            // do a thread dump
+            ThreadInfo[] threads = ManagementFactory.getThreadMXBean().dumpAllThreads(true, true);
+            StringBuffer sb = new StringBuffer();
+            for(ThreadInfo info : threads) {
+                sb.append(info);
+                sb.append("\n");
+            }
+            logger.warn("Thread dump:{}", sb);
+            System.exit(1);
+        } catch (InterruptedException e) {
+            logger.info("Interrupted, not going to call System.exit(1)");
+        }
+    }
+}
+
+
 class MXBeanImpl implements ShutdownRuntimeMXBean {
     private final ShutdownService impl;
 
@@ -92,13 +152,13 @@ class MXBeanImpl implements ShutdownRuntimeMXBean {
     }
 
     @Override
-    public void shutdown(String inputSecret, String nullableReason) {
+    public void shutdown(String inputSecret, Long maxWaitTime, String nullableReason) {
         Optional<String> optionalReason;
         if (nullableReason == null) {
             optionalReason = Optional.absent();
         } else {
             optionalReason = Optional.of(nullableReason);
         }
-        impl.shutdown(inputSecret, optionalReason);
+        impl.shutdown(inputSecret, maxWaitTime, optionalReason);
     }
 }
index 78b44ab..883735c 100644 (file)
@@ -37,10 +37,6 @@ module shutdown-impl {
                 type string;
                 default "";
             }
-            leaf old-secret {
-                type string;
-                default "";
-            }
         }
     }
 
@@ -63,6 +59,10 @@ module shutdown-impl {
             leaf input-secret {
                 type string;
             }
+            leaf max-wait-time {
+                type uint32;
+                description "Maximum time in milliseconds before process is forcibly exited. Zero or null cancels this functionality.";
+            }
             leaf reason {
                 type string;
             }
index 86cd6fa..5887e98 100644 (file)
@@ -11,8 +11,6 @@ import org.junit.Before;
 import org.junit.Test;
 import org.mockito.Mock;
 import org.mockito.MockitoAnnotations;
-import org.opendaylight.controller.config.api.ValidationException;
-import org.opendaylight.controller.config.api.ValidationException.ExceptionMessageWithStackTrace;
 import org.opendaylight.controller.config.api.jmx.ObjectNameUtil;
 import org.opendaylight.controller.config.manager.impl.AbstractConfigTest;
 import org.opendaylight.controller.config.manager.impl.factoriesresolver.HardcodedModuleFactoriesResolver;
@@ -23,11 +21,8 @@ import org.osgi.framework.Bundle;
 import javax.management.JMX;
 import javax.management.ObjectName;
 import java.util.Collections;
-import java.util.Map;
 
 import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 import static org.mockito.Mockito.doNothing;
 import static org.mockito.Mockito.doReturn;
@@ -49,6 +44,9 @@ public class ShutdownTest extends AbstractConfigTest {
         doReturn(mockedSysBundle).when(mockedContext).getBundle(0);
         mockedContext.getBundle(0);
         doNothing().when(mockedSysBundle).stop();
+        ConfigTransactionJMXClient transaction = configRegistryClient.createTransaction();
+        // initialize default instance
+        transaction.commit();
     }
 
     @Test
@@ -62,21 +60,20 @@ public class ShutdownTest extends AbstractConfigTest {
         }
     }
 
+    private static final ObjectName runtimeON = ObjectNameUtil.createRuntimeBeanName(NAME, NAME, Collections.<String, String>emptyMap());
+
     @Test
     public void testWithoutSecret() throws Exception {
-        ConfigTransactionJMXClient transaction = configRegistryClient.createTransaction();
-        transaction.createModule(NAME, NAME);
-        transaction.commit();
         // test JMX rpc
-        ObjectName runtimeON = ObjectNameUtil.createRuntimeBeanName(NAME, NAME, Collections.<String, String>emptyMap());
+
         ShutdownRuntimeMXBean runtime = configRegistryClient.newMXBeanProxy(runtimeON, ShutdownRuntimeMXBean.class);
         try {
-            runtime.shutdown("foo", null);
+            runtime.shutdown("foo", 60000L, null);
             fail();
         } catch (IllegalArgumentException e) {
             assertEquals("Invalid secret", e.getMessage());
         }
-        runtime.shutdown("", null);
+        runtime.shutdown("", 60000L, null);
         assertStopped();
     }
 
@@ -84,57 +81,31 @@ public class ShutdownTest extends AbstractConfigTest {
     @Test
     public void testWithSecret() throws Exception {
         ConfigTransactionJMXClient transaction = configRegistryClient.createTransaction();
-        ObjectName on = transaction.createModule(NAME, NAME);
+        ObjectName on = transaction.lookupConfigBean(NAME, NAME);
         ShutdownModuleMXBean proxy = transaction.newMXBeanProxy(on, ShutdownModuleMXBean.class);
         String secret = "secret";
         proxy.setSecret(secret);
         transaction.commit();
         shutdownViaRuntimeJMX(secret);
-
-        // test old secret
-        transaction = configRegistryClient.createTransaction();
-        on = transaction.lookupConfigBean(NAME, NAME);
-        proxy = transaction.newMXBeanProxy(on, ShutdownModuleMXBean.class);
-        try {
-            rethrowCause(proxy).getOldSecret();
-            fail();
-        } catch (UnsupportedOperationException e) {
-        }
         try {
-            rethrowCause(proxy).getSecret();
+            ShutdownRuntimeMXBean runtime = JMX.newMXBeanProxy(platformMBeanServer, runtimeON, ShutdownRuntimeMXBean.class);
+            runtime.shutdown("foo", 60000L, null);
             fail();
-        } catch (UnsupportedOperationException e) {
-        }
-        // set secret to nothing
-        String newSecret = "newSecret";
-        proxy.setSecret(newSecret);
-        try {
-            transaction.commit();
-            fail("Old secret not provided - should fail validation");
-        } catch (ValidationException e) {
-            Map<String, Map<String, ExceptionMessageWithStackTrace>> failedValidations = e.getFailedValidations();
-            assertTrue(failedValidations.containsKey(NAME));
-            ExceptionMessageWithStackTrace exceptionMessageWithStackTrace = failedValidations.get(NAME).get(NAME);
-            assertNotNull(exceptionMessageWithStackTrace);
-            assertEquals("OldSecret Invalid old secret", exceptionMessageWithStackTrace.getMessage());
-
+        } catch (IllegalArgumentException e) {
+            assertEquals("Invalid secret", e.getMessage());
         }
-        proxy.setOldSecret(secret);
-        transaction.commit();
-        shutdownViaRuntimeJMX(newSecret);
     }
 
     private void shutdownViaRuntimeJMX(String secret) throws Exception {
         // test JMX rpc
-        ObjectName runtimeON = ObjectNameUtil.createRuntimeBeanName(NAME, NAME, Collections.<String, String>emptyMap());
         ShutdownRuntimeMXBean runtime = JMX.newMXBeanProxy(platformMBeanServer, runtimeON, ShutdownRuntimeMXBean.class);
         try {
-            runtime.shutdown("", null);
+            runtime.shutdown("", 60000L, null);
             fail();
         } catch (IllegalArgumentException e) {
             assertEquals("Invalid secret", e.getMessage());
         }
-        runtime.shutdown(secret, null);
+        runtime.shutdown(secret, 60000L, null);
         assertStopped();
     }
 
index a860524..625e4ab 100644 (file)
@@ -10,10 +10,14 @@ package org.opendaylight.controller.netconf.confignetconfconnector.mapping.attri
 
 import com.google.common.base.Preconditions;
 import org.opendaylight.controller.netconf.util.xml.XmlElement;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 import java.util.List;
 
 public class SimpleAttributeReadingStrategy extends AbstractAttributeReadingStrategy {
+    private static final Logger logger = LoggerFactory.getLogger(SimpleAttributeReadingStrategy.class);
+
 
     public SimpleAttributeReadingStrategy(String nullableDefault) {
         super(nullableDefault);
@@ -25,7 +29,13 @@ public class SimpleAttributeReadingStrategy extends AbstractAttributeReadingStra
         Preconditions.checkState(configNodes.size() == 1, "This element should be present only once " + xmlElement
                 + " but was " + configNodes.size());
 
-        String textContent = xmlElement.getTextContent();
+        String textContent = "";
+        try{
+            textContent = xmlElement.getTextContent();
+        }catch(IllegalStateException | NullPointerException e) {
+            // yuma sends <attribute /> for empty value instead of <attribute></attribute>
+            logger.warn("Ignoring exception caused by failure to read text element", e);
+        }
 
         Preconditions.checkNotNull(textContent, "This element should contain text %s", xmlElement);
         return AttributeConfigElement.create(postprocessNullableDefault(getNullableDefault()),
index 212214c..18a94c6 100644 (file)
@@ -272,7 +272,9 @@ public class XmlElement {
 
     public String getTextContent() {
         Node textChild = element.getFirstChild();
-        Preconditions.checkState(textChild instanceof Text, getName() + " should contain text");
+        Preconditions.checkNotNull(textChild, "Child node expected, got null for " + getName() + " : " + element);
+        Preconditions.checkState(textChild instanceof Text, getName() + " should contain text." +
+                Text.class.getName() + " expected, got " + textChild);
         String content = textChild.getTextContent();
         // Trim needed
         return content.trim();