brooklyn-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From tbou...@apache.org
Subject [4/6] brooklyn-server git commit: Merge initial catalog and persisted catalog
Date Tue, 17 Oct 2017 09:19:34 GMT
Merge initial catalog and persisted catalog


Project: http://git-wip-us.apache.org/repos/asf/brooklyn-server/repo
Commit: http://git-wip-us.apache.org/repos/asf/brooklyn-server/commit/802cbb9b
Tree: http://git-wip-us.apache.org/repos/asf/brooklyn-server/tree/802cbb9b
Diff: http://git-wip-us.apache.org/repos/asf/brooklyn-server/diff/802cbb9b

Branch: refs/heads/master
Commit: 802cbb9b2219976cc87a50d45dd61502b7a09a68
Parents: 7c6f7d4
Author: Aled Sage <aled.sage@gmail.com>
Authored: Mon Oct 2 18:30:10 2017 +0100
Committer: Aled Sage <aled.sage@gmail.com>
Committed: Tue Oct 17 09:02:51 2017 +0100

----------------------------------------------------------------------
 .../brooklyn/api/catalog/BrooklynCatalog.java   |  11 +
 .../catalog/internal/BasicBrooklynCatalog.java  |  36 ++
 .../catalog/internal/CatalogInitialization.java | 462 +++++++++++--------
 .../internal/AbstractManagementContext.java     |   8 +-
 .../mgmt/internal/LocalManagementContext.java   |   4 -
 .../core/mgmt/rebind/RebindIteration.java       |  37 +-
 .../ha/HighAvailabilityManagerTestFixture.java  |   3 +-
 .../mgmt/rebind/RebindCatalogEntityTest.java    |   8 +-
 .../core/mgmt/rebind/RebindEntityTest.java      |   2 +
 .../rebind/RebindHistoricCatalogItemTest.java   |  53 +++
 .../core/mgmt/rebind/RebindTestFixture.java     |  11 -
 .../core/mgmt/rebind/RebindTestUtils.java       |   3 +
 .../entity/LocalManagementContextForTests.java  |  12 +
 .../core/mgmt/rebind/catalog-myapp_1.0.0        |  36 ++
 .../brooklyn/launcher/common/BasicLauncher.java | 125 ++---
 .../BrooklynLauncherRebindCatalogTest.java      | 120 ++++-
 .../brooklyn/launcher/BrooklynLauncherTest.java |  10 +-
 .../resources/rebind-test-empty-catalog.bom     |  25 +
 .../rest/testing/BrooklynRestApiTest.java       |   4 +-
 .../brooklyn/rest/BrooklynRestApiLauncher.java  |   3 +-
 .../brooklyn/rest/HaMasterCheckFilterTest.java  |   9 +-
 .../main/java/org/apache/brooklyn/cli/Main.java |  76 +--
 22 files changed, 632 insertions(+), 426 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/802cbb9b/api/src/main/java/org/apache/brooklyn/api/catalog/BrooklynCatalog.java
----------------------------------------------------------------------
diff --git a/api/src/main/java/org/apache/brooklyn/api/catalog/BrooklynCatalog.java b/api/src/main/java/org/apache/brooklyn/api/catalog/BrooklynCatalog.java
index e03adbf..cb305ef 100644
--- a/api/src/main/java/org/apache/brooklyn/api/catalog/BrooklynCatalog.java
+++ b/api/src/main/java/org/apache/brooklyn/api/catalog/BrooklynCatalog.java
@@ -213,6 +213,17 @@ public interface BrooklynCatalog {
     void addItem(CatalogItem<?,?> item);
 
     /**
+     * adds the given items to the catalog, similar to {@link #reset(Collection)} but where it 
+     * just adds without removing the existing content. Note this is very different from 
+     * {@link #addItem(CatalogItem)}, which adds to the 'manual' catalog.
+     *
+     * @since 0.13.0 (only for legacy backwards compatibility)
+     * @deprecated since 0.13.0; instead use bundles in persisted state!
+     */
+    @Deprecated
+    void addCatalogLegacyItemsOnRebind(Iterable<? extends CatalogItem<?,?>> items);
+
+    /**
      * Creates a catalog item and adds it to the 'manual' catalog,
      * with the corresponding Class definition (loaded by a classloader)
      * registered and available in the classloader.

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/802cbb9b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java
index 4126d5d..fe0bffe 100644
--- a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java
+++ b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java
@@ -1826,6 +1826,42 @@ public class BasicBrooklynCatalog implements BrooklynCatalog {
     }
 
     @Override @Deprecated /** @deprecated see super */
+    public void addCatalogLegacyItemsOnRebind(Iterable<? extends CatalogItem<?,?>> items) {
+        addCatalogLegacyItemsOnRebind(items, true);
+    }
+    
+    private void addCatalogLegacyItemsOnRebind(Iterable<? extends CatalogItem<?,?>> items, boolean failOnLoadError) {
+        specCache.invalidate();
+        
+        log.debug("Adding manual catalog items to "+mgmt+": "+items);
+        checkNotNull(items, "item");
+        for (CatalogItem<?,?> item : items) {
+            CatalogItemDtoAbstract<?,?> cdto;
+            if (item instanceof CatalogItemDtoAbstract) {
+                cdto = (CatalogItemDtoAbstract<?,?>) item;
+            } else if (item instanceof CatalogItemDo && ((CatalogItemDo<?,?>)item).getDto() instanceof CatalogItemDtoAbstract) {
+                cdto = (CatalogItemDtoAbstract<?,?>) ((CatalogItemDo<?,?>)item).getDto();
+            } else {
+                throw new IllegalArgumentException("Expected items of type "+CatalogItemDtoAbstract.class.getSimpleName());
+            }
+            cdto.setManagementContext((ManagementContextInternal) mgmt);
+            
+            try {
+                CatalogUtils.installLibraries(mgmt, item.getLibraries());
+            } catch (Exception e) {
+                Exceptions.propagateIfFatal(e);
+                if (failOnLoadError) {
+                    Exceptions.propagateAnnotated("Loading bundles for catalog item " + item.getCatalogItemId() + " failed", e);
+                } else {
+                    log.error("Loading bundles for catalog item " + item + " failed: " + e.getMessage(), e);
+                }
+            }
+            catalog.addEntry((CatalogItemDtoAbstract<?,?>)item);
+        }
+        catalog.load(mgmt, null, failOnLoadError);
+    }
+
+    @Override @Deprecated /** @deprecated see super */
     public CatalogItem<?,?> addItem(Class<?> type) {
         //assume forceUpdate for backwards compatibility
         log.debug("Adding manual catalog item to "+mgmt+": "+type);

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/802cbb9b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogInitialization.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogInitialization.java b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogInitialization.java
index 5a67b1f..e11b64d 100644
--- a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogInitialization.java
+++ b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogInitialization.java
@@ -18,27 +18,39 @@
  */
 package org.apache.brooklyn.core.catalog.internal;
 
+import static com.google.common.base.Preconditions.checkNotNull;
+
 import java.io.File;
 import java.io.IOException;
 import java.io.InputStream;
+import java.util.ArrayList;
 import java.util.Collection;
+import java.util.Dictionary;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
 
 import org.apache.brooklyn.api.catalog.BrooklynCatalog;
 import org.apache.brooklyn.api.catalog.CatalogItem;
+import org.apache.brooklyn.api.catalog.CatalogItem.CatalogItemType;
+import org.apache.brooklyn.api.entity.EntitySpec;
+import org.apache.brooklyn.api.internal.AbstractBrooklynObjectSpec;
 import org.apache.brooklyn.api.mgmt.ManagementContext;
 import org.apache.brooklyn.api.mgmt.ha.ManagementNodeState;
 import org.apache.brooklyn.api.mgmt.rebind.RebindExceptionHandler;
 import org.apache.brooklyn.api.objs.BrooklynObjectType;
+import org.apache.brooklyn.api.typereg.BrooklynTypeRegistry.RegisteredTypeKind;
 import org.apache.brooklyn.api.typereg.ManagedBundle;
 import org.apache.brooklyn.api.typereg.RegisteredType;
 import org.apache.brooklyn.core.mgmt.ManagementContextInjectable;
 import org.apache.brooklyn.core.mgmt.ha.OsgiBundleInstallationResult;
+import org.apache.brooklyn.core.mgmt.ha.OsgiManager;
 import org.apache.brooklyn.core.mgmt.internal.ManagementContextInternal;
+import org.apache.brooklyn.core.objs.BrooklynTypes;
 import org.apache.brooklyn.core.server.BrooklynServerConfig;
 import org.apache.brooklyn.core.typereg.RegisteredTypePredicates;
+import org.apache.brooklyn.core.typereg.RegisteredTypes;
 import org.apache.brooklyn.util.collections.MutableList;
 import org.apache.brooklyn.util.collections.MutableMap;
 import org.apache.brooklyn.util.collections.MutableSet;
@@ -47,19 +59,24 @@ import org.apache.brooklyn.util.exceptions.Exceptions;
 import org.apache.brooklyn.util.exceptions.FatalRuntimeException;
 import org.apache.brooklyn.util.exceptions.PropagatedRuntimeException;
 import org.apache.brooklyn.util.exceptions.RuntimeInterruptedException;
+import org.apache.brooklyn.util.exceptions.UserFacingException;
+import org.apache.brooklyn.util.guava.Maybe;
 import org.apache.brooklyn.util.javalang.JavaClassNames;
 import org.apache.brooklyn.util.os.Os;
 import org.apache.brooklyn.util.osgi.VersionedName;
 import org.apache.brooklyn.util.text.Strings;
+import org.apache.brooklyn.util.time.Duration;
+import org.osgi.framework.Bundle;
 import org.osgi.framework.BundleException;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import com.google.common.annotations.Beta;
-import com.google.common.base.Function;
-import com.google.common.base.Preconditions;
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Splitter;
 import com.google.common.base.Stopwatch;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 
 @Beta
@@ -82,25 +99,26 @@ public class CatalogInitialization implements ManagementContextInjectable {
      */
 
     private static final Logger log = LoggerFactory.getLogger(CatalogInitialization.class);
-    
+
     private String initialUri;
 
-    private boolean disallowLocal = false;
-    private List<Function<CatalogInitialization, Void>> callbacks = MutableList.of();
-    /** has run an unofficial initialization (i.e. an early load, triggered by an early read of the catalog) */
-    private boolean hasRunUnofficialInitialization = false; 
+    /** has run the initial catalog initialization */
+    private boolean hasRunInitialCatalogInitialization = false; 
+
     /** has run an official initialization, but it is not a permanent one (e.g. during a hot standby mode, or a run failed) */
-    private boolean hasRunTransientOfficialInitialization = false; 
+    private boolean hasRunPersistenceInitialization = false; 
+
     /** has run an official initialization which is permanent (node is master, and the new catalog is now set) */
     private boolean hasRunFinalInitialization = false;
-    /** is running a populate method; used to prevent recursive loops */
-    private boolean isPopulating = false;
-    
+
     private ManagementContextInternal managementContext;
     private boolean isStartingUp = false;
     private boolean failOnStartupErrors = false;
     
-    private Object populatingCatalogMutex = new Object();
+    /** is running a populate method; used to prevent recursive loops */
+    private boolean isPopulatingInitial = false;
+
+    private final Object populatingCatalogMutex = new Object();
     
     public CatalogInitialization() {
         this(null);
@@ -112,7 +130,7 @@ public class CatalogInitialization implements ManagementContextInjectable {
     
     @Override
     public void setManagementContext(ManagementContext managementContext) {
-        Preconditions.checkNotNull(managementContext, "management context");
+        checkNotNull(managementContext, "management context");
         if (this.managementContext!=null && managementContext!=this.managementContext)
             throw new IllegalStateException("Cannot switch management context, from "+this.managementContext+" to "+managementContext);
         this.managementContext = (ManagementContextInternal) managementContext;
@@ -128,88 +146,70 @@ public class CatalogInitialization implements ManagementContextInjectable {
         this.failOnStartupErrors = startupFailOnCatalogErrors;
     }
 
-    public CatalogInitialization addPopulationCallback(Function<CatalogInitialization, Void> callback) {
-        callbacks.add(callback);
-        return this;
-    }
-    
     public ManagementContextInternal getManagementContext() {
-        return Preconditions.checkNotNull(managementContext, "management context has not been injected into "+this);
+        return checkNotNull(managementContext, "management context has not been injected into "+this);
     }
 
     /** Returns true if the canonical initialization has completed, 
      * that is, an initialization which is done when a node is rebinded as master
      * (or an initialization done by the startup routines when not running persistence);
      * see also {@link #hasRunAnyInitialization()}. */
-    public boolean hasRunFinalInitialization() { return hasRunFinalInitialization; }
+    private boolean hasRunFinalInitialization() {
+        return hasRunFinalInitialization;
+    }
     
-    /** Returns true if an official initialization has run,
-     * even if it was a transient run, e.g. so that the launch sequence can tell whether rebind has triggered initialization */
-    public boolean hasRunOfficialInitialization() { return hasRunFinalInitialization || hasRunTransientOfficialInitialization; }
+    private boolean hasRunInitialCatalogInitialization() {
+        return hasRunInitialCatalogInitialization;
+    }
     
-    /** Returns true if the initializer has run at all,
-     * including transient initializations which might be needed before a canonical becoming-master rebind,
-     * for instance because the catalog is being accessed before loading rebind information
-     * (done by {@link #populateUnofficial(BasicBrooklynCatalog)}) */
-    public boolean hasRunAnyInitialization() { return hasRunFinalInitialization || hasRunTransientOfficialInitialization || hasRunUnofficialInitialization; }
+    /**
+     * Returns true if we have added catalog bundles/items from persisted state.
+     */
+    private boolean hasRunPersistenceInitialization() {
+        return hasRunFinalInitialization || hasRunPersistenceInitialization;
+    }
+    
+    /**
+     * Returns true if the initializer has run at all.
+     */
+    @VisibleForTesting
+    @Beta
+    public boolean hasRunAnyInitialization() {
+        return hasRunFinalInitialization || hasRunInitialCatalogInitialization || hasRunPersistenceInitialization;
+    }
 
     /**
-     * This method will almost certainly be changed. It is an interim step, to move the catalog-initialization
-     * decisions and logic out of {@link org.apache.brooklyn.core.mgmt.rebind.RebindIteration} and into 
-     * a single place. We can then make smarter decisions about what to do with the persisted state's catalog.
+     * Populates the initial catalog (i.e. from the initial .bom file).
      * 
-     * @param mode
-     * @param persistedState
-     * @param exceptionHandler
-     * @param rebindLogger
+     * Expected to be called exactly once at startup.
      */
-    @Beta
-    public void populateCatalog(ManagementNodeState mode, PersistedCatalogState persistedState, RebindExceptionHandler exceptionHandler, RebindLogger rebindLogger) {
-        // Always installing the bundles from persisted state
-        installBundles(mode, persistedState, exceptionHandler, rebindLogger);
-        
-        // Decide whether to add the persisted catalog items, or to use the "initial items".
-        // Logic copied (unchanged) from RebindIteration.installBundlesAndRebuildCatalog.
-        boolean isEmpty = persistedState.isEmpty();
-        Collection<CatalogItem<?,?>> itemsForResettingCatalog = null;
-        boolean needsInitialItemsLoaded;
-        boolean needsAdditionalItemsLoaded;
-        if (!isEmpty) {
-            rebindLogger.debug("RebindManager clearing local catalog and loading from persisted state");
-            itemsForResettingCatalog = persistedState.getLegacyCatalogItems();
-            needsInitialItemsLoaded = false;
-            // only apply "add" if we haven't yet done so while in MASTER mode
-            needsAdditionalItemsLoaded = !hasRunFinalInitialization();
-        } else {
-            if (hasRunFinalInitialization()) {
-                rebindLogger.debug("RebindManager has already done the final official run, not doing anything (even though persisted state empty)");
-                needsInitialItemsLoaded = false;
-                needsAdditionalItemsLoaded = false;
-            } else {
-                rebindLogger.debug("RebindManager loading initial catalog locally because persisted state empty and the final official run has not yet been performed");
-                needsInitialItemsLoaded = true;
-                needsAdditionalItemsLoaded = true;
-            }
+    public void populateInitialCatalogOnly() {
+        if (log.isDebugEnabled()) {
+            log.debug("Populating only the initial catalog; from "+JavaClassNames.callerNiceClassAndMethod(1));
         }
+        synchronized (populatingCatalogMutex) {
+            if (hasRunInitialCatalogInitialization()) {
+                throw new IllegalStateException("Catalog initialization called to populate only initial, even though it has already run it");
+            }
 
-        // TODO in read-only mode, perhaps do this less frequently than entities etc, maybe only if things change?
-        populateCatalog(mode, needsInitialItemsLoaded, needsAdditionalItemsLoaded, itemsForResettingCatalog);
+            populateInitialCatalogImpl(true);
+            onFinalCatalog();
+        }
     }
-    
-    /** makes or updates the mgmt catalog, based on the settings in this class 
-     * @param nodeState the management node for which this is being read; if master, then we expect this run to be the last one,
-     *   and so subsequent applications should ignore any initialization data (e.g. on a subsequent promotion to master, 
-     *   after a master -> standby -> master cycle)
-     * @param needsInitialItemsLoaded whether the catalog needs the initial items loaded
-     * @param needsAdditionalItemsLoaded whether the catalog needs the additions loaded
-     * @param optionalExplicitItemsForResettingCatalog
-     *   if supplied, the catalog is reset to contain only these items, before calling any other initialization
-     *   for use primarily when rebinding
+
+    /**
+     * Adds the given persisted catalog items.
+     * 
+     * Can be called multiple times, e.g.:
+     * <ul>
+     *   <li>if "hot-standby" then will be called repeatedly, as we rebind to the persisted state
+     *   <li> if being promoted to master then we will be called (and may already have been called for "hot-standby").
+     * </ul>
      */
-    public void populateCatalog(ManagementNodeState nodeState, boolean needsInitialItemsLoaded, boolean needsAdditionsLoaded, Collection<CatalogItem<?, ?>> optionalExplicitItemsForResettingCatalog) {
+    public void populateInitialAndPersistedCatalog(ManagementNodeState mode, PersistedCatalogState persistedState, RebindExceptionHandler exceptionHandler, RebindLogger rebindLogger) {
         if (log.isDebugEnabled()) {
-            String message = "Populating catalog for "+nodeState+", needsInitial="+needsInitialItemsLoaded+", needsAdditional="+needsAdditionsLoaded+", explicitItems="+(optionalExplicitItemsForResettingCatalog==null ? "null" : optionalExplicitItemsForResettingCatalog.size())+"; from "+JavaClassNames.callerNiceClassAndMethod(1);
-            if (!ManagementNodeState.isHotProxy(nodeState)) {
+            String message = "Add persisted catalog for "+mode+", persistedBundles="+persistedState.getBundles().size()+", legacyItems="+persistedState.getLegacyCatalogItems().size()+"; from "+JavaClassNames.callerNiceClassAndMethod(1);
+            if (!ManagementNodeState.isHotProxy(mode)) {
                 log.debug(message);
             } else {
                 // in hot modes, make this message trace so we don't get too much output then
@@ -217,75 +217,184 @@ public class CatalogInitialization implements ManagementContextInjectable {
             }
         }
         synchronized (populatingCatalogMutex) {
-            try {
-                if (hasRunFinalInitialization() && (needsInitialItemsLoaded || needsAdditionsLoaded)) {
-                    // if we have already run "final" then we should only ever be used to reset the catalog, 
-                    // not to initialize or add; e.g. we are being given a fixed list on a subsequent master rebind after the initial master rebind 
-                    log.warn("Catalog initialization called to populate initial, even though it has already run the final official initialization");
-                }
-                isPopulating = true;
-                BasicBrooklynCatalog catalog = (BasicBrooklynCatalog) managementContext.getCatalog();
-                if (!catalog.getCatalog().isLoaded()) {
-                    catalog.load();
-                } else {
-                    if (needsInitialItemsLoaded && hasRunAnyInitialization()) {
-                        // an indication that something caused it to load early; not severe, but unusual
-                        if (hasRunTransientOfficialInitialization) {
-                            log.debug("Catalog initialization now populating, but has noted a previous official run which was not final (probalby loaded while in a standby mode, or a previous run failed); overwriting any items installed earlier");
-                        } else {
-                            log.warn("Catalog initialization now populating, but has noted a previous unofficial run (it may have been an early web request); overwriting any items installed earlier");
-                        }
-                        catalog.reset(ImmutableList.<CatalogItem<?,?>>of());
+            if (hasRunFinalInitialization()) {
+                log.warn("Catalog initialization called to add persisted catalog, even though it has already run the final 'master' initialization; mode="+mode+" (perhaps previously demoted from master?)");      
+                hasRunFinalInitialization = false;
+            }
+            if (hasRunPersistenceInitialization()) {
+                // Multiple calls; will need to reset (only way to clear out the previous persisted state's catalog)
+                if (log.isDebugEnabled()) {
+                    String message = "Catalog initialization repeated call to add persisted catalog, resetting catalog (including initial) to start from clean slate; mode="+mode;
+                    if (!ManagementNodeState.isHotProxy(mode)) {
+                        log.debug(message);
+                    } else {
+                        // in hot modes, make this message trace so we don't get too much output then
+                        log.trace(message);
                     }
                 }
+            } else if (hasRunInitialCatalogInitialization()) {
+                throw new IllegalStateException("Catalog initialization already run for initial catalog by mechanism other than populating persisted state; mode="+mode);      
+            }
+            
+            populateInitialCatalogImpl(true);
+            addPersistedCatalogImpl(persistedState, exceptionHandler, rebindLogger);
+            
+            if (mode == ManagementNodeState.MASTER) {
+                // TODO ideally this would remain false until it has *persisted* the changed catalog;
+                // if there is a subsequent startup failure the forced additions will not be persisted,
+                // but nor will they be loaded on a subsequent run.
+                // callers will have to restart a brooklyn, or reach into this class to change this field,
+                // or (recommended) manually adjust the catalog.
+                // TODO also, if a node comes up in standby, the addition might not take effector for a while
+                //
+                // however since these options are mainly for use on the very first brooklyn run, it's not such a big deal; 
+                // once up and running the typical way to add items is via the REST API
+                onFinalCatalog();
+            }
+        }
+    }
 
-                populateCatalogImpl(catalog, needsInitialItemsLoaded, needsAdditionsLoaded, optionalExplicitItemsForResettingCatalog);
-                if (nodeState == ManagementNodeState.MASTER) {
-                    // TODO ideally this would remain false until it has *persisted* the changed catalog;
-                    // if there is a subsequent startup failure the forced additions will not be persisted,
-                    // but nor will they be loaded on a subsequent run.
-                    // callers will have to restart a brooklyn, or reach into this class to change this field,
-                    // or (recommended) manually adjust the catalog.
-                    // TODO also, if a node comes up in standby, the addition might not take effector for a while
-                    //
-                    // however since these options are mainly for use on the very first brooklyn run, it's not such a big deal; 
-                    // once up and running the typical way to add items is via the REST API
-                    hasRunFinalInitialization = true;
-                }
-            } catch (Throwable e) {
-                log.warn("Error populating catalog (rethrowing): "+e, e);
-                throw Exceptions.propagate(e);
-            } finally {
-                if (!hasRunFinalInitialization) {
-                    hasRunTransientOfficialInitialization = true;
-                }
-                isPopulating = false;
+    /**
+     * Populates the initial catalog, but not via an official code-path.
+     * 
+     * Expected to be called only during tests, where the test has not gone through the same 
+     * management-context lifecycle as is done in BasicLauncher.
+     * 
+     * Subsequent calls will fail to things like {@link #populateInitialCatalog()} or 
+     * {@link #populateInitialAndPersistedCatalog(ManagementNodeState, PersistedCatalogState, RebindExceptionHandler, RebindLogger)}.
+     */
+    @VisibleForTesting
+    @Beta
+    public void unofficialPopulateInitialCatalog() {
+        if (log.isDebugEnabled()) {
+            log.debug("Unofficially populate initial catalog; should be used only by tests! Called from "+JavaClassNames.callerNiceClassAndMethod(1));
+        }
+        synchronized (populatingCatalogMutex) {
+            if (hasRunInitialCatalogInitialization()) {
+                return;
             }
+
+            populateInitialCatalogImpl(true);
         }
     }
 
-    private void populateCatalogImpl(BasicBrooklynCatalog catalog, boolean needsInitialItemsLoaded, boolean needsAdditionsLoaded, Collection<CatalogItem<?, ?>> optionalItemsForResettingCatalog) {
-        if (optionalItemsForResettingCatalog!=null) {
-            catalog.reset(optionalItemsForResettingCatalog);
+    public void handleException(Throwable throwable, Object details) {
+        if (throwable instanceof InterruptedException)
+            throw new RuntimeInterruptedException((InterruptedException) throwable);
+        if (throwable instanceof RuntimeInterruptedException)
+            throw (RuntimeInterruptedException) throwable;
+
+        if (details instanceof CatalogItem) {
+            if (((CatalogItem<?,?>)details).getCatalogItemId() != null) {
+                details = ((CatalogItem<?,?>)details).getCatalogItemId();
+            }
         }
+        PropagatedRuntimeException wrap = new PropagatedRuntimeException("Error loading catalog item "+details, throwable);
+        log.warn(Exceptions.collapseText(wrap));
+        log.debug("Trace for: "+wrap, wrap);
+
+        getManagementContext().errors().add(wrap);
         
-        if (needsInitialItemsLoaded) {
-            populateInitial(catalog);
+        if (isStartingUp && failOnStartupErrors) {
+            throw new FatalRuntimeException("Unable to load catalog item "+details, wrap);
+        }
+    }
+    
+    private void confirmCatalog() {
+        // Force load of catalog (so web console is up to date)
+        Stopwatch time = Stopwatch.createStarted();
+        Iterable<RegisteredType> all = getManagementContext().getTypeRegistry().getAll();
+        int errors = 0;
+        for (RegisteredType rt: all) {
+            if (RegisteredTypes.isTemplate(rt)) {
+                // skip validation of templates, they might contain instructions,
+                // and additionally they might contain multiple items in which case
+                // the validation below won't work anyway (you need to go via a deployment plan)
+            } else {
+                if (rt.getKind()==RegisteredTypeKind.UNRESOLVED) {
+                    errors++;
+                    handleException(new UserFacingException("Unresolved type in catalog"), rt);
+                }
+            }
         }
 
-        if (needsAdditionsLoaded) {
-            populateViaCallbacks(catalog);
+        // and force resolution of legacy items
+        BrooklynCatalog catalog = getManagementContext().getCatalog();
+        Iterable<CatalogItem<Object, Object>> items = catalog.getCatalogItemsLegacy();
+        for (CatalogItem<Object, Object> item: items) {
+            try {
+                if (item.getCatalogItemType()==CatalogItemType.TEMPLATE) {
+                    // skip validation of templates, they might contain instructions,
+                    // and additionally they might contain multiple items in which case
+                    // the validation below won't work anyway (you need to go via a deployment plan)
+                } else {
+                    AbstractBrooklynObjectSpec<?, ?> spec = catalog.peekSpec(item);
+                    if (spec instanceof EntitySpec) {
+                        BrooklynTypes.getDefinedEntityType(((EntitySpec<?>)spec).getType());
+                    }
+                    log.debug("Catalog loaded spec "+spec+" for item "+item);
+                }
+            } catch (Throwable throwable) {
+                handleException(throwable, item);
+            }
         }
+        
+        log.debug("Catalog (size "+Iterables.size(all)+", of which "+Iterables.size(items)+" legacy) confirmed in "+Duration.of(time)+(errors>0 ? ", errors found ("+errors+")" : ""));
+        // nothing else added here
     }
 
-    protected void populateInitial(BasicBrooklynCatalog catalog) {
-        if (disallowLocal) {
-            if (!hasRunFinalInitialization()) {
-                log.debug("CLI initial catalog not being read when local catalog load mode is disallowed.");
-            }
+    private void populateInitialCatalogImpl(boolean reset) {
+        assert Thread.holdsLock(populatingCatalogMutex);
+        
+        if (isPopulatingInitial) {
+            // Avoid recursively loops, where getCatalog() calls unofficialPopulateInitialCatalog(), but populateInitialCatalogImpl() also calls getCatalog()
             return;
         }
+        
+        isPopulatingInitial = true;
+        try {
+            BasicBrooklynCatalog catalog = (BasicBrooklynCatalog) managementContext.getCatalog();
+            if (!catalog.getCatalog().isLoaded()) {
+                catalog.load();
+            } else {
+                if (reset) {
+                    catalog.reset(ImmutableList.<CatalogItem<?,?>>of());
+                }
+            }
+
+            populateViaInitialBomImpl(catalog);
 
+        } catch (Throwable e) {
+            log.warn("Error populating catalog (rethrowing): "+e, e);
+            throw Exceptions.propagate(e);
+        } finally {
+            isPopulatingInitial = false;
+            hasRunInitialCatalogInitialization = true;
+        }
+    }
+
+    private void addPersistedCatalogImpl(PersistedCatalogState persistedState, RebindExceptionHandler exceptionHandler, RebindLogger rebindLogger) {
+        assert Thread.holdsLock(populatingCatalogMutex);
+
+        try {
+            // Always installing the bundles from persisted state
+            installBundles(persistedState.getBundles(), exceptionHandler, rebindLogger);
+            
+            BrooklynCatalog catalog = managementContext.getCatalog();
+            catalog.addCatalogLegacyItemsOnRebind(persistedState.getLegacyCatalogItems());
+        } finally {
+            hasRunPersistenceInitialization = true;
+        }
+    }
+    
+    private void onFinalCatalog() {
+        assert Thread.holdsLock(populatingCatalogMutex);
+        
+        hasRunFinalInitialization = true;
+        confirmCatalog();
+    }
+
+    private void populateViaInitialBomImpl(BasicBrooklynCatalog catalog) {
 //        B1) look for --catalog-initial, if so read it, then go to C1
 //        B2) look for BrooklynServerConfig.BROOKLYN_CATALOG_URL, if so, read it, supporting YAML, then go to C1
 //        B3) look for ~/.brooklyn/catalog.bom, if exists, read it then go to C1
@@ -341,70 +450,18 @@ public class CatalogInitialization implements ManagementContextInjectable {
         }
     }
 
-    protected void populateViaCallbacks(BasicBrooklynCatalog catalog) {
-        for (Function<CatalogInitialization, Void> callback: callbacks)
-            callback.apply(this);
-    }
-
-    /** Creates the catalog based on parameters set here, if not yet loaded,
-     * but ignoring persisted state and warning if persistence is on and we are starting up
-     * (because the official persistence is preferred and the catalog will be subsequently replaced);
-     * for use when the catalog is accessed before persistence is completed. 
-     * <p>
-     * This method is primarily used during testing, which in many cases does not enforce the full startup order
-     * and which wants a local catalog in any case. It may also be invoked if a client requests the catalog
-     * while the server is starting up. */
-    public void populateUnofficial(BasicBrooklynCatalog catalog) {
-        synchronized (populatingCatalogMutex) {
-            // check isPopulating in case this method gets called from inside another populate call
-            if (hasRunAnyInitialization() || isPopulating) return;
-            log.debug("Populating catalog unofficially ("+catalog+")");
-            isPopulating = true;
-            try {
-                if (isStartingUp) {
-                    log.warn("Catalog access requested when not yet initialized; populating best effort rather than through recommended pathway. Catalog data may be replaced subsequently.");
-                }
-                populateCatalogImpl(catalog, true, true, null);
-            } finally {
-                hasRunUnofficialInitialization = true;
-                isPopulating = false;
-            }
-        }
-    }
-
-    public void handleException(Throwable throwable, Object details) {
-        if (throwable instanceof InterruptedException)
-            throw new RuntimeInterruptedException((InterruptedException) throwable);
-        if (throwable instanceof RuntimeInterruptedException)
-            throw (RuntimeInterruptedException) throwable;
-
-        if (details instanceof CatalogItem) {
-            if (((CatalogItem<?,?>)details).getCatalogItemId() != null) {
-                details = ((CatalogItem<?,?>)details).getCatalogItemId();
-            }
-        }
-        PropagatedRuntimeException wrap = new PropagatedRuntimeException("Error loading catalog item "+details, throwable);
-        log.warn(Exceptions.collapseText(wrap));
-        log.debug("Trace for: "+wrap, wrap);
-
-        getManagementContext().errors().add(wrap);
-        
-        if (isStartingUp && failOnStartupErrors) {
-            throw new FatalRuntimeException("Unable to load catalog item "+details, wrap);
-        }
-    }
-    
-    private void installBundles(ManagementNodeState mode, PersistedCatalogState persistedState, RebindExceptionHandler exceptionHandler, RebindLogger rebindLogger) {
+    private void installBundles(Map<VersionedName, InstallableManagedBundle> bundles, RebindExceptionHandler exceptionHandler, RebindLogger rebindLogger) {
         List<OsgiBundleInstallationResult> installs = MutableList.of();
 
         // Install the bundles
-        for (String bundleId : persistedState.getBundleIds()) {
+        for (Map.Entry<VersionedName, InstallableManagedBundle> entry : bundles.entrySet()) {
+            VersionedName bundleId = entry.getKey();
+            InstallableManagedBundle installableBundle = entry.getValue();
             rebindLogger.debug("RebindManager installing bundle {}", bundleId);
-            InstallableManagedBundle installableBundle = persistedState.getInstallableManagedBundle(bundleId);
             try (InputStream in = installableBundle.getInputStream()) {
                 installs.add(installBundle(installableBundle.getManagedBundle(), in));
             } catch (Exception e) {
-                exceptionHandler.onCreateFailed(BrooklynObjectType.MANAGED_BUNDLE, bundleId, installableBundle.getManagedBundle().getSymbolicName(), e);
+                exceptionHandler.onCreateFailed(BrooklynObjectType.MANAGED_BUNDLE, bundleId.toString(), installableBundle.getManagedBundle().getSymbolicName(), e);
             }
         }
         
@@ -461,11 +518,11 @@ public class CatalogInitialization implements ManagementContextInjectable {
     /** install the bundles into brooklyn and osgi, but do not start nor validate;
      * caller (rebind) will do that manually, doing each step across all bundles before proceeding 
      * to prevent reference errors */
-    public OsgiBundleInstallationResult installBundle(ManagedBundle bundle, InputStream zipInput) {
+    private OsgiBundleInstallationResult installBundle(ManagedBundle bundle, InputStream zipInput) {
         return getManagementContext().getOsgiManager().get().installDeferredStart(bundle, zipInput, false).get();
     }
     
-    public void startBundle(OsgiBundleInstallationResult br) throws BundleException {
+    private void startBundle(OsgiBundleInstallationResult br) throws BundleException {
         if (br.getDeferredStart()!=null) {
             br.getDeferredStart().run();
         }
@@ -481,26 +538,27 @@ public class CatalogInitialization implements ManagementContextInjectable {
         public InputStream getInputStream() throws IOException;
     }
     
-    public interface PersistedCatalogState {
-
-        /**
-         * Whether the persisted state is entirely empty.
-         */
-        public boolean isEmpty();
-
-        /**
-         * The persisted catalog items (from the {@code /catalog} sub-directory of the persisted state).
-         */
-        public Collection<CatalogItem<?,?>> getLegacyCatalogItems();
+    public static class PersistedCatalogState {
+        private final Map<VersionedName, InstallableManagedBundle> bundles;
+        private final Collection<CatalogItem<?, ?>> legacyCatalogItems;
         
+        public PersistedCatalogState(Map<VersionedName, InstallableManagedBundle> bundles, Collection<CatalogItem<?, ?>> legacyCatalogItems) {
+            this.bundles = checkNotNull(bundles, "bundles");
+            this.legacyCatalogItems = checkNotNull(legacyCatalogItems, "legacyCatalogItems");
+        }
+
         /**
          * The persisted bundles (from the {@code /bundles} sub-directory of the persisted state).
          */
-        public Set<String> getBundleIds();
+        public Map<VersionedName, InstallableManagedBundle> getBundles() {
+            return bundles;
+        }
         
         /**
-         * Loads the details of a particular bundle, so it can be installed.
+         * The persisted catalog items (from the {@code /catalog} sub-directory of the persisted state).
          */
-        public InstallableManagedBundle getInstallableManagedBundle(String id);
+        public Collection<CatalogItem<?,?>> getLegacyCatalogItems() {
+            return legacyCatalogItems;
+        }
     }
 }

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/802cbb9b/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/AbstractManagementContext.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/AbstractManagementContext.java b/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/AbstractManagementContext.java
index 27645bc..3cdd944 100644
--- a/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/AbstractManagementContext.java
+++ b/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/AbstractManagementContext.java
@@ -421,12 +421,6 @@ public abstract class AbstractManagementContext implements ManagementContextInte
 
     @Override
     public BrooklynCatalog getCatalog() {
-        if (!getCatalogInitialization().hasRunAnyInitialization()) {
-            // catalog init is needed; normally this will be done from start sequence,
-            // but if accessed early -- and in tests -- we will load it here
-            getCatalogInitialization().setManagementContext(this);
-            getCatalogInitialization().populateUnofficial(catalog);
-        }
         return catalog;
     }
     
@@ -505,7 +499,7 @@ public abstract class AbstractManagementContext implements ManagementContextInte
         return uri;
     }
     
-    private Object catalogInitMutex = new Object();
+    private final Object catalogInitMutex = new Object();
     @Override
     public CatalogInitialization getCatalogInitialization() {
         synchronized (catalogInitMutex) {

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/802cbb9b/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/LocalManagementContext.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/LocalManagementContext.java b/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/LocalManagementContext.java
index 4836d70..9076845 100644
--- a/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/LocalManagementContext.java
+++ b/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/LocalManagementContext.java
@@ -162,10 +162,6 @@ public class LocalManagementContext extends AbstractManagementContext {
         this(builder, null);
     }
     
-    public LocalManagementContext(BrooklynProperties brooklynProperties, Map<String, Object> brooklynAdditionalProperties) {
-        this(Builder.fromProperties(brooklynProperties), brooklynAdditionalProperties);
-    }
-    
     public LocalManagementContext(Builder builder, Map<String, Object> brooklynAdditionalProperties) {
         super(builder.build());
         

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/802cbb9b/core/src/main/java/org/apache/brooklyn/core/mgmt/rebind/RebindIteration.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/brooklyn/core/mgmt/rebind/RebindIteration.java b/core/src/main/java/org/apache/brooklyn/core/mgmt/rebind/RebindIteration.java
index 326ebc0..20c20e3 100644
--- a/core/src/main/java/org/apache/brooklyn/core/mgmt/rebind/RebindIteration.java
+++ b/core/src/main/java/org/apache/brooklyn/core/mgmt/rebind/RebindIteration.java
@@ -109,6 +109,7 @@ import org.apache.brooklyn.util.core.flags.FlagUtils;
 import org.apache.brooklyn.util.exceptions.Exceptions;
 import org.apache.brooklyn.util.guava.Maybe;
 import org.apache.brooklyn.util.javalang.Reflections;
+import org.apache.brooklyn.util.osgi.VersionedName;
 import org.apache.brooklyn.util.text.Strings;
 import org.apache.brooklyn.util.time.Duration;
 import org.apache.brooklyn.util.time.Time;
@@ -350,42 +351,14 @@ public abstract class RebindIteration {
             }
         }
         
-        class PersistedCatalogStateImpl implements CatalogInitialization.PersistedCatalogState {
-            private final boolean isEmpty;
-            private final Collection<CatalogItem<?, ?>> legacyCatalogItems;
-            private final Map<String, ? extends InstallableManagedBundle> bundles;
-            
-            PersistedCatalogStateImpl(boolean isEmpty, Collection<CatalogItem<?, ?>> legacyCatalogItems, Map<String, ? extends InstallableManagedBundle> bundles) {
-                this.isEmpty = isEmpty;
-                this.legacyCatalogItems = legacyCatalogItems;
-                this.bundles = bundles;
-            }
-            
-            @Override public boolean isEmpty() {
-                return isEmpty;
-            }
-
-            @Override public Collection<CatalogItem<?, ?>> getLegacyCatalogItems() {
-                return legacyCatalogItems;
-            }
-
-            @Override public Set<String> getBundleIds() {
-                return bundles.keySet();
-            }
-
-            @Override public InstallableManagedBundle getInstallableManagedBundle(String id) {
-                return bundles.get(id);
-            }
-        }
-        
-        Map<String, InstallableManagedBundleImpl> bundles = new LinkedHashMap<>();
+        Map<VersionedName, InstallableManagedBundle> bundles = new LinkedHashMap<>();
         Collection<CatalogItem<?,?>> legacyCatalogItems = new ArrayList<>();
         
         // Find the bundles
         if (rebindManager.persistBundlesEnabled) {
             for (ManagedBundleMemento bundleMemento : mementoManifest.getBundles().values()) {
                 ManagedBundle managedBundle = instantiator.newManagedBundle(bundleMemento);
-                bundles.put(bundleMemento.getId(), new InstallableManagedBundleImpl(bundleMemento, managedBundle));
+                bundles.put(managedBundle.getVersionedName(), new InstallableManagedBundleImpl(bundleMemento, managedBundle));
             }
         } else {
             logRebindingDebug("Not rebinding bundles; feature disabled: {}", mementoManifest.getBundleIds());
@@ -433,10 +406,10 @@ public abstract class RebindIteration {
 
 
         // Delegates to CatalogInitialization; see notes there.
-        CatalogInitialization.PersistedCatalogState persistedCatalogState = new PersistedCatalogStateImpl(isEmpty, legacyCatalogItems, bundles);
+        CatalogInitialization.PersistedCatalogState persistedCatalogState = new CatalogInitialization.PersistedCatalogState(bundles, legacyCatalogItems);
         
         CatalogInitialization catInit = managementContext.getCatalogInitialization();
-        catInit.populateCatalog(mode, persistedCatalogState, exceptionHandler, rebindLogger);
+        catInit.populateInitialAndPersistedCatalog(mode, persistedCatalogState, exceptionHandler, rebindLogger);
     }
 
     protected void instantiateLocationsAndEntities() {

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/802cbb9b/core/src/test/java/org/apache/brooklyn/core/mgmt/ha/HighAvailabilityManagerTestFixture.java
----------------------------------------------------------------------
diff --git a/core/src/test/java/org/apache/brooklyn/core/mgmt/ha/HighAvailabilityManagerTestFixture.java b/core/src/test/java/org/apache/brooklyn/core/mgmt/ha/HighAvailabilityManagerTestFixture.java
index c4d8a3d..ff02c1c 100644
--- a/core/src/test/java/org/apache/brooklyn/core/mgmt/ha/HighAvailabilityManagerTestFixture.java
+++ b/core/src/test/java/org/apache/brooklyn/core/mgmt/ha/HighAvailabilityManagerTestFixture.java
@@ -116,7 +116,8 @@ public abstract class HighAvailabilityManagerTestFixture {
     protected ManagementContextInternal newLocalManagementContext() {
         BrooklynProperties brooklynProperties = BrooklynProperties.Factory.newEmpty();
         brooklynProperties.put(BrooklynServerConfig.MANAGEMENT_NODE_STATE_LISTENERS, ImmutableList.of(stateListener));
-        return LocalManagementContextForTests.newInstance(brooklynProperties);
+        ManagementContextInternal result = LocalManagementContextForTests.newInstance(brooklynProperties);
+        return result;
     }
 
     protected abstract PersistenceObjectStore newPersistenceObjectStore();

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/802cbb9b/core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindCatalogEntityTest.java
----------------------------------------------------------------------
diff --git a/core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindCatalogEntityTest.java b/core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindCatalogEntityTest.java
index 6809bac..b53d92c 100644
--- a/core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindCatalogEntityTest.java
+++ b/core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindCatalogEntityTest.java
@@ -116,9 +116,9 @@ public class RebindCatalogEntityTest extends RebindTestFixture<StartableApplicat
     
     @Test(invocationCount=100, groups="Integration")
     public void testRestoresAppFromCatalogClassloaderManyTimes() throws Exception {
-	    // Need to fix package name and rebuild brooklyn-AppInCatalog.jar
-    	//  or better add it as a new test-bundles subproject
-    	testRestoresAppFromCatalogClassloader();
+        // Need to fix package name and rebuild brooklyn-AppInCatalog.jar
+        //  or better add it as a new test-bundles subproject
+        testRestoresAppFromCatalogClassloader();
     }
     
     // TODO Not using RebindTestUtils.rebind(mementoDir, getClass().getClassLoader());
@@ -150,4 +150,4 @@ public class RebindCatalogEntityTest extends RebindTestFixture<StartableApplicat
         return (StartableApplication) newApps.get(0);
     }
 
-}
+}
\ No newline at end of file

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/802cbb9b/core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindEntityTest.java
----------------------------------------------------------------------
diff --git a/core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindEntityTest.java b/core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindEntityTest.java
index 8c2e7b7..422ba4c 100644
--- a/core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindEntityTest.java
+++ b/core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindEntityTest.java
@@ -61,6 +61,7 @@ import org.apache.brooklyn.core.entity.trait.Resizable;
 import org.apache.brooklyn.core.entity.trait.Startable;
 import org.apache.brooklyn.core.location.LocationConfigTest.MyLocation;
 import org.apache.brooklyn.core.mgmt.internal.LocalManagementContext;
+import org.apache.brooklyn.core.mgmt.internal.ManagementContextInternal;
 import org.apache.brooklyn.core.sensor.BasicAttributeSensor;
 import org.apache.brooklyn.core.sensor.BasicSensorEvent;
 import org.apache.brooklyn.core.sensor.DependentConfiguration;
@@ -648,6 +649,7 @@ public class RebindEntityTest extends RebindTestFixtureWithApp {
 
         RebindTestUtils.waitForPersisted(origManagementContext);
         newManagementContext = RebindTestUtils.newPersistingManagementContextUnstarted(mementoDir, classLoader);
+
         List<Application> newApps = newManagementContext.getRebindManager().rebind(classLoader, null, ManagementNodeState.MASTER);
         newManagementContext.getRebindManager().startPersistence();
         

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/802cbb9b/core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindHistoricCatalogItemTest.java
----------------------------------------------------------------------
diff --git a/core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindHistoricCatalogItemTest.java b/core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindHistoricCatalogItemTest.java
new file mode 100644
index 0000000..0d1b004
--- /dev/null
+++ b/core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindHistoricCatalogItemTest.java
@@ -0,0 +1,53 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.brooklyn.core.mgmt.rebind;
+
+import static org.testng.Assert.assertEquals;
+
+import org.apache.brooklyn.api.objs.BrooklynObjectType;
+import org.apache.brooklyn.api.typereg.BrooklynTypeRegistry.RegisteredTypeKind;
+import org.apache.brooklyn.api.typereg.RegisteredType;
+import org.testng.annotations.Test;
+
+public class RebindHistoricCatalogItemTest extends AbstractRebindHistoricTest {
+    
+    /**
+     * Purpose of test is for rebinding to historic state created before we auto-wrapped .bom 
+     * catalog items in bundles.
+     * 
+     * The persisted state contains a catalog item (which is not wrapped as a bundle).
+     * It was created in brooklyn 0.11.0 via {@code br catalog add app.bom}, using:
+     * <pre>
+     *   brooklyn.catalog:
+     *     id: catalog-myapp
+     *     version: 1.0.0
+     *     itemType: entity
+     *     item:
+     *       type: org.apache.brooklyn.entity.stock.BasicApplication
+     * </pre>
+     */
+    @Test
+    public void testLegacyCatalogItem() throws Exception {
+        addMemento(BrooklynObjectType.CATALOG_ITEM, "catalog", "myapp_1.0.0");
+        rebind();
+        
+        RegisteredType type = mgmt().getTypeRegistry().get("myapp", "1.0.0");
+        assertEquals(type.getKind(), RegisteredTypeKind.SPEC);
+    }
+}

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/802cbb9b/core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindTestFixture.java
----------------------------------------------------------------------
diff --git a/core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindTestFixture.java b/core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindTestFixture.java
index c7d97a9..bad753d 100644
--- a/core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindTestFixture.java
+++ b/core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindTestFixture.java
@@ -18,16 +18,11 @@
  */
 package org.apache.brooklyn.core.mgmt.rebind;
 
-import static org.testng.Assert.assertEquals;
-
 import java.io.File;
 import java.util.List;
 import java.util.Map;
-import java.util.Set;
 import java.util.concurrent.Callable;
 
-import org.apache.brooklyn.api.catalog.BrooklynCatalog;
-import org.apache.brooklyn.api.catalog.CatalogItem;
 import org.apache.brooklyn.api.entity.Application;
 import org.apache.brooklyn.api.mgmt.ManagementContext;
 import org.apache.brooklyn.api.mgmt.Task;
@@ -35,9 +30,7 @@ import org.apache.brooklyn.api.mgmt.ha.HighAvailabilityMode;
 import org.apache.brooklyn.api.mgmt.rebind.RebindExceptionHandler;
 import org.apache.brooklyn.api.mgmt.rebind.RebindManager;
 import org.apache.brooklyn.api.mgmt.rebind.mementos.BrooklynMementoManifest;
-import org.apache.brooklyn.core.catalog.internal.CatalogUtils;
 import org.apache.brooklyn.core.entity.Entities;
-import org.apache.brooklyn.core.entity.EntityFunctions;
 import org.apache.brooklyn.core.entity.EntityPredicates;
 import org.apache.brooklyn.core.entity.StartableApplication;
 import org.apache.brooklyn.core.entity.trait.Startable;
@@ -56,10 +49,6 @@ import org.slf4j.LoggerFactory;
 import org.testng.annotations.AfterMethod;
 import org.testng.annotations.BeforeMethod;
 
-import com.google.common.collect.FluentIterable;
-import com.google.common.collect.Iterables;
-import com.google.common.collect.Sets;
-
 public abstract class RebindTestFixture<T extends StartableApplication> {
 
     private static final Logger LOG = LoggerFactory.getLogger(RebindTestFixture.class);

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/802cbb9b/core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindTestUtils.java
----------------------------------------------------------------------
diff --git a/core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindTestUtils.java b/core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindTestUtils.java
index ba0ece2..911db07 100644
--- a/core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindTestUtils.java
+++ b/core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindTestUtils.java
@@ -284,10 +284,12 @@ public class RebindTestUtils {
 
         public LocalManagementContext buildStarted() {
             LocalManagementContext unstarted = buildUnstarted();
+            
             // Follows BasicLauncher logic for initialising persistence.
             // TODO It should really be encapsulated in a common entry point
             if (persistMode == PersistMode.DISABLED) {
                 unstarted.generateManagementPlaneId();
+                unstarted.getCatalogInitialization().populateInitialCatalogOnly();
             } else if (haMode == HighAvailabilityMode.DISABLED) {
                 unstarted.getRebindManager().rebind(classLoader, null, ManagementNodeState.MASTER);
                 unstarted.getRebindManager().startPersistence();
@@ -478,6 +480,7 @@ public class RebindTestUtils {
             // Would that affect any tests?
             newManagementContext = new LocalManagementContextForTests(BrooklynProperties.Factory.newDefault());
         }
+        
         if (!hasPersister) {
             if (objectStore == null) {
                 objectStore = new FileBasedObjectStore(checkNotNull(mementoDir, "mementoDir and objectStore must not both be null"));

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/802cbb9b/core/src/test/java/org/apache/brooklyn/core/test/entity/LocalManagementContextForTests.java
----------------------------------------------------------------------
diff --git a/core/src/test/java/org/apache/brooklyn/core/test/entity/LocalManagementContextForTests.java b/core/src/test/java/org/apache/brooklyn/core/test/entity/LocalManagementContextForTests.java
index 9996294..4634bc5 100644
--- a/core/src/test/java/org/apache/brooklyn/core/test/entity/LocalManagementContextForTests.java
+++ b/core/src/test/java/org/apache/brooklyn/core/test/entity/LocalManagementContextForTests.java
@@ -20,6 +20,7 @@ package org.apache.brooklyn.core.test.entity;
 
 import java.util.Map;
 
+import org.apache.brooklyn.api.catalog.BrooklynCatalog;
 import org.apache.brooklyn.config.ConfigKey;
 import org.apache.brooklyn.core.internal.BrooklynProperties;
 import org.apache.brooklyn.core.mgmt.ha.OsgiManager;
@@ -209,4 +210,15 @@ public class LocalManagementContextForTests extends LocalManagementContext {
         return builder(true).enableOsgiReusable().build();
     }
 
+    @Override
+    public BrooklynCatalog getCatalog() {
+        if (!getCatalogInitialization().hasRunAnyInitialization()) {
+            // Before catalog init, the catalog will be empty.
+            // Normally the BasicLauncher (either the classic BrooklynLauncher or the OsgiLauncher)
+            // will have ensured the catalog initialization is called. But for tests, the lifecycle
+            // for the management context is unfortunately different.
+            getCatalogInitialization().unofficialPopulateInitialCatalog();
+        }
+        return catalog;
+    }
 }

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/802cbb9b/core/src/test/resources/org/apache/brooklyn/core/mgmt/rebind/catalog-myapp_1.0.0
----------------------------------------------------------------------
diff --git a/core/src/test/resources/org/apache/brooklyn/core/mgmt/rebind/catalog-myapp_1.0.0 b/core/src/test/resources/org/apache/brooklyn/core/mgmt/rebind/catalog-myapp_1.0.0
new file mode 100644
index 0000000..e948184
--- /dev/null
+++ b/core/src/test/resources/org/apache/brooklyn/core/mgmt/rebind/catalog-myapp_1.0.0
@@ -0,0 +1,36 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements.  See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership.  The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License.  You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied.  See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+
+<catalogItem>
+  <brooklynVersion>0.12.0-20170901.1331</brooklynVersion>
+  <type>org.apache.brooklyn.core:org.apache.brooklyn.core.catalog.internal.CatalogEntityItemDto</type>
+  <id>myapp:1.0.0</id>
+  <catalogItemId>myapp:1.0.0</catalogItemId>
+  <searchPath class="ImmutableList"/>
+  <registeredTypeName>myapp</registeredTypeName>
+  <version>1.0.0</version>
+  <planYaml>services:
+- type: org.apache.brooklyn.entity.stock.BasicApplication</planYaml>
+  <libraries class="ImmutableList" reference="../searchPath"/>
+  <catalogItemType>ENTITY</catalogItemType>
+  <catalogItemJavaType>org.apache.brooklyn.api:org.apache.brooklyn.api.entity.Entity</catalogItemJavaType>
+  <specType>org.apache.brooklyn.api:org.apache.brooklyn.api.entity.EntitySpec</specType>
+  <deprecated>false</deprecated>
+  <disabled>false</disabled>
+</catalogItem>

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/802cbb9b/launcher-common/src/main/java/org/apache/brooklyn/launcher/common/BasicLauncher.java
----------------------------------------------------------------------
diff --git a/launcher-common/src/main/java/org/apache/brooklyn/launcher/common/BasicLauncher.java b/launcher-common/src/main/java/org/apache/brooklyn/launcher/common/BasicLauncher.java
index ec82d45..19b6e20 100644
--- a/launcher-common/src/main/java/org/apache/brooklyn/launcher/common/BasicLauncher.java
+++ b/launcher-common/src/main/java/org/apache/brooklyn/launcher/common/BasicLauncher.java
@@ -253,12 +253,12 @@ public class BasicLauncher<T extends BasicLauncher<T>> {
     }
 
     public T persistMode(PersistMode persistMode) {
-        this.persistMode = persistMode;
+        this.persistMode = checkNotNull(persistMode, "persistMode");
         return self();
     }
 
     public T highAvailabilityMode(HighAvailabilityMode highAvailabilityMode) {
-        this.highAvailabilityMode = highAvailabilityMode;
+        this.highAvailabilityMode = checkNotNull(highAvailabilityMode, "highAvailabilityMode");
         return self();
     }
 
@@ -389,6 +389,8 @@ public class BasicLauncher<T extends BasicLauncher<T>> {
         if (started) throw new IllegalStateException("Cannot start() or launch() multiple times");
         started = true;
 
+        checkConfiguration();
+        
         initManagementContext();
 
         CatalogInitialization catInit = ((ManagementContextInternal)managementContext).getCatalogInitialization();
@@ -407,6 +409,23 @@ public class BasicLauncher<T extends BasicLauncher<T>> {
         return self();
     }
 
+    private void checkConfiguration() {
+        if (highAvailabilityMode != HighAvailabilityMode.DISABLED) {
+            if (persistMode == PersistMode.DISABLED) {
+                if (highAvailabilityMode == HighAvailabilityMode.AUTO) {
+                    highAvailabilityMode = HighAvailabilityMode.DISABLED;
+                } else {
+                    throw new FatalConfigurationRuntimeException("Cannot specify highAvailability when persistence is disabled");
+                }
+            } else if (persistMode == PersistMode.CLEAN && 
+                    (highAvailabilityMode == HighAvailabilityMode.STANDBY 
+                            || highAvailabilityMode == HighAvailabilityMode.HOT_STANDBY
+                            || highAvailabilityMode == HighAvailabilityMode.HOT_BACKUP)) {
+                throw new FatalConfigurationRuntimeException("Cannot specify highAvailability "+highAvailabilityMode+" when persistence is CLEAN");
+            }
+        }
+    }
+
     /**
      * Starts the web server (with web console) and Brooklyn applications, as per the specifications configured. 
      * @return An object containing details of the web server and the management context.
@@ -415,8 +434,12 @@ public class BasicLauncher<T extends BasicLauncher<T>> {
         if (startedPartTwo) throw new IllegalStateException("Cannot start() or launch() multiple times");
         startedPartTwo = true;
         
-        handlePersistence();
-        populateCatalog(catalogInitialization);
+        if (persistMode != PersistMode.DISABLED) {
+            handlePersistence();
+        } else {
+            ((LocalManagementContext)managementContext).generateManagementPlaneId();
+            populateInitialCatalogNoPersistence(catalogInitialization);
+        }
         markCatalogStarted(catalogInitialization);
         addLocations();
         markStartupComplete();
@@ -471,30 +494,21 @@ public class BasicLauncher<T extends BasicLauncher<T>> {
     protected void startingUp() {
     }
 
-    private void markCatalogStarted(CatalogInitialization catInit) {
-        catInit.setStartingUp(false);
-    }
-
-    protected void populateCatalog(CatalogInitialization catInit) {
+    protected void populateInitialCatalogNoPersistence(CatalogInitialization catInit) {
+        assert persistMode == PersistMode.DISABLED : "persistMode="+persistMode;
+        
         try {
-            // run cat init now if it hasn't yet been run; 
-            // will also run if there was an ignored error in catalog above, allowing it to fail startup here if requested
-            if (catInit!=null && !catInit.hasRunOfficialInitialization()) {
-                if (persistMode==PersistMode.DISABLED) {
-                    LOG.debug("Loading catalog as part of launch sequence (it was not loaded as part of any rebind sequence)");
-                    catInit.populateCatalog(ManagementNodeState.MASTER, true, true, null);
-                } else {
-                    // should have loaded during rebind
-                    ManagementNodeState state = managementContext.getHighAvailabilityManager().getNodeState();
-                    LOG.warn("Loading catalog for "+state+" as part of launch sequence (it was not loaded as part of the rebind sequence)");
-                    catInit.populateCatalog(state, true, true, null);
-                }
-            }
+            LOG.debug("Initializing initial catalog as part of launch sequence (prior to any persistence rebind)");
+            catInit.populateInitialCatalogOnly();
         } catch (Exception e) {
             handleSubsystemStartupError(ignoreCatalogErrors, "initial catalog", e);
         }
     }
 
+    private void markCatalogStarted(CatalogInitialization catInit) {
+        catInit.setStartingUp(false);
+    }
+
     protected void handlePersistence() {
         try {
             initPersistence();
@@ -551,45 +565,38 @@ public class BasicLauncher<T extends BasicLauncher<T>> {
     }
 
     protected void initPersistence() {
-        // Prepare the rebind directory, and initialise the RebindManager as required
+        assert persistMode != PersistMode.DISABLED : "persistMode="+persistMode+"; haMode="+highAvailabilityMode;
+
+        // Prepare the rebind directory, and initialise the RebindManager
         final PersistenceObjectStore objectStore;
-        if (persistMode == PersistMode.DISABLED) {
-            LOG.info("Persistence disabled");
-            objectStore = null;
-            ((LocalManagementContext)managementContext).generateManagementPlaneId();
-        } else {
-            try {
-                if (persistenceLocation == null) {
-                    persistenceLocation = brooklynProperties.getConfig(BrooklynServerConfig.PERSISTENCE_LOCATION_SPEC);
-                }
-                persistenceDir = BrooklynServerPaths.newMainPersistencePathResolver(brooklynProperties).location(persistenceLocation).dir(persistenceDir).resolve();
-                objectStore = BrooklynPersistenceUtils.newPersistenceObjectStore(managementContext, persistenceLocation, persistenceDir, 
-                    persistMode, highAvailabilityMode);
-                    
-                RebindManager rebindManager = managementContext.getRebindManager();
-                
-                BrooklynMementoPersisterToObjectStore persister = new BrooklynMementoPersisterToObjectStore(
-                    objectStore, managementContext);
-                PersistenceExceptionHandler persistenceExceptionHandler = PersistenceExceptionHandlerImpl.builder().build();
-                ((RebindManagerImpl) rebindManager).setPeriodicPersistPeriod(persistPeriod);
-                rebindManager.setPersister(persister, persistenceExceptionHandler);
-            } catch (FatalConfigurationRuntimeException e) {
-                throw e;
-            } catch (Exception e) {
-                Exceptions.propagateIfFatal(e);
-                LOG.debug("Error initializing persistence subsystem (rethrowing): "+e, e);
-                throw new FatalRuntimeException("Error initializing persistence subsystem: "+
-                    Exceptions.collapseText(e), e);
+        try {
+            if (persistenceLocation == null) {
+                persistenceLocation = brooklynProperties.getConfig(BrooklynServerConfig.PERSISTENCE_LOCATION_SPEC);
             }
+            persistenceDir = BrooklynServerPaths.newMainPersistencePathResolver(brooklynProperties).location(persistenceLocation).dir(persistenceDir).resolve();
+            objectStore = BrooklynPersistenceUtils.newPersistenceObjectStore(managementContext, persistenceLocation, persistenceDir, 
+                persistMode, highAvailabilityMode);
+                
+            RebindManager rebindManager = managementContext.getRebindManager();
+            
+            BrooklynMementoPersisterToObjectStore persister = new BrooklynMementoPersisterToObjectStore(
+                objectStore, managementContext);
+            PersistenceExceptionHandler persistenceExceptionHandler = PersistenceExceptionHandlerImpl.builder().build();
+            ((RebindManagerImpl) rebindManager).setPeriodicPersistPeriod(persistPeriod);
+            rebindManager.setPersister(persister, persistenceExceptionHandler);
+        } catch (FatalConfigurationRuntimeException e) {
+            throw e;
+        } catch (Exception e) {
+            Exceptions.propagateIfFatal(e);
+            LOG.debug("Error initializing persistence subsystem (rethrowing): "+e, e);
+            throw new FatalRuntimeException("Error initializing persistence subsystem: "+
+                Exceptions.collapseText(e), e);
         }
         
         // Initialise the HA manager as required
         if (highAvailabilityMode == HighAvailabilityMode.DISABLED) {
             LOG.info("High availability disabled");
         } else {
-            if (objectStore==null)
-                throw new FatalConfigurationRuntimeException("Cannot run in HA mode when no persistence configured.");
-
             HighAvailabilityManager haManager = managementContext.getHighAvailabilityManager();
             ManagementPlaneSyncRecordPersister persister =
                 new ManagementPlaneSyncRecordPersisterToObjectStore(managementContext,
@@ -602,20 +609,20 @@ public class BasicLauncher<T extends BasicLauncher<T>> {
     }
     
     protected void startPersistence() {
+        assert persistMode != PersistMode.DISABLED : "persistMode="+persistMode+"; haMode="+highAvailabilityMode;
+        
         // Now start the HA Manager and the Rebind manager, as required
         if (highAvailabilityMode == HighAvailabilityMode.DISABLED) {
             HighAvailabilityManager haManager = managementContext.getHighAvailabilityManager();
             haManager.disabled();
 
-            if (persistMode != PersistMode.DISABLED) {
-                startPersistenceWithoutHA();
-            }
+            startPersistenceWithoutHA();
             
         } else {
             // Let the HA manager decide when objectstore.prepare and rebindmgr.rebind need to be called 
             // (based on whether other nodes in plane are already running).
             
-            HighAvailabilityMode startMode=null;
+            HighAvailabilityMode startMode;
             switch (highAvailabilityMode) {
                 case AUTO:
                 case MASTER:
@@ -626,9 +633,9 @@ public class BasicLauncher<T extends BasicLauncher<T>> {
                     break;
                 case DISABLED:
                     throw new IllegalStateException("Unexpected code-branch for high availability mode "+highAvailabilityMode);
+                default:
+                    throw new IllegalStateException("Unexpected high availability mode "+highAvailabilityMode);
             }
-            if (startMode==null)
-                throw new IllegalStateException("Unexpected high availability mode "+highAvailabilityMode);
             
             LOG.debug("Management node (with HA) starting");
             HighAvailabilityManager haManager = managementContext.getHighAvailabilityManager();

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/802cbb9b/launcher/src/test/java/org/apache/brooklyn/launcher/BrooklynLauncherRebindCatalogTest.java
----------------------------------------------------------------------
diff --git a/launcher/src/test/java/org/apache/brooklyn/launcher/BrooklynLauncherRebindCatalogTest.java b/launcher/src/test/java/org/apache/brooklyn/launcher/BrooklynLauncherRebindCatalogTest.java
index 1bf94cd..bafb641 100644
--- a/launcher/src/test/java/org/apache/brooklyn/launcher/BrooklynLauncherRebindCatalogTest.java
+++ b/launcher/src/test/java/org/apache/brooklyn/launcher/BrooklynLauncherRebindCatalogTest.java
@@ -20,9 +20,20 @@ package org.apache.brooklyn.launcher;
 
 import java.io.File;
 import java.util.List;
+import java.util.Set;
 
 import javax.annotation.Nullable;
 
+import org.apache.brooklyn.api.catalog.BrooklynCatalog;
+import org.apache.brooklyn.api.catalog.CatalogItem;
+import org.apache.brooklyn.api.typereg.BrooklynTypeRegistry;
+import org.apache.brooklyn.api.typereg.RegisteredType;
+import org.apache.brooklyn.core.catalog.internal.CatalogInitialization;
+import org.apache.brooklyn.core.mgmt.persist.PersistMode;
+import org.apache.brooklyn.core.test.entity.LocalManagementContextForTests;
+import org.apache.brooklyn.util.collections.MutableSet;
+import org.apache.brooklyn.util.core.ResourceUtils;
+import org.apache.brooklyn.util.os.Os;
 import org.apache.commons.collections.IteratorUtils;
 import org.testng.Assert;
 import org.testng.annotations.AfterMethod;
@@ -34,21 +45,14 @@ import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
 import com.google.common.io.Files;
 
-import org.apache.brooklyn.api.catalog.BrooklynCatalog;
-import org.apache.brooklyn.api.catalog.CatalogItem;
-import org.apache.brooklyn.core.catalog.internal.CatalogInitialization;
-import org.apache.brooklyn.core.mgmt.persist.PersistMode;
-import org.apache.brooklyn.core.test.entity.LocalManagementContextForTests;
-import org.apache.brooklyn.util.core.ResourceUtils;
-import org.apache.brooklyn.util.os.Os;
-
 public class BrooklynLauncherRebindCatalogTest {
 
     private static final String TEST_VERSION = "test-version";
     private static final String CATALOG_INITIAL = "classpath://rebind-test-catalog.bom";
+    private static final String CATALOG_EMPTY_INITIAL = "classpath://rebind-test-empty-catalog.bom";
     private static final String CATALOG_ADDITIONS = "rebind-test-catalog-additions.bom";
-    private static final Iterable<String> EXPECTED_DEFAULT_IDS = ImmutableSet.of("one:" + TEST_VERSION, "two:" + TEST_VERSION);
-    private static final Iterable<String> EXPECTED_ADDED_IDS = ImmutableSet.of("three:" + TEST_VERSION, "four:" + TEST_VERSION);
+    private static final Set<String> EXPECTED_DEFAULT_IDS = ImmutableSet.of("one:" + TEST_VERSION, "two:" + TEST_VERSION);
+    private static final Set<String> EXPECTED_ADDED_IDS = ImmutableSet.of("three:" + TEST_VERSION, "four:" + TEST_VERSION);
 
     private List<BrooklynLauncher> launchers = Lists.newCopyOnWriteArrayList();
     
@@ -59,9 +63,13 @@ public class BrooklynLauncherRebindCatalogTest {
         }
         launchers.clear();
     }
-    
+
     private BrooklynLauncher newLauncherForTests(String persistenceDir) {
-        CatalogInitialization catalogInitialization = new CatalogInitialization(CATALOG_INITIAL);
+        return newLauncherForTests(persistenceDir, CATALOG_INITIAL);
+    }
+    
+    private BrooklynLauncher newLauncherForTests(String persistenceDir, String catalogInitial) {
+        CatalogInitialization catalogInitialization = new CatalogInitialization(catalogInitial);
         BrooklynLauncher launcher = BrooklynLauncher.newInstance()
                 .brooklynProperties(LocalManagementContextForTests.builder(true).buildProperties())
                 .catalogInitialization(catalogInitialization)
@@ -73,37 +81,105 @@ public class BrooklynLauncherRebindCatalogTest {
     }
 
     @Test
-    public void testRebindDoesNotEffectCatalog() {
+    public void testRebindGetsInitialCatalog() {
         String persistenceDir = newTempPersistenceContainerName();
 
         BrooklynLauncher launcher = newLauncherForTests(persistenceDir);
         launcher.start();
-        BrooklynCatalog catalog = launcher.getServerDetails().getManagementContext().getCatalog();
+        assertCatalogConsistsOfIds(launcher, EXPECTED_DEFAULT_IDS);
 
-        assertCatalogConsistsOfIds(catalog.getCatalogItems(), EXPECTED_DEFAULT_IDS);
+        launcher.terminate();
 
-        catalog.deleteCatalogItem("one", TEST_VERSION);
-        catalog.deleteCatalogItem("two", TEST_VERSION);
+        BrooklynLauncher newLauncher = newLauncherForTests(persistenceDir);
+        newLauncher.start();
+        assertCatalogConsistsOfIds(newLauncher, EXPECTED_DEFAULT_IDS);
+    }
+
+    @Test
+    public void testRebindPersistsInitialCatalog() {
+        String persistenceDir = newTempPersistenceContainerName();
 
-        Assert.assertEquals(Iterables.size(catalog.getCatalogItems()), 0);
+        BrooklynLauncher launcher = newLauncherForTests(persistenceDir, CATALOG_INITIAL);
+        launcher.start();
+        assertCatalogConsistsOfIds(launcher, EXPECTED_DEFAULT_IDS);
+
+        launcher.terminate();
 
+        BrooklynLauncher newLauncher = newLauncherForTests(persistenceDir, CATALOG_EMPTY_INITIAL);
+        newLauncher.start();
+        assertCatalogConsistsOfIds(newLauncher, EXPECTED_DEFAULT_IDS);
+    }
+
+    @Test
+    public void testRebindGetsUnionOfInitialAndPersisted() {
+        String persistenceDir = newTempPersistenceContainerName();
+
+        BrooklynLauncher launcher = newLauncherForTests(persistenceDir);
+        launcher.start();
+        assertCatalogConsistsOfIds(launcher, EXPECTED_DEFAULT_IDS);
+
+        BrooklynCatalog catalog = launcher.getServerDetails().getManagementContext().getCatalog();
         catalog.addItems(new ResourceUtils(this).getResourceAsString(CATALOG_ADDITIONS));
+        assertCatalogConsistsOfIds(launcher, Iterables.concat(EXPECTED_DEFAULT_IDS, EXPECTED_ADDED_IDS));
+
+        launcher.terminate();
+
+        BrooklynLauncher newLauncher = newLauncherForTests(persistenceDir);
+        newLauncher.start();
+        assertCatalogConsistsOfIds(newLauncher, Iterables.concat(EXPECTED_DEFAULT_IDS, EXPECTED_ADDED_IDS));
+    }
 
-        assertCatalogConsistsOfIds(catalog.getCatalogItems(), EXPECTED_ADDED_IDS);
+    // In CatalogInitialization, we take the union of the initial catalog and the persisted state catalog.
+    // Therefore removals from the original catalog do not take effect.
+    // That is acceptable - better than previously where, after upgrading Brooklyn, one had to run
+    // `br catalog add `${BROOKLYN_HOME}/catalog/catalog.bom` to add the new catalog items to the existing
+    // persisted state.
+    @Test(groups="Broken")
+    public void testRemovedInitialItemStillRemovedAfterRebind() {
+        Set<String> EXPECTED_DEFAULT_IDS_WITHOUT_ONE = MutableSet.<String>builder()
+                .addAll(EXPECTED_DEFAULT_IDS)
+                .remove("one:" + TEST_VERSION).build();
+        
+        String persistenceDir = newTempPersistenceContainerName();
 
+        BrooklynLauncher launcher = newLauncherForTests(persistenceDir);
+        launcher.start();
+
+        BrooklynCatalog catalog = launcher.getServerDetails().getManagementContext().getCatalog();
+        catalog.deleteCatalogItem("one", TEST_VERSION);
+        assertCatalogConsistsOfIds(launcher, EXPECTED_DEFAULT_IDS_WITHOUT_ONE);
+        
         launcher.terminate();
 
         BrooklynLauncher newLauncher = newLauncherForTests(persistenceDir);
         newLauncher.start();
-        assertCatalogConsistsOfIds(newLauncher.getServerDetails().getManagementContext().getCatalog().getCatalogItems(), EXPECTED_ADDED_IDS);
+        assertCatalogConsistsOfIds(newLauncher, EXPECTED_DEFAULT_IDS_WITHOUT_ONE);
+    }
+
+    private void assertCatalogConsistsOfIds(BrooklynLauncher launcher, Iterable<String> ids) {
+        BrooklynTypeRegistry typeRegistry = launcher.getServerDetails().getManagementContext().getTypeRegistry();
+        BrooklynCatalog catalog = launcher.getServerDetails().getManagementContext().getCatalog();
+        assertTypeRegistryConsistsOfIds(typeRegistry.getAll(), ids);
+        assertCatalogConsistsOfIds(catalog.getCatalogItems(), ids);
     }
 
     private void assertCatalogConsistsOfIds(Iterable<CatalogItem<Object, Object>> catalogItems, Iterable<String> ids) {
         Iterable<String> idsFromItems = Iterables.transform(catalogItems, new Function<CatalogItem<?,?>, String>() {
             @Nullable
             @Override
-            public String apply(CatalogItem<?, ?> catalogItem) {
-                return catalogItem.getCatalogItemId();
+            public String apply(CatalogItem<?, ?> input) {
+                return input.getCatalogItemId();
+            }
+        });
+        Assert.assertTrue(compareIterablesWithoutOrderMatters(ids, idsFromItems), String.format("Expected %s, found %s", ids, idsFromItems));
+    }
+
+    private void assertTypeRegistryConsistsOfIds(Iterable<RegisteredType> types, Iterable<String> ids) {
+        Iterable<String> idsFromItems = Iterables.transform(types, new Function<RegisteredType, String>() {
+            @Nullable
+            @Override
+            public String apply(RegisteredType input) {
+                return input.getId();
             }
         });
         Assert.assertTrue(compareIterablesWithoutOrderMatters(ids, idsFromItems), String.format("Expected %s, found %s", ids, idsFromItems));

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/802cbb9b/launcher/src/test/java/org/apache/brooklyn/launcher/BrooklynLauncherTest.java
----------------------------------------------------------------------
diff --git a/launcher/src/test/java/org/apache/brooklyn/launcher/BrooklynLauncherTest.java b/launcher/src/test/java/org/apache/brooklyn/launcher/BrooklynLauncherTest.java
index c8d4352..09c08e9 100644
--- a/launcher/src/test/java/org/apache/brooklyn/launcher/BrooklynLauncherTest.java
+++ b/launcher/src/test/java/org/apache/brooklyn/launcher/BrooklynLauncherTest.java
@@ -35,6 +35,7 @@ import org.apache.brooklyn.core.catalog.internal.CatalogInitialization;
 import org.apache.brooklyn.core.internal.BrooklynProperties;
 import org.apache.brooklyn.core.mgmt.internal.LocalManagementContext;
 import org.apache.brooklyn.core.mgmt.internal.ManagementContextInternal;
+import org.apache.brooklyn.core.mgmt.persist.PersistMode;
 import org.apache.brooklyn.core.server.BrooklynServerConfig;
 import org.apache.brooklyn.core.test.entity.LocalManagementContextForTests;
 import org.apache.brooklyn.core.test.entity.TestApplication;
@@ -272,12 +273,11 @@ public class BrooklynLauncherTest {
     @Test  // takes a bit of time because starts webapp, but also tests rest api so useful
     public void testErrorsCaughtByApiAndRestApiWorks() throws Exception {
         launcher = newLauncherForTests(true)
-                .catalogInitialization(new CatalogInitialization(null).addPopulationCallback(new Function<CatalogInitialization, Void>() {
-                    @Override
-                    public Void apply(CatalogInitialization input) {
+                .catalogInitialization(new CatalogInitialization(null) {
+                    @Override public void populateInitialCatalogOnly() {
                         throw new RuntimeException("deliberate-exception-for-testing");
-                    }
-                }))
+                    }})
+                .persistMode(PersistMode.DISABLED)
                 .installSecurityFilter(false)
                 .start();
         // 'deliberate-exception' error above should be thrown, then caught in this calling thread

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/802cbb9b/launcher/src/test/resources/rebind-test-empty-catalog.bom
----------------------------------------------------------------------
diff --git a/launcher/src/test/resources/rebind-test-empty-catalog.bom b/launcher/src/test/resources/rebind-test-empty-catalog.bom
new file mode 100644
index 0000000..53b914f
--- /dev/null
+++ b/launcher/src/test/resources/rebind-test-empty-catalog.bom
@@ -0,0 +1,25 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#  http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+brooklyn.catalog:
+  version: "test-version"
+  itemType: entity
+  items:
+
+  # Do not scan classpath with for classes with a @Catalog annotation
+  - scanJavaAnnotations: false

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/802cbb9b/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/testing/BrooklynRestApiTest.java
----------------------------------------------------------------------
diff --git a/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/testing/BrooklynRestApiTest.java b/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/testing/BrooklynRestApiTest.java
index b5136e6..2ae0183 100644
--- a/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/testing/BrooklynRestApiTest.java
+++ b/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/testing/BrooklynRestApiTest.java
@@ -140,7 +140,9 @@ public abstract class BrooklynRestApiTest {
     protected synchronized ManagementContext getManagementContext() {
         if (manager==null) {
             if (useLocalScannedCatalog()) {
-                manager = new LocalManagementContext();
+                manager = LocalManagementContextForTests.builder(true)
+                        .enableOsgiReusable()
+                        .build();
                 forceUseOfDefaultCatalogWithJavaClassPath();
             } else {
                 manager = new LocalManagementContextForTests();


Mime
View raw message