Attachment 872924 Details for Bug 1073201 – Patch for master

[patch] Patch for master

0001-BZ-1073201-encountering-unknown-resources-causes-inv.patch (text/plain), 16.17 KB, created by Elias Ross on 2014-03-11 01:40:50 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Elias Ross

Created: 2014-03-11 01:40:50 UTC

Size: 16.17 KB

patch

obsolete

>From 9e325eb895e50512910726a40cf34ca5d6701382 Mon Sep 17 00:00:00 2001
>From: Elias Ross <elias_ross@apple.com>
>Date: Thu, 6 Mar 2014 13:52:28 -0800
>Subject: [PATCH] BZ 1073201 - encountering unknown resources causes inventory
> scan loops
>
>The main fix is keeping a reference to the scheduled service rescan Future and
>prevent a scan from being scheduled again before execution. This also has
>executeServiceScanDeferred() work the same way.
>
>The other related fixes are for:
>1) Only do availability checking for synched/merged/deleted resources, not a
>full scan.  As we have the references for this, it seems worthwhile to prevents
>lots of scans (and overloading the server) if an unknown resource shows up.
>2) Concurrency. Setters/getters should be synchronized if accessed across threads.
>3) Use 0 for scan time, as we can then avoid getting system time.
>4) In cases Executor.submit(Callable) is used and Future isn't needed, use
>Runnable instead.
>---
> .../rhq/core/pc/inventory/InventoryManager.java    | 154 ++++++++++++---------
> .../rhq/core/pc/inventory/ResourceContainer.java   |  11 +-
> 2 files changed, 97 insertions(+), 68 deletions(-)
>
>diff --git a/modules/core/plugin-container/src/main/java/org/rhq/core/pc/inventory/InventoryManager.java b/modules/core/plugin-container/src/main/java/org/rhq/core/pc/inventory/InventoryManager.java
>index 5a02905..504d30d 100644
>--- a/modules/core/plugin-container/src/main/java/org/rhq/core/pc/inventory/InventoryManager.java
>+++ b/modules/core/plugin-container/src/main/java/org/rhq/core/pc/inventory/InventoryManager.java
>@@ -19,6 +19,9 @@
> 
> package org.rhq.core.pc.inventory;
> 
>+import gnu.trove.map.TIntObjectMap;
>+import gnu.trove.map.hash.TIntObjectHashMap;
>+import static java.util.Collections.emptySet;
> import static org.rhq.core.util.StringUtil.isNotBlank;
> 
> import java.io.File;
>@@ -42,6 +45,7 @@
> import java.util.concurrent.CopyOnWriteArraySet;
> import java.util.concurrent.ExecutionException;
> import java.util.concurrent.Future;
>+import java.util.concurrent.ScheduledFuture;
> import java.util.concurrent.ScheduledThreadPoolExecutor;
> import java.util.concurrent.TimeUnit;
> import java.util.concurrent.atomic.AtomicInteger;
>@@ -229,6 +233,12 @@
>      * Handles the resource upgrade during the initialization of the inventory manager.
>      */
>     private final ResourceUpgradeDelegate resourceUpgradeDelegate = new ResourceUpgradeDelegate(this);
>+
>+    /**
>+     * Child scanning process.
>+     */
>+    private volatile Future<?> serviceScan = null;
>+
>     private final PluginComponentFactory pluginFactory;
>     private final EventManager eventManager;
>     private MeasurementManager measurementManager;
>@@ -726,12 +736,16 @@ public InventoryReport executeServiceScanImmediately(Resource resource) {
> 
>     @Override
>     public void executeServiceScanDeferred() {
>-        inventoryThreadPoolExecutor.submit((Callable<InventoryReport>) this.serviceScanExecutor);
>+        // This avoids the possibility of many service scans being scheduled or stacking
>+        if (serviceScan == null || serviceScan.isDone()) {
>+            serviceScan = this.inventoryThreadPoolExecutor.schedule((Runnable) this.serviceScanExecutor,
>+                configuration.getChildResourceDiscoveryDelay(), TimeUnit.SECONDS);
>+        }
>     }
> 
>     public void executeServiceScanDeferred(Resource resource) {
>         RuntimeDiscoveryExecutor discoveryExecutor = new RuntimeDiscoveryExecutor(this, this.configuration, resource);
>-        inventoryThreadPoolExecutor.submit((Callable<InventoryReport>) discoveryExecutor);
>+        inventoryThreadPoolExecutor.submit((Runnable) discoveryExecutor);
>     }
> 
>     /**
>@@ -818,9 +832,9 @@ public void requestAvailabilityCheck(Resource resource) {
> 
>         ResourceContainer resourceContainer = getResourceContainer(resource);
>         if (null != resourceContainer) {
>-            // by setting the avail schedule time to now, this resource will have an avail check performed on
>+            // by setting the avail schedule time to zero, this resource will have an avail check performed on
>             // the next availability scan.
>-            resourceContainer.setAvailabilityScheduleTime(System.currentTimeMillis());
>+            resourceContainer.setAvailabilityScheduleTime(0L);
>         }
>     }
> 
>@@ -1234,7 +1248,7 @@ public boolean handleReport(InventoryReport report) {
>         if (platformSyncInfo != null) {
>             syncPlatform(platformSyncInfo);
>         } else {
>-            purgeObsoleteResources(Collections.<String> emptySet());
>+            purgeObsoleteResources(Collections.<String>emptySet());
> 
>             //can't live without a platform, but we just deleted it. Let's rediscover it.
>             discoverPlatform();
>@@ -1293,7 +1307,7 @@ private void syncPlatform(PlatformSyncInfo platformSyncInfo) {
>                         // Wait a little to get other tasks room for breathing
>                         Thread.sleep(800L);
>                     } catch (InterruptedException e) {
>-                        ;
>+                        Thread.currentThread().interrupt();
>                     }
>                 }
>             }
>@@ -1303,29 +1317,17 @@ private void syncPlatform(PlatformSyncInfo platformSyncInfo) {
> 
>         log.info("Sync Complete: Platform [" + platformSyncInfo.getPlatform().getId() + "].");
> 
>-        // If we synced any Resources, one or more Resource components were probably started, request a
>-        // full avail report to make sure their availabilities are determined on the next avail run (typically
>-        // < 30s away). A full avail report will ensure an initial avail check is performed for a resource.
>-        //
>-        // Also kick off a service scan to scan those Resources for new child Resources. Kick both tasks off
>-        // asynchronously.
>+        // Kick off a service scan to scan those Resources for new child Resources.
>         //
>         // Do this only if we are finished with resource upgrade because no availability checks
>         // or discoveries can happen during upgrade. This is to ensure maximum consistency of the
>         // inventory with the server side as well as to disallow any other server-agent traffic during
>         // the upgrade phase. Not to mention the fact that no thread pools are initialized yet by the
>-        // time the upgrade kicks in..
>+        // time the upgrade kicks in.
>         if (hadSyncedResources && !isResourceUpgradeActive()) {
>-
>             log.info("Sync changes detected, requesting full availability report and service discovery: Platform ["
>                 + platformSyncInfo.getPlatform().getId() + "]");
>-
>-            // TODO: If someday this is undesirable for scalability reasons, we could probably instead call
>-            // requestAvailabilityCheck on each unknown or modified resource.
>-            requestFullAvailabilityReport();
>-
>-            this.inventoryThreadPoolExecutor.schedule((Callable<? extends Object>) this.serviceScanExecutor,
>-                configuration.getChildResourceDiscoveryDelay(), TimeUnit.SECONDS);
>+            executeServiceScanDeferred();
>         }
>     }
> 
>@@ -1366,8 +1368,8 @@ private boolean syncResources(int rootResourceId, Collection<ResourceSyncInfo> s
>                     modifiedResourceIds.size(), deletedResourceIds.size(), newlyCommittedResources.size()));
>             }
> 
>-            mergeUnknownResources(unknownResourceSyncInfos);
>-            mergeModifiedResources(modifiedResourceIds);
>+            Set<Resource> unknownResources = mergeUnknownResources(unknownResourceSyncInfos);
>+            Set<Resource> modifiedResources = mergeModifiedResources(modifiedResourceIds);
>             purgeIgnoredResources(ignoredResources);
> 
>             postProcessNewlyCommittedResources(newlyCommittedResources);
>@@ -1379,8 +1381,22 @@ private boolean syncResources(int rootResourceId, Collection<ResourceSyncInfo> s
>                     (System.currentTimeMillis() - startTime)));
>             }
> 
>-            result = !(syncedResources.isEmpty() && unknownResourceSyncInfos.isEmpty() && modifiedResourceIds.isEmpty());
>+            result = !syncedResources.isEmpty() || !unknownResources.isEmpty() || !modifiedResources.isEmpty();
> 
>+            // If we synced any Resources, one or more Resource components were probably started, request a
>+            // availability report to make sure their availabilities are determined on the next avail run (typically
>+            // < 30s away).
>+            if (!isResourceUpgradeActive() && result) {
>+                for (Resource r : syncedResources) {
>+                    requestAvailabilityCheck(r);
>+                }
>+                for (Resource r : modifiedResources) {
>+                    requestAvailabilityCheck(r);
>+                }
>+                for (Resource r : unknownResources) {
>+                    requestAvailabilityCheck(r);
>+                }
>+            }
>         } catch (Throwable t) {
>             log.warn("Failed to synchronize local inventory with Server inventory for Resource [" + rootResourceId
>                 + "] and its descendants: " + t.getMessage());
>@@ -1419,7 +1435,7 @@ private void registerWithServer() {
> 
>                 // now that we are registered, let's kick off an inventory report
>                 // just to make sure the server has our initial inventory
>-                inventoryThreadPoolExecutor.submit((Callable<InventoryReport>) this.serverScanExecutor);
>+                inventoryThreadPoolExecutor.submit((Runnable) this.serverScanExecutor);
>             } catch (Exception e) {
>                 log.error("Cannot re-register with the agent, something bad is happening", e);
>             }
>@@ -1484,7 +1500,7 @@ public void uninventoryResource(int resourceId) {
>         //happens before any scanning infrastructure is established.
>         if (!isResourceUpgradeActive() && scan) {
>             log.info("Deleted resource #[" + resourceId + "] - this will trigger a server scan now");
>-            inventoryThreadPoolExecutor.submit((Callable<InventoryReport>) this.serverScanExecutor);
>+            inventoryThreadPoolExecutor.submit((Runnable) this.serverScanExecutor);
>         }
>     }
> 
>@@ -3137,55 +3153,63 @@ private void compactConfiguration(Configuration config) {
>         config.resize();
>     }
> 
>-    private void mergeModifiedResources(Set<Integer> modifiedResourceIds) {
>-        if (modifiedResourceIds != null && !modifiedResourceIds.isEmpty()) {
>-            if (log.isDebugEnabled()) {
>-                log.debug("Merging [" + modifiedResourceIds.size() + "] modified Resources into local inventory...");
>-            }
>+    /**
>+     * Merges the supplied resource IDs, returning merged resource objects.
>+     */
>+    private Set<Resource> mergeModifiedResources(Set<Integer> modifiedResourceIds) {
>+        if (modifiedResourceIds.isEmpty()) {
>+            return emptySet();
>+        }
> 
>-            Set<Resource> modifiedResources = configuration.getServerServices().getDiscoveryServerService()
>-                .getResources(modifiedResourceIds, false);
>-            syncSchedules(modifiedResources); // RHQ-792, mtime is the indicator that schedules should be sync'ed too
>-            syncDriftDefinitions(modifiedResources);
>-            for (Resource modifiedResource : modifiedResources) {
>-                compactResource(modifiedResource);
>-                mergeResource(modifiedResource);
>-            }
>+        if (log.isDebugEnabled()) {
>+            log.debug("Merging [" + modifiedResourceIds.size() + "] modified Resources into local inventory...");
>         }
>-        return;
>+
>+        Set<Resource> modifiedResources = configuration.getServerServices().getDiscoveryServerService()
>+            .getResources(modifiedResourceIds, false);
>+        syncSchedules(modifiedResources); // RHQ-792, mtime is the indicator that schedules should be sync'ed too
>+        syncDriftDefinitions(modifiedResources);
>+        for (Resource modifiedResource : modifiedResources) {
>+            compactResource(modifiedResource);
>+            mergeResource(modifiedResource);
>+        }
>+        return modifiedResources;
>     }
> 
>-    private void mergeUnknownResources(Set<ResourceSyncInfo> unknownResourceSyncInfos) {
>-        if (unknownResourceSyncInfos != null && !unknownResourceSyncInfos.isEmpty()) {
>-            if (log.isDebugEnabled()) {
>-                log.debug("Merging [" + unknownResourceSyncInfos.size()
>-                    + "] unknown resources and descendants into inventory...");
>-            }
>+    /**
>+     * Merge unknown resource IDs, returning a set of resources merged.
>+     */
>+    private Set<Resource> mergeUnknownResources(Set<ResourceSyncInfo> unknownResourceSyncInfos) {
>+        if (unknownResourceSyncInfos.isEmpty()) {
>+            return emptySet();
>+        }
>+        if (log.isDebugEnabled()) {
>+            log.debug("Merging [" + unknownResourceSyncInfos.size()
>+                + "] unknown resources and descendants into inventory...");
>+        }
> 
>-            PluginMetadataManager pmm = this.pluginManager.getMetadataManager();
>+        PluginMetadataManager pmm = this.pluginManager.getMetadataManager();
> 
>-            Set<Resource> unknownResources = getResourcesFromSyncInfos(unknownResourceSyncInfos);
>-            Set<Integer> toBeIgnored = new HashSet<Integer>();
>+        Set<Resource> unknownResources = getResourcesFromSyncInfos(unknownResourceSyncInfos);
>+        Set<Resource> merged = new HashSet<Resource>();
> 
>-            for (Resource unknownResource : unknownResources) {
>-                ResourceType resourceType = pmm.getType(unknownResource.getResourceType());
>-                if (resourceType != null) {
>-                    compactResource(unknownResource);
>-                    mergeResource(unknownResource);
>-                    syncSchedulesRecursively(unknownResource);
>-                    syncDriftDefinitionsRecursively(unknownResource);
>-                } else {
>-                    toBeIgnored.add(unknownResource.getId());
>-                    if (log.isDebugEnabled()) {
>-                        log.debug("During an inventory sync, the server gave us resource [" + unknownResource
>-                            + "] but its type is disabled in the agent; ignoring it");
>-                    }
>+        for (Resource unknownResource : unknownResources) {
>+            ResourceType resourceType = pmm.getType(unknownResource.getResourceType());
>+            if (resourceType != null) {
>+                compactResource(unknownResource);
>+                mergeResource(unknownResource);
>+                syncSchedulesRecursively(unknownResource);
>+                syncDriftDefinitionsRecursively(unknownResource);
>+                merged.add(unknownResource);
>+            } else {
>+                if (log.isDebugEnabled()) {
>+                    log.debug("During an inventory sync, the server gave us resource [" + unknownResource
>+                        + "] but its type is disabled in the agent; ignoring it");
>                 }
>             }
>-
>-            unknownResourceSyncInfos.removeAll(toBeIgnored);
>         }
>-        return;
>+
>+        return merged;
>     }
> 
>     private Set<Resource> getResourcesFromSyncInfos(Set<ResourceSyncInfo> syncInfos) {
>diff --git a/modules/core/plugin-container/src/main/java/org/rhq/core/pc/inventory/ResourceContainer.java b/modules/core/plugin-container/src/main/java/org/rhq/core/pc/inventory/ResourceContainer.java
>index 118c8bf..976e518 100644
>--- a/modules/core/plugin-container/src/main/java/org/rhq/core/pc/inventory/ResourceContainer.java
>+++ b/modules/core/plugin-container/src/main/java/org/rhq/core/pc/inventory/ResourceContainer.java
>@@ -314,12 +314,17 @@ public void setAvailabilitySchedule(MeasurementScheduleRequest availabilitySched
>         }
>     }
> 
>-    public long getAvailabilityScheduleTime() {
>+    /**
>+     * Returns the availability schedule time.
>+     */
>+    public synchronized long getAvailabilityScheduleTime() {
>         return availabilityScheduleTime;
>     }
> 
>-    // TODO: Is there a reason for this to be synchronized like the other setters? I don't see why it would need to be.
>-    public void setAvailabilityScheduleTime(long availabilityScheduleTime) {
>+    /**
>+     * Sets the availability schedule time; synchronized to ensure visibility.
>+     */
>+    public synchronized void setAvailabilityScheduleTime(long availabilityScheduleTime) {
>         this.availabilityScheduleTime = availabilityScheduleTime;
>     }
> 
>-- 
>1.8.1.2
>

Actions: View | Diff

Attachments on bug 1073201: 872924