Attachment 827463 Details for Bug 1030063 – Patch on c39bcd8571

[patch] Patch on c39bcd8571

0001-Bug-1030063-Plugin-update-clean-up-listener-nonsense.patch (text/plain), 19.64 KB, created by Elias Ross on 2013-11-21 22:07:03 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Elias Ross

Created: 2013-11-21 22:07:03 UTC

Size: 19.64 KB

patch

obsolete

>From 838b7085674bc21602fe4a2f9b1427274f1cd4cb Mon Sep 17 00:00:00 2001
>From: Elias Ross <elias_ross@apple.com>
>Date: Wed, 13 Nov 2013 10:29:14 -0800
>Subject: [PATCH] Bug 1030063 - Plugin update process clean up
>
>The plugin update is problematic as it is driven by listener ordering and
>possibly has many race conditions like seen in Bug 1025844. For example, the
>listener is actually added before the plugin container starts. Although
>effectively, the dependent systems are initialized before be the listener is
>called, this is not something predictable or clear.
>
>Removed the use of marker files and some unnecessary methods.
>---
> .../java/org/rhq/enterprise/agent/AgentMain.java   | 143 +++++++--------------
> .../org/rhq/enterprise/agent/PluginUpdate.java     |  95 ++------------
> .../agent/i18n/AgentI18NResourceKeys.java          |  10 +-
> 3 files changed, 54 insertions(+), 194 deletions(-)
>
>diff --git a/modules/enterprise/agent/src/main/java/org/rhq/enterprise/agent/AgentMain.java b/modules/enterprise/agent/src/main/java/org/rhq/enterprise/agent/AgentMain.java
>index 1e43996..0a916b7 100644
>--- a/modules/enterprise/agent/src/main/java/org/rhq/enterprise/agent/AgentMain.java
>+++ b/modules/enterprise/agent/src/main/java/org/rhq/enterprise/agent/AgentMain.java
>@@ -28,7 +28,6 @@
> import java.io.FileNotFoundException;
> import java.io.FileOutputStream;
> import java.io.FileWriter;
>-import java.io.FilenameFilter;
> import java.io.IOException;
> import java.io.InputStream;
> import java.io.InputStreamReader;
>@@ -67,11 +66,9 @@
> import org.apache.log4j.Level;
> import org.apache.log4j.LogManager;
> import org.apache.log4j.xml.DOMConfigurator;
>-
> import org.jboss.remoting.invocation.NameBasedInvocation;
> import org.jboss.remoting.security.SSLSocketBuilder;
> import org.jboss.remoting.transport.http.ssl.HTTPSClientInvoker;
>-
> import org.rhq.core.clientapi.server.bundle.BundleServerService;
> import org.rhq.core.clientapi.server.configuration.ConfigurationServerService;
> import org.rhq.core.clientapi.server.content.ContentServerService;
>@@ -393,6 +390,11 @@
>     private boolean m_loggedNativeSystemInfoUnavailableWarning;
>
>     /**
>+     * Plugin update instance, used by management.
>+     */
>+    private PluginUpdate m_pluginUpdate;
>+
>+    /**
>      * The main method that starts the whole thing.
>      *
>      * @param args the arguments passed on the command line (e.g. java org.rhq.enterprise.agent.AgentMain arg1 arg2 arg3)
>@@ -1706,11 +1708,6 @@ private boolean prepareStartupWorkRequiringServer() {
>             m_clientSender.addStateListener(new RegisterStateListener(), true);
>         }
>
>-        // now we want to prepare to update the plugins if told to do so
>-        if (m_configuration.isUpdatePluginsAtStartupEnabled()) {
>-            updatePlugins();
>-        }
>-
>         //the next thing is to setup the conditional restart of the PC if it fails to merge
>         //the upgrade results with the server due to some network glitch
>         m_clientSender.addStateListener(new PluginContainerConditionalRestartListener(), false);
>@@ -1719,12 +1716,20 @@ private boolean prepareStartupWorkRequiringServer() {
>     }
>
>     /**
>-     * This asks that the agent update its plugins. If the RHQ Server is already up and the agent has detected it, this
>-     * method will immediately pull down the new/updated plugins. Otherwise, this will schedule the agent to update the
>-     * plugins from the server once it comes up and the agent detects it.
>+     * Management method to manually update plugins.
>+     * This method will fail if the server is down.
>+     * @throws IllegalStateException if the container is not initialized
>+     * @throws RuntimeException for any other reason (failed to download, etc.)
>      */
>     public void updatePlugins() {
>-        m_clientSender.addStateListener(new UpdatePluginsStateListener(), true);
>+        if (m_pluginUpdate == null) {
>+            throw new IllegalStateException("plugin update uninitialized");
>+        }
>+        try {
>+            m_pluginUpdate.updatePlugins();
>+        } catch (Exception e) {
>+            throw new RuntimeException(e);
>+        }
>     }
>
>     /**
>@@ -1885,59 +1890,34 @@ private void rebootPluginContainer() throws Throwable {
>             return false;
>         }
>
>-        try {
>-            File plugin_dir = pc_config.getPluginDirectory();
>-            boolean keep_waiting = (plugin_dir.list().length == 0)
>-                || PluginUpdate.waitForUpdateToComplete(pc_config, 1000L);
>-
>-            // we block until we get our plugins - there is no sense continuing until we have plugins
>-            // there may be instances, though, where we don't want to block (in unit tests for example)
>-            // so allow this to be configurable via the "update plugins at startup" flag.
>-            if (m_configuration.isUpdatePluginsAtStartupEnabled()) {
>-                boolean notified_user = false;
>-
>-                while (keep_waiting) {
>-                    if (!notified_user) {
>-                        LOG.info(AgentI18NResourceKeys.WAITING_FOR_PLUGINS_WITH_DIR, plugin_dir);
>-                        getOut().println(MSG.getMsg(AgentI18NResourceKeys.WAITING_FOR_PLUGINS));
>-                        notified_user = true;
>-                    } else {
>-                        // let's keep logging this at debug level so we don't look hung
>-                        LOG.debug(AgentI18NResourceKeys.WAITING_FOR_PLUGINS_WITH_DIR, plugin_dir);
>-                    }
>-
>-                    boolean updating = PluginUpdate.waitForUpdateToComplete(pc_config, 30000L);
>-                    int after = plugin_dir.list().length;
>-
>-                    if ((after == 0) && !updating) {
>-                        // still nothing and it doesn't look like we are downloading - try to update them again right now
>-                        // (doing this because I saw a case where the startup update somehow happened just prior to the
>-                        // registration finishing, so the original update was rejected by the server as "unauthorized")
>-                        updatePluginsNow(m_clientSender);
>-                        after = plugin_dir.list().length;
>-                    }
>+        File plugin_dir = pc_config.getPluginDirectory();
>
>-                    keep_waiting = ((after == 0) || (updating));
>-
>-                    if (!keep_waiting) {
>-                        after = plugin_dir.list(new FilenameFilter() {
>-                            public boolean accept(File dir, String name) {
>-                                return name.endsWith(".jar");
>-                            }
>-                        }).length;
>-                        LOG.info(AgentI18NResourceKeys.DONE_WAITING_FOR_PLUGINS, after);
>-                        getOut().println(MSG.getMsg(AgentI18NResourceKeys.DONE_WAITING_FOR_PLUGINS, after));
>-                    }
>+        // we block until we get our plugins - there is no sense continuing until we have plugins
>+        // there may be instances, though, where we don't want to block (in unit tests for example)
>+        // so allow this to be configurable via the "update plugins at startup" flag.
>+        m_pluginUpdate = new PluginUpdate(pc_config.getServerServices().getCoreServerService(), pc_config);
>+        if (m_configuration.isUpdatePluginsAtStartupEnabled()) {
>+            boolean notified_user = false;
>+            // this can block forever...perhaps exit after a few tries?
>+            while (true) {
>+                if (!notified_user) {
>+                    LOG.info(AgentI18NResourceKeys.WAITING_FOR_PLUGINS_WITH_DIR, plugin_dir);
>+                    getOut().println(MSG.getMsg(AgentI18NResourceKeys.WAITING_FOR_PLUGINS));
>+                    notified_user = true;
>+                } else {
>+                    // let's keep logging this at debug level so we don't look hung
>+                    LOG.debug(AgentI18NResourceKeys.WAITING_FOR_PLUGINS_WITH_DIR, plugin_dir);
>+                }
>+                try {
>+                    m_pluginUpdate.updatePlugins();
>+                    break;
>+                } catch (Exception e) {
>+                    LOG.error(e, AgentI18NResourceKeys.UPDATING_PLUGINS_FAILURE, e);
>                 }
>-            } else if (plugin_dir.list().length == 0) {
>-                LOG.warn(AgentI18NResourceKeys.NO_PLUGINS);
>-                getOut().println(MSG.getMsg(AgentI18NResourceKeys.NO_PLUGINS));
>-                return false;
>             }
>-        } catch (InterruptedException e) {
>-            Thread.currentThread().interrupt();
>-            LOG.warn(AgentI18NResourceKeys.PLUGIN_CONTAINER_INITIALIZATION_INTERRUPTED);
>-            getOut().println(MSG.getMsg(AgentI18NResourceKeys.PLUGIN_CONTAINER_INITIALIZATION_INTERRUPTED));
>+        } else if (plugin_dir.list().length == 0) {
>+            LOG.warn(AgentI18NResourceKeys.NO_PLUGINS);
>+            getOut().println(MSG.getMsg(AgentI18NResourceKeys.NO_PLUGINS));
>             return false;
>         }
>
>@@ -3522,29 +3502,6 @@ private void setStarted(boolean started) {
>         m_startTime = (started) ? System.currentTimeMillis() : 0L;
>     }
>
>-    /**
>-     * Immediately sends a request to the server to update the plugins.
>-     *
>-     * @param  sender the sender used to comminucate with server
>-     *
>-     * @return <code>true</code> if the plugins were succesfully updated, <code>false</code> if an error occurred
>-     *
>-     * @see    PluginUpdate
>-     */
>-    private boolean updatePluginsNow(ClientCommandSender sender) {
>-        try {
>-            ClientRemotePojoFactory factory = sender.getClientRemotePojoFactory();
>-            CoreServerService server = factory.getRemotePojo(CoreServerService.class);
>-            PluginContainerConfiguration pc_config = m_configuration.getPluginContainerConfiguration();
>-            PluginUpdate plugin_update = new PluginUpdate(server, pc_config);
>-            plugin_update.updatePlugins();
>-            return true;
>-        } catch (Exception e) {
>-            LOG.warn(e, AgentI18NResourceKeys.UPDATING_PLUGINS_FAILURE);
>-            return false;
>-        }
>-    }
>-
>     private static void reconfigureJavaLogging() {
>         try {
>             LOG.debug(AgentI18NResourceKeys.RECONFIGURE_JAVA_LOGGING_START);
>@@ -3582,21 +3539,6 @@ public boolean stoppedSending(ClientCommandSender sender) {
>     }
>
>     /**
>-     * Listener that will update the plugins once the sender is able to start sending.
>-     */
>-    private class UpdatePluginsStateListener implements ClientCommandSenderStateListener {
>-        public boolean startedSending(ClientCommandSender sender) {
>-            updatePluginsNow(sender);
>-
>-            return false; // no need to keep listening
>-        }
>-
>-        public boolean stoppedSending(ClientCommandSender sender) {
>-            return true; // no-op but keep listening
>-        }
>-    }
>-
>-    /**
>      * Sender listener that will remove the command listener once the sender starts. It will also add the command
>      * listener once the sender stops. The command listener will allow us to immediately turn on the sender when the
>      * server sends us a message. We don't need this command listener once we know the sender has started (because that
>@@ -3814,4 +3756,5 @@ public void run() {
>             }
>         }
>     }
>+
> }
>diff --git a/modules/enterprise/agent/src/main/java/org/rhq/enterprise/agent/PluginUpdate.java b/modules/enterprise/agent/src/main/java/org/rhq/enterprise/agent/PluginUpdate.java
>index 8aa81a0..456f90f 100644
>--- a/modules/enterprise/agent/src/main/java/org/rhq/enterprise/agent/PluginUpdate.java
>+++ b/modules/enterprise/agent/src/main/java/org/rhq/enterprise/agent/PluginUpdate.java
>@@ -26,10 +26,9 @@
> import java.util.HashMap;
> import java.util.List;
> import java.util.Map;
>+import java.util.concurrent.Semaphore;
> import java.util.concurrent.TimeUnit;
> import java.util.concurrent.TimeoutException;
>-import java.util.concurrent.locks.ReadWriteLock;
>-import java.util.concurrent.locks.ReentrantReadWriteLock;
>
> import mazz.i18n.Logger;
>
>@@ -53,59 +52,18 @@
>  * @author Ian Springer
>  */
> public class PluginUpdate {
>-    private static final Logger LOG = AgentI18NFactory.getLogger(PluginUpdate.class);
>
>-    private static final String MARKER_FILENAME = ".updatelock";
>+    private static final Logger LOG = AgentI18NFactory.getLogger(PluginUpdate.class);
>
>     /**
>-     * Static lock that prohibits concurrent plugin updates.
>+     * Lock that prohibits concurrent plugin updates between threads.
>      */
>-    private static final ReadWriteLock lock = new ReentrantReadWriteLock();
>+    private static final Semaphore lock = new Semaphore(1);
>
>     private final CoreServerService coreServerService;
>     private final PluginContainerConfiguration config;
>
>     /**
>-     * All {@link PluginUpdate} objects know if they are currently updating plugins given a specific <code>
>-     * config</code>. Call this static method to ask if any plugin update object is currently updating plugins with the
>-     * given configuration
>-     *
>-     * @param  config used to determine where the plugins are being updated
>-     *
>-     * @return <code>true</code> if a plugin updater object is currently updating plugin; <code>false</code> if all
>-     *         plugins are up-to-date and nothing is being updated anymore.
>-     */
>-    public static boolean isCurrentlyUpdating(PluginContainerConfiguration config) {
>-        File marker = new File(config.getPluginDirectory(), MARKER_FILENAME);
>-        return marker.exists();
>-    }
>-
>-    /**
>-     * Blocks the calling thread for a maximum of the given amount of milliseconds timeout waiting for a plugin update
>-     * to completely. This will return sooner if the update finishes early or if there is no update currently happening.
>-     *
>-     * @param  config  used to determine where the plugins are being updated
>-     * @param  timeout max milliseconds to wait
>-     *
>-     * @return <code>true</code> if a plugin updater object is currently updating plugin and this method timed out;
>-     *         <code>false</code> if all plugins are up-to-date and nothing is being updated anymore.
>-     *
>-     * @throws InterruptedException if thread was interrupted while waiting
>-     */
>-    public static boolean waitForUpdateToComplete(PluginContainerConfiguration config, long timeout)
>-        throws InterruptedException {
>-        long time_limit = System.currentTimeMillis() + timeout;
>-        boolean currently_updating = true; // for us to sleep at least an initial amount before checking the first time
>-
>-        while (currently_updating && (time_limit > System.currentTimeMillis())) {
>-            Thread.sleep(2000L);
>-            currently_updating = isCurrentlyUpdating(config);
>-        }
>-
>-        return currently_updating;
>-    }
>-
>-    /**
>      * Constructor for {@link PluginUpdate}. You can pass in a <code>null</code> <code>core_server_service</code> if you
>      * only plan to use this object to obtain information on the currently installed plugins and not actually update
>      * them.
>@@ -151,13 +109,12 @@ public PluginContainerConfiguration getPluginContainerConfiguration() {
>         List<Plugin> updated_plugins = new ArrayList<Plugin>();
>
>         // block if some other thread is updating, too - we can only ever have one thread updating plugins
>-        if (!PluginUpdate.lock.writeLock().tryLock(3600, TimeUnit.SECONDS)) {
>+        if (!PluginUpdate.lock.tryAcquire(3600, TimeUnit.SECONDS)) {
>             // it should never take this long to update plugins. But if it does, just barf
>             throw new TimeoutException();
>         }
>
>         try {
>-            createMarkerFile();
>
>             try {
>                 List<String> disabled_plugin_names = this.config.getDisabledPlugins();
>@@ -237,15 +194,14 @@ public PluginContainerConfiguration getPluginContainerConfiguration() {
>                 if (last_error != null) {
>                     throw last_error;
>                 }
>-            } finally {
>-                deleteMarkerFile();
>+            } catch (Exception e) {
>+                throw e;
>             }
>         } finally {
>-            PluginUpdate.lock.writeLock().unlock();
>+            PluginUpdate.lock.release();
>         }
>
>         LOG.info(AgentI18NResourceKeys.UPDATING_PLUGINS_COMPLETE);
>-
>         return updated_plugins;
>     }
>
>@@ -408,39 +364,6 @@ private void getPluginArchive(Plugin plugin_to_get) throws Exception {
>         return plugins;
>     }
>
>-    private void createMarkerFile() {
>-        File marker = null;
>-        try {
>-            marker = new File(config.getPluginDirectory(), MARKER_FILENAME);
>-
>-            // shouldn't exist, but if it does, oh well, just reuse it
>-            if (!marker.exists()) {
>-                new FileOutputStream(marker).close();
>-            }
>-        } catch (Exception e) {
>-            LOG.warn(AgentI18NResourceKeys.UPDATING_PLUGINS_MARKER_CREATE_FAILURE, marker, e);
>-        }
>-
>-        return;
>-    }
>-
>-    private void deleteMarkerFile() {
>-        try {
>-            File marker = new File(config.getPluginDirectory(), MARKER_FILENAME);
>-
>-            // it should exist, but if it doesn't oh well, just skip trying to delete it
>-            if (marker.exists()) {
>-                if (!marker.delete()) {
>-                    LOG.warn(AgentI18NResourceKeys.UPDATING_PLUGINS_MARKER_DELETE_FAILURE, marker);
>-                }
>-            }
>-        } catch (Throwable t) {
>-            LOG.warn(AgentI18NResourceKeys.UPDATING_PLUGINS_MARKER_DELETE_FAILURE, MARKER_FILENAME);
>-        }
>-
>-        return;
>-    }
>-
>     private void deleteIllegitimatePlugins(Map<String, Plugin> current_plugins, Map<String, Plugin> latest_plugins_map) {
>         for (Plugin current_plugin : current_plugins.values()) {
>             if (!latest_plugins_map.containsKey(current_plugin.getPath())) {
>@@ -473,4 +396,4 @@ private File getPluginFile(Plugin plugin) {
>         File file = new File(plugin_dir, plugin_filename);
>         return file;
>     }
>-}
>\ No newline at end of file
>+}
>diff --git a/modules/enterprise/agent/src/main/java/org/rhq/enterprise/agent/i18n/AgentI18NResourceKeys.java b/modules/enterprise/agent/src/main/java/org/rhq/enterprise/agent/i18n/AgentI18NResourceKeys.java
>index 16c110a..26bc9c8 100644
>--- a/modules/enterprise/agent/src/main/java/org/rhq/enterprise/agent/i18n/AgentI18NResourceKeys.java
>+++ b/modules/enterprise/agent/src/main/java/org/rhq/enterprise/agent/i18n/AgentI18NResourceKeys.java
>@@ -356,12 +356,6 @@
>     @I18NMessage("Completed updating the plugins to their latest versions.")
>     String UPDATING_PLUGINS_COMPLETE = "PluginUpdate.updating-complete";
>
>-    @I18NMessage("Failed to create updater marker file [{0}] - will continue but agent startup may fail.  If so, restart agent. Cause. {1}")
>-    String UPDATING_PLUGINS_MARKER_CREATE_FAILURE = "PluginUpdate.marker-create-failure";
>-
>-    @I18NMessage("Failed to delete updater marker file [{0}] - will continue but agent startup may fail.  If so, delete the file manually.")
>-    String UPDATING_PLUGINS_MARKER_DELETE_FAILURE = "PluginUpdate.marker-delete-failure";
>-
>     @I18NMessage("The plugin [{0}] is current and does not need to be updated.")
>     String PLUGIN_ALREADY_AT_LATEST = "PluginUpdate.already-at-latest";
>
>@@ -1608,10 +1602,10 @@
>     @I18NMessage("The agent will now wait until it has registered with the server...")
>     String WAITING_TO_BE_REGISTERED_BEGIN = "AgentMain.waiting-to-be-registered-begin";
>
>-    @I18NMessage("The agent does not have plugins - it will now wait for them to be downloaded...")
>+    @I18NMessage("The agent is waiting for plugins to be downloaded...")
>     String WAITING_FOR_PLUGINS = "AgentMain.waiting-for-plugins";
>
>-    @I18NMessage("The agent does not have plugins - it will now wait for them to be downloaded to [{0}]...")
>+    @I18NMessage("The agent is waiting for plugins to be downloaded to [{0}]...")
>     String WAITING_FOR_PLUGINS_WITH_DIR = "AgentMain.waiting-for-plugins-with-dir";
>
>     @I18NMessage("[{0}] plugins downloaded.")
>--
>1.8.1.2
>

Actions: View | Diff

Attachments on bug 1030063: 827463